| Message ID | 20221027164743.194265-5-apatel@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | Nested virtualization fixes for QEMU | expand |
On Fri, Oct 28, 2022 at 2:53 AM Anup Patel <apatel@ventanamicro.com> wrote: > > The time CSR will wrap-around immediately after reaching UINT64_MAX > so we don't need to re-start QEMU timer when timecmp == UINT64_MAX > in riscv_timer_write_timecmp(). I'm not clear what this is fixing? If the guest sets a timer for UINT64_MAX shouldn't that still trigger an event at some point? Alistair > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > --- > target/riscv/time_helper.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c > index 4fb2a471a9..1ee9f94813 100644 > --- a/target/riscv/time_helper.c > +++ b/target/riscv/time_helper.c > @@ -72,6 +72,14 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer *timer, > riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0)); > } > > + /* > + * Don't re-start the QEMU timer when timecmp == UINT64_MAX because > + * time CSR will wrap-around immediately after reaching UINT64_MAX. > + */ > + if (timecmp == UINT64_MAX) { > + return; > + } > + > /* otherwise, set up the future timer interrupt */ > diff = timecmp - rtc_r; > /* back to ns (note args switched in muldiv64) */ > -- > 2.34.1 > >
On Mon, Oct 31, 2022 at 6:25 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Fri, Oct 28, 2022 at 2:53 AM Anup Patel <apatel@ventanamicro.com> wrote: > > > > The time CSR will wrap-around immediately after reaching UINT64_MAX > > so we don't need to re-start QEMU timer when timecmp == UINT64_MAX > > in riscv_timer_write_timecmp(). > > I'm not clear what this is fixing? > > If the guest sets a timer for UINT64_MAX shouldn't that still trigger > an event at some point? Here's what Sstc says about timer interrupt using Sstc: "A supervisor timer interrupt becomes pending - as reflected in the STIP bit in the mip and sip registers - whenever time contains a value greater than or equal to stimecmp, treating the values as unsigned integers. Writes to stimecmp are guaranteed to be reflected in STIP eventually, but not necessarily immediately. The interrupt remains posted until stimecmp becomes greater than time - typically as a result of writing stimecmp." When timecmp = UINT64_MAX, the time CSR will eventually reach timecmp value but on next timer tick the time CSR will wrap-around and become zero which is less than UINT64_MAX. Now, the timer interrupt behaves like a level triggered interrupt so it will become 1 when time = timecmp = UINT64_MAX and next timer tick it will become 0 again because time = 0 < timecmp = UINT64_MAX. This time CSR wrap-around comparison with timecmp is natural to implement in HW but not straight forward in QEMU hence this patch. Software can potentially use timecmp = UINT64_MAX as a way to clear the timer interrupt and keep timer disabled instead of enabling/disabling sie.STIP. This timecmp = UINT64_MAX helps: 1) Linux RISC-V timer driver keep timer interrupt enable/disable state in-sync with Linux interrupt subsystem. 2) Reduce number of traps taken when emulating Sstc for the "Nested Guest" (i.e. Guest running under some "Guest Hypervisor" which in-turn runs under a "Host Hypervisor"). In fact, the SBI set_timer() call also defines similar mechanism to disable timer: "If the supervisor wishes to clear the timer interrupt without scheduling the next timer event, it can either request a timer interrupt infinitely far into the future (i.e., (uint64_t)-1), ...". Regards, Anup > > Alistair > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > --- > > target/riscv/time_helper.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c > > index 4fb2a471a9..1ee9f94813 100644 > > --- a/target/riscv/time_helper.c > > +++ b/target/riscv/time_helper.c > > @@ -72,6 +72,14 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer *timer, > > riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0)); > > } > > > > + /* > > + * Don't re-start the QEMU timer when timecmp == UINT64_MAX because > > + * time CSR will wrap-around immediately after reaching UINT64_MAX. > > + */ > > + if (timecmp == UINT64_MAX) { > > + return; > > + } > > + > > /* otherwise, set up the future timer interrupt */ > > diff = timecmp - rtc_r; > > /* back to ns (note args switched in muldiv64) */ > > -- > > 2.34.1 > > > >
On Mon, Oct 31, 2022 at 1:49 PM Anup Patel <apatel@ventanamicro.com> wrote: > > On Mon, Oct 31, 2022 at 6:25 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > On Fri, Oct 28, 2022 at 2:53 AM Anup Patel <apatel@ventanamicro.com> wrote: > > > > > > The time CSR will wrap-around immediately after reaching UINT64_MAX > > > so we don't need to re-start QEMU timer when timecmp == UINT64_MAX > > > in riscv_timer_write_timecmp(). > > > > I'm not clear what this is fixing? > > > > If the guest sets a timer for UINT64_MAX shouldn't that still trigger > > an event at some point? > > Here's what Sstc says about timer interrupt using Sstc: > "A supervisor timer interrupt becomes pending - as reflected in the > STIP bit in the mip and sip registers - whenever time contains a > value greater than or equal to stimecmp, treating the values as > unsigned integers. Writes to stimecmp are guaranteed to be > reflected in STIP eventually, but not necessarily immediately. > The interrupt remains posted until stimecmp becomes greater > than time - typically as a result of writing stimecmp." > > When timecmp = UINT64_MAX, the time CSR will eventually reach > timecmp value but on next timer tick the time CSR will wrap-around > and become zero which is less than UINT64_MAX. Now, the timer > interrupt behaves like a level triggered interrupt so it will become 1 > when time = timecmp = UINT64_MAX and next timer tick it will > become 0 again because time = 0 < timecmp = UINT64_MAX. Ah, I didn't realise this. Can you add this to the code comment and maybe add this description to the commit message. Otherwise: Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > > This time CSR wrap-around comparison with timecmp is natural > to implement in HW but not straight forward in QEMU hence this > patch. > > Software can potentially use timecmp = UINT64_MAX as a way > to clear the timer interrupt and keep timer disabled instead of > enabling/disabling sie.STIP. This timecmp = UINT64_MAX helps: > 1) Linux RISC-V timer driver keep timer interrupt enable/disable > state in-sync with Linux interrupt subsystem. > 2) Reduce number of traps taken when emulating Sstc for the > "Nested Guest" (i.e. Guest running under some "Guest Hypervisor" > which in-turn runs under a "Host Hypervisor"). > > In fact, the SBI set_timer() call also defines similar mechanism to > disable timer: "If the supervisor wishes to clear the timer interrupt > without scheduling the next timer event, it can either request a timer > interrupt infinitely far into the future (i.e., (uint64_t)-1), ...". > > Regards, > Anup > > > > > Alistair > > > > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > > --- > > > target/riscv/time_helper.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c > > > index 4fb2a471a9..1ee9f94813 100644 > > > --- a/target/riscv/time_helper.c > > > +++ b/target/riscv/time_helper.c > > > @@ -72,6 +72,14 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer *timer, > > > riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0)); > > > } > > > > > > + /* > > > + * Don't re-start the QEMU timer when timecmp == UINT64_MAX because > > > + * time CSR will wrap-around immediately after reaching UINT64_MAX. > > > + */ > > > + if (timecmp == UINT64_MAX) { > > > + return; > > > + } > > > + > > > /* otherwise, set up the future timer interrupt */ > > > diff = timecmp - rtc_r; > > > /* back to ns (note args switched in muldiv64) */ > > > -- > > > 2.34.1 > > > > > >
On Wed, Nov 2, 2022 at 5:40 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Mon, Oct 31, 2022 at 1:49 PM Anup Patel <apatel@ventanamicro.com> wrote: > > > > On Mon, Oct 31, 2022 at 6:25 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > > > On Fri, Oct 28, 2022 at 2:53 AM Anup Patel <apatel@ventanamicro.com> wrote: > > > > > > > > The time CSR will wrap-around immediately after reaching UINT64_MAX > > > > so we don't need to re-start QEMU timer when timecmp == UINT64_MAX > > > > in riscv_timer_write_timecmp(). > > > > > > I'm not clear what this is fixing? > > > > > > If the guest sets a timer for UINT64_MAX shouldn't that still trigger > > > an event at some point? > > > > Here's what Sstc says about timer interrupt using Sstc: > > "A supervisor timer interrupt becomes pending - as reflected in the > > STIP bit in the mip and sip registers - whenever time contains a > > value greater than or equal to stimecmp, treating the values as > > unsigned integers. Writes to stimecmp are guaranteed to be > > reflected in STIP eventually, but not necessarily immediately. > > The interrupt remains posted until stimecmp becomes greater > > than time - typically as a result of writing stimecmp." > > > > When timecmp = UINT64_MAX, the time CSR will eventually reach > > timecmp value but on next timer tick the time CSR will wrap-around > > and become zero which is less than UINT64_MAX. Now, the timer > > interrupt behaves like a level triggered interrupt so it will become 1 > > when time = timecmp = UINT64_MAX and next timer tick it will > > become 0 again because time = 0 < timecmp = UINT64_MAX. > > Ah, I didn't realise this. Can you add this to the code comment and > maybe add this description to the commit message. Otherwise: > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Sure, I will add a detailed comment block in the code itself. Thanks, Anup > > Alistair > > > > > This time CSR wrap-around comparison with timecmp is natural > > to implement in HW but not straight forward in QEMU hence this > > patch. > > > > Software can potentially use timecmp = UINT64_MAX as a way > > to clear the timer interrupt and keep timer disabled instead of > > enabling/disabling sie.STIP. This timecmp = UINT64_MAX helps: > > 1) Linux RISC-V timer driver keep timer interrupt enable/disable > > state in-sync with Linux interrupt subsystem. > > 2) Reduce number of traps taken when emulating Sstc for the > > "Nested Guest" (i.e. Guest running under some "Guest Hypervisor" > > which in-turn runs under a "Host Hypervisor"). > > > > In fact, the SBI set_timer() call also defines similar mechanism to > > disable timer: "If the supervisor wishes to clear the timer interrupt > > without scheduling the next timer event, it can either request a timer > > interrupt infinitely far into the future (i.e., (uint64_t)-1), ...". > > > > Regards, > > Anup > > > > > > > > Alistair > > > > > > > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > > > --- > > > > target/riscv/time_helper.c | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c > > > > index 4fb2a471a9..1ee9f94813 100644 > > > > --- a/target/riscv/time_helper.c > > > > +++ b/target/riscv/time_helper.c > > > > @@ -72,6 +72,14 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer *timer, > > > > riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0)); > > > > } > > > > > > > > + /* > > > > + * Don't re-start the QEMU timer when timecmp == UINT64_MAX because > > > > + * time CSR will wrap-around immediately after reaching UINT64_MAX. > > > > + */ > > > > + if (timecmp == UINT64_MAX) { > > > > + return; > > > > + } > > > > + > > > > /* otherwise, set up the future timer interrupt */ > > > > diff = timecmp - rtc_r; > > > > /* back to ns (note args switched in muldiv64) */ > > > > -- > > > > 2.34.1 > > > > > > > >
diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c index 4fb2a471a9..1ee9f94813 100644 --- a/target/riscv/time_helper.c +++ b/target/riscv/time_helper.c @@ -72,6 +72,14 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer *timer, riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0)); } + /* + * Don't re-start the QEMU timer when timecmp == UINT64_MAX because + * time CSR will wrap-around immediately after reaching UINT64_MAX. + */ + if (timecmp == UINT64_MAX) { + return; + } + /* otherwise, set up the future timer interrupt */ diff = timecmp - rtc_r; /* back to ns (note args switched in muldiv64) */
The time CSR will wrap-around immediately after reaching UINT64_MAX so we don't need to re-start QEMU timer when timecmp == UINT64_MAX in riscv_timer_write_timecmp(). Signed-off-by: Anup Patel <apatel@ventanamicro.com> --- target/riscv/time_helper.c | 8 ++++++++ 1 file changed, 8 insertions(+)