Skip to contentSkip to conversation

ACPI/APEI: correct/extend pgprot_t retrieval to map GHES memory

PATCH Matt Fleming 2 participants 2 messages Dec 29, 2015 – Jan 6, 2016 1 message not in the archive ACPI

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Matt Fleming

author
#1 lore raw
Quoted 4 lines · Jan Beulich

Commit 8ece249a81 ("acpi/apei: Use appropriate pgprot_t to map GHES memory") adjusted ghes_ioremap_pfn_irq() but left ghes_ioremap_pfn_nmi() alone, and while doing the adjustment it introduced an at least latent truncation issue on ix86 (using "unsigned long" for a physical address).

Good catch, but I don't think this is the correct fix. 'paddr' should be u64 because that's the data type used in ghes_copy_tofrom_phys(), and because the value is read from ACPI. The firmware dictates the data types.

Quoted 18 lines
Signed-off-by Jan Beulich <[email protected]>
Cc Jonathan (Zhixiong) Zhang <[email protected]>
Cc Matt Fleming <[email protected]>
Cc Ingo Molnar <[email protected]>
---
 drivers/acpi/apei/ghes.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/acpi/apei/ghes.c +2 -0
@@ -147,17 +147,23 @@ static void ghes_ioremap_exit(void)
 static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)
 {
 	unsigned long vaddr;
+	phys_addr_t paddr;
+	pgprot_t prot;
 
 	vaddr = (unsigned long)GHES_IOREMAP_NMI_PAGE(ghes_ioremap_area->addr);

drivers/acpi/apei/ghes.c static void ghes_ioremap_exit(void) · line 161

Quoted 10 lines
drivers/acpi/apei/ghes.c +5 -2
-	ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
-			   pfn << PAGE_SHIFT, PAGE_KERNEL);
+
+	paddr = pfn << PAGE_SHIFT;
+	prot = arch_apei_get_mem_attribute(paddr);
+
+	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);
 
 	return (void __iomem *)vaddr;
 }

Did you test this change? I didn't suggest fixing this function up originally because it isn't used on arm64.

#2 lore raw
Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 29.12.15 at 13:28, <[email protected]> wrote:
Quoted 4 lines · Jan Beulich

Commit 8ece249a81 ("acpi/apei: Use appropriate pgprot_t to map GHES memory") adjusted ghes_ioremap_pfn_irq() but left ghes_ioremap_pfn_nmi() alone, and while doing the adjustment it introduced an at least latent truncation issue on ix86 (using "unsigned long" for a physical address).

Good catch, but I don't think this is the correct fix. 'paddr' should be u64 because that's the data type used in ghes_copy_tofrom_phys(), and because the value is read from ACPI. The firmware dictates the data types.

Making paddr u64 won't be of any use when phys_addr_t is only 32 bits, since passing the value to arch_apei_get_mem_attribute() and ioremap_page_range() would still cause truncation.

Quoted 18 lines
Quoted 18 lines
drivers/acpi/apei/ghes.c +7 -2
@@ -147,17 +147,23 @@ static void ghes_ioremap_exit(void)
 static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)
 {
 	unsigned long vaddr;
+	phys_addr_t paddr;
+	pgprot_t prot;
 
 	vaddr = (unsigned long)GHES_IOREMAP_NMI_PAGE(ghes_ioremap_area->addr);
-	ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
-			   pfn << PAGE_SHIFT, PAGE_KERNEL);
+
+	paddr = pfn << PAGE_SHIFT;
+	prot = arch_apei_get_mem_attribute(paddr);
+
+	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);
Quoted 5 lines

drivers/acpi/apei/ghes.c static void ghes_ioremap_exit(void) · line 161

Quoted 2 lines
drivers/acpi/apei/ghes.c +0 -0
 	return (void __iomem *)vaddr;
 }

Did you test this change? I didn't suggest fixing this function up originally because it isn't used on arm64.

Do you see anything wrong here? I can't see the change do anything that could break things. And the function - used there or not - is being compiled for ARM64, and even from an abstract perspective it seems wrong to use arch_apei_get_mem_attribute() in its non-NMI counterpart but not here.

Jan