| Message ID | 20200123122141.1419-2-pdurrant@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | purge free_shared_domheap_page() | expand |
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
> -----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
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
> -----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
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;