Mailing List Archive: [PATCH v3] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData

[PATCH v3] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData

Sep 11, 2020, 7:43 AM

Post #1 of 3 (45 views)

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

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

Re: [PATCH v3] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData [ In reply to ]

jbeulich at suse

Sep 14, 2020, 6:30 AM

Post #2 of 3 (44 views)

Permalink

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

Re: [PATCH v3] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData [ In reply to ]

jbeulich at suse

Sep 28, 2020, 1:25 AM

Post #3 of 3 (44 views)

Permalink

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