Patchwork [v3,10/14] ich9: APIs for pc guest info

login
register
mail settings
Submitter Michael S. Tsirkin
Date July 24, 2013, 4:02 p.m.
Message ID <1374681580-17439-11-git-send-email-mst@redhat.com>
Download mbox | patch
Permalink /patch/261464/
State New
Headers show

Comments

Michael S. Tsirkin - July 24, 2013, 4:02 p.m.
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 <mst@redhat.com>
---
 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(+)
Gerd Hoffmann - July 25, 2013, 12:33 p.m.
On 07/24/13 18:02, Michael S. Tsirkin wrote:
> This adds APIs that will be used to fill in
> guest info table, implemented using QOM,
> to various ich9 components.

Uses QOM to lookup the device state without needing nasty hooks in
machine creation.  Then goes copy info directly from device state as the
bits needed are not exported as properties.  Looks reasonable to me.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd
Andreas Färber - July 28, 2013, 12:37 a.m.
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 <mst@redhat.com>
> ---
>  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.

> +    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?

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 */
>
Michael S. Tsirkin - July 28, 2013, 7:35 a.m.
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 <mst@redhat.com>
> > ---
> >  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

Patch

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;
+    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 (!o) {
+        return 0;
+    }
+    return MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
+}
+
 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 */