[Qemu-devel] [PATCH 0/3] acpi_piix4: Add CPU eject handling

[Qemu-devel] [PATCH 0/3] acpi_piix4: Add CPU eject handling

[Vasilis Liaskovitis]

This patch series adds support for CPU _EJ0 callback in Seabios and qemu-kvm.
The first patch defines the CPU eject bitmap in Seabios and writes to it
during the callback. The second patch adds empty stub functions to qemu-kvm to
handle the bitmap writes.

The third patch defines the eject method to handle the CPU_DEAD event
in Liu Ping Fan's cpu lifecycle/destruction patchseries, see:
http://patchwork.ozlabs.org/patch/127832/
This ACPI implementation can be used instead of the cpustate virtio/pci device
in the original series.

Vasilis Liaskovitis (2):
  acpi_piix4: Add CPU ejection handling
  acpi_piix4: Call KVM_SETSTATE_VCPU ioctl on cpu ejection

 hw/acpi_piix4.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

-- 
1.7.7.3

[Qemu-devel] [PATCH 1/3][Seabios] Add bitmap for cpu _EJ0 callback

[Vasilis Liaskovitis]

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
---
 src/acpi-dsdt.dsl |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index 7082b65..71d8ac4 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -650,8 +650,15 @@ DefinitionBlock (
             Store(DerefOf(Index(CPON, Arg0)), Local0)
             If (Local0) { Return(0xF) } Else { Return(0x0) }
         }
+        /* CPU eject notify method */
+        OperationRegion(PREJ, SystemIO, 0xaf20, 32)
+        Field (PREJ, ByteAcc, NoLock, Preserve)
+        {
+            PRE, 256
+        }
         Method (CPEJ, 2, NotSerialized) {
             // _EJ0 method - eject callback
+            Store(ShiftLeft(1, Arg0), PRE)
             Sleep(200)
         }
 
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH 1/3][Seabios] Add bitmap for cpu _EJ0 callback

[Kevin O'Connor]

On Fri, Jan 13, 2012 at 12:11:30PM +0100, Vasilis Liaskovitis wrote:
> 
> Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>

The SeaBIOS change is okay with me, but the qemu/kvm change needs to
be accepted first.

[...]
>          Method (CPEJ, 2, NotSerialized) {
>              // _EJ0 method - eject callback
> +            Store(ShiftLeft(1, Arg0), PRE)
>              Sleep(200)
>          }

Is the Sleep() still needed?

-Kevin

Re: [Qemu-devel] [PATCH 1/3][Seabios] Add bitmap for cpu _EJ0 callback

[Vasilis Liaskovitis]

On Fri, Jan 13, 2012 at 07:27:01PM -0500, Kevin O'Connor wrote:
> On Fri, Jan 13, 2012 at 12:11:30PM +0100, Vasilis Liaskovitis wrote:
> > 
> > Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> 
> The SeaBIOS change is okay with me, but the qemu/kvm change needs to
> be accepted first.
> 
> [...]
> >          Method (CPEJ, 2, NotSerialized) {
> >              // _EJ0 method - eject callback
> > +            Store(ShiftLeft(1, Arg0), PRE)
> >              Sleep(200)
> >          }
> 
> Is the Sleep() still needed?

I believe it's unneccesary. I 'll test without it and resend.
thanks,

- Vasilis

Re: [Qemu-devel] [PATCH 1/3][Seabios] Add bitmap for cpu _EJ0 callback

[Vasilis Liaskovitis]

On Fri, Jan 13, 2012 at 07:27:01PM -0500, Kevin O'Connor wrote:
> 
> [...]
> >          Method (CPEJ, 2, NotSerialized) {
> >              // _EJ0 method - eject callback
> > +            Store(ShiftLeft(1, Arg0), PRE)
> >              Sleep(200)
> >          }
>
I have another question here: the PCI _EJO callback seems to return 0x0, but
the CPU _EJ0 doesn't return anything. THe ACPIspec4.0a draft section 6.3.3
mentions that _EJx methods have no return value. Is the above difference
intentional?

thanks,

- Vasilis

Re: [Qemu-devel] [PATCH 1/3][Seabios] Add bitmap for cpu _EJ0 callback

[Kevin O'Connor]

On Thu, Jan 19, 2012 at 03:02:30PM +0100, Vasilis Liaskovitis wrote:
> On Fri, Jan 13, 2012 at 07:27:01PM -0500, Kevin O'Connor wrote:
> > 
> > [...]
> > >          Method (CPEJ, 2, NotSerialized) {
> > >              // _EJ0 method - eject callback
> > > +            Store(ShiftLeft(1, Arg0), PRE)
> > >              Sleep(200)
> > >          }
> >
> I have another question here: the PCI _EJO callback seems to return 0x0, but
> the CPU _EJ0 doesn't return anything. THe ACPIspec4.0a draft section 6.3.3
> mentions that _EJx methods have no return value. Is the above difference
> intentional?

If the spec says it doesn't return anything, but the acpi code is,
it's probably just an error.

-Kevin

[Qemu-devel] [PATCH 2/3] acpi_piix4: Add stub functions for CPU eject callback

[Vasilis Liaskovitis]

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
---
 hw/acpi_piix4.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index d5743b6..8bf30dd 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -37,6 +37,7 @@
 
 #define GPE_BASE 0xafe0
 #define PROC_BASE 0xaf00
+#define PROC_EJ_BASE 0xaf20
 #define GPE_LEN 4
 #define PCI_BASE 0xae00
 #define PCI_EJ_BASE 0xae08
@@ -493,6 +494,17 @@ static void pcihotplug_write(void *opaque, uint32_t addr, uint32_t val)
     PIIX4_DPRINTF("pcihotplug write %x <== %d\n", addr, val);
 }
 
+static uint32_t cpuej_read(void *opaque, uint32_t addr)
+{
+    PIIX4_DPRINTF("cpuej read %x\n", addr);
+    return 0;
+}
+
+static void cpuej_write(void *opaque, uint32_t addr, uint32_t val)
+{
+    PIIX4_DPRINTF("cpuej write %x <== %d\n", addr, val);
+}
+
 static uint32_t pciej_read(void *opaque, uint32_t addr)
 {
     PIIX4_DPRINTF("pciej read %x\n", addr);
@@ -553,6 +565,9 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
     register_ioport_write(PROC_BASE, 32, 1, gpe_writeb, s);
     register_ioport_read(PROC_BASE, 32, 1,  gpe_readb, s);
 
+    register_ioport_write(PROC_EJ_BASE, 32, 1, cpuej_write, s);
+    register_ioport_read(PROC_EJ_BASE, 32, 1,  cpuej_read, s);
+
     register_ioport_write(PCI_BASE, 8, 4, pcihotplug_write, pci0_status);
     register_ioport_read(PCI_BASE, 8, 4,  pcihotplug_read, pci0_status);
 
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH 2/3] acpi_piix4: Add stub functions for CPU eject callback

[Avi Kivity]

On 01/13/2012 01:11 PM, Vasilis Liaskovitis wrote:
> Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> ---
>  hw/acpi_piix4.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index d5743b6..8bf30dd 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -37,6 +37,7 @@
>  
>  #define GPE_BASE 0xafe0
>  #define PROC_BASE 0xaf00
> +#define PROC_EJ_BASE 0xaf20
>

We're adding stuff to piix4 which was never there.  At a minimum this
needs to be documented.  Also needs to be -M pc-1.1 and later only.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/3] acpi_piix4: Add stub functions for CPU eject callback

[Vasilis Liaskovitis]

On Sun, Jan 15, 2012 at 02:38:52PM +0200, Avi Kivity wrote:
> On 01/13/2012 01:11 PM, Vasilis Liaskovitis wrote:
> > Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> > ---
> >  hw/acpi_piix4.c |   15 +++++++++++++++
> >  1 files changed, 15 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> > index d5743b6..8bf30dd 100644
> > --- a/hw/acpi_piix4.c
> > +++ b/hw/acpi_piix4.c
> > @@ -37,6 +37,7 @@
> >  
> >  #define GPE_BASE 0xafe0
> >  #define PROC_BASE 0xaf00
> > +#define PROC_EJ_BASE 0xaf20
> >
> 
> We're adding stuff to piix4 which was never there.  At a minimum this
> needs to be documented.  Also needs to be -M pc-1.1 and later only.

Where should this be documented? PCI/ACPI hotplug addresses are documented in
docs/specs/acpi_pci_hotplug.txt but for CPU hotplug documentation (i.e.
for the existing PROC_BASE) I don't see relevant documentation. I will
create a docs/specs/acpi_cpu_hotplug.txt if that sounds reasonable.

For pc-1.1, a new QEMUmachine type will be needed I assume. Should a check be
made against the machine version in the piix4 code? any relevant examples? 

thanks,

- Vasilis

Re: [Qemu-devel] [PATCH 2/3] acpi_piix4: Add stub functions for CPU eject callback

[Avi Kivity]

