[PATCH for-4.17 0/2] vapic: followup from assisted APIC series

[PATCH for-4.17 0/2] vapic: followup from assisted APIC series

[Roger Pau Monne]

Hello,

Patch 1 is a plain bugfix for a feature we expose in viridian, which is
kind of unrelated to the series but patch 2 in its current form depends
on it.

Patch 2 is the actual meat and changes which features get reported as
part of the x{2,}APIC assist flag.  This is an attempt to repurpose the
flags so we don't release a version of Xen with a set of flags that we
will then either remove or use to report different hardware features.

Thanks, Roger.

Roger Pau Monne (2):
  viridian: suggest MSR APIC accesses if MSR accesses are accelerated
  hvm/apic: repurpose the reporting of the APIC assist options

 xen/arch/x86/hvm/viridian/viridian.c |  6 +++++-
 xen/arch/x86/hvm/vmx/vmcs.c          |  8 ++++----
 xen/arch/x86/hvm/vmx/vmx.c           | 25 ++++++++++++++++++-------
 xen/arch/x86/traps.c                 |  4 +---
 4 files changed, 28 insertions(+), 15 deletions(-)

-- 
2.37.3

[PATCH for-4.17 1/2] viridian: suggest MSR APIC accesses if MSR accesses are accelerated

[Roger Pau Monne]

The "APIC register virtualization" Intel hardware feature applies to
both MMIO or MSR APIC accesses depending on whether "virtualize x2APIC
mode" is also available.

As such also suggest MSR APIC accesses if both "APIC register
virtualization" and "virtualize x2APIC mode" features are available.

Fixes: 7f2e992b82 ('VMX/Viridian: suppress MSR-based APIC suggestion when having APIC-V')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/viridian/viridian.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 25dca93e8b..c4fa0a8b32 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -197,7 +197,11 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
         res->a = CPUID4A_RELAX_TIMER_INT;
         if ( viridian_feature_mask(d) & HVMPV_hcall_remote_tlb_flush )
             res->a |= CPUID4A_HCALL_REMOTE_TLB_FLUSH;
-        if ( !cpu_has_vmx_apic_reg_virt )
+        /*
+         * Suggest x2APIC mode by default, unless xAPIC registers are hardware
+         * virtualized and x2APIC ones aren't.
+         */
+        if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode )
             res->a |= CPUID4A_MSR_BASED_APIC;
         if ( viridian_feature_mask(d) & HVMPV_hcall_ipi )
             res->a |= CPUID4A_SYNTHETIC_CLUSTER_IPI;
-- 
2.37.3

[PATCH for-4.17 2/2] hvm/apic: repurpose the reporting of the APIC assist options

[Roger Pau Monne]

The current reporting of the hardware assisted APIC options is done by
checking "virtualize APIC accesses" which is not very helpful, as that
feature doesn't avoid a vmexit, instead it does provide some help in
order to detect APIC MMIO accesses in vmexit processing.

Repurpose the current reporting of xAPIC assistance to instead report
such feature as present when there's support for "TPR shadow" and
"APIC register virtualization" because in that case some xAPIC MMIO
register accesses are handled directly by the hardware, without
requiring a vmexit.

For symetry also change assisted x2APIC reporting to require
"virtualize x2APIC mode" and "APIC register virtualization", dropping
the option to also be reported when "virtual interrupt delivery" is
available.  Presence of the "virtual interrupt delivery" feature will
be reported using a different option.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I find the logic in vmx_vlapic_msr_changed() hard to follow, but I
don't want to rewrite the function logic at this point.
---
 xen/arch/x86/hvm/viridian/viridian.c |  2 +-
 xen/arch/x86/hvm/vmx/vmcs.c          |  8 ++++----
 xen/arch/x86/hvm/vmx/vmx.c           | 25 ++++++++++++++++++-------
 xen/arch/x86/traps.c                 |  4 +---
 4 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index c4fa0a8b32..bafd8e90de 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
          * Suggest x2APIC mode by default, unless xAPIC registers are hardware
          * virtualized and x2APIC ones aren't.
          */
