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

[Vasilis Liaskovitis]

Signed-off-by: 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)
}

[Vasilis Liaskovitis]

Signed-off-by: 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);

[Avi Kivity]

Post by Vasilis Liaskovitis
---
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.

[Vasilis Liaskovitis]

Post by Avi Kivity

Post by Vasilis Liaskovitis
---
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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Avi Kivity]

Post by Vasilis Liaskovitis

Post by Avi Kivity

Post by Vasilis Liaskovitis
---
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

Post by Vasilis Liaskovitis
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.

Post by Vasilis Liaskovitis
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.

[Vasilis Liaskovitis]

Signed-off-by: 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);
}

[Jan Kiszka]

Post by Vasilis Liaskovitis
---
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?

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

Jan

[Vasilis Liaskovitis]

Post by Jan Kiszka

Post by Vasilis Liaskovitis
---
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.

Post by Jan Kiszka
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;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Jan Kiszka]

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

[Vasilis Liaskovitis]

Post by Jan Kiszka
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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Kevin O'Connor]

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

[...]

Post by Vasilis Liaskovitis
Method (CPEJ, 2, NotSerialized) {
// _EJ0 method - eject callback
+ Store(ShiftLeft(1, Arg0), PRE)
Sleep(200)
}

Is the Sleep() still needed?

-Kevin
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Vasilis Liaskovitis]

Post by Kevin O'Connor
The SeaBIOS change is okay with me, but the qemu/kvm change needs to
be accepted first.
[...]

Post by Vasilis Liaskovitis
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

[Vasilis Liaskovitis]

[...]

Post by Vasilis Liaskovitis
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Kevin O'Connor]

Post by Vasilis Liaskovitis

[...]

Post by Vasilis Liaskovitis
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html