On 01/16/2012 01:32 PM, Vasilis Liaskovitis wrote:
> On Sun, Jan 15, 2012 at 02:38:52PM +0200, Avi Kivity wrote:
> > On 01/13/2012 01:11 PM, Vasilis Liaskovitis wrote:
> > > Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> > > ---
> > >  hw/acpi_piix4.c |   15 +++++++++++++++
> > >  1 files changed, 15 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> > > index d5743b6..8bf30dd 100644
> > > --- a/hw/acpi_piix4.c
> > > +++ b/hw/acpi_piix4.c
> > > @@ -37,6 +37,7 @@
> > >  
> > >  #define GPE_BASE 0xafe0
> > >  #define PROC_BASE 0xaf00
> > > +#define PROC_EJ_BASE 0xaf20
> > >
> > 
> > We're adding stuff to piix4 which was never there.  At a minimum this
> > needs to be documented.  Also needs to be -M pc-1.1 and later only.
>
> Where should this be documented? PCI/ACPI hotplug addresses are documented in
> docs/specs/acpi_pci_hotplug.txt 

A pleasant surprise

> but for CPU hotplug documentation (i.e.
> for the existing PROC_BASE) I don't see relevant documentation. I will
> create a docs/specs/acpi_cpu_hotplug.txt if that sounds reasonable.

I suggest renaming it to acpi_hotplug.txt, so it covers both cases.

> For pc-1.1, a new QEMUmachine type will be needed I assume. Should a check be
> made against the machine version in the piix4 code? any relevant examples? 
>

The standard practice is to set a property.  See for example
pc_machine_v0_14 in hw/pc_piix.c, it autosets properties for devices
(erroneously called "drivers" in the code).

btw, I notice the I/O ports are write only and don't remember their
state.  I can't think offhand if there's anything bad about it (in fact
not having state makes live migration more robust), but perhaps someone
else will.


-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH 3/3] acpi_piix4: Call KVM_SETSTATE_VCPU ioctl on cpu ejection

[Vasilis Liaskovitis]

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
---
 hw/acpi_piix4.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 8bf30dd..12eef55 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -502,6 +502,27 @@ static uint32_t cpuej_read(void *opaque, uint32_t addr)
 
 static void cpuej_write(void *opaque, uint32_t addr, uint32_t val)
 {
+    struct kvm_vcpu_state state;
+    CPUState *env;
+    int cpu;
+    int ret;
+
+    cpu = ffs(val);
+    /* zero means no bit was set, i.e. no CPU ejection happened */
+    if (!cpu)
+       return;
+    cpu--;
+    env = cpu_phyid_to_cpu((uint64_t)cpu);
+    if (env != NULL) {
+        if (env->state == CPU_STATE_ZAPREQ) {
+            state.vcpu_id = env->cpu_index;
+            state.state = 1;
+            ret = kvm_vm_ioctl(env->kvm_state, KVM_SETSTATE_VCPU, &state);
+            if (ret)
+                fprintf(stderr, "KVM_SETSTATE_VCPU failed: %s\n",
+                        strerror(ret));
+        }
+    }
     PIIX4_DPRINTF("cpuej write %x <== %d\n", addr, val);
 }
 
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH 3/3] acpi_piix4: Call KVM_SETSTATE_VCPU ioctl on cpu ejection

[Jan Kiszka]

On 2012-01-13 12:11, Vasilis Liaskovitis wrote:
> Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> ---
>  hw/acpi_piix4.c |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 8bf30dd..12eef55 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -502,6 +502,27 @@ static uint32_t cpuej_read(void *opaque, uint32_t addr)
>  
>  static void cpuej_write(void *opaque, uint32_t addr, uint32_t val)
>  {
> +    struct kvm_vcpu_state state;
> +    CPUState *env;
> +    int cpu;
> +    int ret;
> +
> +    cpu = ffs(val);
> +    /* zero means no bit was set, i.e. no CPU ejection happened */
> +    if (!cpu)
> +       return;
> +    cpu--;
> +    env = cpu_phyid_to_cpu((uint64_t)cpu);
> +    if (env != NULL) {
> +        if (env->state == CPU_STATE_ZAPREQ) {
> +            state.vcpu_id = env->cpu_index;
> +            state.state = 1;
> +            ret = kvm_vm_ioctl(env->kvm_state, KVM_SETSTATE_VCPU, &state);

That breaks in the absence of KVM or if it is not enabled.

Also, where was this IOCTL introduced? Where are the linux header changes?

> +            if (ret)
> +                fprintf(stderr, "KVM_SETSTATE_VCPU failed: %s\n",
> +                        strerror(ret));
> +        }
> +    }
>      PIIX4_DPRINTF("cpuej write %x <== %d\n", addr, val);
>  }
>  

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 3/3] acpi_piix4: Call KVM_SETSTATE_VCPU ioctl on cpu ejection

