Re: [Xen-devel] [PATCH v13 1/2] vmx: VT-d posted-interrupt core logic handling

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.02.16 at 02:30, <feng.wu@xxxxxxxxx> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Wednesday, February 24, 2016 8:50 PM
>> To: George Dunlap <george.dunlap@xxxxxxxxxx>; Wu, Feng
>> <feng.wu@xxxxxxxxx>
>> Cc: Doug Goldstein <cardoe@xxxxxxxxxx>; Andrew Cooper
>> <andrew.cooper3@xxxxxxxxxx>; Dario Faggioli <dario.faggioli@xxxxxxxxxx>;
>> GeorgeDunlap <george.dunlap@xxxxxxxxxxxxx>; Tian, Kevin
>> <kevin.tian@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxx; Keir Fraser <keir@xxxxxxx>
>> 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@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel