On x86, idle and other system domains are implicitly PV. While I
couldn't spot any cases where this is actively a problem, some cases
required quite close inspection to be certain there couldn't e.g. be
some ASSERT_UNREACHABLE() that would trigger in this case. Let's be on
the safe side and make sure these always have is_pv_domain() returning
true.

For the build to still work, this requires a few adjustments elsewhere.
In particular is_pv_64bit_domain() now gains a CONFIG_PV dependency,
which means that is_pv_32bit_domain() || is_pv_64bit_domain() is no
longer guaranteed to be the same as is_pv_domain().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Add comment.

--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -571,7 +571,7 @@ int __init construct_dom0(struct domain
 
     if ( is_hvm_domain(d) )
         rc = dom0_construct_pvh(d, image, image_headroom, initrd, cmdline);
-    else if ( is_pv_domain(d) )
+    else if ( is_pv_64bit_domain(d) || is_pv_32bit_domain(d) )
         rc = dom0_construct_pv(d, image, image_headroom, initrd, cmdline);
     else
         panic("Cannot construct Dom0. No guest interface available\n");
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1564,6 +1564,7 @@ arch_do_vcpu_op(
  */
 static void load_segments(struct vcpu *n)
 {
+#ifdef CONFIG_PV
     struct cpu_user_regs *uregs = &n->arch.user_regs;
     unsigned long gsb = 0, gss = 0;
     bool compat = is_pv_32bit_vcpu(n);
@@ -1729,6 +1730,7 @@ static void load_segments(struct vcpu *n
         regs->cs            = FLAT_KERNEL_CS;
         regs->rip           = pv->failsafe_callback_eip;
     }
+#endif
 }
 
 /*
@@ -1743,6 +1745,7 @@ static void load_segments(struct vcpu *n
  */
 static void save_segments(struct vcpu *v)
 {
+#ifdef CONFIG_PV
     struct cpu_user_regs *regs = &v->arch.user_regs;
 
     read_sregs(regs);
@@ -1768,6 +1771,7 @@ static void save_segments(struct vcpu *v
         else
             v->arch.pv.gs_base_user = gs_base;
     }
+#endif
 }
 
 void paravirt_ctxt_switch_from(struct vcpu *v)
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -457,13 +457,13 @@ long arch_do_domctl(
     case XEN_DOMCTL_set_address_size:
         if ( is_hvm_domain(d) )
             ret = -EOPNOTSUPP;
+        else if ( is_pv_64bit_domain(d) && domctl->u.address_size.size == 32 )
+            ret = switch_compat(d);
         else if ( is_pv_domain(d) )
         {
             if ( ((domctl->u.address_size.size == 64) && !d->arch.pv.is_32bit) ||
                  ((domctl->u.address_size.size == 32) &&  d->arch.pv.is_32bit) )
                 ret = 0;
-            else if ( domctl->u.address_size.size == 32 )
-                ret = switch_compat(d);
             else
                 ret = -EINVAL;
         }
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1036,9 +1036,14 @@ static always_inline bool is_control_dom
 
 #define VM_ASSIST(d, t) (test_bit(VMASST_TYPE_ ## t, &(d)->vm_assist))
 
+/*
+ * Note that is_pv_domain() can return true (for system domains) even when
+ * both is_pv_64bit_domain() and is_pv_32bit_domain() return false. IOW
+ * system domains can be considered PV without specific bitness.
+ */
 static always_inline bool is_pv_domain(const struct domain *d)
 {
-    return IS_ENABLED(CONFIG_PV) &&
+    return IS_ENABLED(CONFIG_X86) &&
         evaluate_nospec(!(d->options & XEN_DOMCTL_CDF_hvm));
 }
 
@@ -1064,7 +1069,7 @@ static always_inline bool is_pv_32bit_vc
 
 static always_inline bool is_pv_64bit_domain(const struct domain *d)
 {
-    if ( !is_pv_domain(d) )
+    if ( !IS_ENABLED(CONFIG_PV) || !is_pv_domain(d) )
         return false;
 
 #ifdef CONFIG_PV32

On Thu, Apr 15, 2021 at 11:34:55AM +0200, Jan Beulich wrote:
> On x86, idle and other system domains are implicitly PV. While I
> couldn't spot any cases where this is actively a problem, some cases
> required quite close inspection to be certain there couldn't e.g. be
> some ASSERT_UNREACHABLE() that would trigger in this case. Let's be on
> the safe side and make sure these always have is_pv_domain() returning
> true.
> 
> For the build to still work, this requires a few adjustments elsewhere.
> In particular is_pv_64bit_domain() now gains a CONFIG_PV dependency,
> which means that is_pv_32bit_domain() || is_pv_64bit_domain() is no
> longer guaranteed to be the same as is_pv_domain().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Add comment.

Sorry for not replying earlier, I've been thinking about this because
I don't really like this approach as I think it makes code harder to
follow for two reasons, first is_pv_32bit_domain() ||
is_pv_64bit_domain() != is_pv_domain(), which I could live with, and
then also is_pv_64bit_domain() returning different values for system
domains depending on whether CONFIG_PV is enabled.

Given that AFAICT this patch is not fixing any actively identified
issue I would rather prefer to introduce is_system_domain and use it
when appropriate?

I think that would be clearer long term, and avoid tying ourselves
deeper into aliasing system domain with PV domains.

Thanks, Roger.

On 15.04.2021 12:53, Roger Pau Monné wrote:
> On Thu, Apr 15, 2021 at 11:34:55AM +0200, Jan Beulich wrote:
>> On x86, idle and other system domains are implicitly PV. While I
>> couldn't spot any cases where this is actively a problem, some cases
>> required quite close inspection to be certain there couldn't e.g. be
>> some ASSERT_UNREACHABLE() that would trigger in this case. Let's be on
>> the safe side and make sure these always have is_pv_domain() returning
>> true.
>>
>> For the build to still work, this requires a few adjustments elsewhere.
>> In particular is_pv_64bit_domain() now gains a CONFIG_PV dependency,
>> which means that is_pv_32bit_domain() || is_pv_64bit_domain() is no
>> longer guaranteed to be the same as is_pv_domain().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Add comment.
> 
> Sorry for not replying earlier, I've been thinking about this because
> I don't really like this approach as I think it makes code harder to
> follow for two reasons, first is_pv_32bit_domain() ||
> is_pv_64bit_domain() != is_pv_domain(), which I could live with, and
> then also is_pv_64bit_domain() returning different values for system
> domains depending on whether CONFIG_PV is enabled.

Well, okay, I'll consider the patch rejected then, despite thinking
that it could save us from subtle issues down the road.

> Given that AFAICT this patch is not fixing any actively identified
> issue I would rather prefer to introduce is_system_domain and use it
> when appropriate?
> 
> I think that would be clearer long term, and avoid tying ourselves
> deeper into aliasing system domain with PV domains.

Of course, but it won't help until we've audited and (if needed)
amended all code using is_pv_*() or e.g. implying PV when !is_hvm_*().

Patch 2, while grouped with this one, is technically independent.
Therefore I'd still appreciate separate feedback there.

Jan