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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.02.2020 18:21, Tamas K Lengyel wrote:
> On Fri, Feb 7, 2020 at 10:16 AM Tamas K Lengyel <tamas@xxxxxxxxxxxxx> wrote:
>>
>> 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.
> 
> One thing to add though regarding using p2m_access_rw here is that in
> case something would trigger an X violation it would lead to an event
> being sent to a vm_event subscriber, which they may not be able to
> make sense of.

Hmm, good point.

> So I would suggest that if you want to make this
> pagetable entry R/W only to use a p2m_type for that instead of a
> p2m_access.

This would then take a further type derived from p2m_mmio_direct,
with its driving moved to the hypercall interface (as Xen can't
tell which one is which, e.g. ROM vs "ordinary" MMIO, _except_ in
this special case here). So I guess the patch as is looks to be
the better alternative overall.

Jan

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