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

2023-02-16 Thread Andrew Cooper 
On 16/02/2023 1:36 pm, Xenia Ragiadakou wrote:
> Hi Andrew,
>
> On 2/16/23 12:28, Andrew Cooper wrote:
>> On 13/02/2023 11:50 am, Xenia Ragiadakou wrote:
>>> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>>> b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>>> index 234da4a7f4..97d6b810ec 100644
>>> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>>> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>>> @@ -85,7 +85,7 @@ typedef enum {
>>>   void vmx_asm_vmexit_handler(struct cpu_user_regs);
>>>   void vmx_intr_assist(void);
>>>   void noreturn cf_check vmx_do_resume(void);
>>> -void vmx_vlapic_msr_changed(struct vcpu *v);
>>> +void cf_check vmx_vlapic_msr_changed(struct vcpu *v);
>>
>> Hi,
>>
>> I see this patch has been committed, but this public declaration should
>> deleted, and vmx_vlapic_msr_changed() made static now that it's only
>> referenced in vmx.c.
>
> It is also used in vmcs.c

Oh, so it is.  It just doesn't show up on the patch diff.

That use likely won't survive fixing the Intel APIC-V logic, but I guess
we're stuck with it for now.

Sorry for the noise.

>
>>
>> It needs a forward declaration in vmx.c because of its position relative
>> to the vmx_function_table, but that's fine - we've got plenty of other
>> examples like this.
>>
>> Could I talk you into doing an incremental fix?
>>
>> Alternatively, we could get better cleanup by forward declaring just
>> {vmx,svm}_function_table, then moving the tables to the very bottom of
>> {vmx,svm}.c at which point we can drop all the forward declarations.
>>
>> Oh top of that, I suspect we have other public function definitions
>> which can turn static, if you happen to spot any while doing this.
>
> Sure I could try to cleanup {svm,vmx}.c and the corresponding headers.

Thankyou.

~Andrew

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

2023-02-16 Thread Xenia Ragiadakou 

Hi Andrew,

On 2/16/23 12:28, Andrew Cooper wrote:

On 13/02/2023 11:50 am, Xenia Ragiadakou wrote:

diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h 
b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index 234da4a7f4..97d6b810ec 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -85,7 +85,7 @@ typedef enum {
  void vmx_asm_vmexit_handler(struct cpu_user_regs);
  void vmx_intr_assist(void);
  void noreturn cf_check vmx_do_resume(void);
-void vmx_vlapic_msr_changed(struct vcpu *v);
+void cf_check vmx_vlapic_msr_changed(struct vcpu *v);


Hi,

I see this patch has been committed, but this public declaration should
deleted, and vmx_vlapic_msr_changed() made static now that it's only
referenced in vmx.c.


It is also used in vmcs.c



It needs a forward declaration in vmx.c because of its position relative
to the vmx_function_table, but that's fine - we've got plenty of other
examples like this.

Could I talk you into doing an incremental fix?

Alternatively, we could get better cleanup by forward declaring just
{vmx,svm}_function_table, then moving the tables to the very bottom of
{vmx,svm}.c at which point we can drop all the forward declarations.

Oh top of that, I suspect we have other public function definitions
which can turn static, if you happen to spot any while doing this.


Sure I could try to cleanup {svm,vmx}.c and the corresponding headers.



Thanks,

~Andrew


--
Xenia

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

2023-02-16 Thread Andrew Cooper 
On 13/02/2023 11:50 am, Xenia Ragiadakou wrote:
> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h 
> b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> index 234da4a7f4..97d6b810ec 100644
> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> @@ -85,7 +85,7 @@ typedef enum {
>  void vmx_asm_vmexit_handler(struct cpu_user_regs);
>  void vmx_intr_assist(void);
>  void noreturn cf_check vmx_do_resume(void);
> -void vmx_vlapic_msr_changed(struct vcpu *v);
> +void cf_check vmx_vlapic_msr_changed(struct vcpu *v);

Hi,

I see this patch has been committed, but this public declaration should
deleted, and vmx_vlapic_msr_changed() made static now that it's only
referenced in vmx.c.

It needs a forward declaration in vmx.c because of its position relative
to the vmx_function_table, but that's fine - we've got plenty of other
examples like this.

Could I talk you into doing an incremental fix?

Alternatively, we could get better cleanup by forward declaring just
{vmx,svm}_function_table, then moving the tables to the very bottom of
{vmx,svm}.c at which point we can drop all the forward declarations.

Oh top of that, I suspect we have other public function definitions
which can turn static, if you happen to spot any while doing this.

Thanks,

~Andrew

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

2023-02-15 Thread Tian, Kevin 
> From: Xenia Ragiadakou 
> Sent: Monday, February 13, 2023 7:50 PM
> 
> 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_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 

Reviewed-by: Kevin Tian

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

2023-02-13 Thread Jan Beulich 
On 13.02.2023 12:50, 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_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 

Reviewed-by: Jan Beulich

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

2023-02-13 Thread 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_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 
---

Changes in v3:
  - add cf_check attribute in both the function declaration and definition of
vmx_vlapic_msr_changed since it is used as callback, bug reported by Jan
  - initialize update_vlapic_mode in the vmx_function_table initializer to
keep unconditionally initialized cbs in a single place, reported by Jan

 xen/arch/x86/hvm/vlapic.c  | 6 +++---
 xen/arch/x86/hvm/vmx/vmx.c | 3 ++-
 xen/arch/x86/include/asm/hvm/hvm.h | 7 +++
 xen/arch/x86/include/asm/hvm/vmx/vmx.h | 2 +-
 4 files changed, 13 insertions(+), 5 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 
 #include 
 #include 
-#include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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..0ec33bcc18 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2763,6 +2763,7 @@ static struct hvm_function_table __initdata_cf_clobber 
vmx_function_table = {
 .nhvm_vcpu_vmexit_event = nvmx_vmexit_event,
 .nhvm_intr_blocked= nvmx_intr_blocked,
 .nhvm_domain_relinquish_resources = nvmx_domain_relinquish_resources,
+.update_vlapic_mode = vmx_vlapic_msr_changed,
 .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
 .enable_msr_interception = vmx_enable_msr_interception,
 .altp2m_vcpu_update_p2m = vmx_vcpu_update_eptp,
@@ -3472,7 +3473,7 @@ static void vmx_install_vlapic_mapping(struct vcpu *v)
 vmx_vmcs_exit(v);
 }
 
-void vmx_vlapic_msr_changed(struct vcpu *v)
+void cf_check vmx_vlapic_msr_changed(struct vcpu *v)
 {
 int virtualize_x2apic_mode;
 struct vlapic *vlapic = vcpu_vlapic(v);
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
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h 
b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index 234da4a7f4..97d6b810ec 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -85,7 +85,7 @@ typedef enum {
 void vmx_asm_vmexit_handler(struct cpu_user_regs);
 void vmx_intr_assist(void);
 void noreturn cf_check vmx_do_resume(void);
-void vmx_vlapic_msr_changed(struct vcpu *v);
+void cf_check vmx_vlapic_msr_changed(struct vcpu *v);
 struct hvm_emulate_ctxt;
 void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt);
 void vmx_realmode(struct cpu_user_regs *regs);
-- 
2.37.2