[Qemu-devel] Re: [PATCH] pass info about hpets to seabios.

[Gleb Natapov]

On Sun, Jun 13, 2010 at 07:41:14PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Sun, Jun 13, 2010 at 06:56:37PM +0200, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> Currently HPET ACPI table is created regardless of whether qemu actually
> >>> created hpet device. This may confuse some guests that don't check that
> >>> hpet is functional before using it. Solve this by passing info about
> >>> hpets in qemu to seabios via fw config interface. Additional benefit is
> >>> that seabios no longer uses hard coded hpet configuration. Proposed
> >>> interface supports up to 256 hpets. This is the number defined by hpet 
> >>> spec.
> >> Nice, this lays the ground for adding hpets via -device.
> >>
> >> (But I think I read there can only be 8 hpets with a total sum of 256
> >> timers.)
> >>
> > Ah, correct. I thought to myself 256 hpets should be to much :)
> > 
> >>> Signed-off-by: Gleb Natapov <address@hidden>
> >>> diff --git a/hw/hpet.c b/hw/hpet.c
> >>> index 93fc399..f2a4514 100644
> >>> --- a/hw/hpet.c
> >>> +++ b/hw/hpet.c
> >>> @@ -73,6 +73,8 @@ typedef struct HPETState {
> >>>      uint64_t hpet_counter;      /* main counter */
> >>>  } HPETState;
> >>>  
> >>> +struct hpet_fw_config hpet_cfg = {.valid = 1};
> >>> +
> >>>  static uint32_t hpet_in_legacy_mode(HPETState *s)
> >>>  {
> >>>      return s->config & HPET_CFG_LEGACY;
> >>> @@ -661,6 +663,9 @@ static void hpet_reset(DeviceState *d)
> >>>           */
> >>>          hpet_pit_enable();
> >>>      }
> >>> +    hpet_cfg.count = 1;
> >>> +    hpet_cfg.hpet.event_timer_block_id = (uint32_t)s->capability;
> >> The number of timers, thus the content of capability can change on
> >> vmload. So you need to update hpet_cfg there as well.
> >>
> > How it can change? User is required to run the same command line on src
> > and dst, no?
> 
> Generally, yes. But there is no technical need to match the hpet props
> so far, they are included in the vmstate (implicitly).
> 
Assumption that config at src and dst are identical is everywhere in the
code and in the future we should be able to pass actual config as part
of the migration protocol. Better fail migration if we can detect that
configs are different then trying to fix some things, but not others.

> > 
> >> And I think we can move the capability setup into init. But this is not
> >> directly related to this patch, would just avoid adding this hunk to
> >> hpet_reset.
> > I actually did that initially and tried to init hpet_cfg there too, but
> > then noticed that mmio[0].addr below is not initialized at init time yet.
> 
> Indeed.
> 
> You may try sysbus_init_mmio_cb instead for a callback to be invoked
> once the address was set. That handler could also be called on vmload.
> 
sysbus_init_mmio_cb calls callback instead of
cpu_register_physical_memory(). Looks like unneeded complication. Doing
it at reset should be fine as far as I see.

--
                        Gleb.