Patchwork [5/7,v6] introduce a new qom device to deal with panicked event

login
register
mail settings
Submitter Wen Congyang
Date July 6, 2012, 9:41 a.m.
Message ID <4FF6B2AC.9010204@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/169401/
State New
Headers show

Comments

Wen Congyang - July 6, 2012, 9:41 a.m.
If the target is x86/x86_64, the guest's kernel will write 0x01 to the
port KVM_PV_PORT when it is panciked. This patch introduces a new qom
device kvm_pv_ioport to listen this I/O port, and deal with panicked
event according to panicked_action's value. The possible actions are:
1. emit QEVENT_GUEST_PANICKED only
2. emit QEVENT_GUEST_PANICKED and pause the guest
3. emit QEVENT_GUEST_PANICKED and poweroff the guest
4. emit QEVENT_GUEST_PANICKED and reset the guest

I/O ports does not work for some targets(for example: s390). And you
can implement another qom device, and include it's code into pv_event.c
for such target.

Note: if we emit QEVENT_GUEST_PANICKED only, and the management
application does not receive this event(the management may not
run when the event is emitted), the management won't know the
guest is panicked.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 hw/kvm/Makefile.objs |    2 +-
 hw/kvm/pv_event.c    |   73 +++++++++++++++++++++++++++
 hw/kvm/pv_ioport.c   |  133 ++++++++++++++++++++++++++++++++++++++++++++++++++
 kvm-stub.c           |    9 +++
 kvm.h                |    3 +
 vl.c                 |    4 ++
 6 files changed, 223 insertions(+), 1 deletions(-)
 create mode 100644 hw/kvm/pv_event.c
 create mode 100644 hw/kvm/pv_ioport.c
Jan Kiszka - July 6, 2012, 11:05 a.m.
On 2012-07-06 11:41, Wen Congyang wrote:
> If the target is x86/x86_64, the guest's kernel will write 0x01 to the
> port KVM_PV_PORT when it is panciked. This patch introduces a new qom
> device kvm_pv_ioport to listen this I/O port, and deal with panicked
> event according to panicked_action's value. The possible actions are:
> 1. emit QEVENT_GUEST_PANICKED only
> 2. emit QEVENT_GUEST_PANICKED and pause the guest
> 3. emit QEVENT_GUEST_PANICKED and poweroff the guest
> 4. emit QEVENT_GUEST_PANICKED and reset the guest
> 
> I/O ports does not work for some targets(for example: s390). And you
> can implement another qom device, and include it's code into pv_event.c
> for such target.
> 
> Note: if we emit QEVENT_GUEST_PANICKED only, and the management
> application does not receive this event(the management may not
> run when the event is emitted), the management won't know the
> guest is panicked.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  hw/kvm/Makefile.objs |    2 +-
>  hw/kvm/pv_event.c    |   73 +++++++++++++++++++++++++++
>  hw/kvm/pv_ioport.c   |  133 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  kvm-stub.c           |    9 +++
>  kvm.h                |    3 +
>  vl.c                 |    4 ++
>  6 files changed, 223 insertions(+), 1 deletions(-)
>  create mode 100644 hw/kvm/pv_event.c
>  create mode 100644 hw/kvm/pv_ioport.c
> 
> diff --git a/hw/kvm/Makefile.objs b/hw/kvm/Makefile.objs
> index 226497a..23e3b30 100644
> --- a/hw/kvm/Makefile.objs
> +++ b/hw/kvm/Makefile.objs
> @@ -1 +1 @@
> -obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o
> +obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o pv_event.o
> diff --git a/hw/kvm/pv_event.c b/hw/kvm/pv_event.c
> new file mode 100644
> index 0000000..d7ded37
> --- /dev/null
> +++ b/hw/kvm/pv_event.c
> @@ -0,0 +1,73 @@
> +/*
> + * QEMU KVM support, paravirtual event device
> + *
> + * Copyright Fujitsu, Corp. 2012
> + *
> + * Authors:
> + *     Wen Congyang <wency@cn.fujitsu.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <linux/kvm_para.h>
> +#include <asm/kvm_para.h>
> +#include <qobject.h>
> +#include <qjson.h>
> +#include <monitor.h>
> +#include <sysemu.h>
> +#include <kvm.h>
> +
> +/* Possible values for action parameter. */
> +#define PANICKED_REPORT     1   /* emit QEVENT_GUEST_PANICKED only */
> +#define PANICKED_PAUSE      2   /* emit QEVENT_GUEST_PANICKED and pause VM */
> +#define PANICKED_POWEROFF   3   /* emit QEVENT_GUEST_PANICKED and quit VM */
> +#define PANICKED_RESET      4   /* emit QEVENT_GUEST_PANICKED and reset VM */
> +
> +static int panicked_action = PANICKED_REPORT;

Avoid global variables please when there are device states. This one is
unneeded anyway (and will generate warnings when build without KVM_PV_PORT).

