APIC virtualization support is currently implemented only for Intel VT-x.
To aid future work on separating AMD-V from Intel VT-x code, instead of
calling directly vmx_vlapic_msr_changed() from common hvm code, add a stub
to the hvm_function_table, named set_virtual_apic_mode (to match the name
of the corresponding function in Linux for cross reference).
Then, create a wrapper function, called hvm_set_virtual_apic_mode(), to be
used by common hvm code.
For Intel VT-x, initialize the stub only when either virtual xAPIC mode or
virtual x2APIC mode is supported.

After the change above, do not include header asm/hvm/vmx/vmx.h as it is
not required anymore and resolve subsequent build errors for implicit
declaration of functions ‘TRACE_2_LONG_3D’ and ‘TRC_PAR_LONG’ by including
missing asm/hvm/trace.h header.

No functional change intended.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/x86/hvm/vlapic.c          | 6 +++---
 xen/arch/x86/hvm/vmx/vmx.c         | 4 ++++
 xen/arch/x86/include/asm/hvm/hvm.h | 6 ++++++
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index eb32f12e2d..34a7eea3a1 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -37,8 +37,8 @@
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/io.h>
 #include <asm/hvm/support.h>
-#include <asm/hvm/vmx/vmx.h>
 #include <asm/hvm/nestedhvm.h>
+#include <asm/hvm/trace.h>
 #include <asm/hvm/viridian.h>
 #include <public/hvm/ioreq.h>
 #include <public/hvm/params.h>
@@ -1165,7 +1165,7 @@ int guest_wrmsr_apic_base(struct vcpu *v, uint64_t value)
     if ( vlapic_x2apic_mode(vlapic) )
         set_x2apic_id(vlapic);
 
-    vmx_vlapic_msr_changed(vlapic_vcpu(vlapic));
+    hvm_set_virtual_apic_mode(vlapic_vcpu(vlapic));
 
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
                 "apic base msr is 0x%016"PRIx64, vlapic->hw.apic_base_msr);
@@ -1561,7 +1561,7 @@ static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
          unlikely(vlapic_x2apic_mode(s)) )
         return -EINVAL;
 
-    vmx_vlapic_msr_changed(v);
+    hvm_set_virtual_apic_mode(v);
 
     return 0;
 }
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 270bc98195..6138dc0885 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3011,6 +3011,10 @@ const struct hvm_function_table * __init start_vmx(void)
         setup_ept_dump();
     }
 
+    if ( cpu_has_vmx_virtualize_apic_accesses ||
+         cpu_has_vmx_virtualize_x2apic_mode )
+        vmx_function_table.set_virtual_apic_mode = vmx_vlapic_msr_changed;
+
     if ( cpu_has_vmx_virtual_intr_delivery )
     {
         vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap;
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index 80e4565bd2..b690e2924c 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -217,6 +217,7 @@ struct hvm_function_table {
     void (*handle_eoi)(uint8_t vector, int isr);
     int (*pi_update_irte)(const struct vcpu *v, const struct pirq *pirq,
                           uint8_t gvec);
+    void (*set_virtual_apic_mode)(struct vcpu *v);
 
     /*Walk nested p2m  */
     int (*nhvm_hap_walk_L1_p2m)(struct vcpu *v, paddr_t L2_gpa,
@@ -786,6 +787,11 @@ static inline int hvm_pi_update_irte(const struct vcpu *v,
     return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec);
 }
 
+static inline void hvm_set_virtual_apic_mode(struct vcpu *v)
+{
+    alternative_vcall(hvm_funcs.set_virtual_apic_mode, v);
+}
+
 #else  /* CONFIG_HVM */
 
 #define hvm_enabled false
-- 
2.37.2

On 06/02/2023 12:58 pm, Xenia Ragiadakou wrote:
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 270bc98195..6138dc0885 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3011,6 +3011,10 @@ const struct hvm_function_table * __init start_vmx(void)
>          setup_ept_dump();
>      }
>  
> +    if ( cpu_has_vmx_virtualize_apic_accesses ||
> +         cpu_has_vmx_virtualize_x2apic_mode )

x2apic_mode is definitely wrong here, but I think apic_accesses is too. 
The top of vmx_vlapic_msr_changed() is buggy too.

Right now, the hook is called unconditionally.  Given no adjustment in
vmx_vlapic_msr_changed(), the new form (using an alternative) needs
calling unconditionally too.

Naming wise, Linux is fairly bogus too.  This should be
hvm_update_vlapic_mode(), but I suspect the hook will disappear in due
course.

> diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
> index 80e4565bd2..b690e2924c 100644
> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -786,6 +787,11 @@ static inline int hvm_pi_update_irte(const struct vcpu *v,
>      return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec);
>  }
>  
> +static inline void hvm_set_virtual_apic_mode(struct vcpu *v)
> +{
> +    alternative_vcall(hvm_funcs.set_virtual_apic_mode, v);

This has to be something like:

if ( hvm_funcs.set_virtual_apic_mode )
    alternative_vcall(...)

Otherwise, Xen will BUG() every time an SVM guest modifies MSR_APIC_BASE.

~Andrew

On 2/6/23 15:32, Andrew Cooper wrote:
> On 06/02/2023 12:58 pm, Xenia Ragiadakou wrote:
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 270bc98195..6138dc0885 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3011,6 +3011,10 @@ const struct hvm_function_table * __init start_vmx(void)
>>           setup_ept_dump();
>>       }
>>   
>> +    if ( cpu_has_vmx_virtualize_apic_accesses ||
>> +         cpu_has_vmx_virtualize_x2apic_mode )
> 
> x2apic_mode is definitely wrong here, but I think apic_accesses is too.

Why?

> The top of vmx_vlapic_msr_changed() is buggy too.
> 
> Right now, the hook is called unconditionally.  Given no adjustment in
> vmx_vlapic_msr_changed(), the new form (using an alternative) needs
> calling unconditionally too.

Ok, I will initialize it unconditionally when the vmx_function_table is 
defined.

> 
> Naming wise, Linux is fairly bogus too.  This should be
> hvm_update_vlapic_mode(), but I suspect the hook will disappear in due
> course.

I can change the name. I gave the same name for cross reference purposes.

> 
>> diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
>> index 80e4565bd2..b690e2924c 100644
>> --- a/xen/arch/x86/include/asm/hvm/hvm.h
>> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
>> @@ -786,6 +787,11 @@ static inline int hvm_pi_update_irte(const struct vcpu *v,
>>       return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec);
>>   }
>>   
>> +static inline void hvm_set_virtual_apic_mode(struct vcpu *v)
>> +{
>> +    alternative_vcall(hvm_funcs.set_virtual_apic_mode, v);
> 
> This has to be something like:
> 
> if ( hvm_funcs.set_virtual_apic_mode )
>      alternative_vcall(...)
> 
> Otherwise, Xen will BUG() every time an SVM guest modifies MSR_APIC_BASE.

Yes, that's true. I will fix it.

> 
> ~Andrew

-- 
Xenia