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

[Hu Tao <hutao@cn.fujitsu.com>]

Hi,

On Wed, Apr 10, 2013 at 01:54:04PM +0200, Markus Armbruster wrote:
> Hu Tao <hutao@cn.fujitsu.com> 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 <wency@cn.fujitsu.com>
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  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 <wency@cn.fujitsu.com>
> > + *     Hu Tao <hutao@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 <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:

Sorry, I must have missed them.

> 
> 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

It was the intention to support multiple events, but Gleb suggested to do
only panic event. So pvpanic device supports only panic event.

> bit only when the other bits are all off.  Why?  Won't we regret this if
> we ever want to define additional bits?

Other bits are reserved now, and must be written as 0. (see patch 5/7)
If we define these bits in the further for whatever purposes, we have to
update code here.

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

See above.


-- 
Regards,
Hu Tao