Re: [Qemu-devel] [PATCH 2/2] acpi: remove static gpe and pci0_status variables

[Isaku Yamahata]

Hi Blue.
I send out very similar patches before and got acked-by from Gerd.
But they haven't been merged yet. Please look at them.
Instead of reinventing similar patches, those patches should be merged.
If necessary, I'm willing to rebase them and resend them.

As for code iteself, the hotplug callback argument was DeviceState
instead of PCIDevice as Gerd suggested.

[Qemu-devel] [PATCH V12 00/27] split out piix specific part from pc emulator 
and some clean ups
[Qemu-devel] [PATCH V12 21/27] acpi_piix4: qdevfy.
[Qemu-devel] [PATCH V12 22/27] pci hotplug: add argument to pci hot plug
[Qemu-devel] [PATCH V12 23/27] pci hotadd, acpi_piix4: remove global var

http://lists.nongnu.org/archive/html/qemu-devel/2010-01/msg00282.html
http://lists.nongnu.org/archive/html/qemu-devel/2010-01/msg00297.html
http://lists.nongnu.org/archive/html/qemu-devel/2010-01/msg00293.html
http://lists.nongnu.org/archive/html/qemu-devel/2010-01/msg00285.html



On Mon, May 10, 2010 at 11:51:22PM +0300, Blue Swirl wrote:
> Make gpe and pci0_status fields in PIIX4PMState.
> 
> Signed-off-by: Blue Swirl <address@hidden>
> ---
>  hw/acpi.c |   93 +++++++++++++++++++++++++++++++++---------------------------
>  hw/pc.c   |    1 -
>  hw/pc.h   |    1 -
>  hw/pci.c  |   12 +++++--
>  hw/pci.h  |    6 ++-
>  5 files changed, 63 insertions(+), 50 deletions(-)
> 
> diff --git a/hw/acpi.c b/hw/acpi.c
> index bb2974e..6db1a12 100644
> --- a/hw/acpi.c
> +++ b/hw/acpi.c
> @@ -30,6 +30,20 @@
> 
>  #define ACPI_DBG_IO_ADDR  0xb044
> 
> +#define GPE_BASE 0xafe0
> +#define PCI_BASE 0xae00
> +#define PCI_EJ_BASE 0xae08
> +
> +struct gpe_regs {
> +    uint16_t sts; /* status */
> +    uint16_t en;  /* enabled */
> +};
> +
> +struct pci_status {
> +    uint32_t up;
> +    uint32_t down;
> +};
> +
>  typedef struct PIIX4PMState {
>      PCIDevice dev;
>      uint16_t pmsts;
> @@ -52,6 +66,8 @@ typedef struct PIIX4PMState {
>      qemu_irq cmos_s3;
>      qemu_irq smi_irq;
>      int kvm_enabled;
> +    struct gpe_regs gpe;
> +    struct pci_status pci0_status;
>  } PIIX4PMState;
> 
>  #define RSM_STS (1 << 15)
> @@ -497,16 +513,19 @@ static void piix4_powerdown(void *opaque, int
> irq, int power_failing)
>      }
>  }
> 
> +static void piix4_acpi_system_hot_add_init(PCIBus *bus, PCIDevice
> *hotplug_dev);
> +
>  i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>                         qemu_irq sci_irq, qemu_irq cmos_s3, qemu_irq smi_irq,
>                         int kvm_enabled)
>  {
>      PIIX4PMState *s;
> +    PCIDevice *d;
>      uint8_t *pci_conf;
> 
> -    s = (PIIX4PMState *)pci_register_device(bus,
> -                                         "PM", sizeof(PIIX4PMState),
> -                                         devfn, NULL, pm_write_config);
> +    d = pci_register_device(bus, "PM", sizeof(PIIX4PMState), devfn, NULL,
> +                            pm_write_config);
> +    s = container_of(d, PIIX4PMState, dev);
>      pci_conf = s->dev.config;
>      pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
>      pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371AB_3);
> @@ -557,26 +576,11 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn,
> uint32_t smb_io_base,
>      s->smi_irq = smi_irq;
>      qemu_register_reset(piix4_reset, s);
> 
> +    piix4_acpi_system_hot_add_init(bus, d);
> +
>      return s->smbus;
>  }
> 
> -#define GPE_BASE 0xafe0
> -#define PCI_BASE 0xae00
> -#define PCI_EJ_BASE 0xae08
> -
> -struct gpe_regs {
> -    uint16_t sts; /* status */
> -    uint16_t en;  /* enabled */
> -};
> -
> -struct pci_status {
> -    uint32_t up;
> -    uint32_t down;
> -};
> -
> -static struct gpe_regs gpe;
> -static struct pci_status pci0_status;
> -
>  static uint32_t gpe_read_val(uint16_t val, uint32_t addr)
>  {
>      if (addr & 1)
> @@ -714,46 +718,51 @@ static void pciej_write(void *opaque, uint32_t
> addr, uint32_t val)
>  #endif
>  }
> 
> -static int piix4_device_hotplug(PCIDevice *dev, int state);
> +static int piix4_device_hotplug(PCIDevice *hotplug_dev, PCIDevice *dev,
> +                                int state);
> 
> -void piix4_acpi_system_hot_add_init(PCIBus *bus)
> +static void piix4_acpi_system_hot_add_init(PCIBus *bus, PCIDevice 
> *hotplug_dev)
>  {
> -    register_ioport_write(GPE_BASE, 4, 1, gpe_writeb, &gpe);
> -    register_ioport_read(GPE_BASE, 4, 1,  gpe_readb, &gpe);
> +    PIIX4PMState *s = container_of(hotplug_dev, PIIX4PMState, dev);
> 
> -    register_ioport_write(PCI_BASE, 8, 4, pcihotplug_write, &pci0_status);
> -    register_ioport_read(PCI_BASE, 8, 4,  pcihotplug_read, &pci0_status);
> +    register_ioport_write(GPE_BASE, 4, 1, gpe_writeb, s);
> +    register_ioport_read(GPE_BASE, 4, 1,  gpe_readb, s);
> +
> +    register_ioport_write(PCI_BASE, 8, 4, pcihotplug_write, s);
> +    register_ioport_read(PCI_BASE, 8, 4,  pcihotplug_read, s);
> 
>      register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, bus);
>      register_ioport_read(PCI_EJ_BASE, 4, 4,  pciej_read, bus);
> 
> -    pci_bus_hotplug(bus, piix4_device_hotplug);
> +    pci_bus_hotplug(bus, piix4_device_hotplug, hotplug_dev);
>  }
> 
> -static void enable_device(struct pci_status *p, struct gpe_regs *g, int slot)
> +static void enable_device(PIIX4PMState *s, int slot)
>  {
> -    g->sts |= 2;
> -    p->up |= (1 << slot);
> +    s->gpe.sts |= 2;
> +    s->pci0_status.up |= (1 << slot);
>  }
> 
> -static void disable_device(struct pci_status *p, struct gpe_regs *g, int 
> slot)
> +static void disable_device(PIIX4PMState *s, int slot)
>  {
> -    g->sts |= 2;
> -    p->down |= (1 << slot);
> +    s->gpe.sts |= 2;
> +    s->pci0_status.down |= (1 << slot);
>  }
> 
> -static int piix4_device_hotplug(PCIDevice *dev, int state)
> +static int piix4_device_hotplug(PCIDevice *hotplug_dev, PCIDevice *dev,
> +                                int state)
>  {
> -    PIIX4PMState *s = container_of(dev, PIIX4PMState, dev);
> +    PIIX4PMState *s = container_of(hotplug_dev, PIIX4PMState, dev);
>      int slot = PCI_SLOT(dev->devfn);
> 
> -    pci0_status.up = 0;
> -    pci0_status.down = 0;
> -    if (state)
> -        enable_device(&pci0_status, &gpe, slot);
> -    else
> -        disable_device(&pci0_status, &gpe, slot);
> -    if (gpe.en & 2) {
> +    s->pci0_status.up = 0;
> +    s->pci0_status.down = 0;
> +    if (state) {
> +        enable_device(s, slot);
> +    } else {
> +        disable_device(s, slot);
> +    }
> +    if (s->gpe.en & 2) {
>          qemu_set_irq(s->irq, 1);
>          qemu_set_irq(s->irq, 0);
>      }
> diff --git a/hw/pc.c b/hw/pc.c
> index db2b9a2..e41db68 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1046,7 +1046,6 @@ static void pc_init1(ram_addr_t ram_size,
>              qdev_prop_set_ptr(eeprom, "data", eeprom_buf + (i * 256));
>              qdev_init_nofail(eeprom);
>          }
> -        piix4_acpi_system_hot_add_init(pci_bus);
>      }
> 
>      if (i440fx_state) {
> diff --git a/hw/pc.h b/hw/pc.h
> index d11a576..088937a 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -94,7 +94,6 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn,
> uint32_t smb_io_base,
>                         qemu_irq sci_irq, qemu_irq cmos_s3, qemu_irq smi_irq,
>                         int kvm_enabled);
>  void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr);
> -void piix4_acpi_system_hot_add_init(PCIBus *bus);
> 
>  /* hpet.c */
>  extern int no_hpet;
> diff --git a/hw/pci.c b/hw/pci.c
> index f167436..9d19cb8 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -42,6 +42,7 @@ struct PCIBus {
>      pci_set_irq_fn set_irq;
>      pci_map_irq_fn map_irq;
>      pci_hotplug_fn hotplug;
> +    PCIDevice *hotplug_dev;
>      void *irq_opaque;
>      PCIDevice *devices[256];
>      PCIDevice *parent_dev;
> @@ -233,10 +234,12 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn
> set_irq, pci_map_irq_fn map_irq,
>      bus->irq_count = qemu_mallocz(nirq * sizeof(bus->irq_count[0]));
>  }
> 
> -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug)
> +void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug,
> +                     PCIDevice *hotplug_dev)
>  {
>      bus->qbus.allow_hotplug = 1;
>      bus->hotplug = hotplug;
> +    bus->hotplug_dev = hotplug_dev;
>  }
> 
>  void pci_bus_set_mem_base(PCIBus *bus, target_phys_addr_t base)
> @@ -1660,8 +1663,9 @@ static int pci_qdev_init(DeviceState *qdev,
> DeviceInfo *base)
>          pci_dev->romfile = qemu_strdup(info->romfile);
>      pci_add_option_rom(pci_dev);
> 
> -    if (qdev->hotplugged)
> -        bus->hotplug(pci_dev, 1);
> +    if (qdev->hotplugged) {
> +        bus->hotplug(bus->hotplug_dev, pci_dev, 1);
> +    }
>      return 0;
>  }
> 
> @@ -1669,7 +1673,7 @@ static int pci_unplug_device(DeviceState *qdev)
>  {
>      PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
> 
> -    dev->bus->hotplug(dev, 0);
> +    dev->bus->hotplug(dev->bus->hotplug_dev, dev, 0);
>      return 0;
>  }
> 
> diff --git a/hw/pci.h b/hw/pci.h
> index 625188c..31e0438 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -209,13 +209,15 @@ int pci_device_load(PCIDevice *s, QEMUFile *f);
> 
>  typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
>  typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
> -typedef int (*pci_hotplug_fn)(PCIDevice *pci_dev, int state);
> +typedef int (*pci_hotplug_fn)(PCIDevice *hotplug_dev, PCIDevice *pci_dev,
> +                              int state);
>  void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
>                           const char *name, int devfn_min);
>  PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min);
>  void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn 
> map_irq,
>                    void *irq_opaque, int nirq);
> -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug);
> +void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug,
> +                     PCIDevice *hotplug_dev);
>  PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>                           pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
>                           void *irq_opaque, int devfn_min, int nirq);
> -- 
> 1.6.2.4
> 

-- 
yamahata