Re: [Qemu-devel] [PATCH] acpi: add acpi_send_gpe_event() to rise sci for hotplug

[Zhu Guihua]

On 03/16/2015 10:35 PM, Michael S. Tsirkin wrote:
> On Mon, Mar 16, 2015 at 05:39:44PM +0800, Zhu Guihua wrote:
> > Add a new API named acpi_send_gpe_event() to send hotplug SCI.
> > This API can be used by pci, cpu and memory hotplug.
> >
> > Signed-off-by: Zhu Guihua <address@hidden>
> Good cleanup, but needs to fix up the coding style.

Got it, I will fix this.

Thanks,
Zhu

> > ---
> >   hw/acpi/core.c                   |  7 +++++++
> >   hw/acpi/cpu_hotplug.c            |  3 +--
> >   hw/acpi/memory_hotplug.c         |  3 +--
> >   hw/acpi/pcihp.c                  |  7 ++-----
> >   include/hw/acpi/acpi.h           | 10 ++++++++++
> >   include/hw/acpi/memory_hotplug.h |  2 --
> >   include/hw/acpi/pc-hotplug.h     |  1 -
> >   7 files changed, 21 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> > index 51913d6..8f9386d 100644
> > --- a/hw/acpi/core.c
> > +++ b/hw/acpi/core.c
> > @@ -666,6 +666,13 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
> >       return val;
> >   }
> >   
> > +void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
> > +                         Acpi_Hotplug_Status status)
> > +{
> > +    ar->gpe.sts[0] |= status;
> > +    acpi_update_sci(ar, irq);
> > +}
> > +
> >   void acpi_update_sci(ACPIREGS *regs, qemu_irq irq)
> >   {
> >       int sci_level, pm1a_sts;
> > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> > index b8ebfad..f5b9972 100644
> > --- a/hw/acpi/cpu_hotplug.c
> > +++ b/hw/acpi/cpu_hotplug.c
> > @@ -59,8 +59,7 @@ void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq,
> >           return;
> >       }
> >   
> > -    ar->gpe.sts[0] |= ACPI_CPU_HOTPLUG_STATUS;
> > -    acpi_update_sci(ar, irq);
> > +    acpi_send_gpe_event(ar, irq, ACPI_CPU_HOTPLUG_STATUS);
> >   }
> >   
> >   void acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > index c6580da..0736726 100644
> > --- a/hw/acpi/memory_hotplug.c
> > +++ b/hw/acpi/memory_hotplug.c
> > @@ -191,8 +191,7 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, 
> > MemHotplugState *mem_st,
> >       mdev->is_inserting = true;
> >   
> >       /* do ACPI magic */
> > -    ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
> > -    acpi_update_sci(ar, irq);
> > +    acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS);
> >       return;
> >   }
> >   
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index 612fec0..4b9b2b0 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -46,7 +46,6 @@
> >   # define ACPI_PCIHP_DPRINTF(format, ...)     do { } while (0)
> >   #endif
> >   
> > -#define ACPI_PCI_HOTPLUG_STATUS 2
> >   #define ACPI_PCIHP_ADDR 0xae00
> >   #define ACPI_PCIHP_SIZE 0x0014
> >   #define ACPI_PCIHP_LEGACY_SIZE 0x000f
> > @@ -203,8 +202,7 @@ void acpi_pcihp_device_plug_cb(ACPIREGS *ar, qemu_irq irq, 
> > AcpiPciHpState *s,
> >   
> >       s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
> >   
> > -    ar->gpe.sts[0] |= ACPI_PCI_HOTPLUG_STATUS;
> > -    acpi_update_sci(ar, irq);
> > +    acpi_send_gpe_event(ar, irq, ACPI_PCI_HOTPLUG_STATUS);
> >   }
> >   
> >   void acpi_pcihp_device_unplug_cb(ACPIREGS *ar, qemu_irq irq, AcpiPciHpState *s,
> > @@ -221,8 +219,7 @@ void acpi_pcihp_device_unplug_cb(ACPIREGS *ar, qemu_irq 
> > irq, AcpiPciHpState *s,
> >   
> >       s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
> >   
> > -    ar->gpe.sts[0] |= ACPI_PCI_HOTPLUG_STATUS;
> > -    acpi_update_sci(ar, irq);
> > +    acpi_send_gpe_event(ar, irq, ACPI_PCI_HOTPLUG_STATUS);
> >   }
> >   
> >   static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
> > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> > index 1f678b4..1945dcc 100644
> > --- a/include/hw/acpi/acpi.h
> > +++ b/include/hw/acpi/acpi.h
> > @@ -91,6 +91,13 @@
> >   /* PM2_CNT */
> >   #define ACPI_BITMASK_ARB_DISABLE                0x0001
> >   
> > +/* ACPI HOTPLUG STATUS */
> Pls drop this comment, it's not helping.
> And pls don't write comments in all caps.
>
> > +typedef enum {
> > +    ACPI_PCI_HOTPLUG_STATUS = 2,
> > +    ACPI_CPU_HOTPLUG_STATUS = 4,
> > +    ACPI_MEMORY_HOTPLUG_STATUS = 8,
> > +} Acpi_Hotplug_Status;
> > +
> That's not how we name types.
> We either do enum acpi_hotplug_status or
> (preferable) AcpiHotplugStatus.
>
>
> >   /* structs */
> >   typedef struct ACPIPMTimer ACPIPMTimer;
> >   typedef struct ACPIPM1EVT ACPIPM1EVT;
> > @@ -172,6 +179,9 @@ void acpi_gpe_reset(ACPIREGS *ar);
> >   void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val);
> >   uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr);
> >   
> > +void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
> > +                         Acpi_Hotplug_Status status);
> > +
> >   void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
> >   
> >   /* acpi.c */
> > diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> > index 7bbf8a0..b56b5c2 100644
> > --- a/include/hw/acpi/memory_hotplug.h
> > +++ b/include/hw/acpi/memory_hotplug.h
> > @@ -5,8 +5,6 @@
> >   #include "hw/acpi/acpi.h"
> >   #include "migration/vmstate.h"
> >   
> > -#define ACPI_MEMORY_HOTPLUG_STATUS 8
> > -
> >   typedef struct MemStatus {
> >       DeviceState *dimm;
> >       bool is_enabled;
> > diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
> > index efa6ed7..a5a3ac0 100644
> > --- a/include/hw/acpi/pc-hotplug.h
> > +++ b/include/hw/acpi/pc-hotplug.h
> > @@ -16,7 +16,6 @@
> >    * ONLY DEFINEs are permited in this file since it's shared
> >    * between C and ASL code.
> >    */
> > -#define ACPI_CPU_HOTPLUG_STATUS 4
> >   
> >   /* Limit for CPU arch IDs for CPU hotplug. All hotpluggable CPUs should
> >    * have CPUClass.get_arch_id() < ACPI_CPU_HOTPLUG_ID_LIMIT.
> > --
> > 1.9.3
> .