I don't expect it was intended to default PVH Dom0 to "no assist" mode.
Introduce command line (sub-)options allowing to suppress enabling of
the assists, paralleling the guest config settings for DomU, but restore
the defaulting to "enabled".

Fixes: 2ce11ce249a3 ("x86/HVM: allow per-domain usage of hardware virtualized APIC")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Guard the setting of XEN_X86_ASSISTED_X{,2}APIC by assists actually
    being available.
---
Besides the issue caused here (the manifestation of which appears to
correlate with the other fallout Andrew is trying to deal with) I'm
observing further warnings, but I guess these have been there for some
time (perhaps forever): When parsing AML and encountering the objects
describing the CPUs, Linux would find entries with the original APIC
IDs. If those don't match the ones we assign in pvh_setup_acpi_madt(),
the kernel will wrongly consider the entries to describe further CPUs,
which it therefore would deem hot-pluggable. This again results in
warnings, this time "NR_CPUS/possible_cpus limit of ... reached".

--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -767,7 +767,8 @@ Specify the bit width of the DMA heap.
 
 ### dom0
     = List of [ pv | pvh, shadow=<bool>, verbose=<bool>,
-                cpuid-faulting=<bool>, msr-relaxed=<bool> ]
+                cpuid-faulting=<bool>, msr-relaxed=<bool>,
+                assisted-xapic=<bool>, assisted-x2apic=<bool> ]
 
     Applicability: x86
 
@@ -828,6 +829,10 @@ Controls for how dom0 is constructed on
 
     If using this option is necessary to fix an issue, please report a bug.
 
+*   The `assisted-xapic` and `assisted-x2apic` options, defaulting to true,
+    allow disabling of the respective hardware assists.  These are applicable
+    to PVH Dom0 only, and their effect is limited to VT-x.
+
 ### dom0-cpuid
     = List of comma separated booleans
 
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -293,6 +293,12 @@ static int __init cf_check parse_dom0_pa
             opt_dom0_cpuid_faulting = val;
         else if ( (val = parse_boolean("msr-relaxed", s, ss)) >= 0 )
             opt_dom0_msr_relaxed = val;
+#ifdef CONFIG_HVM
+        else if ( (val = parse_boolean("assisted-xapic", s, ss)) >= 0 )
+            opt_dom0_assisted_xapic = val;
+        else if ( (val = parse_boolean("assisted-x2apic", s, ss)) >= 0 )
+            opt_dom0_assisted_x2apic = val;
+#endif
         else
             rc = -EINVAL;
 
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -55,6 +55,9 @@
  */
 #define HVM_VM86_TSS_SIZE 265
 
+bool __initdata opt_dom0_assisted_xapic = true;
+bool __initdata opt_dom0_assisted_x2apic = true;
+
 static unsigned int __initdata acpi_intr_overrides;
 static struct acpi_madt_interrupt_override __initdata *intsrcovr;
 
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -68,6 +68,14 @@ extern bool opt_dom0_verbose;
 extern bool opt_dom0_cpuid_faulting;
 extern bool opt_dom0_msr_relaxed;
 
+#ifdef CONFIG_HVM
+extern bool opt_dom0_assisted_xapic;
+extern bool opt_dom0_assisted_x2apic;
+#else
+#define opt_dom0_assisted_xapic false
+#define opt_dom0_assisted_x2apic false
+#endif
+
 #define max_init_domid (0)
 
 #endif
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -784,6 +784,11 @@ static struct domain *__init create_dom0
 
         dom0_cfg.arch.emulation_flags |=
             XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC | XEN_X86_EMU_VPCI;
+
+        if ( assisted_xapic_available && opt_dom0_assisted_xapic )
+            dom0_cfg.arch.misc_flags |= XEN_X86_ASSISTED_XAPIC;
+        if ( assisted_x2apic_available && opt_dom0_assisted_x2apic )
+            dom0_cfg.arch.misc_flags |= XEN_X86_ASSISTED_X2APIC;
     }
 
     if ( iommu_enabled )

On Mon, Sep 26, 2022 at 11:58:34AM +0200, Jan Beulich wrote:
> I don't expect it was intended to default PVH Dom0 to "no assist" mode.
> Introduce command line (sub-)options allowing to suppress enabling of
> the assists, paralleling the guest config settings for DomU, but restore
> the defaulting to "enabled".
> 
> Fixes: 2ce11ce249a3 ("x86/HVM: allow per-domain usage of hardware virtualized APIC")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> v2: Guard the setting of XEN_X86_ASSISTED_X{,2}APIC by assists actually
>     being available.
> ---
> Besides the issue caused here (the manifestation of which appears to
> correlate with the other fallout Andrew is trying to deal with) I'm
> observing further warnings, but I guess these have been there for some
> time (perhaps forever): When parsing AML and encountering the objects
> describing the CPUs, Linux would find entries with the original APIC
> IDs. If those don't match the ones we assign in pvh_setup_acpi_madt(),
> the kernel will wrongly consider the entries to describe further CPUs,
> which it therefore would deem hot-pluggable. This again results in
> warnings, this time "NR_CPUS/possible_cpus limit of ... reached".

Hm, I'm handling this differently on FreeBSD AFAICT, by using a Xen
specific driver for the Processor objects when running as dom0, which
replaces the usage of the native driver.  The only function of that
driver being the uploading of the performance states in the Processor
object to Xen.

