Re: [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data}

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>

Cheers,

--
Julien Grall