diff mbox

[v3,11/14] piix: APIs for pc guest info

Message ID 20130725093248.GA27524@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin July 25, 2013, 9:32 a.m. UTC 
On Wed, Jul 24, 2013 at 07:02:13PM +0300, Michael S. Tsirkin wrote:
> This adds APIs that will be used to fill in guest info table,
> implemented using QOM, to various piix components.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/acpi/piix4.c         | 29 +++++++++++++++++++++++++++--
>  hw/mips/mips_malta.c    |  2 +-
>  hw/pci-host/piix.c      |  8 ++++++++
>  include/hw/i386/pc.h    |  1 +
>  include/qemu/typedefs.h |  1 +
>  5 files changed, 38 insertions(+), 3 deletions(-)

Ouch, forgot to git add a file, so it's missing.
include/hw/acpi/piix4.h below.
Corrected patch with this chunk added below.
It's trivial so not reposting.

--->

Subject: piix: APIs for pc guest info

This adds APIs that will be used to fill in guest info table,
implemented using QOM, to various piix components.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Comments

Andreas Färber July 28, 2013, 12:12 a.m. UTC | #1 
Am 25.07.2013 11:32, schrieb Michael S. Tsirkin:
> This adds APIs that will be used to fill in guest info table,
> implemented using QOM, to various piix components.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index c885690..2128f13 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -29,6 +29,7 @@
>  #include "exec/ioport.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "exec/address-spaces.h"
> +#include "hw/acpi/piix4.h"
>  
>  //#define DEBUG
>  
> @@ -63,7 +64,7 @@ typedef struct CPUStatus {
>      uint8_t sts[PIIX4_PROC_LEN];
>  } CPUStatus;
>  
> -typedef struct PIIX4PMState {
> +struct PIIX4PMState {
>      /*< private >*/
>      PCIDevice parent_obj;
>      /*< public >*/
> @@ -96,7 +97,7 @@ typedef struct PIIX4PMState {
>  
>      CPUStatus gpe_cpu;
>      Notifier cpu_added_notifier;
> -} PIIX4PMState;
> +};
>  
>  #define TYPE_PIIX4_PM "PIIX4_PM"
>  

Here add

#define PIIX4_PM(obj) OBJECT_CHECK(PIIX4PMState, obj, TYPE_PIIX4_PM)

for the general public ...

> @@ -458,6 +459,30 @@ static int piix4_pm_initfn(PCIDevice *dev)
>      return 0;
>  }
>  
> +PIIX4PMState *piix4_pm_find(void)
> +{
> +    bool ambig;
> +    Object *o = object_resolve_path_type("", "PIIX4_PM", &ambig);
> +
> +    if (ambig || !o) {
> +        return NULL;
> +    }
> +    return OBJECT_CHECK(PIIX4PMState, o, "PIIX4_PM");

... instead of open-coding it where only you use it, please.

> +}
> +
> +void piix4_pm_get_acpi_pm_info(PIIX4PMState *s, AcpiPmInfo *info)
> +{
> +        info->s3_disabled = s->disable_s3;
> +        info->s4_disabled = s->disable_s4;
> +        info->s4_val = s->s4_val;

These three are accessible as qdev/QOM properties.

> +
> +        info->acpi_enable_cmd = ACPI_ENABLE;
> +        info->acpi_disable_cmd = ACPI_DISABLE;
> +        info->gpe0_blk = GPE_BASE;
> +        info->gpe0_blk_len = GPE_LEN;

So here the issue is that values are constant?

> +        info->sci_int = 9;

Magic number - use a define to share it with wherever it is used?

> +}
> +
>  i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>                         qemu_irq sci_irq, qemu_irq smi_irq,
>                         int kvm_enabled, FWCfgState *fw_cfg)
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index de87241..14573ab 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -965,7 +965,7 @@ void mips_malta_init(QEMUMachineInitArgs *args)
>      pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
>      pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
>      smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
> -                          isa_get_irq(NULL, 9), NULL, 0, NULL);
> +                          isa_get_irq(NULL, 9), NULL, 0, NULL, NULL);

This looks fishy. Might belong into a different patch? Implementation
didn't change above.

>      /* TODO: Populate SPD eeprom data.  */
>      smbus_eeprom_init(smbus, 8, NULL, 0);
>      pit = pit_init(isa_bus, 0x40, 0, NULL);
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 3908860..daefdfb 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -349,6 +349,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
>      return b;
>  }
>  
> +PCIBus *find_i440fx(void)
> +{
> +    PCIHostState *s = OBJECT_CHECK(PCIHostState,
> +                                   object_resolve_path("/machine/i440fx", NULL),
> +                                   TYPE_PCI_HOST_BRIDGE);

Open-coded OBJECT_CHECK() - should use existing PCI_HOST_BRIDGE(...).

> +    return s ? s->bus : NULL;
> +}

Is this function really necessary? /machine/i440fx/pci.0 is a trivial
addition to the path that's already being used here. You can do:
PCIBus *bus = PCI_BUS(object_resolve_path("/machine/i440fx/pci.0"));
where you actually need to access it.

Andreas

