Re: [for 4.22 v5 02/18] xen/riscv: introduce VMID allocation and manegement

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/6/25 3:05 PM, Jan Beulich wrote:

> On 20.10.2025 17:57, Oleksii Kurochko wrote:
>
> +static unsigned int vmidlen_detect(void)
> +{
> +    unsigned int vmid_bits;
> +
> +    /*
> +     * According to the RISC-V Privileged Architecture Spec:
> +     *   When MODE=Bare, guest physical addresses are equal to supervisor
> +     *   physical addresses, and there is no further memory protection
> +     *   for a guest virtual machine beyond the physical memory protection
> +     *   scheme described in Section "Physical Memory Protection".
> +     *   In this case, the remaining fields in hgatp must be set to zeros.
> +     * Thereby it is necessary to set gstage_mode not equal to Bare.
> +     */
> +    ASSERT(gstage_mode != HGATP_MODE_OFF);
> +    csr_write(CSR_HGATP,
> +              MASK_INSR(gstage_mode, HGATP_MODE_MASK) | HGATP_VMID_MASK);
> +    vmid_bits = MASK_EXTR(csr_read(CSR_HGATP), HGATP_VMID_MASK);
> +    vmid_bits = flsl(vmid_bits);
> +    csr_write(CSR_HGATP, _AC(0, UL));
> +
> +    /*
> +     * From RISC-V spec:
> +     *   Speculative executions of the address-translation algorithm behave as
> +     *   non-speculative executions of the algorithm do, except that they must
> +     *   not set the dirty bit for a PTE, they must not trigger an exception,
> +     *   and they must not create address-translation cache entries if those
> +     *   entries would have been invalidated by any SFENCE.VMA instruction
> +     *   executed by the hart since the speculative execution of the algorithm
> +     *   began.
> +     *
> +     * Also, despite of the fact here it is mentioned that when V=0 two-stage
> +     * address translation is inactivated:
> +     *   The current virtualization mode, denoted V, indicates whether the hart
> +     *   is currently executing in a guest. When V=1, the hart is either in
> +     *   virtual S-mode (VS-mode), or in virtual U-mode (VU-mode) atop a guest
> +     *   OS running in VS-mode. When V=0, the hart is either in M-mode, in
> +     *   HS-mode, or in U-mode atop an OS running in HS-mode. The
> +     *   virtualization mode also indicates whether two-stage address
> +     *   translation is active (V=1) or inactive (V=0).
> +     * But on the same side, writing to hgatp register activates it:
> +     *   The hgatp register is considered active for the purposes of
> +     *   the address-translation algorithm unless the effective privilege mode
> +     *   is U and hstatus.HU=0.
> +     *
> +     * Thereby it leaves some room for speculation even in this stage of boot,
> +     * so it could be that we polluted local TLB so flush all guest TLB.
> +     */
> +    local_hfence_gvma_all();
> That's a lot of redundancy with gstage_mode_detect(). The function call here
> actually renders the one there redundant, afaict. Did you consider putting a
> single instance at the end of it in pre_gstage_init()? Otherwise at least
> don't repeat the comment here, but merely point at the other one?
>     
Agree, it could be moved to the end of pre_gstage_init().

> > +    return vmid_bits;
> > +}
> > +
> > +void vmid_init(void)
> >       
> This (and its helper) isn't __init because you intend to also call it during
> bringup of secondary processors?
>     
Yes, I wasn't able to find that VMIDLEN is guaranteed to be same for all
harts.

> > +    unsigned int vmid_len = vmidlen_detect();
> > +    struct vmid_data *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 && (vmid_len > 1);
> > +
> > +    if ( g_vmid_used < 0 )
> > +    {
> > +        g_vmid_used = data->used;
> > +        printk("VMIDs use is %sabled\n", data->used ? "en" : "dis");
> > +    }
> > +    else if ( g_vmid_used != data->used )
> > +        printk("CPU%u: VMIDs use is %sabled\n", smp_processor_id(),
> > +               data->used ? "en" : "dis");
> > +
> > +    /* Zero indicates 'invalid generation', so we start the count at one. */
> > +    data->generation = 1;
> > +
> > +    /* Zero indicates 'VMIDs use disabled', so we start the count at one. */
> > +    data->next_vmid = 1;
> > +}
> > +
> > +void vmid_flush_vcpu(struct vcpu *v)
> > +{
> > +    write_atomic(&v->arch.vmid.generation, 0);
> > +}
> > +
> > +void vmid_flush_hart(void)
> > +{
> > +    struct vmid_data *data = ""
> > +
> > +    if ( !data->used )
> > +        return;
> > +
> > +    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__);
> >       
> Is logging of the function name of any value here?
>     
Agree, there is no any sense for the logging of the function name.

>  Also, despite the x86
> original havinbg it like this - generally no full stops please if log
> messages. "VMID generation overrun; disabling VMIDs\n" would do.
>     
Sure, I will drop it and will try to not add it in such cases. But could you
please remind (if I asked that before) me what is the reason why full stop
shouldn't be presented in such cases?

> > +bool vmid_handle_vmenter(struct vcpu_vmid *vmid)
> > +{
> > +    struct vmid_data *data = ""
> > +
> > +    /* Test if VCPU has valid VMID. */
> >       
> x86 has a ->disabled check up from here; why do you not check ->used?
>     
The x86 comment confused me, at first I thought the check was related to
erratum #170, but now I see that it might actually be useful here, so I'll add:
    if ( !data->used )
        goto disabled;

Thanks.

~ Oleksii