Re: [Qemu-devel] [PATCH v18 3/7] introduce a new qom device to deal with panicked event

[Markus Armbruster]

Hu Tao <address@hidden> writes:

> pvpanic device is used to send guest panic event from guest to qemu.
>
> When guest panic happens, pvpanic device driver will write a event
> number to IO port 0x505(which is the IO port occupied by pvpanic device,
> by default). On receiving the event, pvpanic device will pause guest
> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.
>
> Signed-off-by: Wen Congyang <address@hidden>
> Signed-off-by: Hu Tao <address@hidden>
> ---
>  hw/misc/Makefile.objs |   2 +
>  hw/misc/pvpanic.c     | 116 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 118 insertions(+)
>  create mode 100644 hw/misc/pvpanic.c
>
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 03699c3..d72ea83 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -38,3 +38,5 @@ obj-$(CONFIG_OMAP) += omap_tap.o
>  obj-$(CONFIG_PXA2XX) += pxa2xx_pcmcia.o
>  obj-$(CONFIG_SLAVIO) += slavio_misc.o
>  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
> +
> +common-obj-y += pvpanic.o
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> new file mode 100644
> index 0000000..5118fd7
> --- /dev/null
> +++ b/hw/misc/pvpanic.c
> @@ -0,0 +1,116 @@
> +/*
> + * QEMU simulated pvpanic device.
> + *
> + * Copyright Fujitsu, Corp. 2013
> + *
> + * Authors:
> + *     Wen Congyang <address@hidden>
> + *     Hu Tao <address@hidden>
> + *
> + * 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 <qapi/qmp/qobject.h>
> +#include <qapi/qmp/qjson.h>
> +#include <monitor/monitor.h>
> +#include <sysemu/sysemu.h>
> +#include <sysemu/kvm.h>
> +
> +/* The bit of supported pv event */
> +#define PVPANIC_F_PANICKED      0
> +
> +/* The pv event value */
> +#define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
> +
> +#define TYPE_ISA_PVPANIC_DEVICE    "pvpanic"
> +#define ISA_PVPANIC_DEVICE(obj)    \
> +    OBJECT_CHECK(PVPanicState, (obj), TYPE_ISA_PVPANIC_DEVICE)
> +
> +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 handle_event(int event)
> +{
> +    if (event == PVPANIC_PANICKED) {
> +        panicked_mon_event("pause");
> +        vm_stop(RUN_STATE_GUEST_PANICKED);
> +        return;
> +    }
> +}

I've asked these questions before, if you answered them, I missed it:

1. Your event value looks like it encodes multiple events as bits.  Only
one bit is defined so far (PVEVENT_F_PANICKED).  But you recognize this
bit only when the other bits are all off.  Why?  Won't we regret this if
we ever want to define additional bits?

2. You silently ignore unrecognized event values.  Shouldn't we log
them?

> +
> +#include "hw/isa/isa.h"
> +
> +typedef struct PVPanicState {
> +    ISADevice parent_obj;
> +
> +    MemoryRegion io;
> +    uint16_t ioport;
> +} PVPanicState;
> +
> +/* return supported events on read */
> +static uint64_t pvpanic_ioport_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return PVPANIC_PANICKED;
> +}
> +
> +static void pvpanic_ioport_write(void *opaque, hwaddr addr, uint64_t val,
> +                                 unsigned size)
> +{
> +    handle_event(val);
> +}
> +
> +static const MemoryRegionOps pvpanic_ops = {
> +    .read = pvpanic_ioport_read,
> +    .write = pvpanic_ioport_write,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static int pvpanic_isa_initfn(ISADevice *dev)
> +{
> +    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> +
> +    memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
> +    isa_register_ioport(dev, &s->io, s->ioport);
> +
> +    return 0;
> +}
> +
> +static Property pvpanic_isa_properties[] = {
> +    DEFINE_PROP_UINT16("ioport", PVPanicState, ioport, 0x505),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pvpanic_isa_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
> +
> +    ic->init = pvpanic_isa_initfn;
> +    dc->no_user = 1;
> +    dc->props = pvpanic_isa_properties;
> +}
> +
> +static TypeInfo pvpanic_isa_info = {
> +    .name          = TYPE_ISA_PVPANIC_DEVICE,
> +    .parent        = TYPE_ISA_DEVICE,
> +    .instance_size = sizeof(PVPanicState),
> +    .class_init    = pvpanic_isa_class_init,
> +};
> +
> +static void pvpanic_register_types(void)
> +{
> +    type_register_static(&pvpanic_isa_info);
> +}
> +
> +type_init(pvpanic_register_types)