> +
>  /* PIIX3 PCI to ISA bridge */
>  static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
>  {
> diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
> new file mode 100644
> index 0000000..2876428
> --- /dev/null
> +++ b/include/hw/acpi/piix4.h
> @@ -0,0 +1,10 @@
> +#ifndef HW_ACPI_PIIX4_H
> +#define HW_ACPI_PIIX4_H
> +
> +#include "qemu/typedefs.h"
> +
> +PIIX4PMState *piix4_pm_find(void);
> +
> +void piix4_pm_get_acpi_pm_info(PIIX4PMState *, AcpiPmInfo *);
> +
> +#endif
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 7c0bd50..76af5cd 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -186,6 +186,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
>                      MemoryRegion *pci_memory,
>                      MemoryRegion *ram_memory);
>  
> +PCIBus *find_i440fx(void);
>  /* piix4.c */
>  extern PCIDevice *piix4_dev;
>  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index cb66e19..7d42693 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -65,6 +65,7 @@ typedef struct QEMUSGList QEMUSGList;
>  typedef struct SHPCDevice SHPCDevice;
>  typedef struct FWCfgState FWCfgState;
>  typedef struct PcGuestInfo PcGuestInfo;
> +typedef struct PIIX4PMState PIIX4PMState;
>  typedef struct AcpiPmInfo AcpiPmInfo;
>  
>  #endif /* QEMU_TYPEDEFS_H */
>
Michael S. Tsirkin July 28, 2013, 7:30 a.m. UTC | #2 
On Sun, Jul 28, 2013 at 02:12:49AM +0200, Andreas Färber wrote:
> Am 25.07.2013 11:32, schrieb Michael S. Tsirkin:
> > This adds APIs that will be used to fill in guest info table,
> > implemented using QOM, to various piix components.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index c885690..2128f13 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -29,6 +29,7 @@
> >  #include "exec/ioport.h"
> >  #include "hw/nvram/fw_cfg.h"
> >  #include "exec/address-spaces.h"
> > +#include "hw/acpi/piix4.h"
> >  
> >  //#define DEBUG
> >  
> > @@ -63,7 +64,7 @@ typedef struct CPUStatus {
> >      uint8_t sts[PIIX4_PROC_LEN];
> >  } CPUStatus;
> >  
> > -typedef struct PIIX4PMState {
> > +struct PIIX4PMState {
> >      /*< private >*/
> >      PCIDevice parent_obj;
> >      /*< public >*/
> > @@ -96,7 +97,7 @@ typedef struct PIIX4PMState {
> >  
> >      CPUStatus gpe_cpu;
> >      Notifier cpu_added_notifier;
> > -} PIIX4PMState;
> > +};
> >  
> >  #define TYPE_PIIX4_PM "PIIX4_PM"
> >  
> 
> Here add
> 
> #define PIIX4_PM(obj) OBJECT_CHECK(PIIX4PMState, obj, TYPE_PIIX4_PM)
> 
> for the general public ...
> 
> > @@ -458,6 +459,30 @@ static int piix4_pm_initfn(PCIDevice *dev)
> >      return 0;
> >  }
> >  
> > +PIIX4PMState *piix4_pm_find(void)
> > +{
> > +    bool ambig;
> > +    Object *o = object_resolve_path_type("", "PIIX4_PM", &ambig);
> > +
> > +    if (ambig || !o) {
> > +        return NULL;
> > +    }
> > +    return OBJECT_CHECK(PIIX4PMState, o, "PIIX4_PM");
> 
> ... instead of open-coding it where only you use it, please.

I don't really understand in which direction you want to take this.
On the one hand, you are saying "there's one caller of
this function so please open-code it".
On the other hand, you are saying "it does not matter that
there's one user of this macro put it in the header".

Is the point that macros are somehow better than functions so they
should be used where possible?


> > +}
> > +
> > +void piix4_pm_get_acpi_pm_info(PIIX4PMState *s, AcpiPmInfo *info)
> > +{
> > +        info->s3_disabled = s->disable_s3;
> > +        info->s4_disabled = s->disable_s4;
> > +        info->s4_val = s->s4_val;
> 
> These three are accessible as qdev/QOM properties.
> 
> > +
> > +        info->acpi_enable_cmd = ACPI_ENABLE;
> > +        info->acpi_disable_cmd = ACPI_DISABLE;
> > +        info->gpe0_blk = GPE_BASE;
> > +        info->gpe0_blk_len = GPE_LEN;
> 
> So here the issue is that values are constant?

Yes.

> > +        info->sci_int = 9;
> 
> Magic number - use a define to share it with wherever it is used?
> 
> > +}
> > +
> >  i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> >                         qemu_irq sci_irq, qemu_irq smi_irq,
> >                         int kvm_enabled, FWCfgState *fw_cfg)
> > diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> > index de87241..14573ab 100644
> > --- a/hw/mips/mips_malta.c
> > +++ b/hw/mips/mips_malta.c
> > @@ -965,7 +965,7 @@ void mips_malta_init(QEMUMachineInitArgs *args)
> >      pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
> >      pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
> >      smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
> > -                          isa_get_irq(NULL, 9), NULL, 0, NULL);
> > +                          isa_get_irq(NULL, 9), NULL, 0, NULL, NULL);
> 
> This looks fishy. Might belong into a different patch? Implementation
> didn't change above.
> 
> >      /* TODO: Populate SPD eeprom data.  */
> >      smbus_eeprom_init(smbus, 8, NULL, 0);
> >      pit = pit_init(isa_bus, 0x40, 0, NULL);
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index 3908860..daefdfb 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -349,6 +349,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
> >      return b;
> >  }
> >  
> > +PCIBus *find_i440fx(void)
> > +{
> > +    PCIHostState *s = OBJECT_CHECK(PCIHostState,
> > +                                   object_resolve_path("/machine/i440fx", NULL),
> > +                                   TYPE_PCI_HOST_BRIDGE);
> 
> Open-coded OBJECT_CHECK() - should use existing PCI_HOST_BRIDGE(...).
> 
> > +    return s ? s->bus : NULL;
> > +}
> 
> Is this function really necessary? /machine/i440fx/pci.0 is a trivial
> addition to the path that's already being used here. You can do:
> PCIBus *bus = PCI_BUS(object_resolve_path("/machine/i440fx/pci.0"));
> where you actually need to access it.
> 
> Andreas


I don't mind but I would like to avoid callers hard-coding
paths, in this case "i440fx".
Why the aversion to functions?