[Vasilis Liaskovitis]

On Fri, Jan 13, 2012 at 12:58:53PM +0100, Jan Kiszka wrote:
> On 2012-01-13 12:11, Vasilis Liaskovitis wrote:
> > Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> > ---
> >  hw/acpi_piix4.c |   21 +++++++++++++++++++++
> >  1 files changed, 21 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> > index 8bf30dd..12eef55 100644
> > --- a/hw/acpi_piix4.c
> > +++ b/hw/acpi_piix4.c
> > @@ -502,6 +502,27 @@ static uint32_t cpuej_read(void *opaque, uint32_t addr)
> >  
> >  static void cpuej_write(void *opaque, uint32_t addr, uint32_t val)
> >  {
> > +    struct kvm_vcpu_state state;
> > +    CPUState *env;
> > +    int cpu;
> > +    int ret;
> > +
> > +    cpu = ffs(val);
> > +    /* zero means no bit was set, i.e. no CPU ejection happened */
> > +    if (!cpu)
> > +       return;
> > +    cpu--;
> > +    env = cpu_phyid_to_cpu((uint64_t)cpu);
> > +    if (env != NULL) {
> > +        if (env->state == CPU_STATE_ZAPREQ) {
> > +            state.vcpu_id = env->cpu_index;
> > +            state.state = 1;
> > +            ret = kvm_vm_ioctl(env->kvm_state, KVM_SETSTATE_VCPU, &state);
> 
> That breaks in the absence of KVM or if it is not enabled.

Right, I will rework.

Do we expect icc-bus related changes on a CPU unplug? This patch does not
handle this yet.

> 
> Also, where was this IOCTL introduced? Where are the linux header changes?


The headers are here:
http://patchwork.ozlabs.org/patch/127834/

And the ioctl is introduced here:
http://patchwork.ozlabs.org/patch/127828/

Though the actual ioctl code seems to have dropped through the cracks in the
above patch. A sample implementation against 3.1.0 is below, but I have not
included it in the patch series. I expect the ioctl implementation to be part
of Liu 's kernel kvm-related series. In any case, this third patch depends on
the cpu zap/lifecycle patchseries and perhaps should be reviewed separately
from the first 2.

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 6d3a724..8dd9ebd 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2095,6 +2095,22 @@ static long kvm_vm_ioctl(struct file *filp,
 		r = kvm_ioeventfd(kvm, &data);
 		break;
 	}
+	case KVM_SETSTATE_VCPU: {
+		struct kvm_vcpu_state vcpu_state;
+		struct kvm_vcpu *vcpu;
+		int idx;
+		r = -EFAULT;
+		if (copy_from_user(&vcpu_state, argp,
+					sizeof(struct kvm_vcpu_state)))
+			goto out;
+		idx = srcu_read_lock(&kvm->srcu);
+		kvm_for_each_vcpu(vcpu, kvm)
+		if (vcpu_state.vcpu_id == vcpu->vcpu_id)
+			vcpu->state = vcpu_state.state;
+		srcu_read_unlock(&kvm->srcu, idx);
+		r = 0;
+		break;
+	}
 #ifdef CONFIG_KVM_APIC_ARCHITECTURE
 	case KVM_SET_BOOT_CPU_ID:
 		r = 0;

Re: [Qemu-devel] [PATCH 0/3] acpi_piix4: Add CPU eject handling

[Jan Kiszka]

On 2012-01-13 12:11, Vasilis Liaskovitis wrote:
> This patch series adds support for CPU _EJ0 callback in Seabios and qemu-kvm.
> The first patch defines the CPU eject bitmap in Seabios and writes to it
> during the callback. The second patch adds empty stub functions to qemu-kvm to
> handle the bitmap writes.

Please work against upstream (uq/master for kvm-related patches), not
qemu-kvm. It possibly makes no technical difference here, but we do not
want to let the code bases needlessly diverge again. If if does make a
difference and upstream lacks further bits, push them first.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 0/3] acpi_piix4: Add CPU eject handling

[Vasilis Liaskovitis]

On Fri, Jan 13, 2012 at 12:58:10PM +0100, Jan Kiszka wrote:
> Please work against upstream (uq/master for kvm-related patches), not
> qemu-kvm. It possibly makes no technical difference here, but we do not
> want to let the code bases needlessly diverge again. If if does make a
> difference and upstream lacks further bits, push them first.

Apologies, I will from now on.

thanks,

- Vasilis