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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.06.2025 11:46, Oleksii Kurochko wrote:
> On 6/18/25 5:46 PM, Jan Beulich wrote:
>> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>>> --- /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?

Depends on the condition for when to flush. Of course performance is
unavoidably going to suffer if you have only very few VMIDs to use.
Nevertheless, as indicated before, the model used on x86 may be a
candidate to use here, too. See hvm_asid_handle_vmenter() for the
core (and vendor-independent) part of it.

> What about then to allocate VMID per-domain?

That's what you're doing right now, isn't it? And that gets problematic when
you have only very few bits in hgatp.VMID, as mentioned below.

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

Right, but then why -2? (Maybe this is moot now that you agreed that
INVALID_VMID can be defined differently.

>> 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().

Right, but just to be clear: That flush should not be conditional upon
VMIDLEN being 0. In whatever model you chose, the handling of this special
case should come out "natural".

>>> +        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()?

I don't know, but dealing with the issue there feels wrong. However,
before deciding where to do something, it needs to be clear what you
actually want to achieve. To me at least, that's not clear at all.

>>> +    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;
>      }

Which, as said, is a problem when there are only very few VMIDs.

> 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.

Let's assume over the uptime of a system you cycle through all VMIDs a thousand
times. While you manage to delay some TLB flushes, the percentage of ones 
actually
saved is going to be very low then.

>>> +    }
>>> +
>>> +    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.

But only if you actually came through p2m_init() prior to the error. My point
is: If you allocate a VMID here anyway, why first set the field like this?
(Again, this is likely moot since the allocation scheme is likely to change
altogether.)

Jan