> > +
> >  /* PIIX3 PCI to ISA bridge */
> >  static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
> >  {
> > diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
> > new file mode 100644
> > index 0000000..2876428
> > --- /dev/null
> > +++ b/include/hw/acpi/piix4.h
> > @@ -0,0 +1,10 @@
> > +#ifndef HW_ACPI_PIIX4_H
> > +#define HW_ACPI_PIIX4_H
> > +
> > +#include "qemu/typedefs.h"
> > +
> > +PIIX4PMState *piix4_pm_find(void);
> > +
> > +void piix4_pm_get_acpi_pm_info(PIIX4PMState *, AcpiPmInfo *);
> > +
> > +#endif
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 7c0bd50..76af5cd 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -186,6 +186,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> >                      MemoryRegion *pci_memory,
> >                      MemoryRegion *ram_memory);
> >  
> > +PCIBus *find_i440fx(void);
> >  /* piix4.c */
> >  extern PCIDevice *piix4_dev;
> >  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
> > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > index cb66e19..7d42693 100644
> > --- a/include/qemu/typedefs.h
> > +++ b/include/qemu/typedefs.h
> > @@ -65,6 +65,7 @@ typedef struct QEMUSGList QEMUSGList;
> >  typedef struct SHPCDevice SHPCDevice;
> >  typedef struct FWCfgState FWCfgState;
> >  typedef struct PcGuestInfo PcGuestInfo;
> > +typedef struct PIIX4PMState PIIX4PMState;
> >  typedef struct AcpiPmInfo AcpiPmInfo;
> >  
> >  #endif /* QEMU_TYPEDEFS_H */
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber July 28, 2013, 9:38 a.m. UTC | #3 
Am 28.07.2013 09:30, schrieb Michael S. Tsirkin:
> On Sun, Jul 28, 2013 at 02:12:49AM +0200, Andreas Färber wrote:
>> Am 25.07.2013 11:32, schrieb Michael S. Tsirkin:
>>> This adds APIs that will be used to fill in guest info table,
>>> implemented using QOM, to various piix components.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>
>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>>> index c885690..2128f13 100644
>>> --- a/hw/acpi/piix4.c
>>> +++ b/hw/acpi/piix4.c
>>> @@ -29,6 +29,7 @@
>>>  #include "exec/ioport.h"
>>>  #include "hw/nvram/fw_cfg.h"
>>>  #include "exec/address-spaces.h"
>>> +#include "hw/acpi/piix4.h"
>>>  
>>>  //#define DEBUG
>>>  
>>> @@ -63,7 +64,7 @@ typedef struct CPUStatus {
>>>      uint8_t sts[PIIX4_PROC_LEN];
>>>  } CPUStatus;
>>>  
>>> -typedef struct PIIX4PMState {
>>> +struct PIIX4PMState {
>>>      /*< private >*/
>>>      PCIDevice parent_obj;
>>>      /*< public >*/
>>> @@ -96,7 +97,7 @@ typedef struct PIIX4PMState {
>>>  
>>>      CPUStatus gpe_cpu;
>>>      Notifier cpu_added_notifier;
>>> -} PIIX4PMState;
>>> +};
>>>  
>>>  #define TYPE_PIIX4_PM "PIIX4_PM"
>>>  
>>
>> Here add
>>
>> #define PIIX4_PM(obj) OBJECT_CHECK(PIIX4PMState, obj, TYPE_PIIX4_PM)
>>
>> for the general public ...
>>
>>> @@ -458,6 +459,30 @@ static int piix4_pm_initfn(PCIDevice *dev)
>>>      return 0;
>>>  }
>>>  
>>> +PIIX4PMState *piix4_pm_find(void)
>>> +{
>>> +    bool ambig;
>>> +    Object *o = object_resolve_path_type("", "PIIX4_PM", &ambig);
>>> +
>>> +    if (ambig || !o) {
>>> +        return NULL;
>>> +    }
>>> +    return OBJECT_CHECK(PIIX4PMState, o, "PIIX4_PM");
>>
>> ... instead of open-coding it where only you use it, please.
> 
> I don't really understand in which direction you want to take this.
> On the one hand, you are saying "there's one caller of
> this function so please open-code it".
> On the other hand, you are saying "it does not matter that
> there's one user of this macro put it in the header".
> 
> Is the point that macros are somehow better than functions so they
> should be used where possible?

No, the point is reuse: For QOM realize we will very likely need the
same thing. It was not my decision to make these macros, I'm just making
Anthony's QOM pattern consistent.

>>> +}
>>> +
>>> +void piix4_pm_get_acpi_pm_info(PIIX4PMState *s, AcpiPmInfo *info)
>>> +{
>>> +        info->s3_disabled = s->disable_s3;
>>> +        info->s4_disabled = s->disable_s4;
>>> +        info->s4_val = s->s4_val;
>>
>> These three are accessible as qdev/QOM properties.
>>
>>> +
>>> +        info->acpi_enable_cmd = ACPI_ENABLE;
>>> +        info->acpi_disable_cmd = ACPI_DISABLE;
>>> +        info->gpe0_blk = GPE_BASE;
>>> +        info->gpe0_blk_len = GPE_LEN;
>>
>> So here the issue is that values are constant?
> 
> Yes.
> 
>>> +        info->sci_int = 9;
>>
>> Magic number - use a define to share it with wherever it is used?
>>
>>> +}
>>> +
>>>  i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>>>                         qemu_irq sci_irq, qemu_irq smi_irq,
>>>                         int kvm_enabled, FWCfgState *fw_cfg)
>>> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
>>> index de87241..14573ab 100644
>>> --- a/hw/mips/mips_malta.c
>>> +++ b/hw/mips/mips_malta.c
>>> @@ -965,7 +965,7 @@ void mips_malta_init(QEMUMachineInitArgs *args)
>>>      pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
>>>      pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
>>>      smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
>>> -                          isa_get_irq(NULL, 9), NULL, 0, NULL);
>>> +                          isa_get_irq(NULL, 9), NULL, 0, NULL, NULL);
>>
>> This looks fishy. Might belong into a different patch? Implementation
>> didn't change above.
>>
>>>      /* TODO: Populate SPD eeprom data.  */
>>>      smbus_eeprom_init(smbus, 8, NULL, 0);
>>>      pit = pit_init(isa_bus, 0x40, 0, NULL);
>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>>> index 3908860..daefdfb 100644
>>> --- a/hw/pci-host/piix.c
>>> +++ b/hw/pci-host/piix.c
>>> @@ -349,6 +349,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
>>>      return b;
>>>  }
>>>  
>>> +PCIBus *find_i440fx(void)
>>> +{
>>> +    PCIHostState *s = OBJECT_CHECK(PCIHostState,
>>> +                                   object_resolve_path("/machine/i440fx", NULL),
>>> +                                   TYPE_PCI_HOST_BRIDGE);
>>
>> Open-coded OBJECT_CHECK() - should use existing PCI_HOST_BRIDGE(...).
>>
>>> +    return s ? s->bus : NULL;
>>> +}
>>
>> Is this function really necessary? /machine/i440fx/pci.0 is a trivial
>> addition to the path that's already being used here. You can do:
>> PCIBus *bus = PCI_BUS(object_resolve_path("/machine/i440fx/pci.0"));
>> where you actually need to access it.
> 
> 
> I don't mind but I would like to avoid callers hard-coding
> paths, in this case "i440fx".
> Why the aversion to functions?