-        if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode )
+        if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) )
             res->a |= CPUID4A_MSR_BASED_APIC;
         if ( viridian_feature_mask(d) & HVMPV_hcall_ipi )
             res->a |= CPUID4A_SYNTHETIC_CLUSTER_IPI;
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index a1aca1ec04..7bb96e1a8e 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1136,7 +1136,7 @@ static int construct_vmcs(struct vcpu *v)
 
     if ( !has_assisted_xapic(d) )
         v->arch.hvm.vmx.secondary_exec_control &=
-            ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
+            ~SECONDARY_EXEC_APIC_REGISTER_VIRT;
 
     if ( cpu_has_vmx_secondary_exec_control )
         __vmwrite(SECONDARY_VM_EXEC_CONTROL,
@@ -2156,10 +2156,10 @@ int __init vmx_vmcs_init(void)
     if ( !ret )
     {
         /* Check whether hardware supports accelerated xapic and x2apic. */
-        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
+        assisted_xapic_available = cpu_has_vmx_tpr_shadow &&
+                                   cpu_has_vmx_apic_reg_virt;
         assisted_x2apic_available = cpu_has_vmx_virtualize_x2apic_mode &&
-                                    (cpu_has_vmx_apic_reg_virt ||
-                                     cpu_has_vmx_virtual_intr_delivery);
+                                    cpu_has_vmx_apic_reg_virt;
         register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
     }
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index e624b415c9..bf0fe3355c 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3405,25 +3405,29 @@ static void vmx_install_vlapic_mapping(struct vcpu *v)
 
 void vmx_vlapic_msr_changed(struct vcpu *v)
 {
+    bool virtualize_x2apic_mode = has_assisted_x2apic(v->domain) ||
+                                  (cpu_has_vmx_virtualize_x2apic_mode &&
+                                   cpu_has_vmx_virtual_intr_delivery);
     struct vlapic *vlapic = vcpu_vlapic(v);
     unsigned int msr;
 
-    if ( !has_assisted_xapic(v->domain) &&
-         !has_assisted_x2apic(v->domain) )
+    if ( !cpu_has_vmx_virtualize_apic_accesses &&
+         !virtualize_x2apic_mode )
         return;
 
     vmx_vmcs_enter(v);
     v->arch.hvm.vmx.secondary_exec_control &=
         ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
-          SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE);
+          SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
+          SECONDARY_EXEC_APIC_REGISTER_VIRT);
     if ( !vlapic_hw_disabled(vlapic) &&
          (vlapic_base_address(vlapic) == APIC_DEFAULT_PHYS_BASE) )
     {
-        if ( has_assisted_x2apic(v->domain) && vlapic_x2apic_mode(vlapic) )
+        if ( virtualize_x2apic_mode && vlapic_x2apic_mode(vlapic) )
         {
             v->arch.hvm.vmx.secondary_exec_control |=
                 SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
-            if ( cpu_has_vmx_apic_reg_virt )
+            if ( has_assisted_x2apic(v->domain) )
             {
                 for ( msr = MSR_X2APIC_FIRST;
                       msr <= MSR_X2APIC_LAST; msr++ )
@@ -3432,6 +3436,10 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
                 vmx_set_msr_intercept(v, MSR_X2APIC_PPR, VMX_MSR_R);
                 vmx_set_msr_intercept(v, MSR_X2APIC_TMICT, VMX_MSR_R);
                 vmx_set_msr_intercept(v, MSR_X2APIC_TMCCT, VMX_MSR_R);
+
+                v->arch.hvm.vmx.secondary_exec_control |=
+                    SECONDARY_EXEC_APIC_REGISTER_VIRT;
+
             }
             if ( cpu_has_vmx_virtual_intr_delivery )
             {
@@ -3440,9 +3448,12 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
                 vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W);
             }
         }
-        else if ( has_assisted_xapic(v->domain) )
+        else if ( vlapic_xapic_mode(vlapic) )
             v->arch.hvm.vmx.secondary_exec_control |=
