Re: [Qemu-devel] [PATCH v4 19/23] piix: APIs for pc guest info

[Michael S. Tsirkin]

On Mon, Sep 23, 2013 at 01:14:11PM +0200, Igor Mammedov wrote:
> On Mon, 23 Sep 2013 13:10:49 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Mon, Sep 23, 2013 at 11:27:09AM +0200, Igor Mammedov wrote:
> > > On Sun, 22 Sep 2013 16:37:59 +0300
> > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > 
> > > > This adds APIs that will be used to fill in guest acpi tables.
> > > > Some required information is still lacking in QOM, so we
> > > > fall back on lookups by type and returning explicit types.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > > > ---
> > > >  include/hw/acpi/piix4.h |  8 ++++++++
> > > >  include/hw/i386/pc.h    |  1 +
> > > >  hw/acpi/piix4.c         | 44 
> > > > ++++++++++++++++++++++++++++++++++++++++----
> > > >  hw/pci-host/piix.c      |  8 ++++++++
> > > >  4 files changed, 57 insertions(+), 4 deletions(-)
> > > >  create mode 100644 include/hw/acpi/piix4.h
> > > > 
> > > > diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
> > > > new file mode 100644
> > > > index 0000000..65e6fd7
> > > > --- /dev/null
> > > > +++ b/include/hw/acpi/piix4.h
> > > > @@ -0,0 +1,8 @@
> > > > +#ifndef HW_ACPI_PIIX4_H
> > > > +#define HW_ACPI_PIIX4_H
> > > > +
> > > > +#include "qemu/typedefs.h"
> > > > +
> > > > +Object *piix4_pm_find(void);
> > > > +
> > > > +#endif
> > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > index f966cef..2a5d996 100644
> > > > --- a/include/hw/i386/pc.h
> > > > +++ b/include/hw/i386/pc.h
> > > > @@ -193,6 +193,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/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > index 4b8c1da..3bcd890 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
> > > >  
> > > > @@ -69,6 +70,8 @@ typedef struct PIIX4PMState {
> > > >      /*< public >*/
> > > >  
> > > >      MemoryRegion io;
> > > > +    uint32_t io_base;
> > > > +
> > > >      MemoryRegion io_gpe;
> > > >      MemoryRegion io_pci;
> > > >      MemoryRegion io_cpu;
> > > > @@ -152,14 +155,13 @@ static void apm_ctrl_changed(uint32_t val, void 
> > > > *arg)
> > > >  static void pm_io_space_update(PIIX4PMState *s)
> > > >  {
> > > >      PCIDevice *d = PCI_DEVICE(s);
> > > > -    uint32_t pm_io_base;
> > > >  
> > > > -    pm_io_base = le32_to_cpu(*(uint32_t *)(d->config + 0x40));
> > > > -    pm_io_base &= 0xffc0;
> > > > +    s->io_base = le32_to_cpu(*(uint32_t *)(d->config + 0x40));
> > > > +    s->io_base &= 0xffc0;
> > > >  
> > > >      memory_region_transaction_begin();
> > > >      memory_region_set_enabled(&s->io, d->config[0x80] & 1);
> > > > -    memory_region_set_address(&s->io, pm_io_base);
> > > > +    memory_region_set_address(&s->io, s->io_base);
> > > >      memory_region_transaction_commit();
> > > >  }
> > > >  
> > > > @@ -407,6 +409,28 @@ static void piix4_pm_machine_ready(Notifier *n, 
> > > > void *opaque)
> > > >          (memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
> > > >  }
> > > >  
> > > > +static void piix4_pm_add_propeties(PIIX4PMState *s)
> > > > +{
> > > > +    static const uint8_t acpi_enable_cmd = ACPI_ENABLE;
> > > > +    static const uint8_t acpi_disable_cmd = ACPI_DISABLE;
> > > > +    static const uint32_t gpe0_blk = GPE_BASE;
> > > > +    static const uint32_t gpe0_blk_len = GPE_LEN;
> > > > +    static const uint16_t sci_int = 9;
> > > > +
> > > > +    object_property_add_uint8_ptr(OBJECT(s), 
> > > > ACPI_PM_PROP_ACPI_ENABLE_CMD,
> > > > +                                  &acpi_enable_cmd, NULL);
> > > > +    object_property_add_uint8_ptr(OBJECT(s), 
> > > > ACPI_PM_PROP_ACPI_DISABLE_CMD,
> > > > +                                  &acpi_disable_cmd, NULL);
> > > > +    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> > > > +                                  &gpe0_blk, NULL);
> > > > +    object_property_add_uint32_ptr(OBJECT(s), 
> > > > ACPI_PM_PROP_GPE0_BLK_LEN,
> > > > +                                  &gpe0_blk_len, NULL);
> > > > +    object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
> > > > +                                  &sci_int, NULL);
> > > > +    object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
> > > > +                                  &s->io_base, NULL);
> > > > +}
> > > > +
> > > >  static int piix4_pm_initfn(PCIDevice *dev)
> > > >  {
> > > >      PIIX4PMState *s = PIIX4_PM(dev);
> > > > @@ -456,9 +480,21 @@ static int piix4_pm_initfn(PCIDevice *dev)
> > > >  
> > > >      piix4_acpi_system_hot_add_init(pci_address_space_io(dev), 
> > > > dev->bus, s);
> > > >  
> > > > +    piix4_pm_add_propeties(s);
> > > >      return 0;
> > > >  }
> > > >  
> > > > +Object *piix4_pm_find(void)
> > > > +{
> > > > +    bool ambig;
> > > > +    Object *o = object_resolve_path_type("", TYPE_PIIX4_PM, &ambig);
> > > > +
> > > > +    if (ambig || !o) {
> > > > +        return NULL;
> > > > +    }
> > > > +    return o;
> > > > +}
> > > > +
> > > >  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/pci-host/piix.c b/hw/pci-host/piix.c
> > > > index c041149..bad3953 100644
> > > > --- a/hw/pci-host/piix.c
> > > > +++ b/hw/pci-host/piix.c
> > > > @@ -416,6 +416,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > >      return b;
> > > >  }
> > > >  
> > > > +PCIBus *find_i440fx(void)
> > > > +{
> > > > +    PCIHostState *s = OBJECT_CHECK(PCIHostState,
> > > > +                                   
> > > > object_resolve_path("/machine/i440fx", NULL),
> > > > +                                   TYPE_PCI_HOST_BRIDGE);
> > > wouldn't object_resolve_path_type() be cleaner here?
> > 
> > Why would it?  /machine/i440fx is an absolute path
> > so it's unambigious, adding type is not needed.
> > 
> > And if we do we'd just have to repeat TYPE_PCI_HOST_BRIDGE twice
> > since we need the real type anyway.
> > 
> > What did I miss?
> > 
> object_resolve_path_type() does type check when object_resolve_path() doesn't.
> but since you need PCIHostState as result then you'll need cast anyway, so 
> both
> object_resolve_path_type() or object_resolve_path() would do.
> 
> But why not use an exiting type macro for this which does cast+type check
> instead of open coding it:
> 
> PCIHostState *s = PCI_HOST_BRIDGE(object_resolve_path("/machine/i440fx", 
> NULL));

Good point, I'll do it like this.
I won't repost just for this unless there are more comments.

> > > 
> > > > +    return s ? s->bus : NULL;
> > > > +}
> > > > +
> > > >  /* PIIX3 PCI to ISA bridge */
> > > >  static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
> > > >  {