Simply because QMP cannot call functions. It has to work with qom-list
and qom-get, so this is a test case showing what is missing and can IMO
easily be addressed for both parties.

The suggested cast to PCI_BUS() lets you use regular qdev functions btw
as a shortcut, QMP users would need to iterate children of that node.

The suggested "pci.0" is considered stable for -device ...,bus=pci.0
according to review feedback the Xen people have received for libxl.

Andreas

> 
>>> +
>>>  /* PIIX3 PCI to ISA bridge */
>>>  static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
>>>  {
>>> diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
>>> new file mode 100644
>>> index 0000000..2876428
>>> --- /dev/null
>>> +++ b/include/hw/acpi/piix4.h
>>> @@ -0,0 +1,10 @@
>>> +#ifndef HW_ACPI_PIIX4_H
>>> +#define HW_ACPI_PIIX4_H
>>> +
>>> +#include "qemu/typedefs.h"
>>> +
>>> +PIIX4PMState *piix4_pm_find(void);
>>> +
>>> +void piix4_pm_get_acpi_pm_info(PIIX4PMState *, AcpiPmInfo *);
>>> +
>>> +#endif
>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>> index 7c0bd50..76af5cd 100644
>>> --- a/include/hw/i386/pc.h
>>> +++ b/include/hw/i386/pc.h
>>> @@ -186,6 +186,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
>>>                      MemoryRegion *pci_memory,
>>>                      MemoryRegion *ram_memory);
>>>  
>>> +PCIBus *find_i440fx(void);
>>>  /* piix4.c */
>>>  extern PCIDevice *piix4_dev;
>>>  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
>>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
>>> index cb66e19..7d42693 100644
>>> --- a/include/qemu/typedefs.h
>>> +++ b/include/qemu/typedefs.h
>>> @@ -65,6 +65,7 @@ typedef struct QEMUSGList QEMUSGList;
>>>  typedef struct SHPCDevice SHPCDevice;
>>>  typedef struct FWCfgState FWCfgState;
>>>  typedef struct PcGuestInfo PcGuestInfo;
>>> +typedef struct PIIX4PMState PIIX4PMState;
>>>  typedef struct AcpiPmInfo AcpiPmInfo;
>>>  
>>>  #endif /* QEMU_TYPEDEFS_H */
>>>
>>
>>
>> -- 
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Michael S. Tsirkin July 28, 2013, 10:14 a.m. UTC | #4 
On Sun, Jul 28, 2013 at 11:38:17AM +0200, Andreas Färber wrote:
> Am 28.07.2013 09:30, schrieb Michael S. Tsirkin:
> > On Sun, Jul 28, 2013 at 02:12:49AM +0200, Andreas Färber wrote:
> >> Am 25.07.2013 11:32, schrieb Michael S. Tsirkin:
> >>> This adds APIs that will be used to fill in guest info table,
> >>> implemented using QOM, to various piix components.
> >>>
> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>> ---
> >>>
> >>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >>> index c885690..2128f13 100644
> >>> --- a/hw/acpi/piix4.c
> >>> +++ b/hw/acpi/piix4.c
> >>> @@ -29,6 +29,7 @@
> >>>  #include "exec/ioport.h"
> >>>  #include "hw/nvram/fw_cfg.h"
> >>>  #include "exec/address-spaces.h"
> >>> +#include "hw/acpi/piix4.h"
> >>>  
> >>>  //#define DEBUG
> >>>  
> >>> @@ -63,7 +64,7 @@ typedef struct CPUStatus {
> >>>      uint8_t sts[PIIX4_PROC_LEN];
> >>>  } CPUStatus;
> >>>  
> >>> -typedef struct PIIX4PMState {
> >>> +struct PIIX4PMState {
> >>>      /*< private >*/
> >>>      PCIDevice parent_obj;
> >>>      /*< public >*/
> >>> @@ -96,7 +97,7 @@ typedef struct PIIX4PMState {
> >>>  
> >>>      CPUStatus gpe_cpu;
> >>>      Notifier cpu_added_notifier;
> >>> -} PIIX4PMState;
> >>> +};
> >>>  
> >>>  #define TYPE_PIIX4_PM "PIIX4_PM"
> >>>  
> >>
> >> Here add
> >>
> >> #define PIIX4_PM(obj) OBJECT_CHECK(PIIX4PMState, obj, TYPE_PIIX4_PM)
> >>
> >> for the general public ...
> >>
> >>> @@ -458,6 +459,30 @@ static int piix4_pm_initfn(PCIDevice *dev)
> >>>      return 0;
> >>>  }
> >>>  
> >>> +PIIX4PMState *piix4_pm_find(void)
> >>> +{
> >>> +    bool ambig;
> >>> +    Object *o = object_resolve_path_type("", "PIIX4_PM", &ambig);
> >>> +
> >>> +    if (ambig || !o) {
> >>> +        return NULL;
> >>> +    }
> >>> +    return OBJECT_CHECK(PIIX4PMState, o, "PIIX4_PM");
> >>
> >> ... instead of open-coding it where only you use it, please.
> > 
> > I don't really understand in which direction you want to take this.
> > On the one hand, you are saying "there's one caller of
> > this function so please open-code it".
> > On the other hand, you are saying "it does not matter that
> > there's one user of this macro put it in the header".
> > 
> > Is the point that macros are somehow better than functions so they
> > should be used where possible?
> 
> No, the point is reuse: For QOM realize we will very likely need the
> same thing. It was not my decision to make these macros, I'm just making
> Anthony's QOM pattern consistent.
> 
> >>> +}
> >>> +
> >>> +void piix4_pm_get_acpi_pm_info(PIIX4PMState *s, AcpiPmInfo *info)
> >>> +{
> >>> +        info->s3_disabled = s->disable_s3;
> >>> +        info->s4_disabled = s->disable_s4;
> >>> +        info->s4_val = s->s4_val;
> >>
> >> These three are accessible as qdev/QOM properties.
> >>
> >>> +
> >>> +        info->acpi_enable_cmd = ACPI_ENABLE;
> >>> +        info->acpi_disable_cmd = ACPI_DISABLE;
> >>> +        info->gpe0_blk = GPE_BASE;
> >>> +        info->gpe0_blk_len = GPE_LEN;
> >>
> >> So here the issue is that values are constant?
> > 
> > Yes.
> > 
> >>> +        info->sci_int = 9;
> >>
> >> Magic number - use a define to share it with wherever it is used?
> >>
> >>> +}
> >>> +
> >>>  i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> >>>                         qemu_irq sci_irq, qemu_irq smi_irq,
> >>>                         int kvm_enabled, FWCfgState *fw_cfg)
> >>> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> >>> index de87241..14573ab 100644
> >>> --- a/hw/mips/mips_malta.c
> >>> +++ b/hw/mips/mips_malta.c
> >>> @@ -965,7 +965,7 @@ void mips_malta_init(QEMUMachineInitArgs *args)
> >>>      pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
> >>>      pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
> >>>      smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
> >>> -                          isa_get_irq(NULL, 9), NULL, 0, NULL);
> >>> +                          isa_get_irq(NULL, 9), NULL, 0, NULL, NULL);
> >>
> >> This looks fishy. Might belong into a different patch? Implementation
> >> didn't change above.
> >>
> >>>      /* TODO: Populate SPD eeprom data.  */
> >>>      smbus_eeprom_init(smbus, 8, NULL, 0);
> >>>      pit = pit_init(isa_bus, 0x40, 0, NULL);
> >>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> >>> index 3908860..daefdfb 100644
> >>> --- a/hw/pci-host/piix.c
> >>> +++ b/hw/pci-host/piix.c
> >>> @@ -349,6 +349,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
> >>>      return b;
> >>>  }
> >>>  
> >>> +PCIBus *find_i440fx(void)
> >>> +{
> >>> +    PCIHostState *s = OBJECT_CHECK(PCIHostState,
> >>> +                                   object_resolve_path("/machine/i440fx", NULL),
> >>> +                                   TYPE_PCI_HOST_BRIDGE);
> >>
> >> Open-coded OBJECT_CHECK() - should use existing PCI_HOST_BRIDGE(...).
> >>
> >>> +    return s ? s->bus : NULL;
> >>> +}
> >>
> >> Is this function really necessary? /machine/i440fx/pci.0 is a trivial
> >> addition to the path that's already being used here. You can do:
> >> PCIBus *bus = PCI_BUS(object_resolve_path("/machine/i440fx/pci.0"));
> >> where you actually need to access it.
> > 
> > 
> > I don't mind but I would like to avoid callers hard-coding
> > paths, in this case "i440fx".
> > Why the aversion to functions?
> 
> Simply because QMP cannot call functions. It has to work with qom-list
> and qom-get, so this is a test case showing what is missing and can IMO
> easily be addressed for both parties.