> +
> +static void panicked_mon_event(const char *action)
> +{
> +    QObject *data;
> +
> +    data = qobject_from_jsonf("{ 'action': %s }", action);
> +    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
> +    qobject_decref(data);
> +}
> +
> +static void panicked_perform_action(uint32_t panicked_action)
> +{
> +    switch (panicked_action) {
> +    case PANICKED_REPORT:
> +        panicked_mon_event("report");
> +        break;
> +
> +    case PANICKED_PAUSE:
> +        panicked_mon_event("pause");
> +        vm_stop(RUN_STATE_GUEST_PANICKED);
> +        break;
> +
> +    case PANICKED_POWEROFF:
> +        panicked_mon_event("poweroff");
> +        exit(0);

We have qemu_system_shutdown_request.

> +        break;
> +    case PANICKED_RESET:
> +        panicked_mon_event("reset");
> +        qemu_system_reset_request();
> +        break;
> +    }
> +}
> +
> +#if defined(KVM_PV_PORT)
> +#include "pv_ioport.c"
> +
> +void kvm_pv_event_init(void)
> +{
> +    pv_ioport_init(panicked_action);
> +}
> +#else
> +void kvm_pv_event_init(void)
> +{
> +}
> +#endif

Generally, the split-up of handling and transport layer is a good idea
to allow other arch to support this interface. However, its current form
is a bit unfortunate as it does not properly separate the logic of the
events (so far only panic action) from the transport mechanism (PIO) and
as it registers the transport as a configurable device, not the event
handler. Make sure that pv_ioport only deals with registering against
the right bus and forwarding of the PV gate accesses to the event
handling layer. Device name and properties should be defined by the
event layer as well (but then registered by the transport layer).

