Re: [Xen-devel] [PATCH v2 09/10] x86/SVM: Hook up miscellaneous AVIC functions

[Suravee Suthikulpanit]

On 01/10/2017 04:00 PM, Jan Beulich wrote:

On 10.01.17 at 09:35,  wrote:

On 01/05/2017 11:05 PM, Jan Beulich wrote:

On 31.12.16 at 06:46,  wrote:

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1438,6 +1438,11 @@ static int svm_cpu_up(void)
 return 0;
 }

+static inline int svm_avic_enabled(void)


bool?


Actually, I declared this as int because the
hvm_function_table.virtual_intr_delivery_enabled() is returning int.


Oh, that's used as a hook function. That's pretty un-obvious for a
function declared inline. Of course in that case you need to match
the hook function type, even if that ought to have return type bool.
Even farther - I question the need for a function here in the first
place, as both VMX and now SVM AVIC return a static value. This
could therefore be a bool flag alongside the various others we
already have near the beginning of the structure, if you're up to
such a change. If you'd rather stick with what's there now, we can
always put together a cleanup patch later on.

Jan



Sure, I can incorporate the changes to make this a bool flag in this patch.

S

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/10] x86/SVM: Hook up miscellaneous AVIC functions

[Jan Beulich]

>>> On 10.01.17 at 09:35,  wrote:
> On 01/05/2017 11:05 PM, Jan Beulich wrote:
> On 31.12.16 at 06:46,  wrote:
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -1438,6 +1438,11 @@ static int svm_cpu_up(void)
>>>  return 0;
>>>  }
>>>
>>> +static inline int svm_avic_enabled(void)
>>
>> bool?
> 
> Actually, I declared this as int because the 
> hvm_function_table.virtual_intr_delivery_enabled() is returning int.

Oh, that's used as a hook function. That's pretty un-obvious for a
function declared inline. Of course in that case you need to match
the hook function type, even if that ought to have return type bool.
Even farther - I question the need for a function here in the first
place, as both VMX and now SVM AVIC return a static value. This
could therefore be a bool flag alongside the various others we
already have near the beginning of the structure, if you're up to
such a change. If you'd rather stick with what's there now, we can
always put together a cleanup patch later on.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/10] x86/SVM: Hook up miscellaneous AVIC functions

[Suravee Suthikulpanit]

Jan,

On 01/05/2017 11:05 PM, Jan Beulich wrote:

On 31.12.16 at 06:46,  wrote:

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1438,6 +1438,11 @@ static int svm_cpu_up(void)
 return 0;
 }

+static inline int svm_avic_enabled(void)


bool?


Actually, I declared this as int because the 
hvm_function_table.virtual_intr_delivery_enabled() is returning int.





@@ -1472,16 +1477,27 @@ const struct hvm_function_table * __init start_svm(void)
 P(cpu_has_svm_decode, "DecodeAssists");
 P(cpu_has_pause_filter, "Pause-Intercept Filter");
 P(cpu_has_tsc_ratio, "TSC Rate MSR");
-P(cpu_has_svm_avic, "AVIC");
-#undef P
-
-if ( !printed )
-printk(" - none\n");

 svm_function_table.hap_supported = !!cpu_has_svm_npt;
 svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB |
 ((cpuid_edx(0x8001) & 0x0400) ? HVM_HAP_SUPERPAGE_1GB : 0);

+if ( !cpu_has_svm_avic )
+svm_avic = 0;
+
+if ( svm_avic )
+{
+svm_function_table.deliver_posted_intr  = svm_avic_deliver_posted_intr;
+svm_function_table.virtual_intr_delivery_enabled = svm_avic_enabled;
+P(cpu_has_svm_avic, "AVIC (enabled)");
+}
+else
+P(cpu_has_svm_avic, "AVIC (disabled)");
+#undef P
+
+if ( !printed )
+printk(" - none\n");


Could I talk you into moving this up a few lines, so that effectively
the last four lines here won't need to move at all?

Jan



Sure, good point.

Thanks,
Suravee

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/10] x86/SVM: Hook up miscellaneous AVIC functions

[Jan Beulich]

>>> On 31.12.16 at 06:46,  wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1438,6 +1438,11 @@ static int svm_cpu_up(void)
>  return 0;
>  }
>  
> +static inline int svm_avic_enabled(void)

bool?