Fine but if the function calls QOM things internally
then where's the problem?

> The suggested cast to PCI_BUS() lets you use regular qdev functions btw
> as a shortcut, QMP users would need to iterate children of that node.
> 
> The suggested "pci.0" is considered stable for -device ...,bus=pci.0
> according to review feedback the Xen people have received for libxl.
> 
> Andreas
> 
> > 
> >>> +
> >>>  /* PIIX3 PCI to ISA bridge */
> >>>  static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
> >>>  {
> >>> diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
> >>> new file mode 100644
> >>> index 0000000..2876428
> >>> --- /dev/null
> >>> +++ b/include/hw/acpi/piix4.h
> >>> @@ -0,0 +1,10 @@
> >>> +#ifndef HW_ACPI_PIIX4_H
> >>> +#define HW_ACPI_PIIX4_H
> >>> +
> >>> +#include "qemu/typedefs.h"
> >>> +
> >>> +PIIX4PMState *piix4_pm_find(void);
> >>> +
> >>> +void piix4_pm_get_acpi_pm_info(PIIX4PMState *, AcpiPmInfo *);
> >>> +
> >>> +#endif
> >>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >>> index 7c0bd50..76af5cd 100644
> >>> --- a/include/hw/i386/pc.h
> >>> +++ b/include/hw/i386/pc.h
> >>> @@ -186,6 +186,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> >>>                      MemoryRegion *pci_memory,
> >>>                      MemoryRegion *ram_memory);
> >>>  
> >>> +PCIBus *find_i440fx(void);
> >>>  /* piix4.c */
> >>>  extern PCIDevice *piix4_dev;
> >>>  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
> >>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> >>> index cb66e19..7d42693 100644
> >>> --- a/include/qemu/typedefs.h
> >>> +++ b/include/qemu/typedefs.h
> >>> @@ -65,6 +65,7 @@ typedef struct QEMUSGList QEMUSGList;
> >>>  typedef struct SHPCDevice SHPCDevice;
> >>>  typedef struct FWCfgState FWCfgState;
> >>>  typedef struct PcGuestInfo PcGuestInfo;
> >>> +typedef struct PIIX4PMState PIIX4PMState;
> >>>  typedef struct AcpiPmInfo AcpiPmInfo;
> >>>  
> >>>  #endif /* QEMU_TYPEDEFS_H */
> >>>
> >>
> >>
> >> -- 
> >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber July 28, 2013, 10:31 a.m. UTC | #5 
Am 28.07.2013 12:14, schrieb Michael S. Tsirkin:
> On Sun, Jul 28, 2013 at 11:38:17AM +0200, Andreas Färber wrote:
>> Am 28.07.2013 09:30, schrieb Michael S. Tsirkin:
>>> On Sun, Jul 28, 2013 at 02:12:49AM +0200, Andreas Färber wrote:
>>>> Am 25.07.2013 11:32, schrieb Michael S. Tsirkin:
>>>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>>>>> index 3908860..daefdfb 100644
>>>>> --- a/hw/pci-host/piix.c
>>>>> +++ b/hw/pci-host/piix.c
>>>>> @@ -349,6 +349,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
>>>>>      return b;
>>>>>  }
>>>>>  
>>>>> +PCIBus *find_i440fx(void)
>>>>> +{
>>>>> +    PCIHostState *s = OBJECT_CHECK(PCIHostState,
>>>>> +                                   object_resolve_path("/machine/i440fx", NULL),
>>>>> +                                   TYPE_PCI_HOST_BRIDGE);
>>>>
>>>> Open-coded OBJECT_CHECK() - should use existing PCI_HOST_BRIDGE(...).
>>>>
>>>>> +    return s ? s->bus : NULL;
>>>>> +}
>>>>
>>>> Is this function really necessary? /machine/i440fx/pci.0 is a trivial
>>>> addition to the path that's already being used here. You can do:
>>>> PCIBus *bus = PCI_BUS(object_resolve_path("/machine/i440fx/pci.0"));
>>>> where you actually need to access it.
>>>
>>>
>>> I don't mind but I would like to avoid callers hard-coding
>>> paths, in this case "i440fx".
>>> Why the aversion to functions?
>>
>> Simply because QMP cannot call functions. It has to work with qom-list
>> and qom-get, so this is a test case showing what is missing and can IMO
>> easily be addressed for both parties.
> 
> Fine but if the function calls QOM things internally
> then where's the problem?