I think we ought to do something similar in Linux and just use the
Processor objects in order to upload the performance related data to
Xen, but ignore anything else.

What happens on PV when the number of vCPU available for dom0 is
smaller than the number of physical CPUs?  Does it also consider the
unmatched Processor AML objects to be hotpluggable CPUs?

> 
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -767,7 +767,8 @@ Specify the bit width of the DMA heap.
>  
>  ### dom0
>      = List of [ pv | pvh, shadow=<bool>, verbose=<bool>,
> -                cpuid-faulting=<bool>, msr-relaxed=<bool> ]
> +                cpuid-faulting=<bool>, msr-relaxed=<bool>,
> +                assisted-xapic=<bool>, assisted-x2apic=<bool> ]
>  
>      Applicability: x86
>  
> @@ -828,6 +829,10 @@ Controls for how dom0 is constructed on
>  
>      If using this option is necessary to fix an issue, please report a bug.
>  
> +*   The `assisted-xapic` and `assisted-x2apic` options, defaulting to true,
> +    allow disabling of the respective hardware assists.  These are applicable
> +    to PVH Dom0 only, and their effect is limited to VT-x.

Explicitly mentioning VT-x here is likely to become stale if AMD is
also updated to support the options.  I might suggest to leave it out,
albeit I won insist if you have a strong opinion about it.

Thanks, Roger.

On 27.09.2022 16:32, Roger Pau Monné wrote:
> On Mon, Sep 26, 2022 at 11:58:34AM +0200, Jan Beulich wrote:
>> I don't expect it was intended to default PVH Dom0 to "no assist" mode.
>> Introduce command line (sub-)options allowing to suppress enabling of
>> the assists, paralleling the guest config settings for DomU, but restore
>> the defaulting to "enabled".
>>
>> Fixes: 2ce11ce249a3 ("x86/HVM: allow per-domain usage of hardware virtualized APIC")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> v2: Guard the setting of XEN_X86_ASSISTED_X{,2}APIC by assists actually
>>     being available.
>> ---
>> Besides the issue caused here (the manifestation of which appears to
>> correlate with the other fallout Andrew is trying to deal with) I'm
>> observing further warnings, but I guess these have been there for some
>> time (perhaps forever): When parsing AML and encountering the objects
>> describing the CPUs, Linux would find entries with the original APIC
>> IDs. If those don't match the ones we assign in pvh_setup_acpi_madt(),
>> the kernel will wrongly consider the entries to describe further CPUs,
>> which it therefore would deem hot-pluggable. This again results in
>> warnings, this time "NR_CPUS/possible_cpus limit of ... reached".
> 
> Hm, I'm handling this differently on FreeBSD AFAICT, by using a Xen
> specific driver for the Processor objects when running as dom0, which
> replaces the usage of the native driver.  The only function of that
> driver being the uploading of the performance states in the Processor
> object to Xen.
> 
> I think we ought to do something similar in Linux and just use the
> Processor objects in order to upload the performance related data to
> Xen, but ignore anything else.
> 
> What happens on PV when the number of vCPU available for dom0 is
> smaller than the number of physical CPUs?  Does it also consider the
> unmatched Processor AML objects to be hotpluggable CPUs?

I have to admit that I don't recall for sure, and I'd rather not write
something I'm not sure of.

>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -767,7 +767,8 @@ Specify the bit width of the DMA heap.
>>  
>>  ### dom0
>>      = List of [ pv | pvh, shadow=<bool>, verbose=<bool>,
>> -                cpuid-faulting=<bool>, msr-relaxed=<bool> ]
>> +                cpuid-faulting=<bool>, msr-relaxed=<bool>,
>> +                assisted-xapic=<bool>, assisted-x2apic=<bool> ]
>>  
>>      Applicability: x86
>>  
>> @@ -828,6 +829,10 @@ Controls for how dom0 is constructed on
>>  
>>      If using this option is necessary to fix an issue, please report a bug.
>>  
>> +*   The `assisted-xapic` and `assisted-x2apic` options, defaulting to true,
>> +    allow disabling of the respective hardware assists.  These are applicable
>> +    to PVH Dom0 only, and their effect is limited to VT-x.
> 
> Explicitly mentioning VT-x here is likely to become stale if AMD is
> also updated to support the options.  I might suggest to leave it out,
> albeit I won insist if you have a strong opinion about it.

At this point the statement expresses reality. Imo the half sentence
wants dropping when AMD gains respective functionality.

Jan

On 27/09/2022 15:38, Jan Beulich wrote:
> On 27.09.2022 16:32, Roger Pau Monné wrote:
>> On Mon, Sep 26, 2022 at 11:58:34AM +0200, Jan Beulich wrote:
>>> I don't expect it was intended to default PVH Dom0 to "no assist" mode.
>>> Introduce command line (sub-)options allowing to suppress enabling of
>>> the assists, paralleling the guest config settings for DomU, but restore
>>> the defaulting to "enabled".
>>>
>>> Fixes: 2ce11ce249a3 ("x86/HVM: allow per-domain usage of hardware virtualized APIC")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

NACK.

You're both on the security team - stop churning code you know perfectly
well is in flux.

This patch goes nowhere until the issues are resolved, and the ABI is
unbroken.  Then and only then *might* there need to be an adjustment for
dom0.

~Andrew