* [Xen-devel] [PATCH 0/3] purge free_shared_domheap_page() @ 2020-01-21 12:00 Paul Durrant 2020-01-21 12:00 ` [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe Paul Durrant ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Paul Durrant @ 2020-01-21 12:00 UTC (permalink / raw) To: xen-devel; +Cc: Paul Durrant Paul Durrant (3): x86 / vmx: make apic_access_mfn type-safe x86 / hvm: add domain_relinquish_resources() method x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE xen/arch/x86/hvm/hvm.c | 2 ++ xen/arch/x86/hvm/mtrr.c | 2 +- xen/arch/x86/hvm/svm/svm.c | 5 ++++ xen/arch/x86/hvm/vmx/vmx.c | 40 ++++++++++++++++++++++-------- xen/arch/x86/mm.c | 10 -------- xen/common/page_alloc.c | 3 ++- xen/include/asm-x86/hvm/hvm.h | 1 + xen/include/asm-x86/hvm/vmx/vmcs.h | 2 +- xen/include/asm-x86/mm.h | 2 -- 9 files changed, 42 insertions(+), 25 deletions(-) -- 2.20.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe 2020-01-21 12:00 [Xen-devel] [PATCH 0/3] purge free_shared_domheap_page() Paul Durrant @ 2020-01-21 12:00 ` Paul Durrant 2020-01-22 2:51 ` Tian, Kevin 2020-01-22 14:05 ` Andrew Cooper 2020-01-21 12:00 ` [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method Paul Durrant 2020-01-21 12:00 ` [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE Paul Durrant 2 siblings, 2 replies; 17+ messages in thread From: Paul Durrant @ 2020-01-21 12:00 UTC (permalink / raw) To: xen-devel Cc: Kevin Tian, Wei Liu, Andrew Cooper, Paul Durrant, Jun Nakajima, Roger Pau Monné 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> --- 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> Cc: Jun Nakajima <jun.nakajima@intel.com> Cc: Kevin Tian <kevin.tian@intel.com> --- xen/arch/x86/hvm/mtrr.c | 2 +- xen/arch/x86/hvm/vmx/vmx.c | 14 +++++++------- xen/include/asm-x86/hvm/vmx/vmcs.h | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) 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..3d90e67a05 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,23 @@ 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))); + 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; -- 2.20.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe 2020-01-21 12:00 ` [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe Paul Durrant @ 2020-01-22 2:51 ` Tian, Kevin 2020-01-22 14:05 ` Andrew Cooper 1 sibling, 0 replies; 17+ messages in thread From: Tian, Kevin @ 2020-01-22 2:51 UTC (permalink / raw) To: Paul Durrant, xen-devel Cc: Nakajima, Jun, Andrew Cooper, Wei Liu, Roger Pau Monné > From: Paul Durrant <pdurrant@amazon.com> > Sent: Tuesday, January 21, 2020 8:00 PM > > 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> Reviewed-by: Kevin Tian <kevin.tian@intel.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe 2020-01-21 12:00 ` [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe Paul Durrant 2020-01-22 2:51 ` Tian, Kevin @ 2020-01-22 14:05 ` Andrew Cooper 2020-01-22 15:48 ` Jan Beulich 1 sibling, 1 reply; 17+ messages in thread From: Andrew Cooper @ 2020-01-22 14:05 UTC (permalink / raw) To: Paul Durrant, xen-devel Cc: Jun Nakajima, Kevin Tian, Wei Liu, Roger Pau Monné On 21/01/2020 12:00, 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> I'm afraid this breaks the idempotency of vmx_free_vlapic_mapping(), which gets in the way of domain/vcpu create/destroy cleanup. Its fine to use 0 as the sentinel. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe 2020-01-22 14:05 ` Andrew Cooper @ 2020-01-22 15:48 ` Jan Beulich 0 siblings, 0 replies; 17+ messages in thread From: Jan Beulich @ 2020-01-22 15:48 UTC (permalink / raw) To: Paul Durrant Cc: Kevin Tian, Wei Liu, Andrew Cooper, Jun Nakajima, xen-devel, Roger Pau Monné On 22.01.2020 15:05, Andrew Cooper wrote: > On 21/01/2020 12:00, 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> > > I'm afraid this breaks the idempotency of vmx_free_vlapic_mapping(), > which gets in the way of domain/vcpu create/destroy cleanup. > > Its fine to use 0 as the sentinel. And with this adjustment Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method 2020-01-21 12:00 [Xen-devel] [PATCH 0/3] purge free_shared_domheap_page() Paul Durrant 2020-01-21 12:00 ` [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe Paul Durrant @ 2020-01-21 12:00 ` Paul Durrant 2020-01-22 15:50 ` Jan Beulich 2020-01-21 12:00 ` [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE Paul Durrant 2 siblings, 1 reply; 17+ messages in thread From: Paul Durrant @ 2020-01-21 12:00 UTC (permalink / raw) To: xen-devel Cc: Kevin Tian, Wei Liu, Andrew Cooper, Paul Durrant, Jun Nakajima, Roger Pau Monné There are two functions in hvm.c to deal with tear-down and a domain: hvm_domain_relinquish_resources() and hvm_domain_destroy(). However, only the latter has an associated method in 'hvm_funcs'. This patch adds a method for the former and stub definitions for SVM and VMX. The VMX method will be used by a subsequent patch. 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> Cc: Jun Nakajima <jun.nakajima@intel.com> Cc: Kevin Tian <kevin.tian@intel.com> --- xen/arch/x86/hvm/hvm.c | 2 ++ xen/arch/x86/hvm/svm/svm.c | 5 +++++ xen/arch/x86/hvm/vmx/vmx.c | 5 +++++ xen/include/asm-x86/hvm/hvm.h | 1 + 4 files changed, 13 insertions(+) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 4723f5d09c..669dce6731 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -715,6 +715,8 @@ int hvm_domain_initialise(struct domain *d) void hvm_domain_relinquish_resources(struct domain *d) { + hvm_funcs.domain_relinquish_resources(d); + if ( hvm_funcs.nhvm_domain_relinquish_resources ) hvm_funcs.nhvm_domain_relinquish_resources(d); diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index b1c376d455..24768e8682 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1155,6 +1155,10 @@ static int svm_domain_initialise(struct domain *d) return 0; } +static void svm_domain_relinquish_resources(struct domain *d) +{ +} + static void svm_domain_destroy(struct domain *d) { } @@ -2425,6 +2429,7 @@ static struct hvm_function_table __initdata svm_function_table = { .cpu_up = svm_cpu_up, .cpu_down = svm_cpu_down, .domain_initialise = svm_domain_initialise, + .domain_relinquish_resources = svm_domain_relinquish_resources, .domain_destroy = svm_domain_destroy, .vcpu_initialise = svm_vcpu_initialise, .vcpu_destroy = svm_vcpu_destroy, diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 3d90e67a05..3fd3ac61e1 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -420,6 +420,10 @@ static int vmx_domain_initialise(struct domain *d) return 0; } +static void vmx_domain_relinquish_resources(struct domain *d) +{ +} + static void vmx_domain_destroy(struct domain *d) { if ( !has_vlapic(d) ) @@ -2241,6 +2245,7 @@ static struct hvm_function_table __initdata vmx_function_table = { .cpu_up_prepare = vmx_cpu_up_prepare, .cpu_dead = vmx_cpu_dead, .domain_initialise = vmx_domain_initialise, + .domain_relinquish_resources = vmx_domain_relinquish_resources, .domain_destroy = vmx_domain_destroy, .vcpu_initialise = vmx_vcpu_initialise, .vcpu_destroy = vmx_vcpu_destroy, diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index 09793c12e9..9eab1d7493 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -107,6 +107,7 @@ struct hvm_function_table { * Initialise/destroy HVM domain/vcpu resources */ int (*domain_initialise)(struct domain *d); + void (*domain_relinquish_resources)(struct domain *d); void (*domain_destroy)(struct domain *d); int (*vcpu_initialise)(struct vcpu *v); void (*vcpu_destroy)(struct vcpu *v); -- 2.20.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method 2020-01-21 12:00 ` [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method Paul Durrant @ 2020-01-22 15:50 ` Jan Beulich 2020-01-22 15:56 ` Durrant, Paul 0 siblings, 1 reply; 17+ messages in thread From: Jan Beulich @ 2020-01-22 15:50 UTC (permalink / raw) To: Paul Durrant Cc: Kevin Tian, Wei Liu, Andrew Cooper, Jun Nakajima, xen-devel, Roger Pau Monné On 21.01.2020 13:00, Paul Durrant wrote: > There are two functions in hvm.c to deal with tear-down and a domain: > hvm_domain_relinquish_resources() and hvm_domain_destroy(). However, only > the latter has an associated method in 'hvm_funcs'. This patch adds > a method for the former and stub definitions for SVM and VMX. Why the stubs? Simply ... > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -715,6 +715,8 @@ int hvm_domain_initialise(struct domain *d) > > void hvm_domain_relinquish_resources(struct domain *d) > { > + hvm_funcs.domain_relinquish_resources(d); ... stick a NULL check around this one. I also wonder whether, it being entirely new, this wouldn't better use alternative call patching right from the beginning. It's not the hottest path, but avoiding indirect calls seems quite desirable, especially when doing so is relatively cheap. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method 2020-01-22 15:50 ` Jan Beulich @ 2020-01-22 15:56 ` Durrant, Paul 2020-01-22 16:00 ` Jan Beulich 0 siblings, 1 reply; 17+ messages in thread From: Durrant, Paul @ 2020-01-22 15:56 UTC (permalink / raw) To: Jan Beulich Cc: Kevin Tian, Wei Liu, Andrew Cooper, Jun Nakajima, xen-devel, Roger Pau Monné > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 22 January 2020 15:51 > To: Durrant, Paul <pdurrant@amazon.co.uk> > Cc: xen-devel@lists.xenproject.org; Andrew Cooper > <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné > <roger.pau@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian > <kevin.tian@intel.com> > Subject: Re: [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() > method > > On 21.01.2020 13:00, Paul Durrant wrote: > > There are two functions in hvm.c to deal with tear-down and a domain: > > hvm_domain_relinquish_resources() and hvm_domain_destroy(). However, > only > > the latter has an associated method in 'hvm_funcs'. This patch adds > > a method for the former and stub definitions for SVM and VMX. > > Why the stubs? Simply ... > > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -715,6 +715,8 @@ int hvm_domain_initialise(struct domain *d) > > > > void hvm_domain_relinquish_resources(struct domain *d) > > { > > + hvm_funcs.domain_relinquish_resources(d); > > ... stick a NULL check around this one. I also wonder whether, it > being entirely new, this wouldn't better use alternative call > patching right from the beginning. It's not the hottest path, but > avoiding indirect calls seems quite desirable, especially when > doing so is relatively cheap. > I'd like it to align with the rest of the hvm_funcs so I'll add the NULL check, but alternatives patch for all hvm_funcs seems like a good thing I the longer term. Paul _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method 2020-01-22 15:56 ` Durrant, Paul @ 2020-01-22 16:00 ` Jan Beulich 2020-01-22 16:02 ` Durrant, Paul 0 siblings, 1 reply; 17+ messages in thread From: Jan Beulich @ 2020-01-22 16:00 UTC (permalink / raw) To: Durrant, Paul Cc: Kevin Tian, Wei Liu, Andrew Cooper, Jun Nakajima, xen-devel, Roger Pau Monné On 22.01.2020 16:56, Durrant, Paul wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 22 January 2020 15:51 >> To: Durrant, Paul <pdurrant@amazon.co.uk> >> Cc: xen-devel@lists.xenproject.org; Andrew Cooper >> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné >> <roger.pau@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian >> <kevin.tian@intel.com> >> Subject: Re: [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() >> method >> >> On 21.01.2020 13:00, Paul Durrant wrote: >>> There are two functions in hvm.c to deal with tear-down and a domain: >>> hvm_domain_relinquish_resources() and hvm_domain_destroy(). However, >> only >>> the latter has an associated method in 'hvm_funcs'. This patch adds >>> a method for the former and stub definitions for SVM and VMX. >> >> Why the stubs? Simply ... >> >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -715,6 +715,8 @@ int hvm_domain_initialise(struct domain *d) >>> >>> void hvm_domain_relinquish_resources(struct domain *d) >>> { >>> + hvm_funcs.domain_relinquish_resources(d); >> >> ... stick a NULL check around this one. I also wonder whether, it >> being entirely new, this wouldn't better use alternative call >> patching right from the beginning. It's not the hottest path, but >> avoiding indirect calls seems quite desirable, especially when >> doing so is relatively cheap. >> > > I'd like it to align with the rest of the hvm_funcs so I'll add the > NULL check, but alternatives patch for all hvm_funcs seems like a > good thing I the longer term. The frequently used ones have been converted already. Hence my suggestion to make new ones use that model from the beginning. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method 2020-01-22 16:00 ` Jan Beulich @ 2020-01-22 16:02 ` Durrant, Paul 0 siblings, 0 replies; 17+ messages in thread From: Durrant, Paul @ 2020-01-22 16:02 UTC (permalink / raw) To: Jan Beulich Cc: Kevin Tian, Wei Liu, Andrew Cooper, Jun Nakajima, xen-devel, Roger Pau Monné > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 22 January 2020 16:01 > To: Durrant, Paul <pdurrant@amazon.co.uk> > Cc: xen-devel@lists.xenproject.org; Andrew Cooper > <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné > <roger.pau@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian > <kevin.tian@intel.com> > Subject: Re: [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() > method > > On 22.01.2020 16:56, Durrant, Paul wrote: > >> -----Original Message----- > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: 22 January 2020 15:51 > >> To: Durrant, Paul <pdurrant@amazon.co.uk> > >> Cc: xen-devel@lists.xenproject.org; Andrew Cooper > >> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné > >> <roger.pau@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Kevin > Tian > >> <kevin.tian@intel.com> > >> Subject: Re: [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() > >> method > >> > >> On 21.01.2020 13:00, Paul Durrant wrote: > >>> There are two functions in hvm.c to deal with tear-down and a domain: > >>> hvm_domain_relinquish_resources() and hvm_domain_destroy(). However, > >> only > >>> the latter has an associated method in 'hvm_funcs'. This patch adds > >>> a method for the former and stub definitions for SVM and VMX. > >> > >> Why the stubs? Simply ... > >> > >>> --- a/xen/arch/x86/hvm/hvm.c > >>> +++ b/xen/arch/x86/hvm/hvm.c > >>> @@ -715,6 +715,8 @@ int hvm_domain_initialise(struct domain *d) > >>> > >>> void hvm_domain_relinquish_resources(struct domain *d) > >>> { > >>> + hvm_funcs.domain_relinquish_resources(d); > >> > >> ... stick a NULL check around this one. I also wonder whether, it > >> being entirely new, this wouldn't better use alternative call > >> patching right from the beginning. It's not the hottest path, but > >> avoiding indirect calls seems quite desirable, especially when > >> doing so is relatively cheap. > >> > > > > I'd like it to align with the rest of the hvm_funcs so I'll add the > > NULL check, but alternatives patch for all hvm_funcs seems like a > > good thing I the longer term. > > The frequently used ones have been converted already. Hence my > suggestion to make new ones use that model from the beginning. > Oh, ok. I'll go look for some examples. Paul _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE 2020-01-21 12:00 [Xen-devel] [PATCH 0/3] purge free_shared_domheap_page() Paul Durrant 2020-01-21 12:00 ` [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe Paul Durrant 2020-01-21 12:00 ` [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method Paul Durrant @ 2020-01-21 12:00 ` Paul Durrant 2020-01-21 12:29 ` Julien Grall ` (2 more replies) 2 siblings, 3 replies; 17+ messages in thread From: Paul Durrant @ 2020-01-21 12:00 UTC (permalink / raw) To: xen-devel Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Jun Nakajima, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Paul Durrant, Ian Jackson, Roger Pau Monné vmx_alloc_vlapic_mapping() currently contains some very odd looking code that allocates a MEMF_no_owner domheap page and then shares with the guest as if it were a xenheap page. This then requires vmx_free_vlapic_mapping() to call a special function in the mm code: free_shared_domheap_page(). By using a 'normal' domheap page (i.e. by not passing MEMF_no_owner to alloc_domheap_page()), the odd looking code in vmx_alloc_vlapic_mapping() can simply use get_page_and_type() to set up a writable mapping before insertion in the P2M and vmx_free_vlapic_mapping() can simply release the page using put_page_alloc_ref() followed by put_page_and_type(). This then allows free_shared_domheap_page() to be purged. There is, however, some fall-out from this simplification: - alloc_domheap_page() will now call assign_pages() and run into the fact that 'max_pages' is not set until some time after domain_create(). To avoid an allocation failure, assign_pages() is modified to ignore the max_pages limit if 'creation_finished' is false. That value is not set to true until domain_unpause_by_systemcontroller() is called, and thus the guest cannot run (and hence cause memory allocation) until creation_finished is set to true. - Because the domheap page is no longer a pseudo-xenheap page, the reference counting will prevent the domain from being destroyed. Thus the call to vmx_free_vlapic_mapping() is moved from the domain_destroy() method into the domain_relinquish_resources() method. Signed-off-by: Paul Durrant <pdurrant@amazon.com> --- Cc: Jun Nakajima <jun.nakajima@intel.com> Cc: Kevin Tian <kevin.tian@intel.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> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Julien Grall <julien@xen.org> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> --- xen/arch/x86/hvm/vmx/vmx.c | 29 ++++++++++++++++++++++------- xen/arch/x86/mm.c | 10 ---------- xen/common/page_alloc.c | 3 ++- xen/include/asm-x86/mm.h | 2 -- 4 files changed, 24 insertions(+), 20 deletions(-) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 3fd3ac61e1..a2e6081485 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -421,10 +421,6 @@ static int vmx_domain_initialise(struct domain *d) } static void vmx_domain_relinquish_resources(struct domain *d) -{ -} - -static void vmx_domain_destroy(struct domain *d) { if ( !has_vlapic(d) ) return; @@ -432,6 +428,10 @@ static void vmx_domain_destroy(struct domain *d) vmx_free_vlapic_mapping(d); } +static void vmx_domain_destroy(struct domain *d) +{ +} + static int vmx_vcpu_initialise(struct vcpu *v) { int rc; @@ -3034,12 +3034,22 @@ static int vmx_alloc_vlapic_mapping(struct domain *d) if ( !cpu_has_vmx_virtualize_apic_accesses ) return 0; - pg = alloc_domheap_page(d, MEMF_no_owner); + pg = alloc_domheap_page(d, 0); if ( !pg ) return -ENOMEM; + + if ( !get_page_and_type(pg, d, PGT_writable_page) ) + { + /* + * The domain can't possibly know about this page yet, so failure + * here is a clear indication of something fishy going on. + */ + domain_crash(d); + return -ENODATA; + } + 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; return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn, @@ -3052,7 +3062,12 @@ static void vmx_free_vlapic_mapping(struct domain *d) mfn_t mfn = d->arch.hvm.vmx.apic_access_mfn; if ( !mfn_eq(mfn, INVALID_MFN) ) - free_shared_domheap_page(mfn_to_page(mfn)); + { + struct page_info *pg = mfn_to_page(mfn); + + put_page_alloc_ref(pg); + put_page_and_type(pg); + } } static void vmx_install_vlapic_mapping(struct vcpu *v) diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 654190e9e9..2a6d2e8af9 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -496,16 +496,6 @@ void share_xen_page_with_guest(struct page_info *page, struct domain *d, spin_unlock(&d->page_alloc_lock); } -void free_shared_domheap_page(struct page_info *page) -{ - put_page_alloc_ref(page); - if ( !test_and_clear_bit(_PGC_xen_heap, &page->count_info) ) - ASSERT_UNREACHABLE(); - page->u.inuse.type_info = 0; - page_set_owner(page, NULL); - free_domheap_page(page); -} - void make_cr3(struct vcpu *v, mfn_t mfn) { struct domain *d = v->domain; diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 919a270587..ef327072ed 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -2269,7 +2269,8 @@ int assign_pages( if ( !(memflags & MEMF_no_refcount) ) { - if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) ) + if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) && + d->creation_finished ) { gprintk(XENLOG_INFO, "Over-allocation for domain %u: " "%u > %u\n", d->domain_id, diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h index 2ca8882ad0..e429f38228 100644 --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -317,8 +317,6 @@ struct page_info #define maddr_get_owner(ma) (page_get_owner(maddr_to_page((ma)))) -extern void free_shared_domheap_page(struct page_info *page); - #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START) extern unsigned long max_page; extern unsigned long total_pages; -- 2.20.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE 2020-01-21 12:00 ` [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE Paul Durrant @ 2020-01-21 12:29 ` Julien Grall 2020-01-21 12:37 ` Durrant, Paul 2020-01-22 3:19 ` Tian, Kevin 2020-01-22 16:17 ` Jan Beulich 2 siblings, 1 reply; 17+ messages in thread From: Julien Grall @ 2020-01-21 12:29 UTC (permalink / raw) To: Paul Durrant, xen-devel Cc: Kevin Tian, Stefano Stabellini, Jun Nakajima, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Roger Pau Monné Hi, On 21/01/2020 12:00, Paul Durrant wrote: > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 919a270587..ef327072ed 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -2269,7 +2269,8 @@ int assign_pages( > > if ( !(memflags & MEMF_no_refcount) ) > { > - if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) ) > + if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) && > + d->creation_finished ) This is not entirely obvious to me how this is safe. What would happen if d->creation_finished is set on another CPU at the same time? At least on Arm, this may not be seen directly. I guess the problem would not only happen in this use case (I am more concerned in the physmap code), but it would be good to document how it is meant to be safe to use. However, AFAIU, the only reason for the check to be here is because d->max_pages is set quite late. How about setting max_pages as part of the domain_create hypercall? Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE 2020-01-21 12:29 ` Julien Grall @ 2020-01-21 12:37 ` Durrant, Paul 0 siblings, 0 replies; 17+ messages in thread From: Durrant, Paul @ 2020-01-21 12:37 UTC (permalink / raw) To: Julien Grall, xen-devel Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Jun Nakajima, Roger Pau Monné > -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of > Julien Grall > Sent: 21 January 2020 12:29 > To: Durrant, Paul <pdurrant@amazon.co.uk>; xen-devel@lists.xenproject.org > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini > <sstabellini@kernel.org>; Jun Nakajima <jun.nakajima@intel.com>; Wei Liu > <wl@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George > Dunlap <George.Dunlap@eu.citrix.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; > Roger Pau Monné <roger.pau@citrix.com> > Subject: Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap > page for APIC_DEFAULT_PHYS_BASE > > Hi, > > On 21/01/2020 12:00, Paul Durrant wrote: > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > > index 919a270587..ef327072ed 100644 > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -2269,7 +2269,8 @@ int assign_pages( > > > > if ( !(memflags & MEMF_no_refcount) ) > > { > > - if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) ) > > + if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) && > > + d->creation_finished ) > > This is not entirely obvious to me how this is safe. What would happen > if d->creation_finished is set on another CPU at the same time? At least > on Arm, this may not be seen directly. > > I guess the problem would not only happen in this use case (I am more > concerned in the physmap code), but it would be good to document how it > is meant to be safe to use. > > However, AFAIU, the only reason for the check to be here is because > d->max_pages is set quite late. How about setting max_pages as part of > the domain_create hypercall? That would be useful but certainly more invasive. There's no way a guest vcpu can see creation_finished set to true as it is still paused. The only concern would be a stub domain causing domheap pages to be allocated on behalf of the guest, and can we not trust a stub domain until it's guest has been unpaused (since there is no scope for the guest to attack it until then)? Paul > > Cheers, > > -- > Julien Grall > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE 2020-01-21 12:00 ` [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE Paul Durrant 2020-01-21 12:29 ` Julien Grall @ 2020-01-22 3:19 ` Tian, Kevin 2020-01-22 11:25 ` Durrant, Paul 2020-01-22 16:17 ` Jan Beulich 2 siblings, 1 reply; 17+ messages in thread From: Tian, Kevin @ 2020-01-22 3:19 UTC (permalink / raw) To: Paul Durrant, xen-devel Cc: Stefano Stabellini, Julien Grall, Nakajima, Jun, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Roger Pau Monné > From: Paul Durrant <pdurrant@amazon.com> > Sent: Tuesday, January 21, 2020 8:00 PM > > vmx_alloc_vlapic_mapping() currently contains some very odd looking code > that allocates a MEMF_no_owner domheap page and then shares with the > guest > as if it were a xenheap page. This then requires vmx_free_vlapic_mapping() > to call a special function in the mm code: free_shared_domheap_page(). > > By using a 'normal' domheap page (i.e. by not passing MEMF_no_owner to > alloc_domheap_page()), the odd looking code in vmx_alloc_vlapic_mapping() > can simply use get_page_and_type() to set up a writable mapping before > insertion in the P2M and vmx_free_vlapic_mapping() can simply release the > page using put_page_alloc_ref() followed by put_page_and_type(). This > then allows free_shared_domheap_page() to be purged. I doubt whether using a normal domheap page is a right thing in concept. Note the APIC access page is the backend for virtual LAPIC_BASE which is a MMIO range from guest p.o.v. > > There is, however, some fall-out from this simplification: > > - alloc_domheap_page() will now call assign_pages() and run into the fact > that 'max_pages' is not set until some time after domain_create(). To > avoid an allocation failure, assign_pages() is modified to ignore the > max_pages limit if 'creation_finished' is false. That value is not set > to true until domain_unpause_by_systemcontroller() is called, and thus > the guest cannot run (and hence cause memory allocation) until > creation_finished is set to true. However, doing so opens the window of no guard of allocations in the whole VM creation time. I'm not sure whether it's a worthwhile just for fixing a bad-looking but conceptually-correct code. > > - Because the domheap page is no longer a pseudo-xenheap page, the > reference counting will prevent the domain from being destroyed. Thus > the call to vmx_free_vlapic_mapping() is moved from the > domain_destroy() method into the domain_relinquish_resources() method. > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > --- > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Kevin Tian <kevin.tian@intel.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> > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Julien Grall <julien@xen.org> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/arch/x86/hvm/vmx/vmx.c | 29 ++++++++++++++++++++++------- > xen/arch/x86/mm.c | 10 ---------- > xen/common/page_alloc.c | 3 ++- > xen/include/asm-x86/mm.h | 2 -- > 4 files changed, 24 insertions(+), 20 deletions(-) > > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index 3fd3ac61e1..a2e6081485 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -421,10 +421,6 @@ static int vmx_domain_initialise(struct domain *d) > } > > static void vmx_domain_relinquish_resources(struct domain *d) > -{ > -} > - > -static void vmx_domain_destroy(struct domain *d) > { > if ( !has_vlapic(d) ) > return; > @@ -432,6 +428,10 @@ static void vmx_domain_destroy(struct domain *d) > vmx_free_vlapic_mapping(d); > } > > +static void vmx_domain_destroy(struct domain *d) > +{ > +} > + > static int vmx_vcpu_initialise(struct vcpu *v) > { > int rc; > @@ -3034,12 +3034,22 @@ static int vmx_alloc_vlapic_mapping(struct > domain *d) > if ( !cpu_has_vmx_virtualize_apic_accesses ) > return 0; > > - pg = alloc_domheap_page(d, MEMF_no_owner); > + pg = alloc_domheap_page(d, 0); > if ( !pg ) > return -ENOMEM; > + > + if ( !get_page_and_type(pg, d, PGT_writable_page) ) > + { > + /* > + * The domain can't possibly know about this page yet, so failure > + * here is a clear indication of something fishy going on. > + */ > + domain_crash(d); > + return -ENODATA; > + } > + > 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; > > return set_mmio_p2m_entry(d, > paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn, > @@ -3052,7 +3062,12 @@ static void vmx_free_vlapic_mapping(struct > domain *d) > mfn_t mfn = d->arch.hvm.vmx.apic_access_mfn; > > if ( !mfn_eq(mfn, INVALID_MFN) ) > - free_shared_domheap_page(mfn_to_page(mfn)); > + { > + struct page_info *pg = mfn_to_page(mfn); > + > + put_page_alloc_ref(pg); > + put_page_and_type(pg); > + } > } > > static void vmx_install_vlapic_mapping(struct vcpu *v) > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index 654190e9e9..2a6d2e8af9 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -496,16 +496,6 @@ void share_xen_page_with_guest(struct page_info > *page, struct domain *d, > spin_unlock(&d->page_alloc_lock); > } > > -void free_shared_domheap_page(struct page_info *page) > -{ > - put_page_alloc_ref(page); > - if ( !test_and_clear_bit(_PGC_xen_heap, &page->count_info) ) > - ASSERT_UNREACHABLE(); > - page->u.inuse.type_info = 0; > - page_set_owner(page, NULL); > - free_domheap_page(page); > -} > - > void make_cr3(struct vcpu *v, mfn_t mfn) > { > struct domain *d = v->domain; > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 919a270587..ef327072ed 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -2269,7 +2269,8 @@ int assign_pages( > > if ( !(memflags & MEMF_no_refcount) ) > { > - if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) ) > + if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) && > + d->creation_finished ) > { > gprintk(XENLOG_INFO, "Over-allocation for domain %u: " > "%u > %u\n", d->domain_id, > diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h > index 2ca8882ad0..e429f38228 100644 > --- a/xen/include/asm-x86/mm.h > +++ b/xen/include/asm-x86/mm.h > @@ -317,8 +317,6 @@ struct page_info > > #define maddr_get_owner(ma) (page_get_owner(maddr_to_page((ma)))) > > -extern void free_shared_domheap_page(struct page_info *page); > - > #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START) > extern unsigned long max_page; > extern unsigned long total_pages; > -- > 2.20.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE 2020-01-22 3:19 ` Tian, Kevin @ 2020-01-22 11:25 ` Durrant, Paul 0 siblings, 0 replies; 17+ messages in thread From: Durrant, Paul @ 2020-01-22 11:25 UTC (permalink / raw) To: Tian, Kevin, xen-devel Cc: Stefano Stabellini, Julien Grall, Nakajima, Jun, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Roger Pau Monné > -----Original Message----- > From: Tian, Kevin <kevin.tian@intel.com> > Sent: 22 January 2020 03:19 > To: Durrant, Paul <pdurrant@amazon.co.uk>; xen-devel@lists.xenproject.org > Cc: Nakajima, Jun <jun.nakajima@intel.com>; Jan Beulich > <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu > <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; George Dunlap > <George.Dunlap@eu.citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; > Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org> > Subject: RE: [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for > APIC_DEFAULT_PHYS_BASE > > > From: Paul Durrant <pdurrant@amazon.com> > > Sent: Tuesday, January 21, 2020 8:00 PM > > > > vmx_alloc_vlapic_mapping() currently contains some very odd looking code > > that allocates a MEMF_no_owner domheap page and then shares with the > > guest > > as if it were a xenheap page. This then requires > vmx_free_vlapic_mapping() > > to call a special function in the mm code: free_shared_domheap_page(). > > > > By using a 'normal' domheap page (i.e. by not passing MEMF_no_owner to > > alloc_domheap_page()), the odd looking code in > vmx_alloc_vlapic_mapping() > > can simply use get_page_and_type() to set up a writable mapping before > > insertion in the P2M and vmx_free_vlapic_mapping() can simply release > the > > page using put_page_alloc_ref() followed by put_page_and_type(). This > > then allows free_shared_domheap_page() to be purged. > > I doubt whether using a normal domheap page is a right thing in concept. > Note the APIC access page is the backend for virtual LAPIC_BASE which is > a MMIO range from guest p.o.v. Well, using a non-owned domheap page as a fake xenheap page just looks weird. Also 'special' code like this is historically a source of XSAs. IMO a straightforward domheap page here is totally appropriate; AFAICT the code is only as it is to work round the max_pages limit and the lack of a relinquish_resources() hvmfunc. > > > > > There is, however, some fall-out from this simplification: > > > > - alloc_domheap_page() will now call assign_pages() and run into the > fact > > that 'max_pages' is not set until some time after domain_create(). To > > avoid an allocation failure, assign_pages() is modified to ignore the > > max_pages limit if 'creation_finished' is false. That value is not set > > to true until domain_unpause_by_systemcontroller() is called, and thus > > the guest cannot run (and hence cause memory allocation) until > > creation_finished is set to true. > > However, doing so opens the window of no guard of allocations in > the whole VM creation time. I'm not sure whether it's a worthwhile > just for fixing a bad-looking but conceptually-correct code. > I don’t think the code is conceptually correct. The lack of guard is only for pages assigned to that domain, and since the domain cannot possibly be running then I don't see it is a problem. That said, an alternative would be to have the domain creation code to set max_pages is to a minimal initial value sufficient to cover any alloc_domheap_pages() calls done by sub-functions. Paul > > > > - Because the domheap page is no longer a pseudo-xenheap page, the > > reference counting will prevent the domain from being destroyed. Thus > > the call to vmx_free_vlapic_mapping() is moved from the > > domain_destroy() method into the domain_relinquish_resources() method. > > > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > > --- > > Cc: Jun Nakajima <jun.nakajima@intel.com> > > Cc: Kevin Tian <kevin.tian@intel.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> > > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Julien Grall <julien@xen.org> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Cc: Stefano Stabellini <sstabellini@kernel.org> > > --- > > xen/arch/x86/hvm/vmx/vmx.c | 29 ++++++++++++++++++++++------- > > xen/arch/x86/mm.c | 10 ---------- > > xen/common/page_alloc.c | 3 ++- > > xen/include/asm-x86/mm.h | 2 -- > > 4 files changed, 24 insertions(+), 20 deletions(-) > > > > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > > index 3fd3ac61e1..a2e6081485 100644 > > --- a/xen/arch/x86/hvm/vmx/vmx.c > > +++ b/xen/arch/x86/hvm/vmx/vmx.c > > @@ -421,10 +421,6 @@ static int vmx_domain_initialise(struct domain *d) > > } > > > > static void vmx_domain_relinquish_resources(struct domain *d) > > -{ > > -} > > - > > -static void vmx_domain_destroy(struct domain *d) > > { > > if ( !has_vlapic(d) ) > > return; > > @@ -432,6 +428,10 @@ static void vmx_domain_destroy(struct domain *d) > > vmx_free_vlapic_mapping(d); > > } > > > > +static void vmx_domain_destroy(struct domain *d) > > +{ > > +} > > + > > static int vmx_vcpu_initialise(struct vcpu *v) > > { > > int rc; > > @@ -3034,12 +3034,22 @@ static int vmx_alloc_vlapic_mapping(struct > > domain *d) > > if ( !cpu_has_vmx_virtualize_apic_accesses ) > > return 0; > > > > - pg = alloc_domheap_page(d, MEMF_no_owner); > > + pg = alloc_domheap_page(d, 0); > > if ( !pg ) > > return -ENOMEM; > > + > > + if ( !get_page_and_type(pg, d, PGT_writable_page) ) > > + { > > + /* > > + * The domain can't possibly know about this page yet, so > failure > > + * here is a clear indication of something fishy going on. > > + */ > > + domain_crash(d); > > + return -ENODATA; > > + } > > + > > 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; > > > > return set_mmio_p2m_entry(d, > > paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn, > > @@ -3052,7 +3062,12 @@ static void vmx_free_vlapic_mapping(struct > > domain *d) > > mfn_t mfn = d->arch.hvm.vmx.apic_access_mfn; > > > > if ( !mfn_eq(mfn, INVALID_MFN) ) > > - free_shared_domheap_page(mfn_to_page(mfn)); > > + { > > + struct page_info *pg = mfn_to_page(mfn); > > + > > + put_page_alloc_ref(pg); > > + put_page_and_type(pg); > > + } > > } > > > > static void vmx_install_vlapic_mapping(struct vcpu *v) > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > > index 654190e9e9..2a6d2e8af9 100644 > > --- a/xen/arch/x86/mm.c > > +++ b/xen/arch/x86/mm.c > > @@ -496,16 +496,6 @@ void share_xen_page_with_guest(struct page_info > > *page, struct domain *d, > > spin_unlock(&d->page_alloc_lock); > > } > > > > -void free_shared_domheap_page(struct page_info *page) > > -{ > > - put_page_alloc_ref(page); > > - if ( !test_and_clear_bit(_PGC_xen_heap, &page->count_info) ) > > - ASSERT_UNREACHABLE(); > > - page->u.inuse.type_info = 0; > > - page_set_owner(page, NULL); > > - free_domheap_page(page); > > -} > > - > > void make_cr3(struct vcpu *v, mfn_t mfn) > > { > > struct domain *d = v->domain; > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > > index 919a270587..ef327072ed 100644 > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -2269,7 +2269,8 @@ int assign_pages( > > > > if ( !(memflags & MEMF_no_refcount) ) > > { > > - if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) ) > > + if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) && > > + d->creation_finished ) > > { > > gprintk(XENLOG_INFO, "Over-allocation for domain %u: " > > "%u > %u\n", d->domain_id, > > diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h > > index 2ca8882ad0..e429f38228 100644 > > --- a/xen/include/asm-x86/mm.h > > +++ b/xen/include/asm-x86/mm.h > > @@ -317,8 +317,6 @@ struct page_info > > > > #define maddr_get_owner(ma) (page_get_owner(maddr_to_page((ma)))) > > > > -extern void free_shared_domheap_page(struct page_info *page); > > - > > #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START) > > extern unsigned long max_page; > > extern unsigned long total_pages; > > -- > > 2.20.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE 2020-01-21 12:00 ` [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE Paul Durrant 2020-01-21 12:29 ` Julien Grall 2020-01-22 3:19 ` Tian, Kevin @ 2020-01-22 16:17 ` Jan Beulich 2020-01-22 16:27 ` Durrant, Paul 2 siblings, 1 reply; 17+ messages in thread From: Jan Beulich @ 2020-01-22 16:17 UTC (permalink / raw) To: Paul Durrant Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Jun Nakajima, xen-devel, Roger Pau Monné On 21.01.2020 13:00, Paul Durrant wrote: > vmx_alloc_vlapic_mapping() currently contains some very odd looking code > that allocates a MEMF_no_owner domheap page and then shares with the guest > as if it were a xenheap page. This then requires vmx_free_vlapic_mapping() > to call a special function in the mm code: free_shared_domheap_page(). > > By using a 'normal' domheap page (i.e. by not passing MEMF_no_owner to > alloc_domheap_page()), the odd looking code in vmx_alloc_vlapic_mapping() > can simply use get_page_and_type() to set up a writable mapping before > insertion in the P2M and vmx_free_vlapic_mapping() can simply release the > page using put_page_alloc_ref() followed by put_page_and_type(). This > then allows free_shared_domheap_page() to be purged. > > There is, however, some fall-out from this simplification: > > - alloc_domheap_page() will now call assign_pages() and run into the fact > that 'max_pages' is not set until some time after domain_create(). To > avoid an allocation failure, assign_pages() is modified to ignore the > max_pages limit if 'creation_finished' is false. That value is not set > to true until domain_unpause_by_systemcontroller() is called, and thus > the guest cannot run (and hence cause memory allocation) until > creation_finished is set to true. But this check is also to guard against the tool stack (or possibly the controlling stubdom) to cause excess allocation. I don't think the checking should be undermined like this (and see also below). Since certainly you've looked into this while creating the patch, could you remind me why it is that this page needs to be owned (as in its owner field set accordingly) by the guest? It's a helper page only, after all. > @@ -3034,12 +3034,22 @@ static int vmx_alloc_vlapic_mapping(struct domain *d) > if ( !cpu_has_vmx_virtualize_apic_accesses ) > return 0; > > - pg = alloc_domheap_page(d, MEMF_no_owner); > + pg = alloc_domheap_page(d, 0); Did you consider passing MEMF_no_refcount here, to avoid the fiddling with assign_pages()? That'll in particular also avoid ... > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -2269,7 +2269,8 @@ int assign_pages( > > if ( !(memflags & MEMF_no_refcount) ) > { > - if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) ) > + if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) && > + d->creation_finished ) > { > gprintk(XENLOG_INFO, "Over-allocation for domain %u: " > "%u > %u\n", d->domain_id, ... invoking domain_adjust_tot_pages() right below here, which is wrong for helper pages like this one (as it reduces the amount the domain is actually permitted to allocate). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE 2020-01-22 16:17 ` Jan Beulich @ 2020-01-22 16:27 ` Durrant, Paul 0 siblings, 0 replies; 17+ messages in thread From: Durrant, Paul @ 2020-01-22 16:27 UTC (permalink / raw) To: Jan Beulich Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Jun Nakajima, xen-devel, Roger Pau Monné > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 22 January 2020 16:17 > To: Durrant, Paul <pdurrant@amazon.co.uk> > Cc: xen-devel@lists.xenproject.org; Jun Nakajima <jun.nakajima@intel.com>; > Kevin Tian <kevin.tian@intel.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné > <roger.pau@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian > Jackson <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>; Konrad > Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini > <sstabellini@kernel.org> > Subject: Re: [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for > APIC_DEFAULT_PHYS_BASE > > On 21.01.2020 13:00, Paul Durrant wrote: > > vmx_alloc_vlapic_mapping() currently contains some very odd looking code > > that allocates a MEMF_no_owner domheap page and then shares with the > guest > > as if it were a xenheap page. This then requires > vmx_free_vlapic_mapping() > > to call a special function in the mm code: free_shared_domheap_page(). > > > > By using a 'normal' domheap page (i.e. by not passing MEMF_no_owner to > > alloc_domheap_page()), the odd looking code in > vmx_alloc_vlapic_mapping() > > can simply use get_page_and_type() to set up a writable mapping before > > insertion in the P2M and vmx_free_vlapic_mapping() can simply release > the > > page using put_page_alloc_ref() followed by put_page_and_type(). This > > then allows free_shared_domheap_page() to be purged. > > > > There is, however, some fall-out from this simplification: > > > > - alloc_domheap_page() will now call assign_pages() and run into the > fact > > that 'max_pages' is not set until some time after domain_create(). To > > avoid an allocation failure, assign_pages() is modified to ignore the > > max_pages limit if 'creation_finished' is false. That value is not set > > to true until domain_unpause_by_systemcontroller() is called, and thus > > the guest cannot run (and hence cause memory allocation) until > > creation_finished is set to true. > > But this check is also to guard against the tool stack (or possibly > the controlling stubdom) to cause excess allocation. I don't think > the checking should be undermined like this (and see also below). > Ok. > Since certainly you've looked into this while creating the patch, > could you remind me why it is that this page needs to be owned (as > in its owner field set accordingly) by the guest? It's a helper > page only, after all. > Not sure why it was done that way. It's inserted into the guest P2M so having it owned by the guest seems like the right thing to do. A malicious guest could decrease-reservation it and I guess it avoids special-casing there. > > @@ -3034,12 +3034,22 @@ static int vmx_alloc_vlapic_mapping(struct > domain *d) > > if ( !cpu_has_vmx_virtualize_apic_accesses ) > > return 0; > > > > - pg = alloc_domheap_page(d, MEMF_no_owner); > > + pg = alloc_domheap_page(d, 0); > > Did you consider passing MEMF_no_refcount here, to avoid the > fiddling with assign_pages()? That'll in particular also > avoid ... > You remember what happened last time we did that (with the ioreq server page), right? That's why assign_pages() vetoes non-refcounted pages. > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -2269,7 +2269,8 @@ int assign_pages( > > > > if ( !(memflags & MEMF_no_refcount) ) > > { > > - if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) ) > > + if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) && > > + d->creation_finished ) > > { > > gprintk(XENLOG_INFO, "Over-allocation for domain %u: " > > "%u > %u\n", d->domain_id, > > ... invoking domain_adjust_tot_pages() right below here, which > is wrong for helper pages like this one (as it reduces the > amount the domain is actually permitted to allocate). > True, but there is 'slop' to deal with things like the ioreq pages and I think this page is logically similar. Paul _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-01-22 16:27 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-21 12:00 [Xen-devel] [PATCH 0/3] purge free_shared_domheap_page() Paul Durrant 2020-01-21 12:00 ` [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe Paul Durrant 2020-01-22 2:51 ` Tian, Kevin 2020-01-22 14:05 ` Andrew Cooper 2020-01-22 15:48 ` Jan Beulich 2020-01-21 12:00 ` [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method Paul Durrant 2020-01-22 15:50 ` Jan Beulich 2020-01-22 15:56 ` Durrant, Paul 2020-01-22 16:00 ` Jan Beulich 2020-01-22 16:02 ` Durrant, Paul 2020-01-21 12:00 ` [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE Paul Durrant 2020-01-21 12:29 ` Julien Grall 2020-01-21 12:37 ` Durrant, Paul 2020-01-22 3:19 ` Tian, Kevin 2020-01-22 11:25 ` Durrant, Paul 2020-01-22 16:17 ` Jan Beulich 2020-01-22 16:27 ` Durrant, Paul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).