The grief with object_path_resolve_type() is that it's a "hack" Paolo
has added for his convenience (in audio code?) that QMP cannot use, so
we need name-based paths to be available.

And to clarify, I have been talking about two different time frames:
Small adjustments that you can make for 1.6 (e.g., casts, q35 property,
different QOM function/callsite used; if Anthony is okay with taking
ACPI at this late point) and post-1.6 cleanups to replace interim
constructs that involve refactorings outside your control (e.g.,
MemoryRegion QOM'ification, adding properties to random devices).

Andreas

>> The suggested cast to PCI_BUS() lets you use regular qdev functions btw
>> as a shortcut, QMP users would need to iterate children of that node.
>>
>> The suggested "pci.0" is considered stable for -device ...,bus=pci.0
>> according to review feedback the Xen people have received for libxl.
Andreas Färber July 28, 2013, 11:08 a.m. UTC | #6 
Am 28.07.2013 12:31, schrieb Andreas Färber:
> Am 28.07.2013 12:14, schrieb Michael S. Tsirkin:
>> On Sun, Jul 28, 2013 at 11:38:17AM +0200, Andreas Färber wrote:
>>> Am 28.07.2013 09:30, schrieb Michael S. Tsirkin:
>>>> On Sun, Jul 28, 2013 at 02:12:49AM +0200, Andreas Färber wrote:
>>>>> Am 25.07.2013 11:32, schrieb Michael S. Tsirkin:
>>>>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>>>>>> index 3908860..daefdfb 100644
>>>>>> --- a/hw/pci-host/piix.c
>>>>>> +++ b/hw/pci-host/piix.c
>>>>>> @@ -349,6 +349,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
>>>>>>      return b;
>>>>>>  }
>>>>>>  
>>>>>> +PCIBus *find_i440fx(void)
>>>>>> +{
>>>>>> +    PCIHostState *s = OBJECT_CHECK(PCIHostState,
>>>>>> +                                   object_resolve_path("/machine/i440fx", NULL),
>>>>>> +                                   TYPE_PCI_HOST_BRIDGE);
>>>>>
>>>>> Open-coded OBJECT_CHECK() - should use existing PCI_HOST_BRIDGE(...).
>>>>>
>>>>>> +    return s ? s->bus : NULL;
>>>>>> +}
>>>>>
>>>>> Is this function really necessary? /machine/i440fx/pci.0 is a trivial
>>>>> addition to the path that's already being used here. You can do:
>>>>> PCIBus *bus = PCI_BUS(object_resolve_path("/machine/i440fx/pci.0"));
>>>>> where you actually need to access it.
>>>>
>>>>
>>>> I don't mind but I would like to avoid callers hard-coding
>>>> paths, in this case "i440fx".
>>>> Why the aversion to functions?
>>>
>>> Simply because QMP cannot call functions. It has to work with qom-list
>>> and qom-get, so this is a test case showing what is missing and can IMO
>>> easily be addressed for both parties.
>>
>> Fine but if the function calls QOM things internally
>> then where's the problem?
> 
> The grief with object_path_resolve_type() is that it's a "hack" Paolo
> has added for his convenience (in audio code?) that QMP cannot use, so
> we need name-based paths to be available.

