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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/24/25 12:44 PM, Jan Beulich wrote:

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

IIUC, so basically it is just a round-robin and when VMIDs are ran out
then just do full guest TLB flush and start to re-use VMIDs from the start.
It makes sense to me, I'll implement something similar. (as I'm not really
sure that we need data->core_asid_generation, probably, I will understand
it better when start to implement 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.
>     
Right, I just phrased my question poorly—sorry about that.
What I meant to ask is: does the approach described above actually depend on whether
VMIDs are allocated per-domain or per-pCPU? It seems that the main advantage of
allocating VMIDs per-pCPU is potentially reducing the number of TLB flushes,
since it's more likely that a platform will have more than VMID_MAX domains than
VMID_MAX physical CPUs—am I right?
> > > > 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.
>     
Yes, another one -1 was because how INVALID_VMID was defined.

> > > 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".
>     
Sure. I have some ideas how to do it 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.
>     
I want to achieve the following behavior: if a mask is empty
(specifically, in our case d->dirty_cpumask), then perform the flush
on all CPUs.

If you think it's not a good idea to change the current implementation
of sbi_rfence_v02(), then I’ll just check if d->dirty_cpumask is empty
before calling sbi_remote_hfence_gvma_vmid(d->dirty_cpumask, 0, 0, p2m->vmid).

If it is empty, I’ll call sbi_remote_hfence_gvma() instead:
  if ( !cpumask_empty(d->dirty_cpumask) )
      sbi_remote_hfence_gvma_vmid(d->dirty_cpumask, 0, 0, p2m->vmid);
  else
      sbi_remote_hfence_gvma(NULL, 0, 0);

A similar check will be needed in p2m_force_tlb_flush_sync(), which is
implemented in one of the following patches in this series.
However, if we instead move the if ( !cpumask_empty(d->dirty_cpumask) )
check into https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/riscv/sbi.c?ref_type=heads#L178,
we could call only:
  sbi_remote_hfence_gvma_vmid(d->dirty_cpumask, 0, 0, p2m->vmid);
and get the same effect, which might result in cleaner code overall
as we already have a similar check (cpumask == NULL) sbi_rfence_v02() and a result of which
is just to send rfence operation to all CPUs.
> > > > +    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.
>     
Then it is just better to update VMID allocation algo.

> > > > +    }
> > > > +
> > > > +    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?
>     
Oh, got your point. Indeed, there is no sense.

> (Again, this is likely moot since the allocation scheme is likely to change
> altogether.)
>     
Yes, it won't be really needed in the new allocation scheme.

Thanks.

~ Oleksii