Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1192886 > unrolled thread

Re: [PATCH V7 4/5] arm64: apei: implement arch_apei_get_mem_attributes()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V7 4/5] arm64: apei: implement  arch_apei_get_mem_attributes() Matt Fleming <matt.fleming@intel.com> - 2015-07-27 11:50 +0200
    Re: [PATCH V7 4/5] arm64: apei: implement  arch_apei_get_mem_attributes() Will Deacon <will.deacon@arm.com> - 2015-07-27 11:50 +0200
      Re: [PATCH V7 4/5] arm64: apei: implement  arch_apei_get_mem_attributes() Matt Fleming <matt.fleming@intel.com> - 2015-07-27 12:00 +0200
        Re: [PATCH V7 4/5] arm64: apei: implement  arch_apei_get_mem_attributes() "Zhang, Jonathan Zhixiong" <zjzhang@codeaurora.org> - 2015-07-29 03:00 +0200

#1192886 — Re: [PATCH V7 4/5] arm64: apei: implement arch_apei_get_mem_attributes()

On Fri, 2015-07-24 at 17:26 +0100, Will Deacon wrote:
> On Fri, Jul 24, 2015 at 05:21:49PM +0100, Catalin Marinas wrote:
> > On Fri, Jul 24, 2015 at 03:57:08PM +0100, Will Deacon wrote:
> > > On Tue, Jul 21, 2015 at 10:59:19PM +0100, Jonathan (Zhixiong) Zhang wrote:
> > > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> > > > +static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
> > > > +{
> > > > +	pgprot_t prot;
> > > > +
> > > > +	prot = efi_mem_attributes(addr);
> > > > +	if (prot & EFI_MEMORY_UC)
> > > > +		return PROT_DEVICE_nGnRnE;
> > > > +	if (prot & EFI_MEMORY_WC)
> > > > +		return PROT_NORMAL_NC;
> > > 
> > > Can we not use pgprot_noncached and pgprot_writecombine for these two?
> > 
> > Actually, why do we even use pgprot_t for prot here? EFI_MEMORY_* don't
> > have anything to do with the arch-specific pgprot_t.
> 
> Good point; the pgprot_t confused me, so my suggestion is much use after
> ll. We're better off with a u64 to avoid further confusion.

Isn't the whole point of arch_apei_get_mem_attribute() to turn an
arch-independent memory attribute (EFI_MEMORY_*) into an arch-specific
value to pass to ioremap_page_range()?

I don't see how you can do that any other way than by using pgprot_t.

Really, the problem here is that ioremap_page_caller() has no notion of
"map this range in a firmware-compatible manner". If we could do, for
example,

	ioremap_page_range(vaddr, vend, paddr, PAGE_FW_COMPAT);

that would allow the innards of the arch-ioremap to figure out exactly
how to map this range so that the firmware could access it coherently.

I suggested this previously but it didn't gain any traction.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1192893

On Mon, Jul 27, 2015 at 10:38:11AM +0100, Matt Fleming wrote:
> On Fri, 2015-07-24 at 17:26 +0100, Will Deacon wrote:
> > On Fri, Jul 24, 2015 at 05:21:49PM +0100, Catalin Marinas wrote:
> > > On Fri, Jul 24, 2015 at 03:57:08PM +0100, Will Deacon wrote:
> > > > On Tue, Jul 21, 2015 at 10:59:19PM +0100, Jonathan (Zhixiong) Zhang wrote:
> > > > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> > > > > +static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
> > > > > +{
> > > > > +	pgprot_t prot;
> > > > > +
> > > > > +	prot = efi_mem_attributes(addr);
> > > > > +	if (prot & EFI_MEMORY_UC)
> > > > > +		return PROT_DEVICE_nGnRnE;
> > > > > +	if (prot & EFI_MEMORY_WC)
> > > > > +		return PROT_NORMAL_NC;
> > > > 
> > > > Can we not use pgprot_noncached and pgprot_writecombine for these two?
> > > 
> > > Actually, why do we even use pgprot_t for prot here? EFI_MEMORY_* don't
> > > have anything to do with the arch-specific pgprot_t.
> > 
> > Good point; the pgprot_t confused me, so my suggestion is much use after
> > ll. We're better off with a u64 to avoid further confusion.
> 
> Isn't the whole point of arch_apei_get_mem_attribute() to turn an
> arch-independent memory attribute (EFI_MEMORY_*) into an arch-specific
> value to pass to ioremap_page_range()?

That bit's fine. The weird bit is:

  pgprot_t prot;

  prot = efi_mem_attributes(addr);

Since that's putting the arch-independent format into the pg_prot.

> I don't see how you can do that any other way than by using pgprot_t.
> 
> Really, the problem here is that ioremap_page_caller() has no notion of
> "map this range in a firmware-compatible manner". If we could do, for
> example,
> 
> 	ioremap_page_range(vaddr, vend, paddr, PAGE_FW_COMPAT);
> 
> that would allow the innards of the arch-ioremap to figure out exactly
> how to map this range so that the firmware could access it coherently.
> 
> I suggested this previously but it didn't gain any traction.

Yeah, or just ioremap_efi.

</me runs away>

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1192912

On Mon, 2015-07-27 at 10:45 +0100, Will Deacon wrote:
> 
> That bit's fine. The weird bit is:
> 
>   pgprot_t prot;
> 
>   prot = efi_mem_attributes(addr);
> 
> Since that's putting the arch-independent format into the pg_prot.

Oops, missed that. Yeah that's funky.

> > I don't see how you can do that any other way than by using pgprot_t.
> > 
> > Really, the problem here is that ioremap_page_caller() has no notion of
> > "map this range in a firmware-compatible manner". If we could do, for
> > example,
> > 
> > 	ioremap_page_range(vaddr, vend, paddr, PAGE_FW_COMPAT);
> > 
> > that would allow the innards of the arch-ioremap to figure out exactly
> > how to map this range so that the firmware could access it coherently.
> > 
> > I suggested this previously but it didn't gain any traction.
> 
> Yeah, or just ioremap_efi.
> 
> </me runs away>

Someone beat you to it ;-)

arch/x86/include/asm/efi.h:#define efi_ioremap(addr, size, type, attr)	ioremap_cache(addr, size)
arch/x86/include/asm/efi.h:extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
arch/x86/platform/efi/efi.c:		va = efi_ioremap(md->phys_addr, size,
arch/x86/platform/efi/efi_64.c:void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
arch/x86/platform/efi/efi_64.c:		efi_ioremap(top, size - (top - phys_addr), type, attribute);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1194750

On 7/27/2015 2:54 AM, Matt Fleming wrote:
> On Mon, 2015-07-27 at 10:45 +0100, Will Deacon wrote:
>>
>> That bit's fine. The weird bit is:
>>
>>    pgprot_t prot;
>>
>>    prot = efi_mem_attributes(addr);
>>
>> Since that's putting the arch-independent format into the pg_prot.
>
> Oops, missed that. Yeah that's funky.
Sorry, that was my mistake. Corrected at V8.
>>> I don't see how you can do that any other way than by using pgprot_t.
>>>
>>> Really, the problem here is that ioremap_page_caller() has no notion of
>>> "map this range in a firmware-compatible manner". If we could do, for
>>> example,
>>>
>>> 	ioremap_page_range(vaddr, vend, paddr, PAGE_FW_COMPAT);
>>>
>>> that would allow the innards of the arch-ioremap to figure out exactly
>>> how to map this range so that the firmware could access it coherently.
With this patch set, arch_apei_get_mem_attribute() is defined for above
mentioned PAGE_FW_COMPAT. If in future there are additional use case for
mapping page in atomic context according to UEFI memory map, the
function name/definition can be generalized.
>>>
>>> I suggested this previously but it didn't gain any traction.
>>
>> Yeah, or just ioremap_efi.
>>
>> </me runs away>
>
> Someone beat you to it ;-)
>
> arch/x86/include/asm/efi.h:#define efi_ioremap(addr, size, type, attr)	ioremap_cache(addr, size)
> arch/x86/include/asm/efi.h:extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
> arch/x86/platform/efi/efi.c:		va = efi_ioremap(md->phys_addr, size,
> arch/x86/platform/efi/efi_64.c:void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
> arch/x86/platform/efi/efi_64.c:		efi_ioremap(top, size - (top - phys_addr), type, attribute);
x86's efi_ioremap() is intended to run at init time only. For the
purpose of this patch set, we would need to define something new
for both archs. We may want to keep it simple at this time, how do you
prefer?

-- 
Jonathan (Zhixiong) Zhang
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web