Re: [PATCH v3 11/39] xen/riscv: implement vCPU context switching

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

[...]

>> diff --git a/xen/arch/riscv/include/asm/csr.h b/xen/arch/riscv/include/asm/csr.h
>> index a5cb24c863..aad82c6a6d 100644
>> --- a/xen/arch/riscv/include/asm/csr.h
>> +++ b/xen/arch/riscv/include/asm/csr.h
>> @@ -40,6 +40,13 @@
>>       csr_write(csr ## H, v_ >> 32);  \
>>   })
>>   
>> +#define csr_read64(csr)                 \
>> +({                                      \
>> +    uint64_t v_ = csr_read(csr ## H);   \
> This violates Misra 20.12 rule (docs/misra/rules.rst) as we use macro
> parameter `csr` as an operand of ##, while it could be expanded as a
> macro (e.g. csr_read64(CSR_HENVCFG)).

To avoid MISRA violation, if i understand it correctly, I will do the 
following:

+#define csr_read64(name)                       \
+({                                             \
+    uint64_t v_ = csr_read(CSR_ ## name ## H); \
+                                               \
+    (v_ << 32) | csr_read(CSR_ ## name);       \

and ...

>> +                                        \
>> +    (v_ << 32) | csr_read(csr);         \
>> +})
>> +
>>   /*
>>    * The two halves are read by separate instructions, so a CSR which hardware
>>    * increments can carry from the low half into the high one in between,
>> @@ -72,6 +79,12 @@
>>       (void)csr ## H;                 \
>>       csr_read(csr);                  \
>>   })
>> +
>> +#define csr_read64(csr)             \
>> +({                                  \
>> +    (void)csr ## H;                 \
> Same as above.

... the following here:

+#define csr_read64(name) csr_read(CSR_ ## name)


>> +    csr_read(csr);                  \
>> +})
>>   #endif

[...]

>> diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
>> index de25607247..02e7f6364f 100644
>> --- a/xen/arch/riscv/p2m.c
>> +++ b/xen/arch/riscv/p2m.c
>> @@ -243,6 +243,15 @@ static void p2m_tlb_flush(struct p2m_domain *p2m)
>>   
>>       p2m->need_flush = false;
>>   
>> +    /*
>> +     * Order the p2m updates above against the read of dirty_cpumask below,
>> +     * pairing with the barrier in p2m_vmid_switch_to(). Either that hart is
>> +     * seen here and gets an HFENCE.GVMA, or it adds itself to the mask
>> +     * afterwards, in which case it starts walking this p2m only once the
>> +     * updates are visible to it.
>> +     */
>> +    smp_mb();
>> +
>>       sbi_remote_hfence_gvma(d->dirty_cpumask, 0, 0);
>>   }
>>   
>> @@ -1504,16 +1513,110 @@ void p2m_ctxt_switch_from(struct vcpu *p)
>>        * VMID, world-switch code should zero vsatp, then swap hgatp, then
>>        * finally write the new vsatp value what will be done in
>>        * p2m_ctxt_switch_to().
>> -     * Note, that also HGATP update could happen in p2m_handle_vmenter().
>>        */
>>       p->arch.vsatp = csr_swap(CSR_VSATP, 0);
>>   
>>       /*
>> -     * Nothing to do with HGATP as it will be update in p2m_ctxt_switch_to()
>> -     * or/and in p2m_handle_vmenter().
>> +     * Nothing to do with HGATP as it will be updated in
>> +     * p2m_ctxt_switch_to().
>>        */
>>   }
>>   
>> +/*
>> + * Domain whose p2m this hart's HGATP points at. ctxt_switch_to() bails out
>> + * early for the idle vCPU, so HGATP survives a pass through idle and keeps
>> + * pointing at the domain which ran here last. That domain, rather than the
>> + * one the scheduler switched away from, is what owns this hart's G-stage
>> + * translations. The hart also keeps its place in the owner's dirty_cpumask:
>> + * p2m_tlb_flush() goes on reaching it, and a domain which idles between two
>> + * runs on the same hart keeps its VMIDs.
>> + *
>> + * TODO: nothing resets hgatp_owner when its domain is destroyed, so a hart
>> + *       which stays idle from the domain's last run until it is freed is
>> + *       left with a dangling pointer. Once domain_relinquish_resources()
>> + *       tears down the p2m, it has to reset hgatp_owner of every hart still
>> + *       pointing at the domain (cmpxchg() against d), i.e. before the RCU
>> + *       grace period of domain_destroy() and so before the domain is freed.
>> + */
>> +static DEFINE_PER_CPU(struct domain *, hgatp_owner);
>> +
>> +/*
>> + * Update this hart's place in the dirty_cpumask of the domains involved and
>> + * claim a VMID for n. Must be called before HGATP is pointed at n's p2m, as
>> + * it is written with the VMID claimed here.
>> + */
>> +static void p2m_vmid_switch_to(struct vcpu *n)
>> +{
>> +    unsigned int cpu = smp_processor_id();
>> +    struct domain *owner = this_cpu(hgatp_owner);
>> +    bool need_flush;
>> +
>> +    ASSERT(!is_idle_vcpu(n));
>> +
>> +    if ( owner != n->domain )
>> +    {
>> +        /*
>> +         * Once this hart drops out of the owner's dirty_cpumask it stops
>> +         * being a target of p2m_tlb_flush(), while its TLB may still hold
>> +         * G-stage translations of that domain: none of the vCPUs of that
>> +         * domain which ran here has had its VMID invalidated. Move the hart
>> +         * to a new VMID generation so that none of them can be reached
>> +         * again.
>> +         *
>> +         * This has to precede the VMID claim below, so that n gets a VMID of
>> +         * the new generation.
>> +         */
>> +        if ( owner )
>> +        {
>> +            vmid_flush_hart();
>> +
>> +            cpumask_clear_cpu(cpu, owner->dirty_cpumask);
>> +        }
>> +
>> +        /*
>> +         * Mark this hart in the incoming domain's dirty_cpumask before HGATP
>> +         * is pointed at its p2m: from that write on the hart may cache the
>> +         * p2m's translations, so p2m_tlb_flush() must already reach it.
>> +         */
>> +        cpumask_set_cpu(cpu, n->domain->dirty_cpumask);
>> +
>> +        /*
>> +         * Pairs with the barrier in p2m_tlb_flush(). cpumask_set_cpu() is an
>> +         * unordered AMO on RISC-V, so without this a concurrent flusher could
>> +         * read the mask without this hart in it while this hart is already
>> +         * walking the p2m it is about to be pointed at.
>> +         */
>> +        smp_mb();
>> +
>> +        this_cpu(hgatp_owner) = n->domain;
>> +    }
>> +
>> +    /*
>> +     * A VMID is meaningful only on the hart whose pool issued it: generations
>> +     * are per-hart counters which all start at 1 and advance independently,
>> +     * so the pair a vCPU brings from another hart may match this hart's
>> +     * generation by coincidence, leaving the vCPU under a VMID which is live
>> +     * here for someone else.
>> +     */
> So if I understand correctly, we are setting generation to 0 to mark the
> vCPU that it is coming from other hart right?

Yes, at least that is the case this code deals with. Strictly speaking,
generation 0 just invalidates the vCPU's VMID, so that a new one is
claimed on the next VM entry; vmid_flush_vcpu() could be used wherever
that is needed, coming from another hart being one such case.

> Nit: If it's the case I may simplify the comment to:
>        /*
>         * VMIDs and generations are per hart, so a pair issued on another hart
>         * may match this hart's generation by coincidence. Reset the generation
>         * to 0, which no hart ever uses, so that a fresh VMID is claimed from
>         * this hart's pool.
>         */
> 

Looks good to me, I'll take it.


>> +    if ( n->arch.last_cpu != cpu )
>> +        vmid_flush_vcpu(n);
>>
> Nit: I had to read this a couple of times to get why the guest entry is too
> late. "makes HGATP live" doesn't say what happens once it is: if it's
> because of speculation filling the TLB with G-stage entries tagged with
> the VMID in HGATP, like you explained in the commit message then it may

I thought that having "HGATP live" would be clear.

> worth spell out:
> 
>        /*
>         * Claim the VMID before the caller writes HGATP rather than on guest
>         * entry: from that write on, speculation may fill the TLB with this
>         * domain's G-stage entries tagged with that VMID. A stale VMID may
>         * already have been re-issued to a vCPU of another domain, which would
>         * then hit these entries.
>         */
> 

Indeed, that's clearer. I'll take your wording.

Thanks!

~ Oleksii