Re: [PATCH v2 03/17] xen/riscv: introduce guest domain's VMID allocation and manegement

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8569 bytes --]


On 6/18/25 5:46 PM, Jan Beulich wrote:
> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>> Implementation is based on Arm code with some minor changes:
>>   - Re-define INVALID_VMID.
>>   - Re-define MAX_VMID.
>>   - Add TLB flushing when VMID is re-used.
>>
>> Also, as a part of this path structure p2m_domain is introduced with
>> vmid member inside it. It is necessary for VMID management functions.
>>
>> Add a bitmap-based allocator to manage VMID space, supporting up to 127
>> VMIDs on RV32 and 16,383 on RV64 platforms, in accordance with the
>> architecture's hgatp VMID field (RV32 - 7 bit long, others - 14 bit long).
>>
>> Reserve the highest VMID as INVALID_VMID to ensure it's not reused.
> Why must that VMID not be (re)used? INVALID_VMID can be any value wider
> than the hgatp.VMID field.

Oh, agree it could be just any value wider tan hgatp.VMID filed. I forgot
about that hgatp.VMID is only 14-bit long value. So we have two extra bits
in uint16_t.

>> --- /dev/null
>> +++ b/xen/arch/riscv/p2m.c
>> @@ -0,0 +1,115 @@
>> +#include <xen/bitops.h>
>> +#include <xen/lib.h>
>> +#include <xen/sched.h>
>> +#include <xen/spinlock.h>
>> +#include <xen/xvmalloc.h>
>> +
>> +#include <asm/p2m.h>
>> +#include <asm/sbi.h>
>> +
>> +static spinlock_t vmid_alloc_lock = SPIN_LOCK_UNLOCKED;
>> +
>> +/*
>> + * hgatp's VMID field is 7 or 14 bits. RV64 may support 14-bit VMID.
>> + * Using a bitmap here limits us to 127 (2^7 - 1) or 16383 (2^14 - 1)
>> + * concurrent domains.
> Which is pretty limiting especially in the RV32 case. Hence why we don't
> assign a permanent ID to VMs on x86, but rather manage IDs per-CPU (note:
> not per-vCPU).

Good point.

I don't believe anyone will use RV32.
For RV64, the available ID space seems sufficiently large.

However, if it turns out that the value isn't large enough even for RV64,
I can rework it to manage IDs per physical CPU.
Wouldn't that approach result in more TLB entries being flushed compared
to per-vCPU allocation, potentially leading to slightly worse performance?

What about then to allocate VMID per-domain?

>> The bitmap space will be allocated dynamically
>> + * based on whether 7 or 14 bit VMIDs are supported.
>> + */
>> +static unsigned long *vmid_mask;
>> +static unsigned long *vmid_flushing_needed;
>> +
>> +/*
>> + * -2 here because:
>> + *    - -1 is needed to get the maximal possible VMID
> I don't follow this part.

Probably, I'm missing something.

hgat.vmid is 7 bit long. BIT(7,U) = 1 << 7 = 128 which is bigger
then 7 bit can cover (0b1000_0000 and 0x111_1111). Thereby the MAX_VMID is:
  BIT(7, U) - 1 (in case of RV32).

>> + */
>> +#ifdef CONFIG_RISCV_32
>> +#define MAX_VMID (BIT(7, U) - 2)
>> +#else
> Better "#elif defined(CONFIG_RISCV_64)"?

First, I read the spec as for other bitness except 32 it will be 14 bit long, but I re-read it and
it is true only for HSXLEN=64, so RV128 will/can have different amount of bit for VMID. I will
update to "#elif defined(CONFIG_RISCV_64)" + #error "Define MAX_VMID" if bitness isn't 32 or 64.


