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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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