Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2020 15:07, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 31 July 2020 13:58
>> To: Paul Durrant <paul@xxxxxxx>
>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Paul Durrant <pdurrant@xxxxxxxxxx>; 
>> Andrew Cooper
>> <andrew.cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau Monné 
>> <roger.pau@xxxxxxxxxx>
>> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in 
>> epte_get_entry_emt()
>>
>> On 31.07.2020 14:39, Paul Durrant wrote:
>>> From: Paul Durrant <pdurrant@xxxxxxxxxx>
>>>
>>> Re-factor the code to take advantage of the fact that the APIC access page 
>>> is
>>> a 'special' page.
>>
>> Hmm, that's going quite as far as I was thinking to go: In
>> particular, you leave in place the set_mmio_p2m_entry() use
>> in vmx_alloc_vlapic_mapping(). With that replaced, the
>> re-ordering in epte_get_entry_emt() that you do shouldn't
>> be necessary; you'd simple drop the checking of the
>> specific MFN.
> 
> Ok, it still needs to go in the p2m though so are you suggesting
> just calling p2m_set_entry() directly?

Yes, if this works. The main question really is whether there are
any hidden assumptions elsewhere that this page gets mapped as an
MMIO one.

>>> --- a/xen/arch/x86/hvm/mtrr.c
>>> +++ b/xen/arch/x86/hvm/mtrr.c
>>> @@ -814,29 +814,22 @@ int epte_get_entry_emt(struct domain *d, unsigned 
>>> long gfn, mfn_t mfn,
>>>          return -1;
>>>      }
>>>
>>> -    if ( direct_mmio )
>>> -    {
>>> -        if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> 
>>> order )
>>> -            return MTRR_TYPE_UNCACHABLE;
>>> -        if ( order )
>>> -            return -1;
>>
>> ... this part of the logic wants retaining, I think, i.e.
>> reporting back to the guest that the mapping needs splitting.
>> I'm afraid I have to withdraw my R-b on patch 1 for this
>> reason, as the check needs to be added there already.
> 
> To be clear... You mean I need the:
> 
> if ( order )
>   return -1;
> 
> there?

Not just - first of all you need to check whether the requested
range overlaps a special page.

Jan