Re: [Qemu-devel] [PATCH v3 10/14] ich9: APIs for pc guest info

[Michael S. Tsirkin]

On Sun, Jul 28, 2013 at 02:37:59AM +0200, Andreas Färber wrote:
> Am 24.07.2013 18:02, schrieb Michael S. Tsirkin:
> > This adds APIs that will be used to fill in
> > guest info table, implemented using QOM,
> > to various ich9 components.
> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> >  hw/acpi/ich9.c            |  6 ++++++
> >  hw/isa/lpc_ich9.c         | 19 +++++++++++++++++++
> >  hw/pci-host/q35.c         | 10 ++++++++++
> >  include/hw/acpi/ich9.h    |  2 ++
> >  include/hw/i386/ich9.h    |  3 +++
> >  include/hw/pci-host/q35.h |  2 ++
> >  6 files changed, 42 insertions(+)
> > 
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 3fb443d..7ea55e1 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -228,3 +228,9 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> >      pm->powerdown_notifier.notify = pm_powerdown_req;
> >      qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> >  }
> > +
> > +void ich9_pm_get_acpi_pm_info(ICH9LPCPMRegs *pm, AcpiPmInfo *info)
> > +{
> > +    info->gpe0_blk = PC_GUEST_PORT_ACPI_PM_BASE + ICH9_PMIO_GPE0_STS;
> > +    info->gpe0_blk_len = ICH9_PMIO_GPE0_LEN;
> > +}
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index d1921aa..12d4a23 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -562,6 +562,25 @@ static bool ich9_rst_cnt_needed(void *opaque)
> >      return (lpc->rst_cnt != 0);
> >  }
> >  
> > +ICH9LPCState *ich9_lpc_find(void)
> > +{
> > +    bool ambig;
> > +    Object *o = object_resolve_path_type("", TYPE_ICH9_LPC_DEVICE, &ambig);
> > +
> > +    if (ambig) {
> > +        return NULL;
> > +    }
> > +    return ICH9_LPC_DEVICE(o);
> > +}
> > +
> > +void ich9_lpc_get_acpi_pm_info(ICH9LPCState *lpc, AcpiPmInfo *info)
> > +{
> > +    info->sci_int = 9;
> 
> Magic value.
> 
> > +    info->acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
> > +    info->acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
> > +    ich9_pm_get_acpi_pm_info(&lpc->pm, info);
> > +}
> > +
> >  static const VMStateDescription vmstate_ich9_rst_cnt = {
> >      .name = "ICH9LPC/rst_cnt",
> >      .version_id = 1,
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index 6b1b3b7..ca6f495 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -298,6 +298,16 @@ static int mch_init(PCIDevice *d)
> >      return 0;
> >  }
> >  
> > +uint64_t mch_mcfg_base(void)
> > +{
> > +    bool ambiguous;
> > +    Object *o = object_resolve_path_type("", TYPE_MCH_PCI_DEVICE, 
> > &ambiguous);
> 
> If you take the minute to add in q35 machine init
> object_property_add_child(qdev_get_machine(), "q35", foo, NULL);
> then not only can you use object_resolve_path("/machine/q35/mch") to
> access it but everyone including qtest can.

That's fine but I'd like to avoid copying string literals around.

> > +    if (!o) {
> > +        return 0;
> > +    }
> > +    return MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
> 
> Another value constant for the device. Taking a wild guess: Is this an
> offset of a memory subregion where long-term Anthony's proposed
> MemoryRegion QOM'ification would allow us to get rid of this helper?

I'm not sure that will be possible because the offset it
programmable by guest. At the moment all guests are required to
program a hard-coded offset, but I think what we'll have long term,
is a guest interface that let us control what guest programs.

> Anyway, I would like it much better as:
> 
> uint64_t mch_mcfg_base()
> {
>     return MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
> }
> 
> with the path resolution stuff, i.e. check for necessity, happening in
> ACPI code, if we don't get around these helpers for now.
> 
> Andreas
> 
> > +}
> > +
> >  static void mch_class_init(ObjectClass *klass, void *data)
> >  {
> >      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> > index b1fe71f..f5e8a88 100644
> > --- a/include/hw/acpi/ich9.h
> > +++ b/include/hw/acpi/ich9.h
> > @@ -49,4 +49,6 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> >  void ich9_pm_iospace_update(ICH9LPCPMRegs *pm, uint32_t pm_io_base);
> >  extern const VMStateDescription vmstate_ich9_pm;
> >  
> > +void ich9_pm_get_acpi_pm_info(ICH9LPCPMRegs *, AcpiPmInfo *);
> > +
> >  #endif /* HW_ACPI_ICH9_H */
> > diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> > index c5f637b..6528dc0 100644
> > --- a/include/hw/i386/ich9.h
> > +++ b/include/hw/i386/ich9.h
> > @@ -66,6 +66,9 @@ typedef struct ICH9LPCState {
> >      qemu_irq *ioapic;
> >  } ICH9LPCState;
> >  
> > +ICH9LPCState *ich9_lpc_find(void);
> > +void ich9_lpc_get_acpi_pm_info(ICH9LPCState *, AcpiPmInfo *);
> > +
> >  #define Q35_MASK(bit, ms_bit, ls_bit) \
> >  ((uint##bit##_t)(((1ULL << ((ms_bit) + 1)) - 1) & ~((1ULL << ls_bit) - 1)))
> >  
> > diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> > index 3cb631e..6337dcf 100644
> > --- a/include/hw/pci-host/q35.h
> > +++ b/include/hw/pci-host/q35.h
> > @@ -154,4 +154,6 @@ typedef struct Q35PCIHost {
> >  #define MCH_PCIE_DEV                           1
> >  #define MCH_PCIE_FUNC                          0
> >  
> > +uint64_t mch_mcfg_base(void);
> > +
> >  #endif /* HW_Q35_H */
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg