[PATCH] x86: Fix build with the get/set_reg() infrastructure

Andrew Cooper posted 1 patch 2 years, 2 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220121104901.22702-1-andrew.cooper3@citrix.com
xen/arch/x86/include/asm/hvm/hvm.h   |  9 +++++++++
xen/arch/x86/include/asm/pv/domain.h | 17 +++++++++++++----
2 files changed, 22 insertions(+), 4 deletions(-)
[PATCH] x86: Fix build with the get/set_reg() infrastructure
Posted by Andrew Cooper 2 years, 2 months ago
I clearly messed up concluding that the stubs were safe to drop.

The is_{pv,hvm}_domain() predicates are not symmetrical with both CONFIG_PV
and CONFIG_HVM.  As a result logic of the form `if ( pv/hvm ) ... else ...`
will always have one side which can't be DCE'd.

While technically only the hvm stubs are needed, due to the use of the
is_pv_domain() predicate in guest_{rd,wr}msr(), sort out the pv stubs too to
avoid leaving a bear trap for future users.

Fixes: 88d3ff7ab15d ("x86/guest: Introduce {get,set}_reg() infrastructure")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/include/asm/hvm/hvm.h   |  9 +++++++++
 xen/arch/x86/include/asm/pv/domain.h | 17 +++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index 842f98763c4b..76170c129c35 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -854,6 +854,15 @@ static inline int hvm_vmtrace_get_option(
     return -EOPNOTSUPP;
 }
 
+static inline uint64_t hvm_get_reg(struct vcpu *v, unsigned int reg)
+{
+    ASSERT_UNREACHABLE();
+}
+static inline void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
+{
+    ASSERT_UNREACHABLE();
+}
+
 #define is_viridian_domain(d) ((void)(d), false)
 #define is_viridian_vcpu(v) ((void)(v), false)
 #define has_viridian_time_ref_count(d) ((void)(d), false)
diff --git a/xen/arch/x86/include/asm/pv/domain.h b/xen/arch/x86/include/asm/pv/domain.h
index 3a67816764c9..4ef4660a01ac 100644
--- a/xen/arch/x86/include/asm/pv/domain.h
+++ b/xen/arch/x86/include/asm/pv/domain.h
@@ -65,10 +65,6 @@ static inline unsigned long get_pcid_bits(const struct vcpu *v, bool is_xpti)
 #endif
 }
 
-/* See hvm_{get,set}_reg() for description. */
-uint64_t pv_get_reg(struct vcpu *v, unsigned int reg);
-void pv_set_reg(struct vcpu *v, unsigned int reg, uint64_t val);
-
 #ifdef CONFIG_PV
 
 void pv_vcpu_destroy(struct vcpu *v);
@@ -93,6 +89,10 @@ unsigned long pv_fixup_guest_cr4(const struct vcpu *v, unsigned long cr4);
 /* Create a cr4 value to load into hardware, based on vcpu settings. */
 unsigned long pv_make_cr4(const struct vcpu *v);
 
+/* See hvm_{get,set}_reg() for description. */
+uint64_t pv_get_reg(struct vcpu *v, unsigned int reg);
+void pv_set_reg(struct vcpu *v, unsigned int reg, uint64_t val);
+
 bool xpti_pcid_enabled(void);
 
 #else  /* !CONFIG_PV */
@@ -106,6 +106,15 @@ static inline int pv_domain_initialise(struct domain *d) { return -EOPNOTSUPP; }
 
 static inline unsigned long pv_make_cr4(const struct vcpu *v) { return ~0ul; }
 
+static inline uint64_t pv_get_reg(struct vcpu *v, unsigned int reg)
+{
+    ASSERT_UNREACHABLE();
+}
+static inline void pv_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
+{
+    ASSERT_UNREACHABLE();
+}
+
 #endif	/* CONFIG_PV */
 
 void paravirt_ctxt_switch_from(struct vcpu *v);
-- 
2.11.0


Re: [PATCH] x86: Fix build with the get/set_reg() infrastructure
Posted by Jan Beulich 2 years, 2 months ago
On 21.01.2022 11:49, Andrew Cooper wrote:
> I clearly messed up concluding that the stubs were safe to drop.
> 
> The is_{pv,hvm}_domain() predicates are not symmetrical with both CONFIG_PV
> and CONFIG_HVM.  As a result logic of the form `if ( pv/hvm ) ... else ...`
> will always have one side which can't be DCE'd.
> 
> While technically only the hvm stubs are needed, due to the use of the
> is_pv_domain() predicate in guest_{rd,wr}msr(), sort out the pv stubs too to
> avoid leaving a bear trap for future users.

Well, as said on irc - only the PV stubs ought to be needed if the
conditionals always used "if ( is_hvm_...() )" / "if ( !is_hvm_...() )".
Despite us making use of this property elsewhere, you appear to dislike
that though ...

Hence / nevertheless:

> Fixes: 88d3ff7ab15d ("x86/guest: Introduce {get,set}_reg() infrastructure")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


Re: [PATCH] x86: Fix build with the get/set_reg() infrastructure
Posted by Andrew Cooper 2 years, 2 months ago
On 21/01/2022 11:03, Jan Beulich wrote:
> On 21.01.2022 11:49, Andrew Cooper wrote:
>> I clearly messed up concluding that the stubs were safe to drop.
>>
>> The is_{pv,hvm}_domain() predicates are not symmetrical with both CONFIG_PV
>> and CONFIG_HVM.  As a result logic of the form `if ( pv/hvm ) ... else ...`
>> will always have one side which can't be DCE'd.
>>
>> While technically only the hvm stubs are needed, due to the use of the
>> is_pv_domain() predicate in guest_{rd,wr}msr(), sort out the pv stubs too to
>> avoid leaving a bear trap for future users.
> Well, as said on irc - only the PV stubs ought to be needed if the
> conditionals always used "if ( is_hvm_...() )" / "if ( !is_hvm_...() )".
> Despite us making use of this property elsewhere, you appear to dislike
> that though ...

Yes - I consider that an accident at best, and not something which
people can reasonably be expected to know or use.

If nothing else, the pv form is the more useful one to use, given a
potential pvh-dom0 future where CONFIG_PV might credibly be dropped by
downstreams.

>
> Hence / nevertheless:
>
>> Fixes: 88d3ff7ab15d ("x86/guest: Introduce {get,set}_reg() infrastructure")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks, but CI still says no.

Some compilers don't tolerate the lack of a return value in a non-void
function, despite the ASSERT_UNREACHABLE().  It might be down to the
absence of __builtin_unreahcable().

I've folded "return 0;" into the two get stubs, and will see how that
fairs, but given how trivial it is, I don't intend to post a v2 before
committing.

~Andrew