-                SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
+                (cpu_has_vmx_virtualize_apic_accesses ?
+                    SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES : 0) |
+                (has_assisted_xapic(v->domain) ?
+                    SECONDARY_EXEC_APIC_REGISTER_VIRT : 0);
     }
     if ( !(v->arch.hvm.vmx.secondary_exec_control &
            SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE) )
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 7207390118..5c0aabe8a3 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1124,8 +1124,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
         if ( !is_hvm_domain(d) || subleaf != 0 )
             break;
 
-        if ( cpu_has_vmx_apic_reg_virt &&
-             has_assisted_xapic(d) )
+        if ( has_assisted_xapic(d) )
             res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT;
 
         /*
@@ -1135,7 +1134,6 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
          * vmx_vlapic_msr_changed()).
          */
         if ( has_assisted_x2apic(d) &&
-             cpu_has_vmx_apic_reg_virt &&
              cpu_has_vmx_virtual_intr_delivery )
             res->a |= XEN_HVM_CPUID_X2APIC_VIRT;
 
-- 
2.37.3

RE: [PATCH for-4.17 0/2] vapic: followup from assisted APIC series

[Henry Wang]

Hi Roger,

Thanks for the patches,

> Subject: [PATCH for-4.17 0/2] vapic: followup from assisted APIC series
> 
> Hello,
> 
> Patch 1 is a plain bugfix for a feature we expose in viridian, which is
> kind of unrelated to the series but patch 2 in its current form depends
> on it.
> 
> Patch 2 is the actual meat and changes which features get reported as
> part of the x{2,}APIC assist flag.  This is an attempt to repurpose the
> flags so we don't release a version of Xen with a set of flags that we
> will then either remove or use to report different hardware features.
> 
> Thanks, Roger.
> 
> Roger Pau Monne (2):
>   viridian: suggest MSR APIC accesses if MSR accesses are accelerated
>   hvm/apic: repurpose the reporting of the APIC assist options

For the whole series:
Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

Re: [PATCH for-4.17 1/2] viridian: suggest MSR APIC accesses if MSR accesses are accelerated

[Paul Durrant]

On 04/11/2022 14:22, Roger Pau Monne wrote:
> The "APIC register virtualization" Intel hardware feature applies to
> both MMIO or MSR APIC accesses depending on whether "virtualize x2APIC
> mode" is also available.
> 
> As such also suggest MSR APIC accesses if both "APIC register
> virtualization" and "virtualize x2APIC mode" features are available.
> 

I'm having trouble reconciling that with the logic below...

> Fixes: 7f2e992b82 ('VMX/Viridian: suppress MSR-based APIC suggestion when having APIC-V')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   xen/arch/x86/hvm/viridian/viridian.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> index 25dca93e8b..c4fa0a8b32 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -197,7 +197,11 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
>           res->a = CPUID4A_RELAX_TIMER_INT;
>           if ( viridian_feature_mask(d) & HVMPV_hcall_remote_tlb_flush )
>               res->a |= CPUID4A_HCALL_REMOTE_TLB_FLUSH;
> -        if ( !cpu_has_vmx_apic_reg_virt )
> +        /*
> +         * Suggest x2APIC mode by default, unless xAPIC registers are hardware
> +         * virtualized and x2APIC ones aren't.
> +         */
> +        if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode )

This means APIC register virt *not* available or virt x2apic *is* 
available. Is the latter gated on the former?

   Paul


>               res->a |= CPUID4A_MSR_BASED_APIC;
>           if ( viridian_feature_mask(d) & HVMPV_hcall_ipi )
>               res->a |= CPUID4A_SYNTHETIC_CLUSTER_IPI;

Re: [PATCH for-4.17 2/2] hvm/apic: repurpose the reporting of the APIC assist options

[Paul Durrant]