> diff --git a/hw/kvm/pv_ioport.c b/hw/kvm/pv_ioport.c
> new file mode 100644
> index 0000000..e93d819
> --- /dev/null
> +++ b/hw/kvm/pv_ioport.c
> @@ -0,0 +1,133 @@
> +/*
> + * QEMU KVM support, paravirtual I/O port device
> + *
> + * Copyright Fujitsu, Corp. 2012
> + *
> + * Authors:
> + *     Wen Congyang <wency@cn.fujitsu.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "hw/isa.h"
> +
> +typedef struct {
> +    ISADevice dev;
> +    MemoryRegion ioport;
> +    uint32_t panicked_action;

As explained above, this layer should not know about things like
"panicked_action".

> +} PVState;
> +
> +static PVState *pv_state;
> +
> +static uint64_t pv_io_read(void *opaque, target_phys_addr_t addr, unsigned size)
> +{
> +    return 1 << KVM_PV_FEATURE_PANICKED;
> +}
> +
> +static void pv_io_write(void *opaque, target_phys_addr_t addr, uint64_t val,
> +                        unsigned size)
> +{
> +    PVState *s = opaque;
> +
> +    if (val == KVM_PV_PANICKED) {
> +        panicked_perform_action(s->panicked_action);
> +    }
> +}
> +
> +static const MemoryRegionOps pv_io_ops = {
> +    .read = pv_io_read,
> +    .write = pv_io_write,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +static int pv_ioport_initfn(ISADevice *dev)
> +{
> +    PVState *s = DO_UPCAST(PVState, dev, dev);
> +
> +    memory_region_init_io(&s->ioport, &pv_io_ops, s, "pv_event", 1);
> +    isa_register_ioport(dev, &s->ioport, KVM_PV_PORT);
> +
> +    pv_state = s;
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription pv_ioport_vmsd = {
> +    .name = "pv_ioport",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(panicked_action, PVState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};

Unneeded as panicked_action is a host-side property, not a
guest-changeable state. Your device is stateless, thus has no vmstate.

> +
> +static Property pv_ioport_properties[] = {
> +    DEFINE_PROP_UINT32("panicked_action", PVState, panicked_action, PANICKED_REPORT),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pv_ioport_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
> +
> +    ic->init = pv_ioport_initfn;
> +    dc->no_user = 1;
> +    dc->vmsd = &pv_ioport_vmsd;
> +    dc->props = pv_ioport_properties;
> +}
> +
> +static TypeInfo pv_ioport_info = {
> +    .name          = "kvm_pv_ioport",
> +    .parent        = TYPE_ISA_DEVICE,
> +    .instance_size = sizeof(PVState),
> +    .class_init    = pv_ioport_class_init,
> +};
> +
> +static void pv_ioport_register_types(void)
> +{
> +    type_register_static(&pv_ioport_info);
> +}
> +
> +type_init(pv_ioport_register_types)
> +
> +static int is_isa_bus(BusState *bus, void *opaque)
> +{
> +    const char *bus_type_name;
> +    ISABus **isa_bus_p = opaque;
> +
> +    bus_type_name = object_class_get_name(bus->obj.class);
> +    if (!strcmp(bus_type_name, TYPE_ISA_BUS)) {
> +        *isa_bus_p = ISA_BUS(&bus->obj);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static ISABus *get_isa_bus(void)
> +{
> +    ISABus *isa_bus = NULL;
> +
> +    qbus_walk_children(sysbus_get_default(), NULL, is_isa_bus, &isa_bus);
> +
> +    return isa_bus;
> +}

Unneeded if the bus is passed on creation from the pc board setup.
That's the official way.

> +
> +static void pv_ioport_init(uint32_t panicked_action)
> +{
> +    ISADevice *dev;
> +    ISABus *bus;
> +
> +    bus = get_isa_bus();
> +    dev = isa_create(bus, "kvm_pv_ioport");
> +    qdev_prop_set_uint32(&dev->qdev, "panicked_action", panicked_action);

Nope, configuration should works via "-global device.property=value".
You likely want to define a special property that translates action
names into enum values, see e.g. the lost tick policy.

> +    qdev_init_nofail(&dev->qdev);
> +}
> diff --git a/kvm-stub.c b/kvm-stub.c
> index ec9a364..a28d078 100644
> --- a/kvm-stub.c
> +++ b/kvm-stub.c
> @@ -151,3 +151,12 @@ int kvm_irqchip_remove_irqfd(KVMState *s, int fd, int virq)
>  {
>      return -ENOSYS;
>  }
> +
> +void kvm_pv_event_init(void)
> +{
> +}
> +
> +int select_panicked_action(const char *p)
> +{
> +    return 0;
> +}

Both will be unneeded.

> diff --git a/kvm.h b/kvm.h
> index 9c7b0ea..1f7c72b 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -218,4 +218,7 @@ void kvm_irqchip_release_virq(KVMState *s, int virq);
>  
>  int kvm_irqchip_add_irqfd(KVMState *s, int fd, int virq);
>  int kvm_irqchip_remove_irqfd(KVMState *s, int fd, int virq);
> +
> +void kvm_pv_event_init(void);
> +int select_panicked_action(const char *p);
>  #endif
> diff --git a/vl.c b/vl.c
> index ea5ef1c..f5cd28d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3622,6 +3622,10 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> +    if (kvm_enabled()) {
> +        kvm_pv_event_init();
> +    }

Initialization is better located in the setup code of the board that
supports this device (here the PC). Very similar to kvm clock.

> +
>      qdev_machine_creation_done();
>  
>      if (rom_load_all() != 0) {
> 

Jan
Wen Congyang - July 18, 2012, 1:54 a.m.
At 07/06/2012 07:05 PM, Jan Kiszka Wrote:
> On 2012-07-06 11:41, Wen Congyang wrote:
>> If the target is x86/x86_64, the guest's kernel will write 0x01 to the
>> port KVM_PV_PORT when it is panciked. This patch introduces a new qom
>> device kvm_pv_ioport to listen this I/O port, and deal with panicked
>> event according to panicked_action's value. The possible actions are:
>> 1. emit QEVENT_GUEST_PANICKED only
>> 2. emit QEVENT_GUEST_PANICKED and pause the guest
>> 3. emit QEVENT_GUEST_PANICKED and poweroff the guest
>> 4. emit QEVENT_GUEST_PANICKED and reset the guest
>>
>> I/O ports does not work for some targets(for example: s390). And you
>> can implement another qom device, and include it's code into pv_event.c
>> for such target.
>>
>> Note: if we emit QEVENT_GUEST_PANICKED only, and the management
>> application does not receive this event(the management may not
>> run when the event is emitted), the management won't know the
>> guest is panicked.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>  hw/kvm/Makefile.objs |    2 +-
>>  hw/kvm/pv_event.c    |   73 +++++++++++++++++++++++++++
>>  hw/kvm/pv_ioport.c   |  133 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  kvm-stub.c           |    9 +++
>>  kvm.h                |    3 +
>>  vl.c                 |    4 ++
>>  6 files changed, 223 insertions(+), 1 deletions(-)
>>  create mode 100644 hw/kvm/pv_event.c
>>  create mode 100644 hw/kvm/pv_ioport.c
>>
>> diff --git a/hw/kvm/Makefile.objs b/hw/kvm/Makefile.objs
>> index 226497a..23e3b30 100644
>> --- a/hw/kvm/Makefile.objs
>> +++ b/hw/kvm/Makefile.objs
>> @@ -1 +1 @@
>> -obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o
>> +obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o pv_event.o
>> diff --git a/hw/kvm/pv_event.c b/hw/kvm/pv_event.c
>> new file mode 100644
>> index 0000000..d7ded37
>> --- /dev/null
>> +++ b/hw/kvm/pv_event.c
>> @@ -0,0 +1,73 @@
>> +/*
>> + * QEMU KVM support, paravirtual event device
>> + *
>> + * Copyright Fujitsu, Corp. 2012
>> + *
>> + * Authors:
>> + *     Wen Congyang <wency@cn.fujitsu.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include <linux/kvm_para.h>
>> +#include <asm/kvm_para.h>
>> +#include <qobject.h>
>> +#include <qjson.h>
>> +#include <monitor.h>
>> +#include <sysemu.h>
>> +#include <kvm.h>
>> +
>> +/* Possible values for action parameter. */
>> +#define PANICKED_REPORT     1   /* emit QEVENT_GUEST_PANICKED only */
>> +#define PANICKED_PAUSE      2   /* emit QEVENT_GUEST_PANICKED and pause VM */
>> +#define PANICKED_POWEROFF   3   /* emit QEVENT_GUEST_PANICKED and quit VM */
>> +#define PANICKED_RESET      4   /* emit QEVENT_GUEST_PANICKED and reset VM */
>> +
>> +static int panicked_action = PANICKED_REPORT;
> 
> Avoid global variables please when there are device states. This one is
> unneeded anyway (and will generate warnings when build without KVM_PV_PORT).

