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

[Michael S. Tsirkin]

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.

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