diff mbox

[v13,1/2] vmx: VT-d posted-interrupt core logic handling

Message ID 56CDB4FD02000078000D5A34@prv-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Beulich Feb. 24, 2016, 12:49 p.m. UTC
>>> 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


(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.

Jan

Comments

Wu, Feng Feb. 26, 2016, 1:30 a.m. UTC | #1
> -----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
Jan Beulich Feb. 26, 2016, 8:28 a.m. UTC | #2
>>> 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
Wu, Feng Feb. 26, 2016, 9:06 a.m. UTC | #3
> 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
diff mbox

Patch

--- 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__ */
 
 /*