Add to that, the way I see QOM devices (as opposed to qdev or pre-qdev
devices) is that they are instantiated from the outside - a different
source file or command line or config file - via QOM APIs, and they
allow other source files to interact with them via mydev_foo(MyDev *d,
...) APIs that operate on an instance.

Having functions to create devices often hints at legacy code from
pre-qdev times (we had such a discussion with Alex when I tried to
qdev'ify the prep_pci device), indicating that either something is not
yet accessible via qdev/QOM APIs (e.g., IRQs) or that the device is not
yet implementing composition properly (e.g., creating a bus after the
bridge device rather than in the bridge, or a PCIDevice after the PHB
rather than by the PHB).

Having functions in the device's file to obtain a random instance of
that device in the form MyDev *mydev_get(void) is what I dislike here.

My personal preference (which you may ignore if others disagree) would
be to have accessors, where unavoidable, in the form:

foo mydev_get_bar(MyDev *s)
{
    return s->baz;
}

which we can then later easily convert into dynamic property getters,
and it would hopefully spare us new per-device ...Info structs by
obtaining the info where and when you need it.
I admit it's a bit of boilerplate to code, but I've seen at most 4 cases
per device where this would apply and I'm saying this with our
longer-term QOM goals in mind.

I'm skeptical towards Igor's suggestion of adding Last Minute 1.6 qdev
properties (as opposed to a composition property, you force me to become
very explicit in my explanations...) as that would bring ABI stability
rules onto us.

Andreas

> And to clarify, I have been talking about two different time frames:
> Small adjustments that you can make for 1.6 (e.g., casts, q35 property,
> different QOM function/callsite used; if Anthony is okay with taking
> ACPI at this late point) and post-1.6 cleanups to replace interim
> constructs that involve refactorings outside your control (e.g.,
> MemoryRegion QOM'ification, adding properties to random devices).
> 
> Andreas
> 
>>> The suggested cast to PCI_BUS() lets you use regular qdev functions btw
>>> as a shortcut, QMP users would need to iterate children of that node.
>>>
>>> The suggested "pci.0" is considered stable for -device ...,bus=pci.0
>>> according to review feedback the Xen people have received for libxl.
Michael S. Tsirkin July 28, 2013, 12:19 p.m. UTC | #7 
On Sun, Jul 28, 2013 at 01:08:13PM +0200, Andreas Färber wrote:
> Am 28.07.2013 12:31, schrieb Andreas Färber:
> > Am 28.07.2013 12:14, schrieb Michael S. Tsirkin:
> >> On Sun, Jul 28, 2013 at 11:38:17AM +0200, Andreas Färber wrote:
> >>> Am 28.07.2013 09:30, schrieb Michael S. Tsirkin:
> >>>> On Sun, Jul 28, 2013 at 02:12:49AM +0200, Andreas Färber wrote:
> >>>>> Am 25.07.2013 11:32, schrieb Michael S. Tsirkin:
> >>>>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> >>>>>> index 3908860..daefdfb 100644
> >>>>>> --- a/hw/pci-host/piix.c
> >>>>>> +++ b/hw/pci-host/piix.c
> >>>>>> @@ -349,6 +349,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
> >>>>>>      return b;
> >>>>>>  }
> >>>>>>  
> >>>>>> +PCIBus *find_i440fx(void)
> >>>>>> +{
> >>>>>> +    PCIHostState *s = OBJECT_CHECK(PCIHostState,
> >>>>>> +                                   object_resolve_path("/machine/i440fx", NULL),
> >>>>>> +                                   TYPE_PCI_HOST_BRIDGE);
> >>>>>
> >>>>> Open-coded OBJECT_CHECK() - should use existing PCI_HOST_BRIDGE(...).
> >>>>>
> >>>>>> +    return s ? s->bus : NULL;
> >>>>>> +}
> >>>>>
> >>>>> Is this function really necessary? /machine/i440fx/pci.0 is a trivial
> >>>>> addition to the path that's already being used here. You can do:
> >>>>> PCIBus *bus = PCI_BUS(object_resolve_path("/machine/i440fx/pci.0"));
> >>>>> where you actually need to access it.
> >>>>
> >>>>
> >>>> I don't mind but I would like to avoid callers hard-coding
> >>>> paths, in this case "i440fx".
> >>>> Why the aversion to functions?
> >>>
> >>> Simply because QMP cannot call functions. It has to work with qom-list
> >>> and qom-get, so this is a test case showing what is missing and can IMO
> >>> easily be addressed for both parties.
> >>
> >> Fine but if the function calls QOM things internally
> >> then where's the problem?
> > 
> > The grief with object_path_resolve_type() is that it's a "hack" Paolo
> > has added for his convenience (in audio code?) that QMP cannot use, so
> > we need name-based paths to be available.
> 
> Add to that, the way I see QOM devices (as opposed to qdev or pre-qdev
> devices) is that they are instantiated from the outside - a different
> source file or command line or config file - via QOM APIs, and they
> allow other source files to interact with them via mydev_foo(MyDev *d,
> ...) APIs that operate on an instance.
> 
> Having functions to create devices often hints at legacy code from
> pre-qdev times (we had such a discussion with Alex when I tried to
> qdev'ify the prep_pci device), indicating that either something is not
> yet accessible via qdev/QOM APIs (e.g., IRQs) or that the device is not
> yet implementing composition properly (e.g., creating a bus after the
> bridge device rather than in the bridge, or a PCIDevice after the PHB
> rather than by the PHB).
> 
> Having functions in the device's file to obtain a random instance of
> that device in the form MyDev *mydev_get(void) is what I dislike here.

Absolutely but what would you do?
E.g. we can't handle more than one instance of PIIX ATM.


> My personal preference (which you may ignore if others disagree) would
> be to have accessors, where unavoidable, in the form:
> 
> foo mydev_get_bar(MyDev *s)
> {
>     return s->baz;
> }

So how about
implementing mydev_get_bar(void) and make that do the lookup
internally using a path?
Also mydev_present to test that.

> 
> which we can then later easily convert into dynamic property getters,
> and it would hopefully spare us new per-device ...Info structs by
> obtaining the info where and when you need it.
> I admit it's a bit of boilerplate to code, but I've seen at most 4 cases
> per device where this would apply and I'm saying this with our
> longer-term QOM goals in mind.
> 
> I'm skeptical towards Igor's suggestion of adding Last Minute 1.6 qdev
> properties (as opposed to a composition property, you force me to become
> very explicit in my explanations...) as that would bring ABI stability
> rules onto us.
> 
> Andreas
> 
> > And to clarify, I have been talking about two different time frames:
> > Small adjustments that you can make for 1.6 (e.g., casts, q35 property,
> > different QOM function/callsite used; if Anthony is okay with taking
> > ACPI at this late point) and post-1.6 cleanups to replace interim
> > constructs that involve refactorings outside your control (e.g.,
> > MemoryRegion QOM'ification, adding properties to random devices).
> > 
> > Andreas
> > 
> >>> The suggested cast to PCI_BUS() lets you use regular qdev functions btw
> >>> as a shortcut, QMP users would need to iterate children of that node.
> >>>
> >>> The suggested "pci.0" is considered stable for -device ...,bus=pci.0
> >>> according to review feedback the Xen people have received for libxl.
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
diff mbox

Patch

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index c885690..2128f13 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -29,6 +29,7 @@ 
 #include "exec/ioport.h"
 #include "hw/nvram/fw_cfg.h"
 #include "exec/address-spaces.h"
+#include "hw/acpi/piix4.h"
 
 //#define DEBUG
 
@@ -63,7 +64,7 @@  typedef struct CPUStatus {
     uint8_t sts[PIIX4_PROC_LEN];
 } CPUStatus;
 
-typedef struct PIIX4PMState {
+struct PIIX4PMState {
     /*< private >*/
     PCIDevice parent_obj;
     /*< public >*/
@@ -96,7 +97,7 @@  typedef struct PIIX4PMState {
 
     CPUStatus gpe_cpu;
     Notifier cpu_added_notifier;
-} PIIX4PMState;
+};
 
 #define TYPE_PIIX4_PM "PIIX4_PM"
 
@@ -458,6 +459,30 @@  static int piix4_pm_initfn(PCIDevice *dev)
     return 0;
 }
 
+PIIX4PMState *piix4_pm_find(void)
+{
+    bool ambig;
+    Object *o = object_resolve_path_type("", "PIIX4_PM", &ambig);
+
+    if (ambig || !o) {
+        return NULL;
+    }
+    return OBJECT_CHECK(PIIX4PMState, o, "PIIX4_PM");
+}
+
+void piix4_pm_get_acpi_pm_info(PIIX4PMState *s, AcpiPmInfo *info)
+{
+        info->s3_disabled = s->disable_s3;
+        info->s4_disabled = s->disable_s4;
+        info->s4_val = s->s4_val;
+
+        info->acpi_enable_cmd = ACPI_ENABLE;
+        info->acpi_disable_cmd = ACPI_DISABLE;
+        info->gpe0_blk = GPE_BASE;
+        info->gpe0_blk_len = GPE_LEN;
+        info->sci_int = 9;
+}
+
 i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
                        qemu_irq sci_irq, qemu_irq smi_irq,
                        int kvm_enabled, FWCfgState *fw_cfg)
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index de87241..14573ab 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -965,7 +965,7 @@  void mips_malta_init(QEMUMachineInitArgs *args)
     pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
     pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
     smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
-                          isa_get_irq(NULL, 9), NULL, 0, NULL);
+                          isa_get_irq(NULL, 9), NULL, 0, NULL, NULL);
     /* TODO: Populate SPD eeprom data.  */
     smbus_eeprom_init(smbus, 8, NULL, 0);
     pit = pit_init(isa_bus, 0x40, 0, NULL);
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 3908860..daefdfb 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -349,6 +349,14 @@  PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
     return b;
 }
 