Hmm, do you mean introduce another qom device to store event action?

Thanks
Wen Congyang

> 
>> +
>> +static void panicked_mon_event(const char *action)
>> +{
>> +    QObject *data;
>> +
>> +    data = qobject_from_jsonf("{ 'action': %s }", action);
>> +    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
>> +    qobject_decref(data);
>> +}
>> +
>> +static void panicked_perform_action(uint32_t panicked_action)
>> +{
>> +    switch (panicked_action) {
>> +    case PANICKED_REPORT:
>> +        panicked_mon_event("report");
>> +        break;
>> +
>> +    case PANICKED_PAUSE:
>> +        panicked_mon_event("pause");
>> +        vm_stop(RUN_STATE_GUEST_PANICKED);
>> +        break;
>> +
>> +    case PANICKED_POWEROFF:
>> +        panicked_mon_event("poweroff");
>> +        exit(0);
> 
> We have qemu_system_shutdown_request.
> 
>> +        break;
>> +    case PANICKED_RESET:
>> +        panicked_mon_event("reset");
>> +        qemu_system_reset_request();
>> +        break;
>> +    }
>> +}
>> +
>> +#if defined(KVM_PV_PORT)
>> +#include "pv_ioport.c"
>> +
>> +void kvm_pv_event_init(void)
>> +{
>> +    pv_ioport_init(panicked_action);
>> +}
>> +#else
>> +void kvm_pv_event_init(void)
>> +{
>> +}
>> +#endif
> 
> Generally, the split-up of handling and transport layer is a good idea
> to allow other arch to support this interface. However, its current form
> is a bit unfortunate as it does not properly separate the logic of the
> events (so far only panic action) from the transport mechanism (PIO) and
> as it registers the transport as a configurable device, not the event
> handler. Make sure that pv_ioport only deals with registering against
> the right bus and forwarding of the PV gate accesses to the event
> handling layer. Device name and properties should be defined by the
> event layer as well (but then registered by the transport layer).
> 
>> diff --git a/hw/kvm/pv_ioport.c b/hw/kvm/pv_ioport.c
>> new file mode 100644
>> index 0000000..e93d819
>> --- /dev/null
>> +++ b/hw/kvm/pv_ioport.c
>> @@ -0,0 +1,133 @@
>> +/*
>> + * QEMU KVM support, paravirtual I/O port device
>> + *
>> + * Copyright Fujitsu, Corp. 2012
>> + *
>> + * Authors:
>> + *     Wen Congyang <wency@cn.fujitsu.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "hw/isa.h"
>> +
>> +typedef struct {
>> +    ISADevice dev;
>> +    MemoryRegion ioport;
>> +    uint32_t panicked_action;
> 
> As explained above, this layer should not know about things like
> "panicked_action".
> 
>> +} PVState;
>> +
>> +static PVState *pv_state;
>> +
>> +static uint64_t pv_io_read(void *opaque, target_phys_addr_t addr, unsigned size)
>> +{
>> +    return 1 << KVM_PV_FEATURE_PANICKED;
>> +}
>> +
>> +static void pv_io_write(void *opaque, target_phys_addr_t addr, uint64_t val,
>> +                        unsigned size)
>> +{
>> +    PVState *s = opaque;
>> +
>> +    if (val == KVM_PV_PANICKED) {
>> +        panicked_perform_action(s->panicked_action);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps pv_io_ops = {
>> +    .read = pv_io_read,
>> +    .write = pv_io_write,
>> +    .impl = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    },
>> +};
>> +
>> +static int pv_ioport_initfn(ISADevice *dev)
>> +{
>> +    PVState *s = DO_UPCAST(PVState, dev, dev);
>> +
>> +    memory_region_init_io(&s->ioport, &pv_io_ops, s, "pv_event", 1);
>> +    isa_register_ioport(dev, &s->ioport, KVM_PV_PORT);
>> +
>> +    pv_state = s;
>> +
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription pv_ioport_vmsd = {
>> +    .name = "pv_ioport",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(panicked_action, PVState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
> 
> Unneeded as panicked_action is a host-side property, not a
> guest-changeable state. Your device is stateless, thus has no vmstate.
> 
>> +
>> +static Property pv_ioport_properties[] = {
>> +    DEFINE_PROP_UINT32("panicked_action", PVState, panicked_action, PANICKED_REPORT),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void pv_ioport_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
>> +
>> +    ic->init = pv_ioport_initfn;
>> +    dc->no_user = 1;
>> +    dc->vmsd = &pv_ioport_vmsd;
>> +    dc->props = pv_ioport_properties;
>> +}
>> +
>> +static TypeInfo pv_ioport_info = {
>> +    .name          = "kvm_pv_ioport",
>> +    .parent        = TYPE_ISA_DEVICE,
>> +    .instance_size = sizeof(PVState),
>> +    .class_init    = pv_ioport_class_init,
>> +};
>> +
>> +static void pv_ioport_register_types(void)
>> +{
>> +    type_register_static(&pv_ioport_info);
>> +}
>> +
>> +type_init(pv_ioport_register_types)
>> +
>> +static int is_isa_bus(BusState *bus, void *opaque)
>> +{
>> +    const char *bus_type_name;
>> +    ISABus **isa_bus_p = opaque;
>> +
>> +    bus_type_name = object_class_get_name(bus->obj.class);
>> +    if (!strcmp(bus_type_name, TYPE_ISA_BUS)) {
>> +        *isa_bus_p = ISA_BUS(&bus->obj);
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static ISABus *get_isa_bus(void)
>> +{
>> +    ISABus *isa_bus = NULL;
>> +
>> +    qbus_walk_children(sysbus_get_default(), NULL, is_isa_bus, &isa_bus);
>> +
>> +    return isa_bus;
>> +}
> 
> Unneeded if the bus is passed on creation from the pc board setup.
> That's the official way.
> 
>> +
>> +static void pv_ioport_init(uint32_t panicked_action)
>> +{
>> +    ISADevice *dev;
>> +    ISABus *bus;
>> +
>> +    bus = get_isa_bus();
>> +    dev = isa_create(bus, "kvm_pv_ioport");
>> +    qdev_prop_set_uint32(&dev->qdev, "panicked_action", panicked_action);
> 
> Nope, configuration should works via "-global device.property=value".
> You likely want to define a special property that translates action
> names into enum values, see e.g. the lost tick policy.
> 
>> +    qdev_init_nofail(&dev->qdev);
>> +}
>> diff --git a/kvm-stub.c b/kvm-stub.c
>> index ec9a364..a28d078 100644
>> --- a/kvm-stub.c
>> +++ b/kvm-stub.c
>> @@ -151,3 +151,12 @@ int kvm_irqchip_remove_irqfd(KVMState *s, int fd, int virq)
>>  {
>>      return -ENOSYS;
>>  }
>> +
>> +void kvm_pv_event_init(void)
>> +{
>> +}
>> +
>> +int select_panicked_action(const char *p)
>> +{
>> +    return 0;
>> +}
> 
> Both will be unneeded.
> 
>> diff --git a/kvm.h b/kvm.h
>> index 9c7b0ea..1f7c72b 100644
>> --- a/kvm.h
>> +++ b/kvm.h
>> @@ -218,4 +218,7 @@ void kvm_irqchip_release_virq(KVMState *s, int virq);
>>  
>>  int kvm_irqchip_add_irqfd(KVMState *s, int fd, int virq);
>>  int kvm_irqchip_remove_irqfd(KVMState *s, int fd, int virq);
>> +
>> +void kvm_pv_event_init(void);
>> +int select_panicked_action(const char *p);
>>  #endif
>> diff --git a/vl.c b/vl.c
>> index ea5ef1c..f5cd28d 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3622,6 +3622,10 @@ int main(int argc, char **argv, char **envp)
>>          exit(1);
>>      }
>>  
>> +    if (kvm_enabled()) {
>> +        kvm_pv_event_init();
>> +    }
> 
> Initialization is better located in the setup code of the board that
> supports this device (here the PC). Very similar to kvm clock.
> 
>> +
>>      qdev_machine_creation_done();
>>  
>>      if (rom_load_all() != 0) {
>>
> 
> Jan
>
Jan Kiszka - July 18, 2012, 9:19 a.m.
On 2012-07-18 03:54, Wen Congyang wrote:
> At 07/06/2012 07:05 PM, Jan Kiszka Wrote:
>> On 2012-07-06 11:41, Wen Congyang wrote:
>>> If the target is x86/x86_64, the guest's kernel will write 0x01 to the
>>> port KVM_PV_PORT when it is panciked. This patch introduces a new qom
>>> device kvm_pv_ioport to listen this I/O port, and deal with panicked
>>> event according to panicked_action's value. The possible actions are:
>>> 1. emit QEVENT_GUEST_PANICKED only
>>> 2. emit QEVENT_GUEST_PANICKED and pause the guest
>>> 3. emit QEVENT_GUEST_PANICKED and poweroff the guest
>>> 4. emit QEVENT_GUEST_PANICKED and reset the guest
>>>
>>> I/O ports does not work for some targets(for example: s390). And you
>>> can implement another qom device, and include it's code into pv_event.c
>>> for such target.
>>>
>>> Note: if we emit QEVENT_GUEST_PANICKED only, and the management
>>> application does not receive this event(the management may not
>>> run when the event is emitted), the management won't know the
>>> guest is panicked.
>>>
>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>> ---
>>>  hw/kvm/Makefile.objs |    2 +-
>>>  hw/kvm/pv_event.c    |   73 +++++++++++++++++++++++++++
>>>  hw/kvm/pv_ioport.c   |  133 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  kvm-stub.c           |    9 +++
>>>  kvm.h                |    3 +
>>>  vl.c                 |    4 ++
>>>  6 files changed, 223 insertions(+), 1 deletions(-)
>>>  create mode 100644 hw/kvm/pv_event.c
>>>  create mode 100644 hw/kvm/pv_ioport.c
>>>
>>> diff --git a/hw/kvm/Makefile.objs b/hw/kvm/Makefile.objs
>>> index 226497a..23e3b30 100644
>>> --- a/hw/kvm/Makefile.objs
>>> +++ b/hw/kvm/Makefile.objs
>>> @@ -1 +1 @@
>>> -obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o
>>> +obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o pv_event.o
>>> diff --git a/hw/kvm/pv_event.c b/hw/kvm/pv_event.c
>>> new file mode 100644
>>> index 0000000..d7ded37
>>> --- /dev/null
>>> +++ b/hw/kvm/pv_event.c
>>> @@ -0,0 +1,73 @@
>>> +/*
>>> + * QEMU KVM support, paravirtual event device
>>> + *
>>> + * Copyright Fujitsu, Corp. 2012
>>> + *
>>> + * Authors:
>>> + *     Wen Congyang <wency@cn.fujitsu.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>> + * See the COPYING file in the top-level directory.
>>> + *
>>> + */
>>> +
>>> +#include <linux/kvm_para.h>
>>> +#include <asm/kvm_para.h>
>>> +#include <qobject.h>
>>> +#include <qjson.h>
>>> +#include <monitor.h>
>>> +#include <sysemu.h>
>>> +#include <kvm.h>
>>> +
>>> +/* Possible values for action parameter. */
>>> +#define PANICKED_REPORT     1   /* emit QEVENT_GUEST_PANICKED only */
>>> +#define PANICKED_PAUSE      2   /* emit QEVENT_GUEST_PANICKED and pause VM */
>>> +#define PANICKED_POWEROFF   3   /* emit QEVENT_GUEST_PANICKED and quit VM */
>>> +#define PANICKED_RESET      4   /* emit QEVENT_GUEST_PANICKED and reset VM */
>>> +
>>> +static int panicked_action = PANICKED_REPORT;
>>
>> Avoid global variables please when there are device states. This one is
>> unneeded anyway (and will generate warnings when build without KVM_PV_PORT).
> 
> Hmm, do you mean introduce another qom device to store event action?