On 04/11/2022 14:22, Roger Pau Monne wrote:
> The current reporting of the hardware assisted APIC options is done by
> checking "virtualize APIC accesses" which is not very helpful, as that
> feature doesn't avoid a vmexit, instead it does provide some help in
> order to detect APIC MMIO accesses in vmexit processing.
> 
> Repurpose the current reporting of xAPIC assistance to instead report
> such feature as present when there's support for "TPR shadow" and
> "APIC register virtualization" because in that case some xAPIC MMIO
> register accesses are handled directly by the hardware, without
> requiring a vmexit.
> 
> For symetry also change assisted x2APIC reporting to require
> "virtualize x2APIC mode" and "APIC register virtualization", dropping
> the option to also be reported when "virtual interrupt delivery" is
> available.  Presence of the "virtual interrupt delivery" feature will
> be reported using a different option.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> I find the logic in vmx_vlapic_msr_changed() hard to follow, but I
> don't want to rewrite the function logic at this point.
> ---
>   xen/arch/x86/hvm/viridian/viridian.c |  2 +-
>   xen/arch/x86/hvm/vmx/vmcs.c          |  8 ++++----
>   xen/arch/x86/hvm/vmx/vmx.c           | 25 ++++++++++++++++++-------
>   xen/arch/x86/traps.c                 |  4 +---
>   4 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> index c4fa0a8b32..bafd8e90de 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
>            * Suggest x2APIC mode by default, unless xAPIC registers are hardware
>            * virtualized and x2APIC ones aren't.
>            */
> -        if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode )
> +        if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) )

So, not sure why this is separated from patch 1 but stated this way it 
seems counterintuitive. We only want to use the viridian MSRs if they 
are going to be more efficient.. which I think is only in the case where 
we have neither an x2apic not an assisted xapic (hence we would trap for 
MMIO).

   Paul

>               res->a |= CPUID4A_MSR_BASED_APIC;
>           if ( viridian_feature_mask(d) & HVMPV_hcall_ipi )
>               res->a |= CPUID4A_SYNTHETIC_CLUSTER_IPI;

Re: [PATCH for-4.17 2/2] hvm/apic: repurpose the reporting of the APIC assist options

[Roger Pau Monné]

On Fri, Nov 04, 2022 at 03:55:54PM +0000, Paul Durrant wrote:
> On 04/11/2022 14:22, Roger Pau Monne wrote:
> > The current reporting of the hardware assisted APIC options is done by
> > checking "virtualize APIC accesses" which is not very helpful, as that
> > feature doesn't avoid a vmexit, instead it does provide some help in
> > order to detect APIC MMIO accesses in vmexit processing.
> > 
> > Repurpose the current reporting of xAPIC assistance to instead report
> > such feature as present when there's support for "TPR shadow" and
> > "APIC register virtualization" because in that case some xAPIC MMIO
> > register accesses are handled directly by the hardware, without
> > requiring a vmexit.
> > 
> > For symetry also change assisted x2APIC reporting to require
> > "virtualize x2APIC mode" and "APIC register virtualization", dropping
> > the option to also be reported when "virtual interrupt delivery" is
> > available.  Presence of the "virtual interrupt delivery" feature will
> > be reported using a different option.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > I find the logic in vmx_vlapic_msr_changed() hard to follow, but I
> > don't want to rewrite the function logic at this point.
> > ---
> >   xen/arch/x86/hvm/viridian/viridian.c |  2 +-
> >   xen/arch/x86/hvm/vmx/vmcs.c          |  8 ++++----
> >   xen/arch/x86/hvm/vmx/vmx.c           | 25 ++++++++++++++++++-------
> >   xen/arch/x86/traps.c                 |  4 +---
> >   4 files changed, 24 insertions(+), 15 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> > index c4fa0a8b32..bafd8e90de 100644
> > --- a/xen/arch/x86/hvm/viridian/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian/viridian.c
> > @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
> >            * Suggest x2APIC mode by default, unless xAPIC registers are hardware
> >            * virtualized and x2APIC ones aren't.
> >            */
> > -        if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode )
> > +        if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) )
> 
> So, not sure why this is separated from patch 1 but stated this way it seems
> counterintuitive. We only want to use the viridian MSRs if they are going to
> be more efficient.. which I think is only in the case where we have neither
> an x2apic not an assisted xapic (hence we would trap for MMIO).