> @@ -1472,16 +1477,27 @@ const struct hvm_function_table * __init 
> start_svm(void)
>  P(cpu_has_svm_decode, "DecodeAssists");
>  P(cpu_has_pause_filter, "Pause-Intercept Filter");
>  P(cpu_has_tsc_ratio, "TSC Rate MSR");
> -P(cpu_has_svm_avic, "AVIC");
> -#undef P
> -
> -if ( !printed )
> -printk(" - none\n");
>  
>  svm_function_table.hap_supported = !!cpu_has_svm_npt;
>  svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB |
>  ((cpuid_edx(0x8001) & 0x0400) ? HVM_HAP_SUPERPAGE_1GB : 0);
>  
> +if ( !cpu_has_svm_avic )
> +svm_avic = 0;
> +
> +if ( svm_avic )
> +{
> +svm_function_table.deliver_posted_intr  = 
> svm_avic_deliver_posted_intr;
> +svm_function_table.virtual_intr_delivery_enabled = svm_avic_enabled;
> +P(cpu_has_svm_avic, "AVIC (enabled)");
> +}
> +else
> +P(cpu_has_svm_avic, "AVIC (disabled)");
> +#undef P
> +
> +if ( !printed )
> +printk(" - none\n");

Could I talk you into moving this up a few lines, so that effectively
the last four lines here won't need to move at all?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/10] x86/SVM: Hook up miscellaneous AVIC functions

[Andrew Cooper]

On 31/12/2016 05:46, Suravee Suthikulpanit wrote:
> Hook up virtual_intr_delivery_enabled and deliver_posted_intr functions
> when AVIC is enabled.
>
> Signed-off-by: Suravee Suthikulpanit 
> Cc: Konrad Rzeszutek Wilk 
> Cc: Jan Beulich 
> Cc: Boris Ostrovsky 
> ---
>  xen/arch/x86/hvm/svm/svm.c | 26 +-
>  xen/include/asm-x86/hvm/svm/avic.h |  3 +++
>  2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 922f48f..7c0cda0 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1438,6 +1438,11 @@ static int svm_cpu_up(void)
>  return 0;
>  }
>  
> +static inline int svm_avic_enabled(void)
> +{
> +return svm_avic;
> +}
> +
>  const struct hvm_function_table * __init start_svm(void)
>  {
>  bool_t printed = 0;
> @@ -1472,16 +1477,27 @@ const struct hvm_function_table * __init 
> start_svm(void)
>  P(cpu_has_svm_decode, "DecodeAssists");
>  P(cpu_has_pause_filter, "Pause-Intercept Filter");
>  P(cpu_has_tsc_ratio, "TSC Rate MSR");
> -P(cpu_has_svm_avic, "AVIC");
> -#undef P
> -
> -if ( !printed )
> -printk(" - none\n");
>  
>  svm_function_table.hap_supported = !!cpu_has_svm_npt;
>  svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB |
>  ((cpuid_edx(0x8001) & 0x0400) ? HVM_HAP_SUPERPAGE_1GB : 0);
>  
> +if ( !cpu_has_svm_avic )
> +svm_avic = 0;
> +
> +if ( svm_avic )
> +{
> +svm_function_table.deliver_posted_intr  = 
> svm_avic_deliver_posted_intr;
> +svm_function_table.virtual_intr_delivery_enabled = svm_avic_enabled;
> +P(cpu_has_svm_avic, "AVIC (enabled)");
> +}
> +else
> +P(cpu_has_svm_avic, "AVIC (disabled)");
> +#undef P
> +
> +if ( !printed )
> +printk(" - none\n");
> +
>  return _function_table;
>  }
>  
> diff --git a/xen/include/asm-x86/hvm/svm/avic.h 
> b/xen/include/asm-x86/hvm/svm/avic.h
> index 1676e01..5be3e76 100644
> --- a/xen/include/asm-x86/hvm/svm/avic.h
> +++ b/xen/include/asm-x86/hvm/svm/avic.h
> @@ -41,4 +41,7 @@ void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs 
> *regs);
>  void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs);
>  
>  void svm_avic_deliver_posted_intr(struct vcpu *v, u8 vector);
> +
> +void setup_avic_dump(void);
> +
>  #endif /* _SVM_AVIC_H_ */

This hunk should be in the subsquent patch.  Otherwise, Reviewed-by:
Andrew Cooper 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel