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