I think you should be fine with one device per bus binding, but those
will consist of a common event layer and just different I/O layers (for
bus registration and access).

Jan
Jan Kiszka - July 18, 2012, 9:25 a.m.
On 2012-07-18 11:19, Jan Kiszka wrote:
> On 2012-07-18 03:54, Wen Congyang wrote:
>> At 07/06/2012 07:05 PM, Jan Kiszka Wrote:
>>> On 2012-07-06 11:41, Wen Congyang wrote:
>>>> If the target is x86/x86_64, the guest's kernel will write 0x01 to the
>>>> port KVM_PV_PORT when it is panciked. This patch introduces a new qom
>>>> device kvm_pv_ioport to listen this I/O port, and deal with panicked
>>>> event according to panicked_action's value. The possible actions are:
>>>> 1. emit QEVENT_GUEST_PANICKED only
>>>> 2. emit QEVENT_GUEST_PANICKED and pause the guest
>>>> 3. emit QEVENT_GUEST_PANICKED and poweroff the guest
>>>> 4. emit QEVENT_GUEST_PANICKED and reset the guest
>>>>
>>>> I/O ports does not work for some targets(for example: s390). And you
>>>> can implement another qom device, and include it's code into pv_event.c
>>>> for such target.
>>>>
>>>> Note: if we emit QEVENT_GUEST_PANICKED only, and the management
>>>> application does not receive this event(the management may not
>>>> run when the event is emitted), the management won't know the
>>>> guest is panicked.
>>>>
>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>> ---
>>>>  hw/kvm/Makefile.objs |    2 +-
>>>>  hw/kvm/pv_event.c    |   73 +++++++++++++++++++++++++++
>>>>  hw/kvm/pv_ioport.c   |  133 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  kvm-stub.c           |    9 +++
>>>>  kvm.h                |    3 +
>>>>  vl.c                 |    4 ++
>>>>  6 files changed, 223 insertions(+), 1 deletions(-)
>>>>  create mode 100644 hw/kvm/pv_event.c
>>>>  create mode 100644 hw/kvm/pv_ioport.c
>>>>
>>>> diff --git a/hw/kvm/Makefile.objs b/hw/kvm/Makefile.objs
>>>> index 226497a..23e3b30 100644
>>>> --- a/hw/kvm/Makefile.objs
>>>> +++ b/hw/kvm/Makefile.objs
>>>> @@ -1 +1 @@
>>>> -obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o
>>>> +obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o pv_event.o
>>>> diff --git a/hw/kvm/pv_event.c b/hw/kvm/pv_event.c
>>>> new file mode 100644
>>>> index 0000000..d7ded37
>>>> --- /dev/null
>>>> +++ b/hw/kvm/pv_event.c
>>>> @@ -0,0 +1,73 @@
>>>> +/*
>>>> + * QEMU KVM support, paravirtual event device
>>>> + *
>>>> + * Copyright Fujitsu, Corp. 2012
>>>> + *
>>>> + * Authors:
>>>> + *     Wen Congyang <wency@cn.fujitsu.com>
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>> + * See the COPYING file in the top-level directory.
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/kvm_para.h>
>>>> +#include <asm/kvm_para.h>
>>>> +#include <qobject.h>
>>>> +#include <qjson.h>
>>>> +#include <monitor.h>
>>>> +#include <sysemu.h>
>>>> +#include <kvm.h>
>>>> +
>>>> +/* Possible values for action parameter. */
>>>> +#define PANICKED_REPORT     1   /* emit QEVENT_GUEST_PANICKED only */
>>>> +#define PANICKED_PAUSE      2   /* emit QEVENT_GUEST_PANICKED and pause VM */
>>>> +#define PANICKED_POWEROFF   3   /* emit QEVENT_GUEST_PANICKED and quit VM */
>>>> +#define PANICKED_RESET      4   /* emit QEVENT_GUEST_PANICKED and reset VM */
>>>> +
>>>> +static int panicked_action = PANICKED_REPORT;
>>>
>>> Avoid global variables please when there are device states. This one is
>>> unneeded anyway (and will generate warnings when build without KVM_PV_PORT).
>>
>> Hmm, do you mean introduce another qom device to store event action?
> 
> I think you should be fine with one device per bus binding, but those
> will consist of a common event layer and just different I/O layers (for
> bus registration and access).

