[PATCH v2] x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback

[PATCH v2] x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback

[Xenia Ragiadakou]

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 update_vlapic_mode, and create a wrapper
function, called hvm_update_vlapic_mode(), to be used by common hvm code.

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>
---

Changes in v2:
  - rename set_virtual_apic_mode to update_vlapic_mode, suggested by Andrew
  - in hvm_update_vlapic_mode(), call the stub only if available, otherwise
    a BUG() will be triggered every time an svm guest writes the APIC_BASE MSR,
    bug reported by Andrew
  - initialize the stub for vmx unconditionally to maintain current behavior
    since no functional change is intended, bug reported by Andrew (here, I
    decided to place the initialization in start_vmx to be closer to the
    initializations of the other stubs that are relevant to apic virtualization)

 xen/arch/x86/hvm/vlapic.c          | 6 +++---
 xen/arch/x86/hvm/vmx/vmx.c         | 2 ++
 xen/arch/x86/include/asm/hvm/hvm.h | 7 +++++++
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index eb32f12e2d..dc93b5e930 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_update_vlapic_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_update_vlapic_mode(v);
 
     return 0;
 }
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 270bc98195..dd57aa7623 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3011,6 +3011,8 @@ const struct hvm_function_table * __init start_vmx(void)
         setup_ept_dump();
     }
 
+    vmx_function_table.update_vlapic_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..43d3fc2498 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 (*update_vlapic_mode)(struct vcpu *v);
 
     /*Walk nested p2m  */
     int (*nhvm_hap_walk_L1_p2m)(struct vcpu *v, paddr_t L2_gpa,
@@ -786,6 +787,12 @@ 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_update_vlapic_mode(struct vcpu *v)
+{
+    if ( hvm_funcs.update_vlapic_mode )
+        alternative_vcall(hvm_funcs.update_vlapic_mode, v);
+}
+
 #else  /* CONFIG_HVM */
 
 #define hvm_enabled false
-- 
2.37.2

Re: [PATCH v2] x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback

[Jan Beulich]

On 07.02.2023 10:43, Xenia Ragiadakou wrote:
> 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 update_vlapic_mode, and create a wrapper
> function, called hvm_update_vlapic_mode(), to be used by common hvm code.
> 
> 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>
> ---
> 
> Changes in v2:
>   - rename set_virtual_apic_mode to update_vlapic_mode, suggested by Andrew
>   - in hvm_update_vlapic_mode(), call the stub only if available, otherwise
>     a BUG() will be triggered every time an svm guest writes the APIC_BASE MSR,
>     bug reported by Andrew
>   - initialize the stub for vmx unconditionally to maintain current behavior
>     since no functional change is intended, bug reported by Andrew (here, I
>     decided to place the initialization in start_vmx to be closer to the
>     initializations of the other stubs that are relevant to apic virtualization)

I disagree with this - unconditional hooks are better put in place right
in vmx_function_table's initializer.

Also now that you use the function as a callback, vmx_vlapic_msr_changed()
will need to have cf_check added (on both declaration and definition, albeit
I keep forgetting why putting it on just the declaration isn't sufficient; I
guess a short comment to that effect next to cf_check's definition in
compiler.h would help).

Jan

Re: [PATCH v2] x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback

[Xenia Ragiadakou]

On 2/13/23 12:09, Jan Beulich wrote:
> On 07.02.2023 10:43, Xenia Ragiadakou wrote:
>> 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 update_vlapic_mode, and create a wrapper
>> function, called hvm_update_vlapic_mode(), to be used by common hvm code.
>>
>> 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>
>> ---
>>
>> Changes in v2:
>>    - rename set_virtual_apic_mode to update_vlapic_mode, suggested by Andrew
>>    - in hvm_update_vlapic_mode(), call the stub only if available, otherwise
>>      a BUG() will be triggered every time an svm guest writes the APIC_BASE MSR,
>>      bug reported by Andrew
>>    - initialize the stub for vmx unconditionally to maintain current behavior
>>      since no functional change is intended, bug reported by Andrew (here, I
>>      decided to place the initialization in start_vmx to be closer to the
>>      initializations of the other stubs that are relevant to apic virtualization)
> 
> I disagree with this - unconditional hooks are better put in place right
> in vmx_function_table's initializer.

Ok. I will fix it.

> 
> Also now that you use the function as a callback, vmx_vlapic_msr_changed()
> will need to have cf_check added (on both declaration and definition, 

Ok will do.

albeit
> I keep forgetting why putting it on just the declaration isn't sufficient; I
> guess a short comment to that effect next to cf_check's definition in
> compiler.h would help).

Yes, that would help. I don't know why it is not sufficient placing it 
just in the declaration.

> 
> Jan

-- 
Xenia

Re: [PATCH v2] x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback

[Andrew Cooper]

On 13/02/2023 10:54 am, Xenia Ragiadakou wrote:
>
> On 2/13/23 12:09, Jan Beulich wrote:
>> I keep forgetting why putting it on just the declaration isn't
>> sufficient; I
>> guess a short comment to that effect next to cf_check's definition in
>> compiler.h would help).
>
> Yes, that would help. I don't know why it is not sufficient placing it
> just in the declaration.

Because it's part of the function's type.

~Andrew

Re: [PATCH v2] x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback

[Jan Beulich]

On 13.02.2023 12:05, Andrew Cooper wrote:
> On 13/02/2023 10:54 am, Xenia Ragiadakou wrote:
>>
>> On 2/13/23 12:09, Jan Beulich wrote:
>>> I keep forgetting why putting it on just the declaration isn't
>>> sufficient; I
>>> guess a short comment to that effect next to cf_check's definition in
>>> compiler.h would help).
>>
>> Yes, that would help. I don't know why it is not sufficient placing it
>> just in the declaration.
> 
> Because it's part of the function's type.

What does that mean? Is that any different from e.g. noreturn, which
also is part of the function's type, and the compiler records this when
seeing the declaration. It only ever accumulates (never removes) such
attributes when seeing a 2nd declaration (unless multiple declarations
are otherwise rejected) or, finally, the definition.

Jan