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

[George Dunlap <george.dunlap@citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 3730 bytes --]

On 24/02/16 03:09, Wu, Feng wrote:
> 
> 
>> -----Original Message-----
>> From: Doug Goldstein [mailto:cardoe@cardoe.com]
>> Sent: Wednesday, February 24, 2016 11:02 AM
>> To: Jan Beulich <JBeulich@suse.com>; Wu, Feng <feng.wu@intel.com>
>> Cc: Tian, Kevin <kevin.tian@intel.com>; Keir Fraser <keir@xen.org>; George
>> Dunlap <george.dunlap@eu.citrix.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Dario Faggioli <dario.faggioli@citrix.com>; xen-
>> devel@lists.xen.org
>> 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@intel.com> 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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vcpu_block_callback.diff --]
[-- Type: text/x-patch; name="vcpu_block_callback.diff", Size: 2863 bytes --]

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9d43f7b..4c9bce2 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1572,6 +1572,15 @@ int arch_vcpu_reset(struct vcpu *v)
     return 0;
 }
 
+
+int arch_vcpu_block(struct vcpu *v) {
+    if ( is_hvm_vcpu(v) ) {
+        return hvm_vcpu_block(v);
+    } else {
+        return 0;
+    }
+}
+
 long
 arch_do_vcpu_op(
     int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a29c421..9ad1cfd 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4237,6 +4237,14 @@ void hvm_task_switch(
     hvm_unmap_entry(nptss_desc);
 }
 
+int hvm_vcpu_block(struct vcpu *v) {
+    if ( hvm_funcs.vcpu_block ) {
+        return hvm_funcs.vcpu_block(v);
+    } else {
+        return 0;
+    }
+}
+
 #define HVMCOPY_from_guest (0u<<0)
 #define HVMCOPY_to_guest   (1u<<0)
 #define HVMCOPY_no_fault   (0u<<1)
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 915f6d8..3926909 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -223,6 +223,8 @@ struct hvm_function_table {
     int (*altp2m_vcpu_emulate_vmfunc)(struct cpu_user_regs *regs);
 
     uint64_t (*scale_tsc)(const struct vcpu *v, uint64_t tsc);
+
+    int (*vcpu_block) (const struct vcpu *v);
 };
 
 extern struct hvm_function_table hvm_funcs;
@@ -443,6 +445,8 @@ void hvm_task_switch(
     uint16_t tss_sel, enum hvm_task_switch_reason taskswitch_reason,
     int32_t errcode);
 
+int hvm_vcpu_block(struct vcpu *v);
+
 enum hvm_access_type {
     hvm_access_insn_fetch,
     hvm_access_none,
@@ -580,12 +584,6 @@ 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);                                                               \
-})
-
 #endif /* __ASM_X86_HVM_HVM_H__ */
 
 /*
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index b47a3fe..89d2a5c 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -678,6 +678,11 @@ void startup_cpu_idle_loop(void);
 extern void (*pm_idle) (void);
 extern void (*dead_idle) (void);
 
+/*
+ * Arch-specific hook to be called when the guest blocks (either in
+ * vcpu_block() or vcpu_poll).
+ */
+int arch_vcpu_block(struct vcpu *v);
 
 /*
  * Creates a continuation to resume the current hypercall. The caller should

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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