| Message ID | 20200904211140.9875-1-s.temerkhanov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData | expand |
On 04.09.2020 23:11, Sergey Temerkhanov wrote: > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -1100,7 +1100,9 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste > { > EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i; > > - if ( desc->Attribute & EFI_MEMORY_RUNTIME ) > + if ( (desc->Attribute & EFI_MEMORY_RUNTIME) || > + desc->Type == EfiRuntimeServicesCode || Nit: One too many blank at the start of the line and one too many ahead of the ||. (Similar issues elsewhere.) I think with the more involved logic this may also want a comment indicating it needs keeping in sync with the other place (and a similar comment should then go there as well). > @@ -1510,6 +1512,21 @@ void __init efi_init_memory(void) > desc->PhysicalStart, desc->PhysicalStart + len - 1, > desc->Type, desc->Attribute); > > + if (efi_enabled(EFI_RS) && > + (!(desc->Attribute & EFI_MEMORY_RUNTIME) && > + (desc->Type == EfiRuntimeServicesCode || > + desc->Type == EfiRuntimeServicesData))) { > + printk(XENLOG_WARNING "Fixing memory attributes for area %013" > + PRIx64 "-%013" PRIx64 "\n", > + desc->PhysicalStart, desc->PhysicalStart + len - 1); Please be more specific with the wording, e.g. "Adding runtime attribute for area ...". Also please wrap the statement such that the format string ends up all on one line, even if the line then exceeds the 80 cols limit. For this you'll want to split between XENLOG_WARNING and the opening quote. > + desc->Attribute |= EFI_MEMORY_RUNTIME; > + if ( !(desc->Attribute & EFI_MEMORY_CACHEABILITY_MASK) ) { > + desc->Attribute |= (desc->Type == EfiRuntimeServicesCode) && > + (efi_bs_revision >= EFI_REVISION(2, 5)) ? > + EFI_MEMORY_WP : EFI_MEMORY_UC; As said elsewhere already, please don't do this as a "proactive" workaround. Also please note that opening braces go on their own lines in our coding style and there ought to be blanks immediately inside if() and alike. See the surrounding code for reference. > --- a/xen/include/efi/efidef.h > +++ b/xen/include/efi/efidef.h > @@ -158,6 +158,9 @@ typedef enum { > #define EFI_MEMORY_UCE 0x0000000000000010 > #define EFI_MEMORY_WP 0x0000000000001000 > > +#define EFI_MEMORY_CACHEABILITY_MASK \ > + 0x000000000000101F Has such a #define appeared in the tree we've imported this from? If not, it probably shouldn't go here. I'm also unconvinced checking the bit EFI_MEMORY_WP occupies is, for the purpose here, legitimate on versions prior to 2.5 - it's a reserved bit there, and hence may have any [benign] meaning. Finally, if this #define is to survive in the first place, please don't use a literal number. Make its value an OR of the various other constants instead, so it becomes self-documenting. Jan
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 5a520bf21d..05cfbf4de0 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -1100,7 +1100,9 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste { EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i; - if ( desc->Attribute & EFI_MEMORY_RUNTIME ) + if ( (desc->Attribute & EFI_MEMORY_RUNTIME) || + desc->Type == EfiRuntimeServicesCode || + desc->Type == EfiRuntimeServicesData ) desc->VirtualStart = desc->PhysicalStart; else desc->VirtualStart = INVALID_VIRTUAL_ADDRESS; @@ -1510,6 +1512,21 @@ void __init efi_init_memory(void) desc->PhysicalStart, desc->PhysicalStart + len - 1, desc->Type, desc->Attribute); + if (efi_enabled(EFI_RS) && + (!(desc->Attribute & EFI_MEMORY_RUNTIME) && + (desc->Type == EfiRuntimeServicesCode || + desc->Type == EfiRuntimeServicesData))) { + printk(XENLOG_WARNING "Fixing memory attributes for area %013" + PRIx64 "-%013" PRIx64 "\n", + desc->PhysicalStart, desc->PhysicalStart + len - 1); + desc->Attribute |= EFI_MEMORY_RUNTIME; + if ( !(desc->Attribute & EFI_MEMORY_CACHEABILITY_MASK) ) { + desc->Attribute |= (desc->Type == EfiRuntimeServicesCode) && + (efi_bs_revision >= EFI_REVISION(2, 5)) ? + EFI_MEMORY_WP : EFI_MEMORY_UC; + } + } + if ( (desc->Attribute & (EFI_MEMORY_WB | EFI_MEMORY_WT)) || (efi_bs_revision >= EFI_REVISION(2, 5) && (desc->Attribute & EFI_MEMORY_WP)) ) diff --git a/xen/include/efi/efidef.h b/xen/include/efi/efidef.h index 86a7e111bf..05170da2db 100644 --- a/xen/include/efi/efidef.h +++ b/xen/include/efi/efidef.h @@ -158,6 +158,9 @@ typedef enum { #define EFI_MEMORY_UCE 0x0000000000000010 #define EFI_MEMORY_WP 0x0000000000001000 +#define EFI_MEMORY_CACHEABILITY_MASK \ + 0x000000000000101F + // physical memory protection on range #define EFI_MEMORY_RP 0x0000000000002000 #define EFI_MEMORY_XP 0x0000000000004000
This helps overcome problems observed with some UEFI implementations which don't set the Attributes field in memery descriptors properly Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com> --- xen/common/efi/boot.c | 19 ++++++++++++++++++- xen/include/efi/efidef.h | 3 +++ 2 files changed, 21 insertions(+), 1 deletion(-)