[Qemu-devel] [PATCH] q35: fix GPE method for cpu hotplug

[Qemu-devel] [PATCH] q35: fix GPE method for cpu hotplug

[Hu Tao]

cpu hotplug uses bit 2 of GPE instead of bit 1.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 src/q35-acpi-dsdt.dsl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/q35-acpi-dsdt.dsl b/src/q35-acpi-dsdt.dsl
index c031d83..417ac48 100644
--- a/src/q35-acpi-dsdt.dsl
+++ b/src/q35-acpi-dsdt.dsl
@@ -415,11 +415,11 @@ DefinitionBlock (
         Method(_L00) {
         }
         Method(_L01) {
+        }
+        Method(_E02) {
             // CPU hotplug event
             \_SB.PRSC()
         }
-        Method(_L02) {
-        }
         Method(_L03) {
         }
         Method(_L04) {
-- 
1.8.1.4

[Qemu-devel] [PATCH 1/2] ich9: update sci on gpe write

[Hu Tao]

OSPM may disable the sci by clearing GPEx_BLK EN bit, in the case
we have to set sci level to 0 or guest will receive sci interrupts
endlessly.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/acpi/ich9.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 3fb443d..8717c15 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -79,6 +79,8 @@ static void ich9_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
 {
     ICH9LPCPMRegs *pm = opaque;
     acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
+
+    pm_update_sci(pm);
 }
 
 static const MemoryRegionOps ich9_gpe_ops = {
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/2] q35: add cpu hotplug support

[Hu Tao]

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/acpi/ich9.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++--
 include/hw/acpi/ich9.h | 11 ++++++
 2 files changed, 100 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 8717c15..146216a 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -43,17 +43,22 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
 #define ICH9_DEBUG(fmt, ...)    do { } while (0)
 #endif
 
+
+#define ICH9_CPU_HOTPLUG_STATUS 4
+
 static void pm_update_sci(ICH9LPCPMRegs *pm)
 {
     int sci_level, pm1a_sts;
 
     pm1a_sts = acpi_pm1_evt_get_sts(&pm->acpi_regs);
 
-    sci_level = (((pm1a_sts & pm->acpi_regs.pm1.evt.en) &
+    sci_level = ((((pm1a_sts & pm->acpi_regs.pm1.evt.en) &
                   (ACPI_BITMASK_RT_CLOCK_ENABLE |
                    ACPI_BITMASK_POWER_BUTTON_ENABLE |
                    ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
-                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
+                   ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
+                 (((pm->acpi_regs.gpe.sts[0] & pm->acpi_regs.gpe.en[0]) &
+          ICH9_CPU_HOTPLUG_STATUS) != 0));
     qemu_set_irq(pm->irq, sci_level);
 
     /* schedule a timer interruption if needed */
@@ -93,6 +98,80 @@ static const MemoryRegionOps ich9_gpe_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    ICH9LPCPMRegs *pm = opaque;
+    CPUStatus *cpus = &pm->gpe_cpu;
+    uint64_t val = cpus->sts[addr];
+
+    ICH9_DEBUG("addr: %" HWADDR_PRIx ", val: %" PRIx64 "\n", addr, val);
+
+    return val;
+}
+
+static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
+                             unsigned int size)
+{
+    /* TODO: implement VCPU removal on guest signal that CPU can be removed */
+}
+
+static const MemoryRegionOps cpu_hotplug_ops = {
+    .read = cpu_status_read,
+    .write = cpu_status_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+typedef enum {
+    PLUG,
+    UNPLUG,
+} HotplugEventType;
+
+static void ich9_cpu_hotplug_req(ICH9LPCPMRegs *pm, CPUState *cpu,
+                                 HotplugEventType action)
+{
+    CPUStatus *g = &pm->gpe_cpu;
+    ACPIGPE *gpe = &pm->acpi_regs.gpe;
+    CPUClass *k = CPU_GET_CLASS(cpu);
+    int64_t cpu_id;
+
+    assert(pm != NULL);
+
+    *gpe->sts = *gpe->sts | ICH9_CPU_HOTPLUG_STATUS;
+    cpu_id = k->get_arch_id(CPU(cpu));
+    if (action == PLUG) {
+        g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
+    } else {
+        g->sts[cpu_id / 8] &= ~(1 << (cpu_id % 8));
+    }
+
+    ICH9_DEBUG("cpu_id: %"PRIx64", action: %s\n", cpu_id,
+               action == PLUG ? "PLUG" : "UNPLUG");
+
+    pm_update_sci(pm);
+}
+
+static void ich9_cpu_added_req(Notifier *n, void *opaque)
+{
+    ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, cpu_added_notifier);
+
+    ich9_cpu_hotplug_req(pm, CPU(opaque), PLUG);
+}
+
+static void ich9_init_cpu_status(CPUState *cpu, void *data)
+{
+    CPUStatus *g = (CPUStatus *)data;
+    CPUClass *k = CPU_GET_CLASS(cpu);
+    int64_t id = k->get_arch_id(cpu);
+
+    g_assert((id / 8) < ICH9_PROC_LEN);
+    g->sts[id / 8] |= (1 << (id % 8));
+}
+
+
 static uint64_t ich9_smi_readl(void *opaque, hwaddr addr, unsigned width)
 {
     ICH9LPCPMRegs *pm = opaque;
@@ -221,6 +300,12 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
                           "apci-gpe0", ICH9_PMIO_GPE0_LEN);
     memory_region_add_subregion(&pm->io, ICH9_PMIO_GPE0_STS, &pm->io_gpe);
 
+    qemu_for_each_cpu(ich9_init_cpu_status, &pm->gpe_cpu);
+    memory_region_init_io(&pm->io_cpu, OBJECT(lpc_pci), &cpu_hotplug_ops, pm,
+                          "acpi-cpu-hotplug", ICH9_PROC_LEN);
+    memory_region_add_subregion(pci_address_space_io(lpc_pci), ICH9_PROC_BASE,
+                                &pm->io_cpu);
+
     memory_region_init_io(&pm->io_smi, OBJECT(lpc_pci), &ich9_smi_ops, pm,
                           "apci-smi", 8);
     memory_region_add_subregion(&pm->io, ICH9_PMIO_SMI_EN, &pm->io_smi);
@@ -229,4 +314,6 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
     qemu_register_reset(pm_reset, pm);
     pm->powerdown_notifier.notify = pm_powerdown_req;
     qemu_register_powerdown_notifier(&pm->powerdown_notifier);
+    pm->cpu_added_notifier.notify = ich9_cpu_added_req;
+    qemu_register_cpu_added_notifier(&pm->cpu_added_notifier);
 }
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index b1fe71f..bac70c6 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -23,6 +23,13 @@
 
 #include "hw/acpi/acpi.h"
 
+#define ICH9_PROC_BASE 0xaf00
+#define ICH9_PROC_LEN 32
+
+typedef struct CPUStatus {
+    uint8_t sts[ICH9_PROC_LEN];
+} CPUStatus;
+
 typedef struct ICH9LPCPMRegs {
     /*
      * In ich9 spec says that pm1_cnt register is 32bit width and
@@ -31,8 +38,11 @@ typedef struct ICH9LPCPMRegs {
      */
     ACPIREGS acpi_regs;
 
+    CPUStatus gpe_cpu;
+
     MemoryRegion io;
     MemoryRegion io_gpe;
+    MemoryRegion io_cpu;
     MemoryRegion io_smi;
 
     uint32_t smi_en;
@@ -42,6 +52,7 @@ typedef struct ICH9LPCPMRegs {
 
     uint32_t pm_io_base;
     Notifier powerdown_notifier;
+    Notifier cpu_added_notifier;
 } ICH9LPCPMRegs;
 
 void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/2] q35: add cpu hotplug support

[Hu Tao]

Added: Igor Mammedov <imammedo@redhat.com>

On Wed, Aug 21, 2013 at 05:04:28PM +0800, Hu Tao wrote:
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/acpi/ich9.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  include/hw/acpi/ich9.h | 11 ++++++
>  2 files changed, 100 insertions(+), 2 deletions(-)

Hi Igor,

Sorry I forgot to add you.

Re: [Qemu-devel] [PATCH 2/2] q35: add cpu hotplug support

[Gerd Hoffmann]

  Hi,

> +#define ICH9_PROC_BASE 0xaf00
> +#define ICH9_PROC_LEN 32

No, please don't.  It makes it impossible to assign the 0xa000 -> 0xafff
I/O port window to a PCI bridge.  Please lets stop occupy random io
ports above 0x1000 and burn I/O address space that way.

I'd suggest to place it at 0x0a00 instead.  Also the dsdt should get a
device with the address in _CRS so the guest knows those ports are used.

cheers,
  Gerd

Re: [Qemu-devel] [SeaBIOS] [PATCH] q35: fix GPE method for cpu hotplug

[Gerd Hoffmann]

  Hi,

>          Method(_L01) {
> +        }
> +        Method(_E02) {
>              // CPU hotplug event
>              \_SB.PRSC()
>          }
> -        Method(_L02) {
> -        }

E02?  Typo?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/2] q35: add cpu hotplug support

[Paolo Bonzini]

Il 21/08/2013 13:01, Gerd Hoffmann ha scritto:
>   Hi,
> 
>> +#define ICH9_PROC_BASE 0xaf00
>> +#define ICH9_PROC_LEN 32
> 
> No, please don't.  It makes it impossible to assign the 0xa000 -> 0xafff
> I/O port window to a PCI bridge.  Please lets stop occupy random io
> ports above 0x1000 and burn I/O address space that way.
> 
> I'd suggest to place it at 0x0a00 instead.  Also the dsdt should get a
> device with the address in _CRS so the guest knows those ports are used.

Would this use 0x0A ("not present, device functional, decoding
resources") for _STA to "indicate a valid device for which no device
driver should be loaded" (quoting from the ACPI spec)?  Hopefully this
prevents Windows from showing the UI.

Paolo

Re: [Qemu-devel] [PATCH 2/2] q35: add cpu hotplug support

[Gerd Hoffmann]

On Mi, 2013-08-21 at 13:27 +0200, Paolo Bonzini wrote:
> Il 21/08/2013 13:01, Gerd Hoffmann ha scritto:
> >   Hi,
> > 
> >> +#define ICH9_PROC_BASE 0xaf00
> >> +#define ICH9_PROC_LEN 32
> > 
> > No, please don't.  It makes it impossible to assign the 0xa000 -> 0xafff
> > I/O port window to a PCI bridge.  Please lets stop occupy random io
> > ports above 0x1000 and burn I/O address space that way.
> > 
> > I'd suggest to place it at 0x0a00 instead.  Also the dsdt should get a
> > device with the address in _CRS so the guest knows those ports are used.
> 
> Would this use 0x0A ("not present, device functional, decoding
> resources") for _STA to "indicate a valid device for which no device
> driver should be loaded" (quoting from the ACPI spec)?  Hopefully this
> prevents Windows from showing the UI.

Sounds reasonable.  Needs careful testing ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/2] q35: add cpu hotplug support

[Andreas Färber]

Am 21.08.2013 11:04, schrieb Hu Tao:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/acpi/ich9.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  include/hw/acpi/ich9.h | 11 ++++++
>  2 files changed, 100 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 8717c15..146216a 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -43,17 +43,22 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
>  #define ICH9_DEBUG(fmt, ...)    do { } while (0)
>  #endif
>  
> +
> +#define ICH9_CPU_HOTPLUG_STATUS 4
> +
>  static void pm_update_sci(ICH9LPCPMRegs *pm)
>  {
>      int sci_level, pm1a_sts;
>  
>      pm1a_sts = acpi_pm1_evt_get_sts(&pm->acpi_regs);
>  
> -    sci_level = (((pm1a_sts & pm->acpi_regs.pm1.evt.en) &
> +    sci_level = ((((pm1a_sts & pm->acpi_regs.pm1.evt.en) &
>                    (ACPI_BITMASK_RT_CLOCK_ENABLE |
>                     ACPI_BITMASK_POWER_BUTTON_ENABLE |
>                     ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
> -                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
> +                   ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
> +                 (((pm->acpi_regs.gpe.sts[0] & pm->acpi_regs.gpe.en[0]) &
> +          ICH9_CPU_HOTPLUG_STATUS) != 0));
>      qemu_set_irq(pm->irq, sci_level);
>  
>      /* schedule a timer interruption if needed */
> @@ -93,6 +98,80 @@ static const MemoryRegionOps ich9_gpe_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    CPUStatus *cpus = &pm->gpe_cpu;
> +    uint64_t val = cpus->sts[addr];
> +
> +    ICH9_DEBUG("addr: %" HWADDR_PRIx ", val: %" PRIx64 "\n", addr, val);
> +
> +    return val;
> +}
> +
> +static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
> +                             unsigned int size)
> +{
> +    /* TODO: implement VCPU removal on guest signal that CPU can be removed */
> +}
> +
> +static const MemoryRegionOps cpu_hotplug_ops = {
> +    .read = cpu_status_read,
> +    .write = cpu_status_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +typedef enum {
> +    PLUG,
> +    UNPLUG,
> +} HotplugEventType;
> +
> +static void ich9_cpu_hotplug_req(ICH9LPCPMRegs *pm, CPUState *cpu,
> +                                 HotplugEventType action)
> +{
> +    CPUStatus *g = &pm->gpe_cpu;
> +    ACPIGPE *gpe = &pm->acpi_regs.gpe;
> +    CPUClass *k = CPU_GET_CLASS(cpu);

cc please. (c is not a reserved symbol, and other classes such as
X86CPUClass could be used in the future.)

> +    int64_t cpu_id;
> +
> +    assert(pm != NULL);
> +
> +    *gpe->sts = *gpe->sts | ICH9_CPU_HOTPLUG_STATUS;
> +    cpu_id = k->get_arch_id(CPU(cpu));
> +    if (action == PLUG) {
> +        g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
> +    } else {
> +        g->sts[cpu_id / 8] &= ~(1 << (cpu_id % 8));
> +    }
> +
> +    ICH9_DEBUG("cpu_id: %"PRIx64", action: %s\n", cpu_id,
> +               action == PLUG ? "PLUG" : "UNPLUG");
> +
> +    pm_update_sci(pm);
> +}
> +
> +static void ich9_cpu_added_req(Notifier *n, void *opaque)
> +{
> +    ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, cpu_added_notifier);
> +
> +    ich9_cpu_hotplug_req(pm, CPU(opaque), PLUG);
> +}
> +
> +static void ich9_init_cpu_status(CPUState *cpu, void *data)
> +{
> +    CPUStatus *g = (CPUStatus *)data;
> +    CPUClass *k = CPU_GET_CLASS(cpu);

cc

> +    int64_t id = k->get_arch_id(cpu);
> +
> +    g_assert((id / 8) < ICH9_PROC_LEN);
> +    g->sts[id / 8] |= (1 << (id % 8));
> +}
> +
> +
>  static uint64_t ich9_smi_readl(void *opaque, hwaddr addr, unsigned width)
>  {
>      ICH9LPCPMRegs *pm = opaque;
> @@ -221,6 +300,12 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>                            "apci-gpe0", ICH9_PMIO_GPE0_LEN);
>      memory_region_add_subregion(&pm->io, ICH9_PMIO_GPE0_STS, &pm->io_gpe);
>  
> +    qemu_for_each_cpu(ich9_init_cpu_status, &pm->gpe_cpu);

Please don't copy this from i440fx, there are patches on the list
dropping qemu_for_each_cpu(). Just use a for loop until they hit master
- still waiting for reviews.

Regards,
Andreas

> +    memory_region_init_io(&pm->io_cpu, OBJECT(lpc_pci), &cpu_hotplug_ops, pm,
> +                          "acpi-cpu-hotplug", ICH9_PROC_LEN);
> +    memory_region_add_subregion(pci_address_space_io(lpc_pci), ICH9_PROC_BASE,
> +                                &pm->io_cpu);
> +
>      memory_region_init_io(&pm->io_smi, OBJECT(lpc_pci), &ich9_smi_ops, pm,
>                            "apci-smi", 8);
>      memory_region_add_subregion(&pm->io, ICH9_PMIO_SMI_EN, &pm->io_smi);
> @@ -229,4 +314,6 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>      qemu_register_reset(pm_reset, pm);
>      pm->powerdown_notifier.notify = pm_powerdown_req;
>      qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> +    pm->cpu_added_notifier.notify = ich9_cpu_added_req;
> +    qemu_register_cpu_added_notifier(&pm->cpu_added_notifier);
>  }
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index b1fe71f..bac70c6 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -23,6 +23,13 @@
>  
>  #include "hw/acpi/acpi.h"
>  
> +#define ICH9_PROC_BASE 0xaf00
> +#define ICH9_PROC_LEN 32
> +
> +typedef struct CPUStatus {
> +    uint8_t sts[ICH9_PROC_LEN];
> +} CPUStatus;
> +
>  typedef struct ICH9LPCPMRegs {
>      /*
>       * In ich9 spec says that pm1_cnt register is 32bit width and
> @@ -31,8 +38,11 @@ typedef struct ICH9LPCPMRegs {
>       */
>      ACPIREGS acpi_regs;
>  
> +    CPUStatus gpe_cpu;
> +
>      MemoryRegion io;
>      MemoryRegion io_gpe;
> +    MemoryRegion io_cpu;
>      MemoryRegion io_smi;
>  
>      uint32_t smi_en;
> @@ -42,6 +52,7 @@ typedef struct ICH9LPCPMRegs {
>  
>      uint32_t pm_io_base;
>      Notifier powerdown_notifier;
> +    Notifier cpu_added_notifier;
>  } ICH9LPCPMRegs;
>  
>  void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [SeaBIOS] [PATCH] q35: fix GPE method for cpu hotplug

[Igor Mammedov]

On Wed, 21 Aug 2013 13:05:27 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> >          Method(_L01) {
> > +        }
> > +        Method(_E02) {
> >              // CPU hotplug event
> >              \_SB.PRSC()
> >          }
> > -        Method(_L02) {
> > -        }
> 
> E02?  Typo?

_E02 is correct. see commit 9c6635bd4 for reasoning.
Perhaps commit message should be more verbose and contain
a reference to that commit.

> 
> cheers,
>   Gerd
> 
> 
> 

Re: [Qemu-devel] [PATCH 1/2] ich9: update sci on gpe write

[Igor Mammedov]

On Wed, 21 Aug 2013 17:04:27 +0800
Hu Tao <hutao@cn.fujitsu.com> wrote:

> OSPM may disable the sci by clearing GPEx_BLK EN bit, in the case
> we have to set sci level to 0 or guest will receive sci interrupts
> endlessly.

Could you make a more verbose comment, referring to relevant ACPI spec chapter
and it would be nice, if you experienced problem with linux guest, to add
symptoms here as well.

commit 633aa0ac did equivalent change to piix4 part, so it's worth to mention
it here.

> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/acpi/ich9.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 3fb443d..8717c15 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -79,6 +79,8 @@ static void ich9_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
>  {
>      ICH9LPCPMRegs *pm = opaque;
>      acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
> +
> +    pm_update_sci(pm);
>  }
>  
>  static const MemoryRegionOps ich9_gpe_ops = {

Re: [Qemu-devel] [PATCH 2/2] q35: add cpu hotplug support

[Igor Mammedov]

On Wed, 21 Aug 2013 17:04:28 +0800
Hu Tao <hutao@cn.fujitsu.com> wrote:

> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/acpi/ich9.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  include/hw/acpi/ich9.h | 11 ++++++
>  2 files changed, 100 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 8717c15..146216a 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -43,17 +43,22 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
>  #define ICH9_DEBUG(fmt, ...)    do { } while (0)
>  #endif
>  
> +
> +#define ICH9_CPU_HOTPLUG_STATUS 4
> +
>  static void pm_update_sci(ICH9LPCPMRegs *pm)
>  {
>      int sci_level, pm1a_sts;
>  
>      pm1a_sts = acpi_pm1_evt_get_sts(&pm->acpi_regs);
>  
> -    sci_level = (((pm1a_sts & pm->acpi_regs.pm1.evt.en) &
> +    sci_level = ((((pm1a_sts & pm->acpi_regs.pm1.evt.en) &
>                    (ACPI_BITMASK_RT_CLOCK_ENABLE |
>                     ACPI_BITMASK_POWER_BUTTON_ENABLE |
>                     ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
> -                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
> +                   ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
> +                 (((pm->acpi_regs.gpe.sts[0] & pm->acpi_regs.gpe.en[0]) &
> +          ICH9_CPU_HOTPLUG_STATUS) != 0));
>      qemu_set_irq(pm->irq, sci_level);
>  
>      /* schedule a timer interruption if needed */
> @@ -93,6 +98,80 @@ static const MemoryRegionOps ich9_gpe_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    CPUStatus *cpus = &pm->gpe_cpu;
> +    uint64_t val = cpus->sts[addr];
> +
> +    ICH9_DEBUG("addr: %" HWADDR_PRIx ", val: %" PRIx64 "\n", addr, val);
> +
> +    return val;
> +}
> +
> +static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
> +                             unsigned int size)
> +{
> +    /* TODO: implement VCPU removal on guest signal that CPU can be removed */
> +}
> +
> +static const MemoryRegionOps cpu_hotplug_ops = {
> +    .read = cpu_status_read,
> +    .write = cpu_status_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +typedef enum {
> +    PLUG,
> +    UNPLUG,
> +} HotplugEventType;
> +
> +static void ich9_cpu_hotplug_req(ICH9LPCPMRegs *pm, CPUState *cpu,
> +                                 HotplugEventType action)
> +{
> +    CPUStatus *g = &pm->gpe_cpu;
> +    ACPIGPE *gpe = &pm->acpi_regs.gpe;
> +    CPUClass *k = CPU_GET_CLASS(cpu);
> +    int64_t cpu_id;
> +
> +    assert(pm != NULL);
> +
> +    *gpe->sts = *gpe->sts | ICH9_CPU_HOTPLUG_STATUS;
> +    cpu_id = k->get_arch_id(CPU(cpu));
> +    if (action == PLUG) {
> +        g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
> +    } else {
> +        g->sts[cpu_id / 8] &= ~(1 << (cpu_id % 8));
> +    }
> +
> +    ICH9_DEBUG("cpu_id: %"PRIx64", action: %s\n", cpu_id,
> +               action == PLUG ? "PLUG" : "UNPLUG");
> +
> +    pm_update_sci(pm);
> +}
> +
> +static void ich9_cpu_added_req(Notifier *n, void *opaque)
> +{
> +    ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, cpu_added_notifier);
> +
> +    ich9_cpu_hotplug_req(pm, CPU(opaque), PLUG);
> +}
> +
> +static void ich9_init_cpu_status(CPUState *cpu, void *data)
> +{
> +    CPUStatus *g = (CPUStatus *)data;
> +    CPUClass *k = CPU_GET_CLASS(cpu);
> +    int64_t id = k->get_arch_id(cpu);
> +
> +    g_assert((id / 8) < ICH9_PROC_LEN);
> +    g->sts[id / 8] |= (1 << (id % 8));
> +}
> +
> +
>  static uint64_t ich9_smi_readl(void *opaque, hwaddr addr, unsigned width)
>  {
>      ICH9LPCPMRegs *pm = opaque;
> @@ -221,6 +300,12 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>                            "apci-gpe0", ICH9_PMIO_GPE0_LEN);
>      memory_region_add_subregion(&pm->io, ICH9_PMIO_GPE0_STS, &pm->io_gpe);
>  
> +    qemu_for_each_cpu(ich9_init_cpu_status, &pm->gpe_cpu);
> +    memory_region_init_io(&pm->io_cpu, OBJECT(lpc_pci), &cpu_hotplug_ops, pm,
> +                          "acpi-cpu-hotplug", ICH9_PROC_LEN);
> +    memory_region_add_subregion(pci_address_space_io(lpc_pci), ICH9_PROC_BASE,
> +                                &pm->io_cpu);
> +
>      memory_region_init_io(&pm->io_smi, OBJECT(lpc_pci), &ich9_smi_ops, pm,
>                            "apci-smi", 8);
>      memory_region_add_subregion(&pm->io, ICH9_PMIO_SMI_EN, &pm->io_smi);
> @@ -229,4 +314,6 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>      qemu_register_reset(pm_reset, pm);
>      pm->powerdown_notifier.notify = pm_powerdown_req;
>      qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> +    pm->cpu_added_notifier.notify = ich9_cpu_added_req;
> +    qemu_register_cpu_added_notifier(&pm->cpu_added_notifier);
>  }
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index b1fe71f..bac70c6 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -23,6 +23,13 @@
>  
>  #include "hw/acpi/acpi.h"
>  
> +#define ICH9_PROC_BASE 0xaf00
> +#define ICH9_PROC_LEN 32
> +
> +typedef struct CPUStatus {
> +    uint8_t sts[ICH9_PROC_LEN];
> +} CPUStatus;
> +
>  typedef struct ICH9LPCPMRegs {
>      /*
>       * In ich9 spec says that pm1_cnt register is 32bit width and
> @@ -31,8 +38,11 @@ typedef struct ICH9LPCPMRegs {
>       */
>      ACPIREGS acpi_regs;
>  
> +    CPUStatus gpe_cpu;
> +
>      MemoryRegion io;
>      MemoryRegion io_gpe;
> +    MemoryRegion io_cpu;
>      MemoryRegion io_smi;
>  
>      uint32_t smi_en;
> @@ -42,6 +52,7 @@ typedef struct ICH9LPCPMRegs {
>  
>      uint32_t pm_io_base;
>      Notifier powerdown_notifier;
> +    Notifier cpu_added_notifier;
>  } ICH9LPCPMRegs;
>  
>  void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,

new code mostly duplicates piix4 parts. Could we unify it and move it into separate file or into core.c?

Re: [Qemu-devel] [PATCH 1/2] ich9: update sci on gpe write

[Hu Tao]

On Thu, Sep 12, 2013 at 05:22:14PM +0200, Igor Mammedov wrote:
> On Wed, 21 Aug 2013 17:04:27 +0800
> Hu Tao <hutao@cn.fujitsu.com> wrote:
> 
> > OSPM may disable the sci by clearing GPEx_BLK EN bit, in the case
> > we have to set sci level to 0 or guest will receive sci interrupts
> > endlessly.
> 
> Could you make a more verbose comment, referring to relevant ACPI spec chapter
> and it would be nice,

See Section 5.6.4, ACPI 5.0, page 221.

>                       if you experienced problem with linux guest, to add
> symptoms here as well.

See http://lists.gnu.org/archive/html/qemu-devel/2012-12/msg02711.html.
I guess without this patch the same thing will happen in guest linux
when hotplugging a vcpu.

> 
> commit 633aa0ac did equivalent change to piix4 part, so it's worth to mention
> it here.
> 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  hw/acpi/ich9.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 3fb443d..8717c15 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -79,6 +79,8 @@ static void ich9_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
> >  {
> >      ICH9LPCPMRegs *pm = opaque;
> >      acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
> > +
> > +    pm_update_sci(pm);
> >  }
> >  
> >  static const MemoryRegionOps ich9_gpe_ops = {

Re: [Qemu-devel] [PATCH 2/2] q35: add cpu hotplug support

[Igor Mammedov]

On Wed, 21 Aug 2013 13:01:32 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > +#define ICH9_PROC_BASE 0xaf00
> > +#define ICH9_PROC_LEN 32
> 
> No, please don't.  It makes it impossible to assign the 0xa000 -> 0xafff
> I/O port window to a PCI bridge.  Please lets stop occupy random io
> ports above 0x1000 and burn I/O address space that way.
I'm curios why 0x1000 is any better than 0xa000, it still would be random
port occupation. Is there any guideline which ports could be used and which
shouldn't?

Could we safely move PIIX CPU hotplug 0xaf00-0xaf20 range below 0x1000?

> 
> I'd suggest to place it at 0x0a00 instead.  Also the dsdt should get a
> device with the address in _CRS so the guest knows those ports are used.


> 
> cheers,
>   Gerd
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH 2/2] q35: add cpu hotplug support

[Gerd Hoffmann]

On Fr, 2013-10-04 at 15:20 +0200, Igor Mammedov wrote:
> On Wed, 21 Aug 2013 13:01:32 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> >   Hi,
> > 
> > > +#define ICH9_PROC_BASE 0xaf00
> > > +#define ICH9_PROC_LEN 32
> > 
> > No, please don't.  It makes it impossible to assign the 0xa000 -> 0xafff
> > I/O port window to a PCI bridge.  Please lets stop occupy random io
> > ports above 0x1000 and burn I/O address space that way.
> I'm curios why 0x1000 is any better than 0xa000, it still would be random
> port occupation. Is there any guideline which ports could be used and which
> shouldn't?

0x1000 doesn't by us anything.  *below* 0x1000 gives us one more pci io
window which we can assign to a pci bridge then.

> Could we safely move PIIX CPU hotplug 0xaf00-0xaf20 range below 0x1000?

I'd love to, but unfortunaly it isn't that easy as the address is
hard-coded in the DSDT.

Once mst acpi generation patches are in it would be a bit simpler, but
we would still break compatibility with seabios versions not fetching
the acpi tables from qemu.

cheers,
  Gerd