* [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data}
@ 2022-01-12 8:45 Jan Beulich
2022-01-12 13:02 ` Luca Fancellu
2022-01-27 10:45 ` Julien Grall
0 siblings, 2 replies; 5+ messages in thread
From: Jan Beulich @ 2022-01-12 8:45 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Sergey Temerkhanov
From: Sergey Temerkhanov <s.temerkhanov@gmail.com>
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>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v4: Drop EFI_MEMORY_CACHEABILITY_MASK. Fold with pre-existing if() (into
switch()). Style.
---
I guess "map_bs" would also want honoring in efi_exit_boot(), but that's
yet another patch then I suppose.
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1094,7 +1094,13 @@ static void __init efi_exit_boot(EFI_HAN
{
EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
- if ( desc->Attribute & EFI_MEMORY_RUNTIME )
+ /*
+ * Runtime services regions 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;
@@ -1545,13 +1551,36 @@ void __init efi_init_memory(void)
ROUNDUP(desc->PhysicalStart + len, PAGE_SIZE));
}
- if ( !efi_enabled(EFI_RS) ||
- (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
- (!map_bs ||
- (desc->Type != EfiBootServicesCode &&
- desc->Type != EfiBootServicesData))) )
+ if ( !efi_enabled(EFI_RS) )
continue;
+ if ( !(desc->Attribute & EFI_MEMORY_RUNTIME) )
+ {
+ switch ( desc->Type )
+ {
+ default:
+ continue;
+
+ /*
+ * Adjust runtime services regions. Keep in sync with
+ * efi_exit_boot().
+ */
+ case EfiRuntimeServicesCode:
+ case EfiRuntimeServicesData:
+ printk(XENLOG_WARNING
+ "Setting RUNTIME attribute for %013" PRIx64 "-%013" PRIx64 "\n",
+ desc->PhysicalStart, desc->PhysicalStart + len - 1);
+ desc->Attribute |= EFI_MEMORY_RUNTIME;
+ break;
+
+ case EfiBootServicesCode:
+ case EfiBootServicesData:
+ if ( !map_bs )
+ continue;
+ break;
+ }
+ }
+
desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
smfn = PFN_DOWN(desc->PhysicalStart);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data}
2022-01-12 8:45 [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data} Jan Beulich
@ 2022-01-12 13:02 ` Luca Fancellu
2022-01-27 10:45 ` Julien Grall
1 sibling, 0 replies; 5+ messages in thread
From: Luca Fancellu @ 2022-01-12 13:02 UTC (permalink / raw)
To: Jan Beulich, Sergey Temerkhanov
Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, Wei Liu
> On 12 Jan 2022, at 08:45, Jan Beulich <jbeulich@suse.com> wrote:
>
> From: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>
> 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>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Hi,
I’ve tested this patch on an arm machine with UEFI boot and it works fine.
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>
Cheers,
Luca
> ---
> v4: Drop EFI_MEMORY_CACHEABILITY_MASK. Fold with pre-existing if() (into
> switch()). Style.
> ---
> I guess "map_bs" would also want honoring in efi_exit_boot(), but that's
> yet another patch then I suppose.
>
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1094,7 +1094,13 @@ static void __init efi_exit_boot(EFI_HAN
> {
> EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>
> - if ( desc->Attribute & EFI_MEMORY_RUNTIME )
> + /*
> + * Runtime services regions 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;
> @@ -1545,13 +1551,36 @@ void __init efi_init_memory(void)
> ROUNDUP(desc->PhysicalStart + len, PAGE_SIZE));
> }
>
> - if ( !efi_enabled(EFI_RS) ||
> - (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> - (!map_bs ||
> - (desc->Type != EfiBootServicesCode &&
> - desc->Type != EfiBootServicesData))) )
> + if ( !efi_enabled(EFI_RS) )
> continue;
>
> + if ( !(desc->Attribute & EFI_MEMORY_RUNTIME) )
> + {
> + switch ( desc->Type )
> + {
> + default:
> + continue;
> +
> + /*
> + * Adjust runtime services regions. Keep in sync with
> + * efi_exit_boot().
> + */
> + case EfiRuntimeServicesCode:
> + case EfiRuntimeServicesData:
> + printk(XENLOG_WARNING
> + "Setting RUNTIME attribute for %013" PRIx64 "-%013" PRIx64 "\n",
> + desc->PhysicalStart, desc->PhysicalStart + len - 1);
> + desc->Attribute |= EFI_MEMORY_RUNTIME;
> + break;
> +
> + case EfiBootServicesCode:
> + case EfiBootServicesData:
> + if ( !map_bs )
> + continue;
> + break;
> + }
> + }
> +
> desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
>
> smfn = PFN_DOWN(desc->PhysicalStart);
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data}
2022-01-12 8:45 [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data} Jan Beulich
2022-01-12 13:02 ` Luca Fancellu
@ 2022-01-27 10:45 ` Julien Grall
2022-01-27 12:25 ` Jan Beulich
1 sibling, 1 reply; 5+ messages in thread
From: Julien Grall @ 2022-01-27 10:45 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu,
Sergey Temerkhanov
Hi Jan,
On 12/01/2022 08:45, Jan Beulich wrote:
> From: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>
> This helps overcome problems observed with some UEFI implementations
Would it be possible to provide some details about the platform? This is
helpful to track why a workaround is present.
> which don't set the Attributes field in memery descriptors properly.
typo: s/memery/memory/
>
> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v4: Drop EFI_MEMORY_CACHEABILITY_MASK. Fold with pre-existing if() (into
> switch()). Style.
> ---
> I guess "map_bs" would also want honoring in efi_exit_boot(), but that's
> yet another patch then I suppose.
IIUC, we would need to invalidate the mapping when map_bs was used. Is
it correct? If so, then I agree this sounds separate to the issue you
are describing here.
>
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1094,7 +1094,13 @@ static void __init efi_exit_boot(EFI_HAN
> {
> EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>
> - if ( desc->Attribute & EFI_MEMORY_RUNTIME )
> + /*
> + * Runtime services regions are always mapped here.
> + * Attributes may be adjusted in efi_init_memory().
> + */
> + if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
> + desc->Type == EfiRuntimeServicesCode ||
> + desc->Type == EfiRuntimeServicesData )
My knowledge with the stub is limited. Would you be able to explain why
we need to map the runtime services even with !efi_enabled(EFI_RS)?
The reason I am asking is if we don't need it, then it would be
sufficient to rely on desc->Attribute because you updated it in
efi_init_memory().
> desc->VirtualStart = desc->PhysicalStart;
> else
> desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
> @@ -1545,13 +1551,36 @@ void __init efi_init_memory(void)
> ROUNDUP(desc->PhysicalStart + len, PAGE_SIZE));
> }
>
> - if ( !efi_enabled(EFI_RS) ||
> - (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> - (!map_bs ||
> - (desc->Type != EfiBootServicesCode &&
> - desc->Type != EfiBootServicesData))) )
> + if ( !efi_enabled(EFI_RS) )
> continue;
>
> + if ( !(desc->Attribute & EFI_MEMORY_RUNTIME) )
> + {
> + switch ( desc->Type )
> + {
> + default:
> + continue;
> +
> + /*
> + * Adjust runtime services regions. Keep in sync with
> + * efi_exit_boot().
> + */
> + case EfiRuntimeServicesCode:
> + case EfiRuntimeServicesData:
> + printk(XENLOG_WARNING
> + "Setting RUNTIME attribute for %013" PRIx64 "-%013" PRIx64 "\n",
> + desc->PhysicalStart, desc->PhysicalStart + len - 1);
> + desc->Attribute |= EFI_MEMORY_RUNTIME;
> + break;
> +
> + case EfiBootServicesCode:
> + case EfiBootServicesData:
> + if ( !map_bs )
> + continue;
> + break;
> + }
> + }
The new code is much easier to read :).
> +
> desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
>
> smfn = PFN_DOWN(desc->PhysicalStart);
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data}
2022-01-27 10:45 ` Julien Grall
@ 2022-01-27 12:25 ` Jan Beulich
2022-01-31 19:03 ` Julien Grall
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2022-01-27 12:25 UTC (permalink / raw)
To: Julien Grall
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu,
Sergey Temerkhanov, xen-devel
On 27.01.2022 11:45, Julien Grall wrote:
> On 12/01/2022 08:45, Jan Beulich wrote:
>> From: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>
>> This helps overcome problems observed with some UEFI implementations
>
> Would it be possible to provide some details about the platform? This is
> helpful to track why a workaround is present.
I cannot provide any details. I took over the patch after pinging
Sergey more than once, without any reaction. It was him to actually
run into the problem in the wild.
>> which don't set the Attributes field in memery descriptors properly.
>
> typo: s/memery/memory/
>
>>
>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v4: Drop EFI_MEMORY_CACHEABILITY_MASK. Fold with pre-existing if() (into
>> switch()). Style.
>> ---
>> I guess "map_bs" would also want honoring in efi_exit_boot(), but that's
>> yet another patch then I suppose.
>
> IIUC, we would need to invalidate the mapping when map_bs was used. Is
> it correct? If so, then I agree this sounds separate to the issue you
> are describing here.
No, the other way around: We'd also need to put in a valid virtual
address there for EfiBootServices{Code,Data}. I expect this wasn't
done when map_bs was introduced because iirc at that time the code
fragment in efi_exit_boot() was entirely dead, present merely for
documentation purposes.
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1094,7 +1094,13 @@ static void __init efi_exit_boot(EFI_HAN
>> {
>> EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>>
>> - if ( desc->Attribute & EFI_MEMORY_RUNTIME )
>> + /*
>> + * Runtime services regions are always mapped here.
>> + * Attributes may be adjusted in efi_init_memory().
>> + */
>> + if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
>> + desc->Type == EfiRuntimeServicesCode ||
>> + desc->Type == EfiRuntimeServicesData )
>
> My knowledge with the stub is limited. Would you be able to explain why
> we need to map the runtime services even with !efi_enabled(EFI_RS)?
In principle we wouldn't need to, but the final setting of this bit
isn't known yet at this point - it'll be known only after parsing
the command line (see parse_efi_param()).
Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data}
2022-01-27 12:25 ` Jan Beulich
@ 2022-01-31 19:03 ` Julien Grall
0 siblings, 0 replies; 5+ messages in thread
From: Julien Grall @ 2022-01-31 19:03 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu,
Sergey Temerkhanov, xen-devel
Hi Jan,
On 27/01/2022 12:25, Jan Beulich wrote:
> On 27.01.2022 11:45, Julien Grall wrote:
>> On 12/01/2022 08:45, Jan Beulich wrote:
>>> From: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>>
>>> This helps overcome problems observed with some UEFI implementations
>>
>> Would it be possible to provide some details about the platform? This is
>> helpful to track why a workaround is present.
>
> I cannot provide any details. I took over the patch after pinging
> Sergey more than once, without any reaction. It was him to actually
> run into the problem in the wild.
This is a quite unfortunate.
>
>>> which don't set the Attributes field in memery descriptors properly.
>>
>> typo: s/memery/memory/
>>
>>>
>>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> v4: Drop EFI_MEMORY_CACHEABILITY_MASK. Fold with pre-existing if() (into
>>> switch()). Style.
>>> ---
>>> I guess "map_bs" would also want honoring in efi_exit_boot(), but that's
>>> yet another patch then I suppose.
>>
>> IIUC, we would need to invalidate the mapping when map_bs was used. Is
>> it correct? If so, then I agree this sounds separate to the issue you
>> are describing here.
>
> No, the other way around: We'd also need to put in a valid virtual
> address there for EfiBootServices{Code,Data}. I expect this wasn't
> done when map_bs was introduced because iirc at that time the code
> fragment in efi_exit_boot() was entirely dead, present merely for
> documentation purposes.
>
>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -1094,7 +1094,13 @@ static void __init efi_exit_boot(EFI_HAN
>>> {
>>> EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>>>
>>> - if ( desc->Attribute & EFI_MEMORY_RUNTIME )
>>> + /*
>>> + * Runtime services regions are always mapped here.
>>> + * Attributes may be adjusted in efi_init_memory().
>>> + */
>>> + if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
>>> + desc->Type == EfiRuntimeServicesCode ||
>>> + desc->Type == EfiRuntimeServicesData )
>>
>> My knowledge with the stub is limited. Would you be able to explain why
>> we need to map the runtime services even with !efi_enabled(EFI_RS)?
>
> In principle we wouldn't need to, but the final setting of this bit
> isn't known yet at this point - it'll be known only after parsing
> the command line (see parse_efi_param()).
Oh. I thought efi_exit_boot() was called *after* efi_init_memory(). Your
answer about 'map_bs' also make more sense.
Thanks for the explanation!
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-31 19:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-12 8:45 [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data} Jan Beulich
2022-01-12 13:02 ` Luca Fancellu
2022-01-27 10:45 ` Julien Grall
2022-01-27 12:25 ` Jan Beulich
2022-01-31 19:03 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.