+PCIBus *find_i440fx(void)
+{
+    PCIHostState *s = OBJECT_CHECK(PCIHostState,
+                                   object_resolve_path("/machine/i440fx", NULL),
+                                   TYPE_PCI_HOST_BRIDGE);
+    return s ? s->bus : NULL;
+}
+
 /* PIIX3 PCI to ISA bridge */
 static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
 {
diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
new file mode 100644
index 0000000..2876428
--- /dev/null
+++ b/include/hw/acpi/piix4.h
@@ -0,0 +1,10 @@ 
+#ifndef HW_ACPI_PIIX4_H
+#define HW_ACPI_PIIX4_H
+
+#include "qemu/typedefs.h"
+
+PIIX4PMState *piix4_pm_find(void);
+
+void piix4_pm_get_acpi_pm_info(PIIX4PMState *, AcpiPmInfo *);
+
+#endif
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 7c0bd50..76af5cd 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -186,6 +186,7 @@  PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                     MemoryRegion *pci_memory,
                     MemoryRegion *ram_memory);
 
+PCIBus *find_i440fx(void);
 /* piix4.c */
 extern PCIDevice *piix4_dev;
 int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index cb66e19..7d42693 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -65,6 +65,7 @@  typedef struct QEMUSGList QEMUSGList;
 typedef struct SHPCDevice SHPCDevice;
 typedef struct FWCfgState FWCfgState;
 typedef struct PcGuestInfo PcGuestInfo;
+typedef struct PIIX4PMState PIIX4PMState;
 typedef struct AcpiPmInfo AcpiPmInfo;
 
 #endif /* QEMU_TYPEDEFS_H */