Re: [Xen-devel] [PATCH v3 2/2] xen/efi: optionally call SetVirtualAddressMap()

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Oct 23, 2019 at 05:37:33PM +0200, Jan Beulich wrote:
> On 13.10.2019 00:11, Marek Marczykowski-Górecki  wrote:
> > Some UEFI implementations are not happy about running in 1:1 addressing,
> > but really virtual address space.
> 
> I have to admit that I find this misleading. There's no true "physical
> mode" on x86-64 anyway. What I assume happens is that people abuse the
> address map change notification to do things beyond the necessary
> ConvertPointer(() calls.

That would indeed match the behaviour. I'll add it to commit message.

> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -88,6 +88,19 @@ config KEXEC
> >  
> >       If unsure, say Y.
> >  
> > +config SET_VIRTUAL_ADDRESS_MAP
> 
> I'm of the strong opinion that this wants to have an EFI_ prefix.

Ok.

> > +    bool "EFI: call SetVirtualAddressMap()" if EXPERT = "y"
> > +    default n
> 
> I don't think you need this line.
> 
> > @@ -1094,6 +1100,26 @@ static void __init efi_exit_boot(EFI_HANDLE 
> > ImageHandle, EFI_SYSTEM_TABLE *Syste
> >      if ( EFI_ERROR(status) )
> >          PrintErrMesg(L"Cannot exit boot services", status);
> >  
> > +#ifdef CONFIG_SET_VIRTUAL_ADDRESS_MAP
> > +    for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> > +    {
> > +        EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> > +
> > +        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
> > +            desc->VirtualStart = desc->PhysicalStart;
> > +        else
> > +            desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
> > +    }
> > +    status = efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size,
> > +                                          mdesc_ver, efi_memmap);
> > +    if ( status != EFI_SUCCESS )
> > +    {
> > +        printk(XENLOG_ERR "EFI: SetVirtualAddressMap() failed (%#lx), 
> > disabling runtime services\n",
> > +               status);
> > +        __clear_bit(EFI_RS, &efi_flags);
> > +    }
> > +#endif
> 
> This new placement undermines (or at least complicates afaict) the
> original intention to allow picking virtual addresses which don't
> match the directmap.

If I read it right, the original intention was to specifically use
directmap, not some other virtual addresses. Which is flawed, because
directmap is mapped with NX, so at least EfiRuntimeServicesCode will
break. This means, even when using directmap, Xen would need to switch
page tables for the runtime call time to allow executing that code.

There is of course an option to rewrite it completely differently,
mapping EFI runtime regions somewhere else (not 1:1 and not re-use
directmap). But I don't think it worth the effort, and also is definitely
too complex this far in 4.13 release cycle.

> I can accept this as an intended tradeoff (as
> you validly mention in the other patch we don't honor the 1:1 map
> requirement at the time of the call with its original placement),
> but it should be mentioned in one of the two patch descriptions.

This is one of the reason why I've decided to split this change into two
parts - remove the old one and add the new one. It is really not "fixing
the old approach", but doing this very differently. I think this patch
description referencing old, never working and never even enabled
approach would be misleading at least. The patch removing the old
approach do list reasons why it was broken.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel