Re: [RFC PATCH 03/14] RISC-V: paravirt: Implement steal-time support

Conor Dooley
2023-04-19T16:42:13Z

[-- Attachment #1.1: Type: text/plain, Size: 2733 bytes --]

On Wed, Apr 19, 2023 at 10:24:27AM +0200, Andrew Jones wrote:
> On Tue, Apr 18, 2023 at 08:02:06PM +0100, Conor Dooley wrote:
> > On Mon, Apr 17, 2023 at 12:33:51PM +0200, Andrew Jones wrote:
> >
> > > +static int pv_time_cpu_online(unsigned int cpu)
> > > +{
> > > + struct sbi_sta_struct *st = this_cpu_ptr(&steal_time);
> > > + phys_addr_t pa = __pa(st);
> > > + unsigned long lo = (unsigned long)pa;
> > > + unsigned long hi = IS_ENABLED(CONFIG_32BIT) ? upper_32_bits((u64)pa) : 0;
> > > +
> > > + return sbi_sta_set_steal_time_shmem(lo, hi, 0);
> > > +}
> > > +
> > > static int pv_time_cpu_down_prepare(unsigned int cpu)
> > > {
> > > - return 0;
> > > + return sbi_sta_set_steal_time_shmem(-1, -1, 0);
> >
> > I'm not really a fan of this -1s without an explanation of what passing
> > -1 to the ecall does.
> >
> > > }
> > >
> > > static u64 pv_time_steal_clock(int cpu)
> > > {
> > > - return 0;
> > > + struct sbi_sta_struct *st = per_cpu_ptr(&steal_time, cpu);
> > > + u32 sequence;
> > > + u64 steal;
> > > +
> > > + do {
> > > + sequence = st->sequence;
> > > + virt_rmb();
> > > + steal = st->steal;
> > > + virt_rmb();
> > > + } while ((sequence & 1) || (sequence != st->sequence));
> >
> > Call me a bit anal, but should we yoink your:
> > | The supervisor-mode software MUST check this field
> > | before and after reading the `steal` field, and
> > | repeat the read if they are different or odd
> > and add it here so that is it immediately obvious without reading the
> > SBI spec what is going on here? Or it is a case of "go read the SBI spec
> > if you have questions about what the kernel is doing w/ SBI stuff?
> >
> > (sidenote, s/they are/it is/?)
>
> Thanks, I updated the spec for its v3 posting. Before posting v3,
> I'd be happy to take more comments on it over on its v2 posting.

I did read it last week or whenever you sent it, but didn't feel
qualified to give an opinion on the content nor did I notice that
grammaro.

I'd have replied there, but I was too lazy to dump your mail from there
into a format readable by a decent mail client. I should just sign up
there with this email address instead.

> > Guess both of my comments are in the same vein, but would make
> > at-a-glance understanding easier IMO.
>
> I'm inclined to say "go read the spec" for everything, since the KVM
> implementation would otherwise also require several comments which
> duplicate the spec, but I'm also OK with adding the comments you've
> suggested here. I'll do that for rfc-v2.

I don't really have strong feelings, the magic -1s can be explained away
with a define so they're sortable without cogging stuff from the spec.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]