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

Paul Durrant posted 2 patches 7 weeks ago

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

Posted by Paul Durrant 7 weeks ago
From: Paul Durrant <pdurrant@amazon.com>

Re-factor the code to take advantage of the fact that the APIC access page is
a 'special' page.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/mtrr.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 3ad813ed15..0992f05e8f 100644
--- 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;
-        *ipat = 1;
-        return MTRR_TYPE_WRBACK;
-    }
-
     if ( !mfn_valid(mfn) )
     {
         *ipat = 1;
         return MTRR_TYPE_UNCACHABLE;
     }
 
-    if ( (!is_iommu_enabled(d) && !cache_flush_permitted(d)) ||
+    if ( (!direct_mmio && !is_iommu_enabled(d) && !cache_flush_permitted(d)) ||
          is_special_page(mfn_to_page(mfn)) )
     {
         *ipat = 1;
         return MTRR_TYPE_WRBACK;
     }
 
+    if ( direct_mmio )
+        return MTRR_TYPE_UNCACHABLE;
+
     gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, _gfn(gfn), order);
     if ( gmtrr_mtype >= 0 )
     {
-- 
2.20.1


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

Posted by Jan Beulich 7 weeks ago
On 31.07.2020 14:39, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> 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. However, ...

> --- 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.

Jan

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

Posted by Paul Durrant 7 weeks ago
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 31 July 2020 13:58
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> 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@amazon.com>
> >
> > 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?

> However, ...
> 
> > --- 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?

  Paul

> 
> Jan


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

Posted by Jan Beulich 7 weeks ago
On 31.07.2020 15:07, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 31 July 2020 13:58
>> To: Paul Durrant <paul@xen.org>
>> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
>> 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@amazon.com>
>>>
>>> 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

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

Posted by Paul Durrant 7 weeks ago
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 31 July 2020 14:41
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Roger Pau Monné' <roger.pau@citrix.com>
> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()
> 
> On 31.07.2020 15:07, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 31 July 2020 13:58
> >> To: Paul Durrant <paul@xen.org>
> >> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> >> 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@amazon.com>
> >>>
> >>> 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.
>

Actually, it occurs to me that logdirty is going to be an issue if I use p2m_ram_rw. If I'm not going to use p2m_mmio_direct then do you have another suggestion?

  Paul


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

Posted by Jan Beulich 7 weeks ago
On 31.07.2020 15:43, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 31 July 2020 14:41
>> To: paul@xen.org
>> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
>> <andrew.cooper3@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Roger Pau Monné' <roger.pau@citrix.com>
>> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()
>>
>> On 31.07.2020 15:07, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: 31 July 2020 13:58
>>>> To: Paul Durrant <paul@xen.org>
>>>> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
>>>> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
>>>> 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@amazon.com>
>>>>>
>>>>> 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.
>>
> 
> Actually, it occurs to me that logdirty is going to be an issue if I
> use p2m_ram_rw. If I'm not going to use p2m_mmio_direct then do you
> have another suggestion?

p2m_ram_rw is also not good because of allowing execution. If we don't
want to create a new type, how about (ab)using p2m_grant_map_rw (in
the sense of Xen granting the domain access to this page)? Possible
problems with this are
- replace_grant_p2m_mapping()
- hvm_translate_get_page()
All other uses of p2m_grant_map_rw and p2m_is_grant() look to be
compatible with this approach.

For replace_grant_p2m_mapping() I think there's no issue because the
grant table code won't find a suitable active entry.

For hvm_translate_get_page() the question is why it checks for the
two grant types in the first place; iow I wonder whether that check
couldn't be dropped.

Independent of this, btw, you may need to call set_gpfn_from_mfn()
alongside p2m_set_entry().

Jan

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

Posted by Paul Durrant 7 weeks ago
> -----Original Message-----
> From: Paul Durrant <xadimgnik@gmail.com>
> Sent: 31 July 2020 14:44
> To: 'Jan Beulich' <jbeulich@suse.com>
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Roger Pau Monné' <roger.pau@citrix.com>
> Subject: RE: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()
> 
> > -----Original Message-----
> > From: Jan Beulich <jbeulich@suse.com>
> > Sent: 31 July 2020 14:41
> > To: paul@xen.org
> > Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> > <andrew.cooper3@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Roger Pau Monné' <roger.pau@citrix.com>
> > Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()
> >
> > On 31.07.2020 15:07, Paul Durrant wrote:
> > >> -----Original Message-----
> > >> From: Jan Beulich <jbeulich@suse.com>
> > >> Sent: 31 July 2020 13:58
> > >> To: Paul Durrant <paul@xen.org>
> > >> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> > >> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> > >> 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@amazon.com>
> > >>>
> > >>> 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.
> >
> 
> Actually, it occurs to me that logdirty is going to be an issue if I use p2m_ram_rw. If I'm not going
> to use p2m_mmio_direct then do you have another suggestion?
> 

Looking further I'm uneasy to move away from setting that APIC access page to anything other than mmio_direct so I'd rather leave the VMX code alone and re-order things here.

  Paul