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

Conor Dooley
2023-04-18T19:02:24Z

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

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/?)

Guess both of my comments are in the same vein, but would make
at-a-glance understanding easier IMO.

Cheers,
Conor.

> +
> + return steal;
> }
>
> int __init pv_time_init(void)
> --
> 2.39.2
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

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