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);
> 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); > >
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
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
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
© 2016 - 2024 Red Hat, Inc.