I've read the MS HTLFS and I guess I got confused, the section about
this CPUID bit states:

"Bit 3: Recommend using MSRs for accessing APIC registers EOI, ICR and
TPR rather than their memory-mapped"

So I've (wrongly) understood that MSRs for accessing APIC registers
was meant to be a recommendation to use x2APIC mode in order to access
those registers.  Didn't realize Viridian had a way to expose certain
APIC registers using MSRs when the APIC is in xAPIC mode.

I withdraw patch 1 and adjust patch 2 accordingly then.

Thanks, Roger.

Re: [PATCH for-4.17 2/2] hvm/apic: repurpose the reporting of the APIC assist options

[Paul Durrant]

On 04/11/2022 16:01, Roger Pau Monné wrote:
> On Fri, Nov 04, 2022 at 03:55:54PM +0000, Paul Durrant wrote:
>> On 04/11/2022 14:22, Roger Pau Monne wrote:
>>> The current reporting of the hardware assisted APIC options is done by
>>> checking "virtualize APIC accesses" which is not very helpful, as that
>>> feature doesn't avoid a vmexit, instead it does provide some help in
>>> order to detect APIC MMIO accesses in vmexit processing.
>>>
>>> Repurpose the current reporting of xAPIC assistance to instead report
>>> such feature as present when there's support for "TPR shadow" and
>>> "APIC register virtualization" because in that case some xAPIC MMIO
>>> register accesses are handled directly by the hardware, without
>>> requiring a vmexit.
>>>
>>> For symetry also change assisted x2APIC reporting to require
>>> "virtualize x2APIC mode" and "APIC register virtualization", dropping
>>> the option to also be reported when "virtual interrupt delivery" is
>>> available.  Presence of the "virtual interrupt delivery" feature will
>>> be reported using a different option.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> I find the logic in vmx_vlapic_msr_changed() hard to follow, but I
>>> don't want to rewrite the function logic at this point.
>>> ---
>>>    xen/arch/x86/hvm/viridian/viridian.c |  2 +-
>>>    xen/arch/x86/hvm/vmx/vmcs.c          |  8 ++++----
>>>    xen/arch/x86/hvm/vmx/vmx.c           | 25 ++++++++++++++++++-------
>>>    xen/arch/x86/traps.c                 |  4 +---
>>>    4 files changed, 24 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
>>> index c4fa0a8b32..bafd8e90de 100644
>>> --- a/xen/arch/x86/hvm/viridian/viridian.c
>>> +++ b/xen/arch/x86/hvm/viridian/viridian.c
>>> @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
>>>             * Suggest x2APIC mode by default, unless xAPIC registers are hardware
>>>             * virtualized and x2APIC ones aren't.
>>>             */
>>> -        if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode )
>>> +        if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) )
>>
>> So, not sure why this is separated from patch 1 but stated this way it seems
>> counterintuitive. We only want to use the viridian MSRs if they are going to
>> be more efficient.. which I think is only in the case where we have neither
>> an x2apic not an assisted xapic (hence we would trap for MMIO).
> 
> I've read the MS HTLFS and I guess I got confused, the section about
> this CPUID bit states:
> 
> "Bit 3: Recommend using MSRs for accessing APIC registers EOI, ICR and
> TPR rather than their memory-mapped"
> 
> So I've (wrongly) understood that MSRs for accessing APIC registers
> was meant to be a recommendation to use x2APIC mode in order to access
> those registers.  Didn't realize Viridian had a way to expose certain
> APIC registers using MSRs when the APIC is in xAPIC mode.
> 

Yeah, I think they predate the existence of x2apic.

> I withdraw patch 1 and adjust patch 2 accordingly then.
> 
Cool. Thanks,

   Paul

