[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 1/2] x86/HVM: expose VM assist hypercall

On 21/04/2020 06:54, Jan Beulich wrote:
On 20.04.2020 19:53, Julien Grall wrote:
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1517,20 +1517,23 @@ long do_vcpu_op(int cmd, unsigned int vc
       return rc;
   -#ifdef VM_ASSIST_VALID
-long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
-               unsigned long valid)
+#ifdef arch_vm_assist_valid

How about naming the function arch_vm_assist_valid_mask?

Certainly a possibility, albeit to me the gain would be marginal
and possibly not outweigh the growth in length. Andrew, any

You have a point regarding the length of the function.

--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -700,6 +700,20 @@ static inline void pv_inject_sw_interrup
+#define PV_VM_ASSIST_VALID  ((1UL << VMASST_TYPE_4gb_segments)        | \
+                             (1UL << VMASST_TYPE_4gb_segments_notify) | \
+                             (1UL << VMASST_TYPE_writable_pagetables) | \
+                             (1UL << VMASST_TYPE_pae_extended_cr3)    | \
+                             (1UL << VMASST_TYPE_architectural_iopl)  | \
+                             (1UL << VMASST_TYPE_runstate_update_flag)| \
+                             (1UL << VMASST_TYPE_m2p_strict))
+#define HVM_VM_ASSIST_VALID (1UL << VMASST_TYPE_runstate_update_flag)
+#define arch_vm_assist_valid(d) \
+    (is_hvm_domain(d) ? HVM_VM_ASSIST_VALID \
+                      : is_pv_32bit_domain(d) ? (uint32_t)PV_VM_ASSIST_VALID \

I understand this is matching the current code, however without
looking at the rest of patch this is not clear why the cast. May
I suggest to add a comment explaining the rationale?

Hmm, I can state that the rationale is history. Many of the assists in
the low 32 bits are for 32-bit guests only. But we can't start refusing
a 64-bit kernel requesting them. The ones in the high 32 bits are, for
now, applicable to 64-bit guests only, and have always been refused for
32-bit ones.
Imo if anything an explanation on where new bits should be put should
go next to the VMASST_TYPE_* definitions in the public header, yet then
again the public headers aren't (imo) a good place to put
implementation detail comments.

How about splitting PV_VM_ASSIST_VALID in two? One would contain 64-bit PV specific flags and the other common PV flags?

This should make the code more obvious and easier to read for someone less familiar with the area.

It also means we could have a BUILD_BUG_ON() to check at build time that no flags are added above 32-bit.


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.