Re: [PATCH 3/3] mark HPET as unlocked

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 20 Jun 2025 18:01:08 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Fri, 20 Jun 2025 at 16:15, Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > Reading QEMU_CLOCK_VIRTUAL is thread-safe.
> >
> > with CLI
> >  -M q35,hpet=on -cpu host -enable-kvm -smp 240,sockets=4
> > patch makes WS2025 guest, that was able to boot in 4/2min, to boot in 2/1min.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/timer/hpet.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> > index 0fd1337a15..1ebd715529 100644
> > --- a/hw/timer/hpet.c
> > +++ b/hw/timer/hpet.c
> > @@ -681,6 +681,8 @@ static void hpet_init(Object *obj)
> >
> >      /* HPET Area */
> >      memory_region_init_io(&s->iomem, obj, &hpet_ram_ops, s, "hpet", HPET_LEN);
> > +    memory_region_enable_lockless_ro_io(&s->iomem);
> > +    s->iomem.disable_reentrancy_guard = true;
> >      sysbus_init_mmio(sbd, &s->iomem);  
> 
> Is this sequence possible?
While unlikely (what I observe from Linux/Windows guest,
they enable timer 1st and only then start readers),
but yes it still possible.

The more convoluted is the switch into disabled state, where
1:
  reader is already in enabled branch and preempted before
  reading clock:  
          case HPET_COUNTER:
            if (hpet_enabled(s)) {
               <yield>
               cur_tick = hpet_get_ticks(s);
2:
  writer saves s->hpet_counter = <now>

3:
  reader resumes and reads newer 'now'

4:
  on next counter read, reader switches to disabled branch
  and sees timer jump back to older s->hpet_counter value.
  and that shouldn't happen.

for this to work s->hpet_counter needs to catch up
the latest read timer value.

Let me try and see what could be done with it.

> 
> thread A
>    takes the BQL
>    enters hpet_ram_write() for HPET_CFG to set ENABLE
>    executes s->config = new_val (setting the ENABLE bit in it)
>    context switch before it gets to the point of setting
>      s->hpet_offset
> 
> thread B
>    enters hpet_ram_read() for HPET_COUNTER (which it can now
>     do because it doesn't need the BQL)
>    hpet_enabled() returns true (it tests s->config)
>    calls hpet_get_ticks which adds hpet_offset to the clock,
>      but hpet_offset has not been correctly set by A yet
> 
> thanks
> -- PMM
>