remove pa->va->pa conversion for efi.acpi
PATCH 1/1 remove pa->va->pa conversion for efi.acpi
David, Andy,
During ia64 kernel initialization, the physical address of the ACPI tables is passed via the EFI Configuration Table. In efi_init() the address is stored as a virtual address in the kernel's efi structure. Later when ACPI is initialized, these are converted back to physical addresses in acpi_find_rsdp() and acpi_os_get_root_pointer().
This patch (against 2.5.75) removes the macro to store the address as virtual and removes the macros doing the reverse conversion back to physical. As I'm currently working on a patch to add support for booting and initializing IA-32 kernels from EFI, this simplifies the changes to drivers/acpi/osl.c to also get the physical address. Fwiw, I was able to boot a slightly older 2.5.69 kernel on an Itanium II system w/o issue with this patch...
thanks,
matt
arch/ia64/kernel/acpi.c +1 -1
@@ -568,7 +568,7 @@unsigned long rsdp_phys =0 ;if (efi.acpi20) - rsdp_phys =__pa (efi.acpi20); + rsdp_phys = efi.acpi20;else if (efi.acpi) printk(KERN_WARNING PREFIX "v1.0/r0.71 tables no longer supported\n");return rsdp_phys;
arch/ia64/kernel/efi.c +2 -2
@@ -525,10 +525,10 @@ efi.mps =__va (config_tables[i].table);printk (" MPS=0x%lx" , config_tables[i].table); }else if (efi_guidcmp (config_tables[i].guid,ACPI_20_TABLE_GUID ) =0 ) { - efi.acpi20 =__va (config_tables[i].table); + efi.acpi20 = config_tables[i].table;printk (" ACPI 2.0=0x%lx" , config_tables[i].table); }else if (efi_guidcmp (config_tables[i].guid,ACPI_TABLE_GUID ) =0 ) { - efi.acpi =__va (config_tables[i].table); + efi.acpi = config_tables[i].table;printk (" ACPI=0x%lx" , config_tables[i].table); }else if (efi_guidcmp (config_tables[i].guid,SMBIOS_TABLE_GUID ) =0 ) { efi.smbios =__va (config_tables[i].table);
drivers/acpi/osl.c +2 -2
@@ -142,9 +142,9 @@#ifdef CONFIG_ACPI_EFI addr->pointer_type =ACPI_PHYSICAL_POINTER ;if (efi.acpi20) - addr->pointer.physical = (acpi_physical_address )virt_to_phys (efi.acpi20); + addr->pointer.physical = (acpi_physical_address ) efi.acpi20;else if (efi.acpi) - addr->pointer.physical = (acpi_physical_address )virt_to_phys (efi.acpi); + addr->pointer.physical = (acpi_physical_address ) efi.acpi;else { printk(KERN_ERR PREFIX "System description tables not found\n");return AE_NOT_FOUND ;
David,
Quoted 8 lines
I'm OK with the change in principle, but the patch as sent is unacceptable because it leaves "acpi" and "acpi20" declared as "void *"
(in struct efi"). If you want them to by physical addresses, this should be changed to "unsigned long" (or perhaps u32 for acpi and u64 for acpi20?). It would be good to rename the members as well (e.g., acpi_phys_addr/acpi20_phys_addr) to make sure that old code will break at compile-time, not at run-time.
Sounds good. I've changed these to unsigned long and added the phys_addr to the names...
Quoted 6 lines
Does ACPI really guarantee that the table is never stored at physical address 0? If not, then leaving it as a virtual address might be safer. (Yes, I know it's very unlikely for today's system, but at least on ia64, pfn 0 is normal RAM so it seems at least in principle possible to store the ACPI table there).
No guarantee that I could find...I suppose this is technically possible. :) However, considering the ACPI routines expect a physical address and thus immediately map it to get the descriptor (either directly with virt_to_phys or via ioremap), this seems redundant. Given the mapping scheme employed, is this still risky? I'd like to reuse the same code path for ia32, but don't want to break ia64.
thanks,
matt
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line · Matthew E
Does ACPI really guarantee that the table is never stored at physical address 0? If not, then leaving it as a virtual address might be safer. (Yes, I know it's very unlikely for today's system, but at least on ia64, pfn 0 is normal RAM so it seems at least in principle possible to store the ACPI table there).
Matthew> No guarantee that I could find...I suppose this is Matthew> technically possible. :) However, considering the ACPI Matthew> routines expect a physical address and thus immediately map Matthew> it to get the descriptor (either directly with virt_to_phys Matthew> or via ioremap), this seems redundant. Given the mapping Matthew> scheme employed, is this still risky? I'd like to reuse Matthew> the same code path for ia32, but don't want to break ia64.
Note that I'm only pointing this out because I thought there were some NULL-pointer checks. If it's a physical address, 0 is a valid address. If it's an (identity-mapped) kernel address, NULL-pointer checks are OK.
--david
Quoted 5 lines
Note that I'm only pointing this out because I thought there were some NULL-pointer checks. If it's a physical address, 0 is a valid address. If it's an (identity-mapped) kernel address, NULL-pointer checks are OK.
Doh! Yes, indeed. Thanks...
matt