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

Groups > linux.kernel > #1189852 > unrolled thread

Re: [PATCH V6 2/4] x86: acpi: 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 V6 2/4] x86: acpi: implement  arch_apei_get_mem_attributes() Matt Fleming <matt@codeblueprint.co.uk> - 2015-07-22 14:10 +0200
    Re: [PATCH V6 2/4] x86: acpi: implement  arch_apei_get_mem_attributes() "Zhang, Jonathan Zhixiong" <zjzhang@codeaurora.org> - 2015-07-22 19:30 +0200

#1189852 — Re: [PATCH V6 2/4] x86: acpi: implement arch_apei_get_mem_attributes()

On Mon, 20 Jul, at 05:32:37PM, Jonathan (Zhixiong) Zhang wrote:
> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
> 
> ... to allow arch specific implementation of getting page
> protection type associated with a physical address.
> 
> If the physical address has memory attributes defined by EFI
> memmap as EFI_MEMORY_UC, the page protection type is
> PAGE_KENERL_NOCACHE. Otherwise, the page protection type is
> PAGE_KERNEL.
> 
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> ---
>  arch/x86/kernel/acpi/apei.c | 10 ++++++++++
>  include/acpi/apei.h         |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/arch/x86/kernel/acpi/apei.c b/arch/x86/kernel/acpi/apei.c
> index c280df6b2aa2..9c6b3c8d81e4 100644
> --- a/arch/x86/kernel/acpi/apei.c
> +++ b/arch/x86/kernel/acpi/apei.c
> @@ -14,6 +14,8 @@
>  
>  #include <acpi/apei.h>
>  
> +#include <linux/efi.h>
> +
>  #include <asm/mce.h>
>  #include <asm/tlbflush.h>
>  
> @@ -60,3 +62,11 @@ void arch_apei_flush_tlb_one(unsigned long addr)
>  {
>  	__flush_tlb_one(addr);
>  }
> +
> +pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
> +{
> +	if (efi_mem_attributes(addr) & EFI_MEMORY_UC)
> +		return PAGE_KERNEL_NOCACHE;
> +
> +	return PAGE_KERNEL;
> +}

Like I mentioned before, this is theoretically racey because depending
on when you call arch_apei_get_mem_attribute() during boot, you'll
potentially return a different protection for the *same* memory region.
This is because on x86 we discard the EFI memory map in
efi_free_boot_services(), after which time efi_mem_attributes() will
always return 0.

Now, hitting that race would depend on a number of things but most
importantly it would require the region of RAM containing the Hardware
Error data to have EFI_MEMORY_UC set in the EFI memmap. For x86 I think
it's fair to say that's extremely unlikely given our cache coherency
architecture.

Also, as Will noted for arm64, this really wants to be static inline.
I'm still hoping the x86/ACPI folks will chime in on this patch.

For x86 we don't need to perform this lookup today for GHES so I would
just always return PAGE_KERNEL but include a comment explaining that
doing anything else is unneeded. Something like this?

---

static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
{

	/*
	 * We currently have no way to lookup the EFI memory map
	 * attributes for a region in a consistent way because the memap
	 * is discarded after efi_free_boot_services(). So if you call
	 * efi_mem_attributes() during boot and at runtime you could
	 * theoretically see different attributes.
	 *
	 * Since we've yet to see any x86 platforms that require
	 * anything other than PAGE_KERNEL (some arm64 platforms require
	 * the equivalent of PAGE_KERNEL_NOCACHE), return that until we
	 * know different.
	 */

	return PAGE_KERNEL;
}

-- 
Matt Fleming, Intel Open Source Technology Center
--
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]

#1190121

Thank you Matt for the great feedback.

On 7/22/2015 5:09 AM, Matt Fleming wrote:
> On Mon, 20 Jul, at 05:32:37PM, Jonathan (Zhixiong) Zhang wrote:
>> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
>>
>> ... to allow arch specific implementation of getting page
>> protection type associated with a physical address.
>>
>> If the physical address has memory attributes defined by EFI
>> memmap as EFI_MEMORY_UC, the page protection type is
>> PAGE_KENERL_NOCACHE. Otherwise, the page protection type is
>> PAGE_KERNEL.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> ---
>>   arch/x86/kernel/acpi/apei.c | 10 ++++++++++
>>   include/acpi/apei.h         |  1 +
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/arch/x86/kernel/acpi/apei.c b/arch/x86/kernel/acpi/apei.c
>> index c280df6b2aa2..9c6b3c8d81e4 100644
>> --- a/arch/x86/kernel/acpi/apei.c
>> +++ b/arch/x86/kernel/acpi/apei.c
>> @@ -14,6 +14,8 @@
>>
>>   #include <acpi/apei.h>
>>
>> +#include <linux/efi.h>
>> +
>>   #include <asm/mce.h>
>>   #include <asm/tlbflush.h>
>>
>> @@ -60,3 +62,11 @@ void arch_apei_flush_tlb_one(unsigned long addr)
>>   {
>>   	__flush_tlb_one(addr);
>>   }
>> +
>> +pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>> +{
>> +	if (efi_mem_attributes(addr) & EFI_MEMORY_UC)
>> +		return PAGE_KERNEL_NOCACHE;
>> +
>> +	return PAGE_KERNEL;
>> +}
>
> Like I mentioned before, this is theoretically racey because depending
> on when you call arch_apei_get_mem_attribute() during boot, you'll
> potentially return a different protection for the *same* memory region.
> This is because on x86 we discard the EFI memory map in
> efi_free_boot_services(), after which time efi_mem_attributes() will
> always return 0.
>
> Now, hitting that race would depend on a number of things but most
> importantly it would require the region of RAM containing the Hardware
> Error data to have EFI_MEMORY_UC set in the EFI memmap. For x86 I think
> it's fair to say that's extremely unlikely given our cache coherency
> architecture.
>
> Also, as Will noted for arm64, this really wants to be static inline.
Yes, will do.
> I'm still hoping the x86/ACPI folks will chime in on this patch.
Same here.
>
> For x86 we don't need to perform this lookup today for GHES so I would
> just always return PAGE_KERNEL but include a comment explaining that
> doing anything else is unneeded. Something like this?
The analysis and comments added make total sense to me. Will do so in V8
of the patch set after Will's feedback on v7.
>
> ---
>
> static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
> {
>
> 	/*
> 	 * We currently have no way to lookup the EFI memory map
> 	 * attributes for a region in a consistent way because the memap
> 	 * is discarded after efi_free_boot_services(). So if you call
> 	 * efi_mem_attributes() during boot and at runtime you could
> 	 * theoretically see different attributes.
> 	 *
> 	 * Since we've yet to see any x86 platforms that require
> 	 * anything other than PAGE_KERNEL (some arm64 platforms require
> 	 * the equivalent of PAGE_KERNEL_NOCACHE), return that until we
> 	 * know different.
> 	 */
>
> 	return PAGE_KERNEL;
> }
>

-- 
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