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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

[...]

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