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: 8259 bytes --]


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 needdata->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); 
elsesbi_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

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