Re: [Xen-devel] [PATCH] x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry()

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Feb 7, 2020 at 9:54 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 07.02.2020 10:52, Roger Pau Monné wrote:
> > On Fri, Feb 07, 2020 at 09:08:15AM +0100, Jan Beulich wrote:
> >> On 06.02.2020 16:20, Jan Beulich wrote:
> >>> --- a/xen/arch/x86/hvm/vmx/vmx.c
> >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >>> @@ -3037,9 +3037,8 @@ static int vmx_alloc_vlapic_mapping(stru
> >>>      share_xen_page_with_guest(pg, d, SHARE_rw);
> >>>      d->arch.hvm.vmx.apic_access_mfn = mfn;
> >>>
> >>> -    return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), 
> >>> mfn,
> >>> -                              PAGE_ORDER_4K,
> >>> -                              p2m_get_hostp2m(d)->default_access);
> >>> +    return set_mmio_p2m_entry(d, gaddr_to_gfn(APIC_DEFAULT_PHYS_BASE), 
> >>> mfn,
> >>> +                              PAGE_ORDER_4K);
> >>>  }
> >>
> >> Upon 2nd thought - does this really want to use default access?
> >> Execute permission for this page looks a little suspicious.
> >> Isn't it the case that this page doesn't (normally?) get
> >> accessed at all, and instead its address serves as an indicator
> >> to the CPU? (I even vaguely recall it having been considered to
> >> consolidate this, to e.g. a single page per domain.) In which
> >> case even p2m_access_n might be good enough?
> >
> > Hm, I think p2m_access_n is not enough, as that would trigger an EPT
> > fault which has preference over the APIC access VM exit (see 29.4.1
> > Priority of APIC-Access VM Exits).
>
> Ah, yes, reading that text I agree. Having just a single such page
> per domain would still seem possible, though. Although, if we meant
> to support a guest moving the APIC base address, then we couldn't,
> again.
>
> > I think setting it to p2m_access_rw should be enough, and we would get
> > EPT faults when trying to execute from APIC page.
>
> Then the other question is whether there's any use for introspection
> to further limit permissions on this (kind of fake) page. Tamas?

I'm not aware of any use-case that would restrict the EPT permission
for MMIO pages. That said, an application could still request that to
be set later on. Since this function here gets called in
vmx_domain_initialise I suspect a mem_access user didn't even have a
chance to change the default_access to anything custom so I venture it
would be safe to choose whatever permission is sensible. If anyone
wants to mess with the permission later they can do that regardless of
what was set here.

Tamas

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