Re: [PATCH for-4.17 2/2] hvm/apic: repurpose the reporting of the APIC assist options

[Roger Pau Monné]

On Fri, Nov 04, 2022 at 04:05:05PM +0000, Paul Durrant wrote:
> On 04/11/2022 16:01, Roger Pau Monné wrote:
> > On Fri, Nov 04, 2022 at 03:55:54PM +0000, Paul Durrant wrote:
> > > On 04/11/2022 14:22, Roger Pau Monne wrote:
> > > > The current reporting of the hardware assisted APIC options is done by
> > > > checking "virtualize APIC accesses" which is not very helpful, as that
> > > > feature doesn't avoid a vmexit, instead it does provide some help in
> > > > order to detect APIC MMIO accesses in vmexit processing.
> > > > 
> > > > Repurpose the current reporting of xAPIC assistance to instead report
> > > > such feature as present when there's support for "TPR shadow" and
> > > > "APIC register virtualization" because in that case some xAPIC MMIO
> > > > register accesses are handled directly by the hardware, without
> > > > requiring a vmexit.
> > > > 
> > > > For symetry also change assisted x2APIC reporting to require
> > > > "virtualize x2APIC mode" and "APIC register virtualization", dropping
> > > > the option to also be reported when "virtual interrupt delivery" is
> > > > available.  Presence of the "virtual interrupt delivery" feature will
> > > > be reported using a different option.
> > > > 
> > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > ---
> > > > I find the logic in vmx_vlapic_msr_changed() hard to follow, but I
> > > > don't want to rewrite the function logic at this point.
> > > > ---
> > > >    xen/arch/x86/hvm/viridian/viridian.c |  2 +-
> > > >    xen/arch/x86/hvm/vmx/vmcs.c          |  8 ++++----
> > > >    xen/arch/x86/hvm/vmx/vmx.c           | 25 ++++++++++++++++++-------
> > > >    xen/arch/x86/traps.c                 |  4 +---
> > > >    4 files changed, 24 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> > > > index c4fa0a8b32..bafd8e90de 100644
> > > > --- a/xen/arch/x86/hvm/viridian/viridian.c
> > > > +++ b/xen/arch/x86/hvm/viridian/viridian.c
> > > > @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
> > > >             * Suggest x2APIC mode by default, unless xAPIC registers are hardware
> > > >             * virtualized and x2APIC ones aren't.
> > > >             */
> > > > -        if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode )
> > > > +        if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) )
> > > 
> > > So, not sure why this is separated from patch 1 but stated this way it seems
> > > counterintuitive. We only want to use the viridian MSRs if they are going to
> > > be more efficient.. which I think is only in the case where we have neither
> > > an x2apic not an assisted xapic (hence we would trap for MMIO).
> > 
> > I've read the MS HTLFS and I guess I got confused, the section about
> > this CPUID bit states:
> > 
> > "Bit 3: Recommend using MSRs for accessing APIC registers EOI, ICR and
> > TPR rather than their memory-mapped"
> > 
> > So I've (wrongly) understood that MSRs for accessing APIC registers
> > was meant to be a recommendation to use x2APIC mode in order to access
> > those registers.  Didn't realize Viridian had a way to expose certain
> > APIC registers using MSRs when the APIC is in xAPIC mode.
> > 
> 
> Yeah, I think they predate the existence of x2apic.
> 
> > I withdraw patch 1 and adjust patch 2 accordingly then.
> > 
> Cool. Thanks,

How does Windows know whether to use xAPIC or x2APIC?

I would assume CPUID4A_MSR_BASED_APIC only makes sense when in xAPIC
mode, as otherwise the registers are already accesses using MSRs.

Thanks, Roger.

Re: [PATCH for-4.17 2/2] hvm/apic: repurpose the reporting of the APIC assist options

[Paul Durrant]

