[v2,1/3] x86 / vmx: make apic_access_mfn type-safe
diff mbox series

Message ID 20200123122141.1419-2-pdurrant@amazon.com
State Superseded
Headers show
Series
  • purge free_shared_domheap_page()
Related show

Commit Message

Paul Durrant Jan. 23, 2020, 12:21 p.m. UTC
Use mfn_t rather than unsigned long and change previous tests against 0 to
tests against INVALID_MFN (also introducing initialization to that value).

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: 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>
Cc: Jun Nakajima <jun.nakajima@intel.com>

v2:
 - Set apic_access_mfn to INVALID_MFN in vmx_free_vlapic_mapping() to make
   the function idempotent
---
 xen/arch/x86/hvm/mtrr.c            |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c         | 15 ++++++++-------
 xen/include/asm-x86/hvm/vmx/vmcs.h |  2 +-
 3 files changed, 10 insertions(+), 9 deletions(-)

Comments

Jan Beulich Jan. 23, 2020, 12:44 p.m. UTC | #1
On 23.01.2020 13:21, Paul Durrant wrote:
> Use mfn_t rather than unsigned long and change previous tests against 0 to
> tests against INVALID_MFN (also introducing initialization to that value).
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Acked-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

No, this isn't what the R-b was given for.

> v2:
>  - Set apic_access_mfn to INVALID_MFN in vmx_free_vlapic_mapping() to make
>    the function idempotent

Andrew had suggested to use 0 instead of INVALID_MFN. I don't see
how you achieved idempotency with this adjustment. Aiui
vmx_free_vlapic_mapping() is supposed to also run correctly if
vmx_alloc_vlapic_mapping() was never called.

Jan
Durrant, Paul Jan. 23, 2020, 1:09 p.m. UTC | #2
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan
> Beulich
> Sent: 23 January 2020 12:45
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: Kevin Tian <kevin.tian@intel.com>; Wei Liu <wl@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; xen-
> devel@lists.xenproject.org; Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v2 1/3] x86 / vmx: make apic_access_mfn
> type-safe
> 
> On 23.01.2020 13:21, Paul Durrant wrote:
> > Use mfn_t rather than unsigned long and change previous tests against 0
> to
> > tests against INVALID_MFN (also introducing initialization to that
> value).
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > Acked-by: Kevin Tian <kevin.tian@intel.com>
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> No, this isn't what the R-b was given for.

Oh, sorry, I misunderstood; I thought the R-b was good as long as idempotency was ensured.

> 
> > v2:
> >  - Set apic_access_mfn to INVALID_MFN in vmx_free_vlapic_mapping() to
> make
> >    the function idempotent
> 
> Andrew had suggested to use 0 instead of INVALID_MFN. I don't see
> how you achieved idempotency with this adjustment. Aiui
> vmx_free_vlapic_mapping() is supposed to also run correctly if
> vmx_alloc_vlapic_mapping() was never called.

It will. vmx_domain_initialise() will set apic_access_mfn to INVALID_MFN so vmx_free_vlapic_mapping() will do nothing.

  Paul
Jan Beulich Jan. 23, 2020, 1:29 p.m. UTC | #3
On 23.01.2020 14:09, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan
>> Beulich
>> Sent: 23 January 2020 12:45
>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>> Cc: Kevin Tian <kevin.tian@intel.com>; Wei Liu <wl@xen.org>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; xen-
>> devel@lists.xenproject.org; Roger Pau Monné <roger.pau@citrix.com>
>> Subject: Re: [Xen-devel] [PATCH v2 1/3] x86 / vmx: make apic_access_mfn
>> type-safe
>>
>> On 23.01.2020 13:21, Paul Durrant wrote:
>>> Use mfn_t rather than unsigned long and change previous tests against 0
>> to
>>> tests against INVALID_MFN (also introducing initialization to that
>> value).
>>>
>>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>>> Acked-by: Kevin Tian <kevin.tian@intel.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> No, this isn't what the R-b was given for.
> 
> Oh, sorry, I misunderstood; I thought the R-b was good as long as idempotency was ensured.
> 
>>
>>> v2:
>>>  - Set apic_access_mfn to INVALID_MFN in vmx_free_vlapic_mapping() to
>> make
>>>    the function idempotent
>>
>> Andrew had suggested to use 0 instead of INVALID_MFN. I don't see
>> how you achieved idempotency with this adjustment. Aiui
>> vmx_free_vlapic_mapping() is supposed to also run correctly if
>> vmx_alloc_vlapic_mapping() was never called.
> 
> It will. vmx_domain_initialise() will set apic_access_mfn to INVALID_MFN
> so vmx_free_vlapic_mapping() will do nothing.

I'm sorry, it was implied that it also needs to work if
vmx_domain_initialise() was never called. Andrew's goal after
all is, aiui, to be able to call "destroy" functions on error
paths irrespective of how far "create" had managed to progress.

