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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 24/02/16 03:09, Wu, Feng wrote:
> 
> 
>> -----Original Message-----
>> From: Doug Goldstein [mailto:cardoe@xxxxxxxxxx]
>> Sent: Wednesday, February 24, 2016 11:02 AM
>> To: Jan Beulich <JBeulich@xxxxxxxx>; Wu, Feng <feng.wu@xxxxxxxxx>
>> Cc: Tian, Kevin <kevin.tian@xxxxxxxxx>; Keir Fraser <keir@xxxxxxx>; George
>> Dunlap <george.dunlap@xxxxxxxxxxxxx>; Andrew Cooper
>> <andrew.cooper3@xxxxxxxxxx>; Dario Faggioli <dario.faggioli@xxxxxxxxxx>; xen-
>> devel@xxxxxxxxxxxxx
>> Subject: Re: [Xen-devel] [PATCH v13 1/2] vmx: VT-d posted-interrupt core 
>> logic
>> handling
>>
>> On 2/23/16 10:34 AM, Jan Beulich wrote:
>>>>>> On 23.02.16 at 09:34, <feng.wu@xxxxxxxxx> wrote:
>>>
>>>> --- a/xen/include/asm-x86/hvm/hvm.h
>>>> +++ b/xen/include/asm-x86/hvm/hvm.h
>>>> @@ -565,6 +565,12 @@ const char *hvm_efer_valid(const struct vcpu *v,
>>>> uint64_t value,
>>>>                             signed int cr0_pg);
>>>>  unsigned long hvm_cr4_guest_reserved_bits(const struct vcpu *v, bool_t
>>>> restore);
>>>>
>>>> +#define arch_vcpu_block(v) ({                                             
>>>>      \
>>>> +    void (*func) (struct vcpu *) = (v)->domain-
>>> arch.hvm_domain.vmx.vcpu_block;\
>>>> +    if ( func )                                                           
>>>>      \
>>>> +        func(v);                                                          
>>>>      \
>>>> +})
>>>
>>> See my comment on v12. The code structure actually was better
>>> there, and all you needed to do is introduce a local variable.
>>
>> Wouldn't this be a bit cleaner (and type-safier (inventing a word here))
>> to do with a static inline function?
> 
> As I mentioned in earlier version, after making it a inline function, I
> encountered building failures, which is related to using
> '(v)->domain->arch.hvm_domain.vmx.vcpu_block ' here since it refers to
> some data structure, it is not so straightforward to address it, so I change
> it to a macro, just like other micros in this file, which refers to
> ' (v)->arch.hvm_vcpu.....'.

I did a brief search for this previous conversation, but I couldn't find
where you said what the build failures were; and I couldn't off the top
of my head imagine why dereferencing the pointer that way in a macro
should be different than dereferencing it in an inline function (and
"since it refers to some data structure" doesn't give me a clue).

Having just gone and made that change, it turns out that at the point of
this definition, the vmx struct hasn't been defined yet, so the compiler
can't verify that the struct elements actually exist.  (The actual error
message is "dereferencing pointer to incomplete type".)

In general, if there is important information like that, you should add
a comment, so that other people coming in and asking this sort of
question can get the answer from the code, rather than having to ask
and/or dig through mailing list archives; e.g.:

/*
 * This must be defined as a macro instead of an inline, because the
 * vmx struct has not yet been defined.
 */

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.

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

It seems like to do the callback properly, we should do something like
the attached.  Jan, what do you think?

It compiles but won't actually do anything at the moment, since it
doesn't define a vmx function for vcpu_block.  You'll need to add that
in, as well as make a 'stub' callback for ARM.

(Thanks Doug, for catching this!)

 -George

Attachment: vcpu_block_callback.diff
Description: Text Data

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