To make this clearer: the I/O layer should embed a common state
structure of the event layer in its device state so that the event layer
can keep things like the action mode there.

Jan

Patch

diff --git a/hw/kvm/Makefile.objs b/hw/kvm/Makefile.objs
index 226497a..23e3b30 100644
--- a/hw/kvm/Makefile.objs
+++ b/hw/kvm/Makefile.objs
@@ -1 +1 @@ 
-obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o
+obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o pv_event.o
diff --git a/hw/kvm/pv_event.c b/hw/kvm/pv_event.c
new file mode 100644
index 0000000..d7ded37
--- /dev/null
+++ b/hw/kvm/pv_event.c
@@ -0,0 +1,73 @@ 
+/*
+ * QEMU KVM support, paravirtual event device
+ *
+ * Copyright Fujitsu, Corp. 2012
+ *
+ * Authors:
+ *     Wen Congyang <wency@cn.fujitsu.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include <linux/kvm_para.h>
+#include <asm/kvm_para.h>
+#include <qobject.h>
+#include <qjson.h>
+#include <monitor.h>
+#include <sysemu.h>
+#include <kvm.h>
+
+/* Possible values for action parameter. */
+#define PANICKED_REPORT     1   /* emit QEVENT_GUEST_PANICKED only */
+#define PANICKED_PAUSE      2   /* emit QEVENT_GUEST_PANICKED and pause VM */
+#define PANICKED_POWEROFF   3   /* emit QEVENT_GUEST_PANICKED and quit VM */
+#define PANICKED_RESET      4   /* emit QEVENT_GUEST_PANICKED and reset VM */
+
+static int panicked_action = PANICKED_REPORT;
+
+static void panicked_mon_event(const char *action)
+{
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'action': %s }", action);
+    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
+    qobject_decref(data);
+}
+
+static void panicked_perform_action(uint32_t panicked_action)
+{
+    switch (panicked_action) {
+    case PANICKED_REPORT:
+        panicked_mon_event("report");
+        break;
+
+    case PANICKED_PAUSE:
+        panicked_mon_event("pause");
+        vm_stop(RUN_STATE_GUEST_PANICKED);
+        break;
+
+    case PANICKED_POWEROFF:
+        panicked_mon_event("poweroff");
+        exit(0);
+        break;
+    case PANICKED_RESET:
+        panicked_mon_event("reset");
+        qemu_system_reset_request();
+        break;
+    }
+}
+
+#if defined(KVM_PV_PORT)
+#include "pv_ioport.c"
+
+void kvm_pv_event_init(void)
+{
+    pv_ioport_init(panicked_action);
+}
+#else
+void kvm_pv_event_init(void)
+{
+}
+#endif
diff --git a/hw/kvm/pv_ioport.c b/hw/kvm/pv_ioport.c
new file mode 100644
index 0000000..e93d819
--- /dev/null
+++ b/hw/kvm/pv_ioport.c
@@ -0,0 +1,133 @@ 
+/*
+ * QEMU KVM support, paravirtual I/O port device
+ *
+ * Copyright Fujitsu, Corp. 2012
+ *
+ * Authors:
+ *     Wen Congyang <wency@cn.fujitsu.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "hw/isa.h"
+
+typedef struct {
+    ISADevice dev;
+    MemoryRegion ioport;
+    uint32_t panicked_action;
+} PVState;
+
+static PVState *pv_state;
+
+static uint64_t pv_io_read(void *opaque, target_phys_addr_t addr, unsigned size)
+{
+    return 1 << KVM_PV_FEATURE_PANICKED;
+}
+
+static void pv_io_write(void *opaque, target_phys_addr_t addr, uint64_t val,
+                        unsigned size)
+{
+    PVState *s = opaque;
+
+    if (val == KVM_PV_PANICKED) {
+        panicked_perform_action(s->panicked_action);
+    }
+}
+
+static const MemoryRegionOps pv_io_ops = {
+    .read = pv_io_read,
+    .write = pv_io_write,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
+static int pv_ioport_initfn(ISADevice *dev)
+{
+    PVState *s = DO_UPCAST(PVState, dev, dev);
+
+    memory_region_init_io(&s->ioport, &pv_io_ops, s, "pv_event", 1);
+    isa_register_ioport(dev, &s->ioport, KVM_PV_PORT);
+
+    pv_state = s;
+
+    return 0;
+}
+
+static const VMStateDescription pv_ioport_vmsd = {
+    .name = "pv_ioport",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(panicked_action, PVState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property pv_ioport_properties[] = {
+    DEFINE_PROP_UINT32("panicked_action", PVState, panicked_action, PANICKED_REPORT),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pv_ioport_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
+
+    ic->init = pv_ioport_initfn;
+    dc->no_user = 1;
+    dc->vmsd = &pv_ioport_vmsd;
+    dc->props = pv_ioport_properties;
+}
+
+static TypeInfo pv_ioport_info = {
+    .name          = "kvm_pv_ioport",
+    .parent        = TYPE_ISA_DEVICE,
+    .instance_size = sizeof(PVState),
+    .class_init    = pv_ioport_class_init,
+};
+
+static void pv_ioport_register_types(void)
+{
+    type_register_static(&pv_ioport_info);
+}
+
+type_init(pv_ioport_register_types)
+
+static int is_isa_bus(BusState *bus, void *opaque)
+{
+    const char *bus_type_name;
+    ISABus **isa_bus_p = opaque;
+
+    bus_type_name = object_class_get_name(bus->obj.class);
+    if (!strcmp(bus_type_name, TYPE_ISA_BUS)) {
+        *isa_bus_p = ISA_BUS(&bus->obj);
+        return -1;
+    }
+
+    return 0;
+}
+
+static ISABus *get_isa_bus(void)
+{
+    ISABus *isa_bus = NULL;
+
+    qbus_walk_children(sysbus_get_default(), NULL, is_isa_bus, &isa_bus);
+
+    return isa_bus;
+}
+
+static void pv_ioport_init(uint32_t panicked_action)
+{
+    ISADevice *dev;
+    ISABus *bus;
+
+    bus = get_isa_bus();
+    dev = isa_create(bus, "kvm_pv_ioport");
+    qdev_prop_set_uint32(&dev->qdev, "panicked_action", panicked_action);
+    qdev_init_nofail(&dev->qdev);
+}
diff --git a/kvm-stub.c b/kvm-stub.c
index ec9a364..a28d078 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -151,3 +151,12 @@  int kvm_irqchip_remove_irqfd(KVMState *s, int fd, int virq)
 {
     return -ENOSYS;
 }
+
+void kvm_pv_event_init(void)
+{
+}
+
+int select_panicked_action(const char *p)
+{
+    return 0;
+}
diff --git a/kvm.h b/kvm.h
index 9c7b0ea..1f7c72b 100644
--- a/kvm.h
+++ b/kvm.h
@@ -218,4 +218,7 @@  void kvm_irqchip_release_virq(KVMState *s, int virq);
 
 int kvm_irqchip_add_irqfd(KVMState *s, int fd, int virq);
 int kvm_irqchip_remove_irqfd(KVMState *s, int fd, int virq);
+
+void kvm_pv_event_init(void);
+int select_panicked_action(const char *p);
 #endif
diff --git a/vl.c b/vl.c
index ea5ef1c..f5cd28d 100644
--- a/vl.c
+++ b/vl.c
@@ -3622,6 +3622,10 @@  int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    if (kvm_enabled()) {
+        kvm_pv_event_init();
+    }
+
     qdev_machine_creation_done();
 
     if (rom_load_all() != 0) {