| Message ID | 20200911144309.4559-1-s.temerkhanov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData | expand |
On 11.09.2020 16:43, Sergey Temerkhanov wrote: > @@ -1510,6 +1517,24 @@ void __init efi_init_memory(void) > desc->PhysicalStart, desc->PhysicalStart + len - 1, > desc->Type, desc->Attribute); > > + /* > + * EfiRuntimeServicesCode and EfiRuntimeServicesData > + * memory ranges are adjusted here. Any changes > + * or adjustments must be kept in sync with efi_exit_boot() > + */ > + if ( efi_enabled(EFI_RS) && > + (!(desc->Attribute & EFI_MEMORY_RUNTIME) && > + (desc->Attribute & EFI_MEMORY_CACHEABILITY_MASK) && > + (desc->Type == EfiRuntimeServicesCode || > + desc->Type == EfiRuntimeServicesData)) ) > + { > + printk(XENLOG_WARNING > + "Setting EFI_RUNTIME memory attribute for area %013" > + PRIx64 "-%013" PRIx64 "\n", > + desc->PhysicalStart, desc->PhysicalStart + len - 1); > + desc->Attribute |= EFI_MEMORY_RUNTIME; > + } So you've moved from always checking for EFI_MEMORY_WP to not checking it at all. Neither is the way to go imo. Similarly, ... > --- a/xen/include/efi/efidef.h > +++ b/xen/include/efi/efidef.h > @@ -158,6 +158,12 @@ typedef enum { > #define EFI_MEMORY_UCE 0x0000000000000010 > #define EFI_MEMORY_WP 0x0000000000001000 > > +#define EFI_MEMORY_CACHEABILITY_MASK ( EFI_MEMORY_UC | \ > + EFI_MEMORY_WC | \ > + EFI_MEMORY_WT | \ > + EFI_MEMORY_WB | \ > + EFI_MEMORY_UCE ) ... this now doesn't really cover what its name suggests. As indicated before, without such a (questionable) #define having appeared in the gnu-efi tree, I don't think we want it, at the very least not in this imported header. But given that it doesn't express what you want anyway, I can only repeat my suggestion to drop this #define altogether. In order to save further rounds, I would offer to finish this patch to a shape that I'd feel comfortable with - if that's okay with you. Jan
On 14.09.2020 15:30, Jan Beulich wrote: > On 11.09.2020 16:43, Sergey Temerkhanov wrote: >> @@ -1510,6 +1517,24 @@ void __init efi_init_memory(void) >> desc->PhysicalStart, desc->PhysicalStart + len - 1, >> desc->Type, desc->Attribute); >> >> + /* >> + * EfiRuntimeServicesCode and EfiRuntimeServicesData >> + * memory ranges are adjusted here. Any changes >> + * or adjustments must be kept in sync with efi_exit_boot() >> + */ >> + if ( efi_enabled(EFI_RS) && >> + (!(desc->Attribute & EFI_MEMORY_RUNTIME) && >> + (desc->Attribute & EFI_MEMORY_CACHEABILITY_MASK) && >> + (desc->Type == EfiRuntimeServicesCode || >> + desc->Type == EfiRuntimeServicesData)) ) >> + { >> + printk(XENLOG_WARNING >> + "Setting EFI_RUNTIME memory attribute for area %013" >> + PRIx64 "-%013" PRIx64 "\n", >> + desc->PhysicalStart, desc->PhysicalStart + len - 1); >> + desc->Attribute |= EFI_MEMORY_RUNTIME; >> + } > > So you've moved from always checking for EFI_MEMORY_WP to not > checking it at all. Neither is the way to go imo. Similarly, ... > >> --- a/xen/include/efi/efidef.h >> +++ b/xen/include/efi/efidef.h >> @@ -158,6 +158,12 @@ typedef enum { >> #define EFI_MEMORY_UCE 0x0000000000000010 >> #define EFI_MEMORY_WP 0x0000000000001000 >> >> +#define EFI_MEMORY_CACHEABILITY_MASK ( EFI_MEMORY_UC | \ >> + EFI_MEMORY_WC | \ >> + EFI_MEMORY_WT | \ >> + EFI_MEMORY_WB | \ >> + EFI_MEMORY_UCE ) > > ... this now doesn't really cover what its name suggests. As > indicated before, without such a (questionable) #define having > appeared in the gnu-efi tree, I don't think we want it, at the > very least not in this imported header. But given that it > doesn't express what you want anyway, I can only repeat my > suggestion to drop this #define altogether. > > In order to save further rounds, I would offer to finish this > patch to a shape that I'd feel comfortable with - if that's > okay with you. Would you mind clarifying whether you intend to finish bringing this change in shape, or whether I should pick it up? Jan
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 5a520bf21d..811d8a0923 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -1100,7 +1100,14 @@ 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 ) + /* + * EfiRuntimeServicesCode and EfiRuntimeServicesData + * memory ranges are always mapped here. + * Attributes may be adjusted in efi_init_memory() + */ + if ( (desc->Attribute & EFI_MEMORY_RUNTIME) || + desc->Type == EfiRuntimeServicesCode || + desc->Type == EfiRuntimeServicesData ) desc->VirtualStart = desc->PhysicalStart; else desc->VirtualStart = INVALID_VIRTUAL_ADDRESS; @@ -1510,6 +1517,24 @@ void __init efi_init_memory(void) desc->PhysicalStart, desc->PhysicalStart + len - 1, desc->Type, desc->Attribute); + /* + * EfiRuntimeServicesCode and EfiRuntimeServicesData + * memory ranges are adjusted here. Any changes + * or adjustments must be kept in sync with efi_exit_boot() + */ + if ( efi_enabled(EFI_RS) && + (!(desc->Attribute & EFI_MEMORY_RUNTIME) && + (desc->Attribute & EFI_MEMORY_CACHEABILITY_MASK) && + (desc->Type == EfiRuntimeServicesCode || + desc->Type == EfiRuntimeServicesData)) ) + { + printk(XENLOG_WARNING + "Setting EFI_RUNTIME memory attribute for area %013" + PRIx64 "-%013" PRIx64 "\n", + desc->PhysicalStart, desc->PhysicalStart + len - 1); + desc->Attribute |= EFI_MEMORY_RUNTIME; + } + 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..3a35bfc8be 100644 --- a/xen/include/efi/efidef.h +++ b/xen/include/efi/efidef.h @@ -158,6 +158,12 @@ typedef enum { #define EFI_MEMORY_UCE 0x0000000000000010 #define EFI_MEMORY_WP 0x0000000000001000 +#define EFI_MEMORY_CACHEABILITY_MASK ( EFI_MEMORY_UC | \ + EFI_MEMORY_WC | \ + EFI_MEMORY_WT | \ + EFI_MEMORY_WB | \ + EFI_MEMORY_UCE ) + // 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 | 27 ++++++++++++++++++++++++++- xen/include/efi/efidef.h | 6 ++++++ 2 files changed, 32 insertions(+), 1 deletion(-)