Re: [PATCH v4 02/18] xen/riscv: introduce VMID allocation and manegement

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.09.2025 23:55, Oleksii Kurochko wrote:
> @@ -151,6 +152,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>  
>      gstage_mode_detect();
>  
> +    vmid_init();

Like for the earlier patch, I'm not convinced this is a function good
to call from the top-level start_xen(). The two functions sitting side
by side actually demonstrates the scalability issue pretty well.

> --- /dev/null
> +++ b/xen/arch/riscv/vmid.c
> @@ -0,0 +1,193 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/domain.h>
> +#include <xen/init.h>
> +#include <xen/sections.h>
> +#include <xen/lib.h>
> +#include <xen/param.h>
> +#include <xen/percpu.h>
> +
> +#include <asm/atomic.h>
> +#include <asm/csr.h>
> +#include <asm/flushtlb.h>
> +#include <asm/p2m.h>
> +
> +/* Xen command-line option to enable VMIDs */
> +static bool __ro_after_init opt_vmid_use_enabled = true;
> +boolean_param("vmid", opt_vmid_use_enabled);

Is there a particular reason to not have the variable be simply opt_vmid,
properly in sync with the command line option?

> +void vmid_init(void)
> +{
> +    static int8_t g_vmid_used = -1;
> +
> +    unsigned int vmid_len = vmidlen_detect();
> +    struct vmid_data *data = &this_cpu(vmid_data);
> +
> +    BUILD_BUG_ON((HGATP_VMID_MASK >> HGATP_VMID_SHIFT) >
> +                 (BIT((sizeof(data->max_vmid) * BITS_PER_BYTE), UL) - 1));
> +
> +    data->max_vmid = BIT(vmid_len, U) - 1;
> +    data->used = !opt_vmid_use_enabled || (vmid_len <= 1);

Since you inverted the sense of variable and field, you also need to invert
the expression here:

    data->used = opt_vmid_use_enabled && (vmid_len > 1);

> +    if ( g_vmid_used < 0 )
> +    {
> +        g_vmid_used = data->used;
> +        printk("VMIDs use is %sabled\n", data->used ? "dis" : "en");

Same here - "dis" and "en" need to switch places.

> +    }
> +    else if ( g_vmid_used != data->used )
> +        printk("CPU%u: VMIDs use is %sabled\n", smp_processor_id(),
> +               data->used ? "dis" : "en");

And again here.

> +void vcpu_vmid_flush_vcpu(struct vcpu *v)

An reason to have two "vcpu" in the name?

> +{
> +    write_atomic(&v->arch.vmid.generation, 0);
> +}
> +
> +void vmid_flush_hart(void)
> +{
> +    struct vmid_data *data = &this_cpu(vmid_data);
> +
> +    if ( data->used )
> +        return;

Again the sense needs reversting.

> +    if ( likely(++data->generation != 0) )
> +        return;
> +
> +    /*
> +     * VMID generations are 64 bit.  Overflow of generations never happens.
> +     * For safety, we simply disable ASIDs, so correctness is established; it
> +     * only runs a bit slower.
> +     */
> +    printk("%s: VMID generation overrun. Disabling VMIDs.\n", __func__);
> +    data->used = 1;

And yet again.

> +bool vmid_handle_vmenter(struct vcpu_vmid *vmid)
> +{
> +    struct vmid_data *data = &this_cpu(vmid_data);
> +
> +    /* Test if VCPU has valid VMID. */
> +    if ( read_atomic(&vmid->generation) == data->generation )
> +        return 0;
> +
> +    /* If there are no free VMIDs, need to go to a new generation. */
> +    if ( unlikely(data->next_vmid > data->max_vmid) )
> +    {
> +        vmid_flush_hart();
> +        data->next_vmid = 1;
> +        if ( data->used )
> +            goto disabled;

And yet another time.

Jan