Jan
Durrant, Paul Jan. 23, 2020, 1:36 p.m. UTC | #4
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 23 January 2020 13:30
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: Kevin Tian <kevin.tian@intel.com>; Wei Liu <wl@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; xen-
> devel@lists.xenproject.org; Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [PATCH v2 1/3] x86 / vmx: make apic_access_mfn type-safe
> 
> On 23.01.2020 14:09, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Jan
> >> Beulich
> >> Sent: 23 January 2020 12:45
> >> To: Durrant, Paul <pdurrant@amazon.co.uk>
> >> Cc: Kevin Tian <kevin.tian@intel.com>; Wei Liu <wl@xen.org>; Andrew
> Cooper
> >> <andrew.cooper3@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>;
> xen-
> >> devel@lists.xenproject.org; Roger Pau Monné <roger.pau@citrix.com>
> >> Subject: Re: [Xen-devel] [PATCH v2 1/3] x86 / vmx: make apic_access_mfn
> >> type-safe
> >>
> >> On 23.01.2020 13:21, Paul Durrant wrote:
> >>> Use mfn_t rather than unsigned long and change previous tests against
> 0
> >> to
> >>> tests against INVALID_MFN (also introducing initialization to that
> >> value).
> >>>
> >>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> >>> Acked-by: Kevin Tian <kevin.tian@intel.com>
> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> No, this isn't what the R-b was given for.
> >
> > Oh, sorry, I misunderstood; I thought the R-b was good as long as
> idempotency was ensured.
> >
> >>
> >>> v2:
> >>>  - Set apic_access_mfn to INVALID_MFN in vmx_free_vlapic_mapping() to
> >> make
> >>>    the function idempotent
> >>
> >> Andrew had suggested to use 0 instead of INVALID_MFN. I don't see
> >> how you achieved idempotency with this adjustment. Aiui
> >> vmx_free_vlapic_mapping() is supposed to also run correctly if
> >> vmx_alloc_vlapic_mapping() was never called.
> >
> > It will. vmx_domain_initialise() will set apic_access_mfn to INVALID_MFN
> > so vmx_free_vlapic_mapping() will do nothing.
> 
> I'm sorry, it was implied that it also needs to work if
> vmx_domain_initialise() was never called. Andrew's goal after
> all is, aiui, to be able to call "destroy" functions on error
> paths irrespective of how far "create" had managed to progress.
> 

Oh, I see. That implication was not at all obvious to me. I thought he was just after being able to 'destroy' as many times as it took to finish, in which case our choice for the value of INVALID_MFN is indeed unfortunate. If that's the goal I'll switch to use _mfn(0) as a comparator.

  Paul

Patch
diff mbox series

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 5ad15eafe0..8356e8de3d 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -818,7 +818,7 @@  int epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
 
     if ( direct_mmio )
     {
-        if ( (mfn_x(mfn) ^ d->arch.hvm.vmx.apic_access_mfn) >> order )
+        if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> order )
             return MTRR_TYPE_UNCACHABLE;
         if ( order )
             return -1;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f83f102638..8706954d73 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -413,6 +413,7 @@  static int vmx_domain_initialise(struct domain *d)
     if ( !has_vlapic(d) )
         return 0;
 
+    d->arch.hvm.vmx.apic_access_mfn = INVALID_MFN;
     if ( (rc = vmx_alloc_vlapic_mapping(d)) != 0 )
         return rc;
 
@@ -3034,7 +3035,7 @@  static int vmx_alloc_vlapic_mapping(struct domain *d)
     mfn = page_to_mfn(pg);
     clear_domain_page(mfn);
     share_xen_page_with_guest(pg, d, SHARE_rw);
-    d->arch.hvm.vmx.apic_access_mfn = mfn_x(mfn);
+    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,
@@ -3043,24 +3044,24 @@  static int vmx_alloc_vlapic_mapping(struct domain *d)
 
 static void vmx_free_vlapic_mapping(struct domain *d)
 {
-    unsigned long mfn = d->arch.hvm.vmx.apic_access_mfn;
+    mfn_t mfn = d->arch.hvm.vmx.apic_access_mfn;
 
-    if ( mfn != 0 )
-        free_shared_domheap_page(mfn_to_page(_mfn(mfn)));
+    d->arch.hvm.vmx.apic_access_mfn = INVALID_MFN;
+    if ( !mfn_eq(mfn, INVALID_MFN) )
+        free_shared_domheap_page(mfn_to_page(mfn));
 }
 
 static void vmx_install_vlapic_mapping(struct vcpu *v)
 {
     paddr_t virt_page_ma, apic_page_ma;
 
-    if ( v->domain->arch.hvm.vmx.apic_access_mfn == 0 )
+    if ( mfn_eq(v->domain->arch.hvm.vmx.apic_access_mfn, INVALID_MFN) )
         return;
 
     ASSERT(cpu_has_vmx_virtualize_apic_accesses);
 
     virt_page_ma = page_to_maddr(vcpu_vlapic(v)->regs_page);
-    apic_page_ma = v->domain->arch.hvm.vmx.apic_access_mfn;
-    apic_page_ma <<= PAGE_SHIFT;
+    apic_page_ma = mfn_to_maddr(v->domain->arch.hvm.vmx.apic_access_mfn);
 
     vmx_vmcs_enter(v);
     __vmwrite(VIRTUAL_APIC_PAGE_ADDR, virt_page_ma);
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index a514299144..be4661a929 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -59,7 +59,7 @@  struct ept_data {
 #define _VMX_DOMAIN_PML_ENABLED    0
 #define VMX_DOMAIN_PML_ENABLED     (1ul << _VMX_DOMAIN_PML_ENABLED)
 struct vmx_domain {
-    unsigned long apic_access_mfn;
+    mfn_t apic_access_mfn;
     /* VMX_DOMAIN_* */
     unsigned int status;