[PATCH] x86/HVM: polish hvm_asid_init() a little

[PATCH] x86/HVM: polish hvm_asid_init() a little

[Jan Beulich]

While the logic there covers asymmetric cases, the resulting log
messages would likely raise more confusion than clarify anything. Split
the BSP action from the AP one, indicating the odd CPU in the AP log
message, thus avoiding the impression that global state would have
changed.

While there also
- move g_disabled into .data.ro_after_init; only the BSP will ever write
  to it,
- make the function's parameter unsigned; no negative values may be
  passed in. Also reflect this in svm_asid_init().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/asid.c
+++ b/xen/arch/x86/hvm/asid.c
@@ -48,20 +48,22 @@ struct hvm_asid_data {
 
 static DEFINE_PER_CPU(struct hvm_asid_data, hvm_asid_data);
 
-void hvm_asid_init(int nasids)
+void hvm_asid_init(unsigned int nasids)
 {
-    static int8_t g_disabled = -1;
+    static int8_t __ro_after_init g_disabled = -1;
     struct hvm_asid_data *data = &this_cpu(hvm_asid_data);
 
     data->max_asid = nasids - 1;
     data->disabled = !opt_asid_enabled || (nasids <= 1);
 
-    if ( g_disabled != data->disabled )
+    if ( g_disabled < 0 )
     {
-        printk("HVM: ASIDs %sabled.\n", data->disabled ? "dis" : "en");
-        if ( g_disabled < 0 )
-            g_disabled = data->disabled;
+        g_disabled = data->disabled;
+        printk("HVM: ASIDs %sabled\n", data->disabled ? "dis" : "en");
     }
+    else if ( g_disabled != data->disabled )
+        printk("HVM: CPU%u: ASIDs %sabled\n", smp_processor_id(),
+               data->disabled ? "dis" : "en");
 
     /* Zero indicates 'invalid generation', so we start the count at one. */
     data->core_asid_generation = 1;
--- a/xen/arch/x86/hvm/svm/asid.c
+++ b/xen/arch/x86/hvm/svm/asid.c
@@ -12,7 +12,7 @@
 
 void svm_asid_init(const struct cpuinfo_x86 *c)
 {
-    int nasids = 0;
+    unsigned int nasids = 0;
 
     /* Check for erratum #170, and leave ASIDs disabled if it's present. */
     if ( !cpu_has_amd_erratum(c, AMD_ERRATUM_170) )
--- a/xen/arch/x86/include/asm/hvm/asid.h
+++ b/xen/arch/x86/include/asm/hvm/asid.h
@@ -13,7 +13,7 @@ struct vcpu;
 struct hvm_vcpu_asid;
 
 /* Initialise ASID management for the current physical CPU. */
-void hvm_asid_init(int nasids);
+void hvm_asid_init(unsigned int nasids);
 
 /* Invalidate a particular ASID allocation: forces re-allocation. */
 void hvm_asid_flush_vcpu_asid(struct hvm_vcpu_asid *asid);

Re: [PATCH] x86/HVM: polish hvm_asid_init() a little

[Jan Beulich]

On 04.08.2025 17:41, Jan Beulich wrote:
> While the logic there covers asymmetric cases, the resulting log
> messages would likely raise more confusion than clarify anything. Split
> the BSP action from the AP one, indicating the odd CPU in the AP log
> message, thus avoiding the impression that global state would have
> changed.
> 
> While there also
> - move g_disabled into .data.ro_after_init; only the BSP will ever write
>   to it,
> - make the function's parameter unsigned; no negative values may be
>   passed in. Also reflect this in svm_asid_init().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Sorry, I meant to Cc: you on the submission, as it was the reviewing of
your copied code which made me spot the shortcomings (which, as indicated,
I think it would be nice if you avoided from the very start).

Jan

> --- a/xen/arch/x86/hvm/asid.c
> +++ b/xen/arch/x86/hvm/asid.c
> @@ -48,20 +48,22 @@ struct hvm_asid_data {
>  
>  static DEFINE_PER_CPU(struct hvm_asid_data, hvm_asid_data);
>  
> -void hvm_asid_init(int nasids)
> +void hvm_asid_init(unsigned int nasids)
>  {
> -    static int8_t g_disabled = -1;
> +    static int8_t __ro_after_init g_disabled = -1;
>      struct hvm_asid_data *data = &this_cpu(hvm_asid_data);
>  
>      data->max_asid = nasids - 1;
>      data->disabled = !opt_asid_enabled || (nasids <= 1);
>  
> -    if ( g_disabled != data->disabled )
> +    if ( g_disabled < 0 )
>      {
> -        printk("HVM: ASIDs %sabled.\n", data->disabled ? "dis" : "en");
> -        if ( g_disabled < 0 )
> -            g_disabled = data->disabled;
> +        g_disabled = data->disabled;
> +        printk("HVM: ASIDs %sabled\n", data->disabled ? "dis" : "en");
>      }
> +    else if ( g_disabled != data->disabled )
> +        printk("HVM: CPU%u: ASIDs %sabled\n", smp_processor_id(),
> +               data->disabled ? "dis" : "en");
>  
>      /* Zero indicates 'invalid generation', so we start the count at one. */
>      data->core_asid_generation = 1;
> --- a/xen/arch/x86/hvm/svm/asid.c
> +++ b/xen/arch/x86/hvm/svm/asid.c
> @@ -12,7 +12,7 @@
>  
>  void svm_asid_init(const struct cpuinfo_x86 *c)
>  {
> -    int nasids = 0;
> +    unsigned int nasids = 0;
>  
>      /* Check for erratum #170, and leave ASIDs disabled if it's present. */
>      if ( !cpu_has_amd_erratum(c, AMD_ERRATUM_170) )
> --- a/xen/arch/x86/include/asm/hvm/asid.h
> +++ b/xen/arch/x86/include/asm/hvm/asid.h
> @@ -13,7 +13,7 @@ struct vcpu;
>  struct hvm_vcpu_asid;
>  
>  /* Initialise ASID management for the current physical CPU. */
> -void hvm_asid_init(int nasids);
> +void hvm_asid_init(unsigned int nasids);
>  
>  /* Invalidate a particular ASID allocation: forces re-allocation. */
>  void hvm_asid_flush_vcpu_asid(struct hvm_vcpu_asid *asid);

Re: [PATCH] x86/HVM: polish hvm_asid_init() a little

[Jason Andryuk]

On 2025-08-04 11:41, Jan Beulich wrote:
> While the logic there covers asymmetric cases, the resulting log
> messages would likely raise more confusion than clarify anything. Split
> the BSP action from the AP one, indicating the odd CPU in the AP log
> message, thus avoiding the impression that global state would have
> changed.
> 
> While there also
> - move g_disabled into .data.ro_after_init; only the BSP will ever write
>    to it,
> - make the function's parameter unsigned; no negative values may be
>    passed in. Also reflect this in svm_asid_init().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Re: [PATCH] x86/HVM: polish hvm_asid_init() a little

[Roger Pau Monné]

On Tue, Aug 05, 2025 at 11:30:58AM -0400, Jason Andryuk wrote:
> On 2025-08-04 11:41, Jan Beulich wrote:
> > While the logic there covers asymmetric cases, the resulting log
> > messages would likely raise more confusion than clarify anything. Split
> > the BSP action from the AP one, indicating the odd CPU in the AP log
> > message, thus avoiding the impression that global state would have
> > changed.
> > 
> > While there also
> > - move g_disabled into .data.ro_after_init; only the BSP will ever write
> >    to it,
> > - make the function's parameter unsigned; no negative values may be
> >    passed in. Also reflect this in svm_asid_init().
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.