On 04/11/2022 16:10, Roger Pau Monné wrote:
> On Fri, Nov 04, 2022 at 04:05:05PM +0000, Paul Durrant wrote:
>> On 04/11/2022 16:01, Roger Pau Monné wrote:
>>> On Fri, Nov 04, 2022 at 03:55:54PM +0000, Paul Durrant wrote:
>>>> On 04/11/2022 14:22, Roger Pau Monne wrote:
>>>>> The current reporting of the hardware assisted APIC options is done by
>>>>> checking "virtualize APIC accesses" which is not very helpful, as that
>>>>> feature doesn't avoid a vmexit, instead it does provide some help in
>>>>> order to detect APIC MMIO accesses in vmexit processing.
>>>>>
>>>>> Repurpose the current reporting of xAPIC assistance to instead report
>>>>> such feature as present when there's support for "TPR shadow" and
>>>>> "APIC register virtualization" because in that case some xAPIC MMIO
>>>>> register accesses are handled directly by the hardware, without
>>>>> requiring a vmexit.
>>>>>
>>>>> For symetry also change assisted x2APIC reporting to require
>>>>> "virtualize x2APIC mode" and "APIC register virtualization", dropping
>>>>> the option to also be reported when "virtual interrupt delivery" is
>>>>> available.  Presence of the "virtual interrupt delivery" feature will
>>>>> be reported using a different option.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>> ---
>>>>> I find the logic in vmx_vlapic_msr_changed() hard to follow, but I
>>>>> don't want to rewrite the function logic at this point.
>>>>> ---
>>>>>     xen/arch/x86/hvm/viridian/viridian.c |  2 +-
>>>>>     xen/arch/x86/hvm/vmx/vmcs.c          |  8 ++++----
>>>>>     xen/arch/x86/hvm/vmx/vmx.c           | 25 ++++++++++++++++++-------
>>>>>     xen/arch/x86/traps.c                 |  4 +---
>>>>>     4 files changed, 24 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
>>>>> index c4fa0a8b32..bafd8e90de 100644
>>>>> --- a/xen/arch/x86/hvm/viridian/viridian.c
>>>>> +++ b/xen/arch/x86/hvm/viridian/viridian.c
>>>>> @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
>>>>>              * Suggest x2APIC mode by default, unless xAPIC registers are hardware
>>>>>              * virtualized and x2APIC ones aren't.
>>>>>              */
>>>>> -        if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode )
>>>>> +        if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) )
>>>>
>>>> So, not sure why this is separated from patch 1 but stated this way it seems
>>>> counterintuitive. We only want to use the viridian MSRs if they are going to
>>>> be more efficient.. which I think is only in the case where we have neither
>>>> an x2apic not an assisted xapic (hence we would trap for MMIO).
>>>
>>> I've read the MS HTLFS and I guess I got confused, the section about
>>> this CPUID bit states:
>>>
>>> "Bit 3: Recommend using MSRs for accessing APIC registers EOI, ICR and
>>> TPR rather than their memory-mapped"
>>>
>>> So I've (wrongly) understood that MSRs for accessing APIC registers
>>> was meant to be a recommendation to use x2APIC mode in order to access
>>> those registers.  Didn't realize Viridian had a way to expose certain
>>> APIC registers using MSRs when the APIC is in xAPIC mode.
>>>
>>
>> Yeah, I think they predate the existence of x2apic.
>>
>>> I withdraw patch 1 and adjust patch 2 accordingly then.
>>>
>> Cool. Thanks,
> 
> How does Windows know whether to use xAPIC or x2APIC?
> 

cpuid? TBH I'm not sure why this recommendation would ever trump x2apic 
anyway.

> I would assume CPUID4A_MSR_BASED_APIC only makes sense when in xAPIC
> mode, as otherwise the registers are already accesses using MSRs.
> 
> Thanks, Roger.

Re: [PATCH for-4.17 2/2] hvm/apic: repurpose the reporting of the APIC assist options

[Roger Pau Monné]

On Fri, Nov 04, 2022 at 05:35:23PM +0000, Paul Durrant wrote:
> On 04/11/2022 16:10, Roger Pau Monné wrote:
> > On Fri, Nov 04, 2022 at 04:05:05PM +0000, Paul Durrant wrote:
> > > On 04/11/2022 16:01, Roger Pau Monné wrote:
> > > > On Fri, Nov 04, 2022 at 03:55:54PM +0000, Paul Durrant wrote:
> > > > > On 04/11/2022 14:22, Roger Pau Monne wrote:
> > > > > > The current reporting of the hardware assisted APIC options is done by
> > > > > > checking "virtualize APIC accesses" which is not very helpful, as that
> > > > > > feature doesn't avoid a vmexit, instead it does provide some help in
> > > > > > order to detect APIC MMIO accesses in vmexit processing.
> > > > > > 
> > > > > > Repurpose the current reporting of xAPIC assistance to instead report
> > > > > > such feature as present when there's support for "TPR shadow" and
> > > > > > "APIC register virtualization" because in that case some xAPIC MMIO
> > > > > > register accesses are handled directly by the hardware, without
> > > > > > requiring a vmexit.
> > > > > > 
> > > > > > For symetry also change assisted x2APIC reporting to require
> > > > > > "virtualize x2APIC mode" and "APIC register virtualization", dropping
> > > > > > the option to also be reported when "virtual interrupt delivery" is
> > > > > > available.  Presence of the "virtual interrupt delivery" feature will
> > > > > > be reported using a different option.
> > > > > > 
> > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > > > ---
> > > > > > I find the logic in vmx_vlapic_msr_changed() hard to follow, but I
> > > > > > don't want to rewrite the function logic at this point.
> > > > > > ---
> > > > > >     xen/arch/x86/hvm/viridian/viridian.c |  2 +-
> > > > > >     xen/arch/x86/hvm/vmx/vmcs.c          |  8 ++++----
> > > > > >     xen/arch/x86/hvm/vmx/vmx.c           | 25 ++++++++++++++++++-------
> > > > > >     xen/arch/x86/traps.c                 |  4 +---
> > > > > >     4 files changed, 24 insertions(+), 15 deletions(-)
> > > > > > 
> > > > > > diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> > > > > > index c4fa0a8b32..bafd8e90de 100644
> > > > > > --- a/xen/arch/x86/hvm/viridian/viridian.c
> > > > > > +++ b/xen/arch/x86/hvm/viridian/viridian.c
> > > > > > @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
> > > > > >              * Suggest x2APIC mode by default, unless xAPIC registers are hardware
> > > > > >              * virtualized and x2APIC ones aren't.
> > > > > >              */
> > > > > > -        if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode )
> > > > > > +        if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) )
> > > > > 
> > > > > So, not sure why this is separated from patch 1 but stated this way it seems
> > > > > counterintuitive. We only want to use the viridian MSRs if they are going to
> > > > > be more efficient.. which I think is only in the case where we have neither
> > > > > an x2apic not an assisted xapic (hence we would trap for MMIO).
> > > > 
> > > > I've read the MS HTLFS and I guess I got confused, the section about
> > > > this CPUID bit states:
> > > > 
> > > > "Bit 3: Recommend using MSRs for accessing APIC registers EOI, ICR and
> > > > TPR rather than their memory-mapped"
> > > > 
> > > > So I've (wrongly) understood that MSRs for accessing APIC registers
> > > > was meant to be a recommendation to use x2APIC mode in order to access
> > > > those registers.  Didn't realize Viridian had a way to expose certain
> > > > APIC registers using MSRs when the APIC is in xAPIC mode.
> > > > 
> > > 
> > > Yeah, I think they predate the existence of x2apic.
> > > 
> > > > I withdraw patch 1 and adjust patch 2 accordingly then.
> > > > 
> > > Cool. Thanks,
> > 
> > How does Windows know whether to use xAPIC or x2APIC?
> > 
> 
> cpuid? TBH I'm not sure why this recommendation would ever trump x2apic
> anyway.

OK, so the recommendation is ignored when running in x2APIC mode,
which should be the default since Xen does always expose x2APIC by
default to guests.

Thanks, Roger.