Message ID | 56CDB4FD02000078000D5A34@prv-mh.provo.novell.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Wednesday, February 24, 2016 8:50 PM > To: George Dunlap <george.dunlap@citrix.com>; Wu, Feng > <feng.wu@intel.com> > Cc: Doug Goldstein <cardoe@cardoe.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; Dario Faggioli <dario.faggioli@citrix.com>; > GeorgeDunlap <george.dunlap@eu.citrix.com>; Tian, Kevin > <kevin.tian@intel.com>; xen-devel@lists.xen.org; Keir Fraser <keir@xen.org> > Subject: Re: [Xen-devel] [PATCH v13 1/2] vmx: VT-d posted-interrupt core logic > handling > > >>> On 24.02.16 at 13:09, <george.dunlap@citrix.com> wrote: > > Another reason for such a comment is that it actually raises awareness > > that the hook isn't properly structured: if you get such a compile > > error, then it's either not defined in the right place, or it's not > > using the right calling convention. > > Well, why I generally agree with you here, I'm afraid there are > many pre-existing instances in our headers. Cleaning that up is > likely going to be a major work item. > > > It also makes me realize that this code will no longer build on ARM, > > since arch_do_block() is only defined in asm-x86 (and not asm-arm). > > The patch has > > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -310,6 +310,8 @@ static inline void free_vcpu_guest_context(struct > vcpu_guest_context *vgc) > xfree(vgc); > } > > +static inline void arch_vcpu_block(struct vcpu *v) {} > + > #endif /* __ASM_DOMAIN_H__ */ > > /* > > (and for the avoidance of doubt there's no arch_do_block() afaics). > > > It seems like to do the callback properly, we should do something like > > the attached. Jan, what do you think? > > Well, as per above that would address the particular issue here > without addressing the many other existing ones, and it would > cause out of line functions _plus_ another indirect call when the > idea is to have such hooks inlined as far as possible. > > But you indeed point out one important problem - the hook as it > is right now lacks a has_hvm_container_vcpu(), and hence would > break for PV guests. Do you mean I need to add has_hvm_container_vcpu() check in macro arch_vcpu_block()? But .vcpu_block is assigned in vmx_pi_hooks_assign(). I am not clear how this hooks can affect PV guests, could you please elaborate a bit more? Thanks a lot! Thanks, Feng > > Jan
>>> On 26.02.16 at 02:30, <feng.wu@intel.com> wrote: > >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Wednesday, February 24, 2016 8:50 PM >> To: George Dunlap <george.dunlap@citrix.com>; Wu, Feng >> <feng.wu@intel.com> >> Cc: Doug Goldstein <cardoe@cardoe.com>; Andrew Cooper >> <andrew.cooper3@citrix.com>; Dario Faggioli <dario.faggioli@citrix.com>; >> GeorgeDunlap <george.dunlap@eu.citrix.com>; Tian, Kevin >> <kevin.tian@intel.com>; xen-devel@lists.xen.org; Keir Fraser <keir@xen.org> >> Subject: Re: [Xen-devel] [PATCH v13 1/2] vmx: VT-d posted-interrupt core logic >> handling >> >> >>> On 24.02.16 at 13:09, <george.dunlap@citrix.com> wrote: >> > Another reason for such a comment is that it actually raises awareness >> > that the hook isn't properly structured: if you get such a compile >> > error, then it's either not defined in the right place, or it's not >> > using the right calling convention. >> >> Well, why I generally agree with you here, I'm afraid there are >> many pre-existing instances in our headers. Cleaning that up is >> likely going to be a major work item. >> >> > It also makes me realize that this code will no longer build on ARM, >> > since arch_do_block() is only defined in asm-x86 (and not asm-arm). >> >> The patch has >> >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -310,6 +310,8 @@ static inline void free_vcpu_guest_context(struct >> vcpu_guest_context *vgc) >> xfree(vgc); >> } >> >> +static inline void arch_vcpu_block(struct vcpu *v) {} >> + >> #endif /* __ASM_DOMAIN_H__ */ >> >> /* >> >> (and for the avoidance of doubt there's no arch_do_block() afaics). >> >> > It seems like to do the callback properly, we should do something like >> > the attached. Jan, what do you think? >> >> Well, as per above that would address the particular issue here >> without addressing the many other existing ones, and it would >> cause out of line functions _plus_ another indirect call when the >> idea is to have such hooks inlined as far as possible. >> >> But you indeed point out one important problem - the hook as it >> is right now lacks a has_hvm_container_vcpu(), and hence would >> break for PV guests. > > Do you mean I need to add has_hvm_container_vcpu() check in > macro arch_vcpu_block()? But .vcpu_block is assigned in > vmx_pi_hooks_assign(). I am not clear how this hooks can affect > PV guests, could you please elaborate a bit more? Thanks a lot! Quoting you patch (v12, because it looks slightly better, but the difference doesn't matter for this discussion): #define arch_vcpu_block(v) ({ \ if ( (v)->domain->arch.hvm_domain.vmx.vcpu_block ) \ (v)->domain->arch.hvm_domain.vmx.vcpu_block((v)); \ }) and quoting asm-x86/domain.h: struct arch_domain { ... union { struct pv_domain pv_domain; struct hvm_domain hvm_domain; }; ... }; Hence accessing the field for PV domains is invalid. Jan
> Quoting you patch (v12, because it looks slightly better, but > the difference doesn't matter for this discussion): > > #define arch_vcpu_block(v) ({ \ > if ( (v)->domain->arch.hvm_domain.vmx.vcpu_block ) \ > (v)->domain->arch.hvm_domain.vmx.vcpu_block((v)); \ > }) > > and quoting asm-x86/domain.h: > > struct arch_domain > { > ... > union { > struct pv_domain pv_domain; > struct hvm_domain hvm_domain; > }; > ... > }; > > Hence accessing the field for PV domains is invalid. Oh, that is right! Accessing 'hvm_domain' itself needs to be gated by has_hvm_container_vcpu(), Thanks for pointing this out! Thanks, Feng > > Jan
--- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -310,6 +310,8 @@ static inline void free_vcpu_guest_context(struct vcpu_guest_context *vgc) xfree(vgc); } +static inline void arch_vcpu_block(struct vcpu *v) {} + #endif /* __ASM_DOMAIN_H__ */ /*