>> +{
>> +    /*
>> +     * Allocate space for vmid_mask and vmid_flushing_needed
>> +     * based on INVALID_VMID as it is the max possible VMID which just
>> +     * was reserved to be INVALID_VMID.
>> +     */
>> +    vmid_mask = xvzalloc_array(unsigned long, BITS_TO_LONGS(INVALID_VMID));
>> +    vmid_flushing_needed =
>> +        xvzalloc_array(unsigned long, BITS_TO_LONGS(INVALID_VMID));
> These both want to use MAX_VMID + 1; there's no logical connection here to
> INVALID_VMID.
>
> Furthermore don't you first need to determine how many bits hgatp.VMID actually
> implements? The 7 and 14 bits respectively are maximum values only, after all.

I missed that it depends on VMIDLEN:
```
The number of VMID bits is UNSPECIFIED and may be zero. The number of implemented VMID bits,
termed VMIDLEN, may be determined by writing one to every bit position in the VMID field, then
reading back the value in hgatp to see which bit positions in the VMID field hold a one. The least-
significant bits of VMID are implemented first: that is, if VMIDLEN > 0, VMID[VMIDLEN-1:0] is
writable. The maximal value of VMIDLEN, termed VMIDMAX, is 7 for Sv32x4 or 14 for Sv39x4,
Sv48x4, and Sv57x4.
```
So yes, I have to determine first how many bits are supported by an implementation.

> VMIDLEN being permitted to be 0, how would you run more than one VM (e.g. Dom0)
> on such a system?

Hmm, good question.

Then it will be needed to flush TLB on each VM switch by using
sbi_remote_hfence_gvma().

>> +    if ( !vmid_mask || !vmid_flushing_needed )
>> +        panic("Could not allocate VMID bitmap space or VMID flushing map\n");
>> +
>> +    set_bit(INVALID_VMID, vmid_mask);
> If (see above) this is really needed, __set_bit() please.
>
>> +}
>> +
>> +int p2m_alloc_vmid(struct domain *d)
> Looks like this can be static? (p2m_free_vmid() has no caller at all, so
> it's not clear what use it is going to be.)

It really can be static. And p2m_free_vmid() too, but as there is no caller
of p2m_free_vmid() probably it makes sense to do in the following way:
   /* Uncomment static when p2m_free_vmid() will be called. */
   /* static */ void p2m_free_vmid(struct domain *d)
Or just drop for the moment when it will be really needed.



>> +        goto out;
>> +    }
>> +
>> +    set_bit(nr, vmid_mask);
> Since you do this under lock, even here __set_bit() ought to be sufficient.
>
>> +    if ( test_bit(p2m->vmid, vmid_flushing_needed) )
>> +    {
>> +        clear_bit(p2m->vmid, vmid_flushing_needed);
> And __clear_bit() here, or yet better use __test_and_clear_bit() in the if().
>
>> +        sbi_remote_hfence_gvma_vmid(d->dirty_cpumask, 0, 0, p2m->vmid);
> You're creating d; it cannot possibly have run on any CPU yet. IOW
> d->dirty_cpumask will be reliably empty here. I think it would be hard to
> avoid issuing the flush to all CPUs here in this scheme.

I didn't double check, but I was sure that in case d->dirty_cpumask is empty then
rfence for all CPUs will be send. But I was wrong about that.

What about just update a code of sbi_rfence_v02()?

At the moment, we have check if a pointer to cpu_mask isn't NULL and if NULL then
do rfence for all CPUs:

static int cf_check sbi_rfence_v02(unsigned long fid,
                                    const cpumask_t *cpu_mask,
                                    vaddr_t start, size_t size,
                                    unsigned long arg4, unsigned long arg5)
{
    ...

     /*
      * hart_mask_base can be set to -1 to indicate that hart_mask can be
      * ignored and all available harts must be considered.
      */
     if ( !cpu_mask )
         return sbi_rfence_v02_real(fid, 0UL, -1UL, start, size, arg4);
    ...

What about  just to add here:
     if ( !cpu_mask || cpumask_empty(cpu_mask) )

Does it make sense?

>> +    spin_unlock(&vmid_alloc_lock);
>> +    return rc;
>> +}
>> +
>> +void p2m_free_vmid(struct domain *d)
>> +{
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +
>> +    spin_lock(&vmid_alloc_lock);
>> +
>> +    if ( p2m->vmid != INVALID_VMID )
>> +    {
>> +        clear_bit(p2m->vmid, vmid_mask);
>> +        set_bit(p2m->vmid, vmid_flushing_needed);
> Does this scheme really avoid any flushes (except near when the system is
> about to go down)?
>
> As to choice of functions - see above.

I think yes, so my idea was that if vmid isn't freed then we have enough free VMID
and in this case flush isn't needed as each vcpu has unique not-used yet VMID,
and if there is no free VMID then and error will return in p2m_alloc_vmid():
     if ( nr == MAX_VMID )
     {
         rc = -EBUSY;
         printk(XENLOG_ERR "p2m.c: dom%pd: VMID pool exhausted\n", d->domain_id);
         goto out;
     }

On other hand, if VMID was freed and then re-used in p2m_alloc_vmid(), then it means
that vmid_flushing_needed will have VMID bit set, what means that a TLB flush is needed.

>
>> +    }
>> +
>> +    spin_unlock(&vmid_alloc_lock);
>> +}
>> +
>> +int p2m_init(struct domain *d)
>> +{
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +    int rc;
>> +
>> +    p2m->vmid = INVALID_VMID;
> Given the absence of callers of p2m_free_vmid() it's also not clear what use
> this is.

Just mark that VMID for this domain wasn't yet allocated.

Anyway, it will be called from arch_domain_create() by arch_domain_destroy() so if the some
error happens during arch_domain_create() and p2m->vmid wasn't allocated yet (so is equal to
INVALID_VMID), it means that there is no sense to update vmid_mask or vmid_flushing_needed.

~ Oleksii


[-- Attachment #2: Type: text/html, Size: 11771 bytes --]