[Qemu-devel] [PATCH 0/6] s390x: support diag288 watchdog

[Qemu-devel] [PATCH 0/6] s390x: support diag288 watchdog

[Cornelia Huck]

The following series of patches enables a watchdog for s390x that is
based upon a hypercall (diagnose 0x288). The same interface is already
used for s390 LPAR, so it makes sense to provide it under qemu as well.

Patches to enable Linux guests to utilize the watchdog have made their
way upstream during the merge window for 4.1 (see commit b2527d20
"s390/watchdog: support for KVM hypervisors and delete pr_info messages").

This patchset is based on top of the s390x patchset I send out last
week (see <1428569498-27393-1-git-send-email-cornelia.huck@de.ibm.com>).
A branch can be found at

git://github.com/cohuck/qemu s390-next-watchdog

Feedback welcome, especially regarding interface sanity.

Mao Chuan Li (1):
  watchdog: Add new Virtual Watchdog action INJECT-NMI

Xu Wang (5):
  s390x/virtio-ccw: enable has_dynamic_sysbus
  watchdog: Add watchdog device diag288 to the sysbus
  s390/kvm: diag288 instruction interception and handling
  watchdog: Add migration support to diag288 watchdog device
  nmi: Implement inject_nmi() for non-monitor context use

 default-configs/s390x-softmmu.mak |   1 +
 hw/core/nmi.c                     |  20 +++++++
 hw/s390x/s390-virtio-ccw.c        |   1 +
 hw/watchdog/Makefile.objs         |   1 +
 hw/watchdog/watchdog.c            |  10 ++++
 hw/watchdog/wdt_diag288.c         | 122 ++++++++++++++++++++++++++++++++++++++
 include/hw/nmi.h                  |   1 +
 include/hw/watchdog/wdt_diag288.h |  36 +++++++++++
 qapi-schema.json                  |   6 +-
 qemu-options.hx                   |   6 +-
 target-s390x/cpu.h                |   1 +
 target-s390x/kvm.c                |  18 ++++++
 target-s390x/misc_helper.c        |  33 +++++++++++
 13 files changed, 253 insertions(+), 3 deletions(-)
 create mode 100644 hw/watchdog/wdt_diag288.c
 create mode 100644 include/hw/watchdog/wdt_diag288.h

-- 
2.3.5

[Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Cornelia Huck]

From: Xu Wang <gesaint@linux.vnet.ibm.com>

We have to enable this flag to support dynamically adding devices to the
sysbus. This change is needed for the the upcoming diag288 watchdog.

Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index afb539a..df89440 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -216,6 +216,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->no_sdcard = 1;
     mc->use_sclp = 1;
     mc->max_cpus = 255;
+    mc->has_dynamic_sysbus = true;
     nc->nmi_monitor_handler = s390_nmi;
 }
 
-- 
2.3.5

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Alexander Graf]

On 04/17/2015 09:52 AM, Cornelia Huck wrote:
> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>
> We have to enable this flag to support dynamically adding devices to the
> sysbus. This change is needed for the the upcoming diag288 watchdog.

s390 doesn't have a "sysbus" per se. Please create a new bus type.


Alex

>
> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>   hw/s390x/s390-virtio-ccw.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index afb539a..df89440 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -216,6 +216,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>       mc->no_sdcard = 1;
>       mc->use_sclp = 1;
>       mc->max_cpus = 255;
> +    mc->has_dynamic_sysbus = true;
>       nc->nmi_monitor_handler = s390_nmi;
>   }
>   

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Cornelia Huck]

On Tue, 21 Apr 2015 21:06:42 +0200
Alexander Graf <agraf@suse.de> wrote:

> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
> > From: Xu Wang <gesaint@linux.vnet.ibm.com>
> >
> > We have to enable this flag to support dynamically adding devices to the
> > sysbus. This change is needed for the the upcoming diag288 watchdog.
> 
> s390 doesn't have a "sysbus" per se. Please create a new bus type.

So what's wrong with the sysbus? I don't see why we should be different
than everyone else.

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Alexander Graf]

On 04/22/2015 10:25 AM, Cornelia Huck wrote:
> On Tue, 21 Apr 2015 21:06:42 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
>>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>>>
>>> We have to enable this flag to support dynamically adding devices to the
>>> sysbus. This change is needed for the the upcoming diag288 watchdog.
>> s390 doesn't have a "sysbus" per se. Please create a new bus type.
> So what's wrong with the sysbus? I don't see why we should be different
> than everyone else.

The idea behind sysbus is that you have MMIO, PIO and IRQ pins 
connecting to a PIC. It provides a lot of infrastructure for those 
interfaces. S390 doesn't use any of them and instead wants registration 
on "diag" interfaces for example which I'd put on the same layer as PIO 
or MMIO registration.


Alex

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Cornelia Huck]

On Wed, 22 Apr 2015 11:14:40 +0200
Alexander Graf <agraf@suse.de> wrote:

> On 04/22/2015 10:25 AM, Cornelia Huck wrote:
> > On Tue, 21 Apr 2015 21:06:42 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> >
> >> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
> >>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
> >>>
> >>> We have to enable this flag to support dynamically adding devices to the
> >>> sysbus. This change is needed for the the upcoming diag288 watchdog.
> >> s390 doesn't have a "sysbus" per se. Please create a new bus type.
> > So what's wrong with the sysbus? I don't see why we should be different
> > than everyone else.
> 
> The idea behind sysbus is that you have MMIO, PIO and IRQ pins 
> connecting to a PIC. It provides a lot of infrastructure for those 
> interfaces. S390 doesn't use any of them and instead wants registration 
> on "diag" interfaces for example which I'd put on the same layer as PIO 
> or MMIO registration.

I don't think a "diag" bus makes sense. The individual diagnoses are
way too heterogenous beyond the fact that they use the same base
instruction.

So where's the proper place for "misc" devices? My impression was that
they can go on the sysbus.

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Alexander Graf]

> Am 22.04.2015 um 13:40 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
> 
> On Wed, 22 Apr 2015 11:14:40 +0200
> Alexander Graf <agraf@suse.de> wrote:
> 
>>> On 04/22/2015 10:25 AM, Cornelia Huck wrote:
>>> On Tue, 21 Apr 2015 21:06:42 +0200
>>> Alexander Graf <agraf@suse.de> wrote:
>>> 
>>>>> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
>>>>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>>>>> 
>>>>> We have to enable this flag to support dynamically adding devices to the
>>>>> sysbus. This change is needed for the the upcoming diag288 watchdog.
>>>> s390 doesn't have a "sysbus" per se. Please create a new bus type.
>>> So what's wrong with the sysbus? I don't see why we should be different
>>> than everyone else.
>> 
>> The idea behind sysbus is that you have MMIO, PIO and IRQ pins 
>> connecting to a PIC. It provides a lot of infrastructure for those 
>> interfaces. S390 doesn't use any of them and instead wants registration 
>> on "diag" interfaces for example which I'd put on the same layer as PIO 
>> or MMIO registration.
> 
> I don't think a "diag" bus makes sense.

You don't need a bus necessarily, just a parent class.

> The individual diagnoses are
> way too heterogenous beyond the fact that they use the same base
> instruction.
> 
> So where's the proper place for "misc" devices? My impression was that
> they can go on the sysbus.
> 

If you really don't want to create your own class, how about you inherit from the DeviceState class?

Alex

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Cornelia Huck]

On Wed, 22 Apr 2015 14:21:36 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> 
> > Am 22.04.2015 um 13:40 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
> > 
> > On Wed, 22 Apr 2015 11:14:40 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> > 
> >>> On 04/22/2015 10:25 AM, Cornelia Huck wrote:
> >>> On Tue, 21 Apr 2015 21:06:42 +0200
> >>> Alexander Graf <agraf@suse.de> wrote:
> >>> 
> >>>>> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
> >>>>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
> >>>>> 
> >>>>> We have to enable this flag to support dynamically adding devices to the
> >>>>> sysbus. This change is needed for the the upcoming diag288 watchdog.
> >>>> s390 doesn't have a "sysbus" per se. Please create a new bus type.
> >>> So what's wrong with the sysbus? I don't see why we should be different
> >>> than everyone else.
> >> 
> >> The idea behind sysbus is that you have MMIO, PIO and IRQ pins 
> >> connecting to a PIC. It provides a lot of infrastructure for those 
> >> interfaces. S390 doesn't use any of them and instead wants registration 
> >> on "diag" interfaces for example which I'd put on the same layer as PIO 
> >> or MMIO registration.
> > 
> > I don't think a "diag" bus makes sense.
> 
> You don't need a bus necessarily, just a parent class.
> 
> > The individual diagnoses are
> > way too heterogenous beyond the fact that they use the same base
> > instruction.
> > 
> > So where's the proper place for "misc" devices? My impression was that
> > they can go on the sysbus.
> > 
> 
> If you really don't want to create your own class, how about you inherit from the DeviceState class?

I tried that for the watchdog, and it certainly works, but some things
end up odd:

- in 'info qtree', the watchdog device does not show up at all
- in the list of devices printed by "-device help", diag288 is now the
  only device without any bus

I would have thought that any device not attached to a specialized bus
should end up on the main system bus, which brings me back to adding it
as a sysbus device ;)

Does sysbus instead need to get more generic? The platform stuff seems
to be a better fit for MMIO, PIO et al.?

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Alexander Graf]

On 04/24/2015 11:07 AM, Cornelia Huck wrote:
> On Wed, 22 Apr 2015 14:21:36 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>>
>>> Am 22.04.2015 um 13:40 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
>>>
>>> On Wed, 22 Apr 2015 11:14:40 +0200
>>> Alexander Graf <agraf@suse.de> wrote:
>>>
>>>>> On 04/22/2015 10:25 AM, Cornelia Huck wrote:
>>>>> On Tue, 21 Apr 2015 21:06:42 +0200
>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>>> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
>>>>>>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>>>>>>>
>>>>>>> We have to enable this flag to support dynamically adding devices to the
>>>>>>> sysbus. This change is needed for the the upcoming diag288 watchdog.
>>>>>> s390 doesn't have a "sysbus" per se. Please create a new bus type.
>>>>> So what's wrong with the sysbus? I don't see why we should be different
>>>>> than everyone else.
>>>> The idea behind sysbus is that you have MMIO, PIO and IRQ pins
>>>> connecting to a PIC. It provides a lot of infrastructure for those
>>>> interfaces. S390 doesn't use any of them and instead wants registration
>>>> on "diag" interfaces for example which I'd put on the same layer as PIO
>>>> or MMIO registration.
>>> I don't think a "diag" bus makes sense.
>> You don't need a bus necessarily, just a parent class.
>>
>>> The individual diagnoses are
>>> way too heterogenous beyond the fact that they use the same base
>>> instruction.
>>>
>>> So where's the proper place for "misc" devices? My impression was that
>>> they can go on the sysbus.
>>>
>> If you really don't want to create your own class, how about you inherit from the DeviceState class?
> I tried that for the watchdog, and it certainly works, but some things
> end up odd:
>
> - in 'info qtree', the watchdog device does not show up at all

Please try "info qom-tree". Andreas also has a patch outstanding that 
shows properties along the way with a verbose switch.

> - in the list of devices printed by "-device help", diag288 is now the
>    only device without any bus

But it's not attached to a bus, so that's reasonable, no?

> I would have thought that any device not attached to a specialized bus
> should end up on the main system bus, which brings me back to adding it
> as a sysbus device ;)

Not really, sysbus is QEMU's wording for what Linux calls "platform 
bus". It's where devices go to that are attached to MMIO/PIO/IRQ lines 
via some fabric that we don't model.

The target for devices that are not the above we now have non-bus'ed 
devices.


Alex


> Does sysbus instead need to get more generic? The platform stuff seems
> to be a better fit for MMIO, PIO et al.?

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Cornelia Huck]

On Mon, 27 Apr 2015 15:57:04 +0200
Alexander Graf <agraf@suse.de> wrote:

> On 04/24/2015 11:07 AM, Cornelia Huck wrote:
> > On Wed, 22 Apr 2015 14:21:36 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> >
> >>
> >>> Am 22.04.2015 um 13:40 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
> >>>
> >>> On Wed, 22 Apr 2015 11:14:40 +0200
> >>> Alexander Graf <agraf@suse.de> wrote:
> >>>
> >>>>> On 04/22/2015 10:25 AM, Cornelia Huck wrote:
> >>>>> On Tue, 21 Apr 2015 21:06:42 +0200
> >>>>> Alexander Graf <agraf@suse.de> wrote:
> >>>>>
> >>>>>>> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
> >>>>>>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
> >>>>>>>
> >>>>>>> We have to enable this flag to support dynamically adding devices to the
> >>>>>>> sysbus. This change is needed for the the upcoming diag288 watchdog.
> >>>>>> s390 doesn't have a "sysbus" per se. Please create a new bus type.
> >>>>> So what's wrong with the sysbus? I don't see why we should be different
> >>>>> than everyone else.
> >>>> The idea behind sysbus is that you have MMIO, PIO and IRQ pins
> >>>> connecting to a PIC. It provides a lot of infrastructure for those
> >>>> interfaces. S390 doesn't use any of them and instead wants registration
> >>>> on "diag" interfaces for example which I'd put on the same layer as PIO
> >>>> or MMIO registration.
> >>> I don't think a "diag" bus makes sense.
> >> You don't need a bus necessarily, just a parent class.
> >>
> >>> The individual diagnoses are
> >>> way too heterogenous beyond the fact that they use the same base
> >>> instruction.
> >>>
> >>> So where's the proper place for "misc" devices? My impression was that
> >>> they can go on the sysbus.
> >>>
> >> If you really don't want to create your own class, how about you inherit from the DeviceState class?
> > I tried that for the watchdog, and it certainly works, but some things
> > end up odd:
> >
> > - in 'info qtree', the watchdog device does not show up at all
> 
> Please try "info qom-tree". Andreas also has a patch outstanding that 
> shows properties along the way with a verbose switch.

While it does show up in info qom-tree, is hiding it from info qtree a
good idea? I'd think that it is still widely used.

> 
> > - in the list of devices printed by "-device help", diag288 is now the
> >    only device without any bus
> 
> But it's not attached to a bus, so that's reasonable, no?

Hm. Are there bus-less devices on other platforms?

> 
> > I would have thought that any device not attached to a specialized bus
> > should end up on the main system bus, which brings me back to adding it
> > as a sysbus device ;)
> 
> Not really, sysbus is QEMU's wording for what Linux calls "platform 
> bus". It's where devices go to that are attached to MMIO/PIO/IRQ lines 
> via some fabric that we don't model.

But in practice sysbus seems to be more like a catch-all: On s390x,
there are already things like the flic, various sclp-related devices,
the virtio bridges or the ipl device sitting on the sysbus. Should they
really be thrown out from the sysbus and dangle as bus-less devices? I
think there is a case to be made for a catch-all bus, even if it is not
the sysbus.

> 
> The target for devices that are not the above we now have non-bus'ed 
> devices.

I'm afraid I can't parse that sentence :)

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Andreas Färber]

Am 27.04.2015 um 16:19 schrieb Cornelia Huck:
> On Mon, 27 Apr 2015 15:57:04 +0200
> Alexander Graf <agraf@suse.de> wrote:
>> On 04/24/2015 11:07 AM, Cornelia Huck wrote:
>>> On Wed, 22 Apr 2015 14:21:36 +0200
>>> Alexander Graf <agraf@suse.de> wrote:
>>>>> Am 22.04.2015 um 13:40 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
>>>>> On Wed, 22 Apr 2015 11:14:40 +0200
>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>>> On 04/22/2015 10:25 AM, Cornelia Huck wrote:
>>>>>>> On Tue, 21 Apr 2015 21:06:42 +0200
>>>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
>>>>>>>>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>>>>>>>>>
>>>>>>>>> We have to enable this flag to support dynamically adding devices to the
>>>>>>>>> sysbus. This change is needed for the the upcoming diag288 watchdog.
>>>>>>>> s390 doesn't have a "sysbus" per se. Please create a new bus type.
>>>>>>> So what's wrong with the sysbus? I don't see why we should be different
>>>>>>> than everyone else.
>>>>>> The idea behind sysbus is that you have MMIO, PIO and IRQ pins
>>>>>> connecting to a PIC. It provides a lot of infrastructure for those
>>>>>> interfaces. S390 doesn't use any of them and instead wants registration
>>>>>> on "diag" interfaces for example which I'd put on the same layer as PIO
>>>>>> or MMIO registration.
>>>>> I don't think a "diag" bus makes sense.
>>>> You don't need a bus necessarily, just a parent class.
>>>>
>>>>> The individual diagnoses are
>>>>> way too heterogenous beyond the fact that they use the same base
>>>>> instruction.
>>>>>
>>>>> So where's the proper place for "misc" devices? My impression was that
>>>>> they can go on the sysbus.
>>>>>
>>>> If you really don't want to create your own class, how about you inherit from the DeviceState class?
>>> I tried that for the watchdog, and it certainly works, but some things
>>> end up odd:
>>>
>>> - in 'info qtree', the watchdog device does not show up at all
>>
>> Please try "info qom-tree". Andreas also has a patch outstanding that 
>> shows properties along the way with a verbose switch.
> 
> While it does show up in info qom-tree, is hiding it from info qtree a
> good idea? I'd think that it is still widely used.

That's why I proposed to drop info qtree, so that people no longer
mistakenly use it and do weird designs because of it. But there was
opposition to it and its incomplete replacements at the time - in 2.3 we
at least have the qom-tree display and an external qom-tree script to
display all the properties.

In the end, the bus view and the composition view complement each other
as long as we can't or don't want to get rid of qbuses completely.

>>> - in the list of devices printed by "-device help", diag288 is now the
>>>    only device without any bus
>>
>> But it's not attached to a bus, so that's reasonable, no?
> 
> Hm. Are there bus-less devices on other platforms?

Take a look at the recent APIC patches, it's being converted from an ICC
bus (for making hot-add work at the time) to bus-less device.

PCMCIA is a bus-less device already IIRC.

Just search for .parent = TYPE_DEVICE. :)

>>> I would have thought that any device not attached to a specialized bus
>>> should end up on the main system bus, which brings me back to adding it
>>> as a sysbus device ;)
>>
>> Not really, sysbus is QEMU's wording for what Linux calls "platform 
>> bus". It's where devices go to that are attached to MMIO/PIO/IRQ lines 
>> via some fabric that we don't model.
> 
> But in practice sysbus seems to be more like a catch-all: On s390x,
> there are already things like the flic, various sclp-related devices,
> the virtio bridges or the ipl device sitting on the sysbus. Should they
> really be thrown out from the sysbus and dangle as bus-less devices? I
> think there is a case to be made for a catch-all bus, even if it is not
> the sysbus.

There's a difference between "dangling" and SysBus. There cannot be
dangling QOM objects - that's part of the ongoing CPU discussion we're
having (and that people seem to keep forgetting). They need to have a
parent, i.e. a child<> property leading to them, recursively forming a
canonical path. Internal devices are usually dangling and that is
currently being caught be placing them in /machine/unattached at
realization time - much too late. Devices added via -device or
device_add are never dangling as they are placed in /machine/peripheral
or /machine/peripheral-anon. A better question is whether that is
actually desired for your PV devices or whether it should just be a
-machine option that enabled a device sitting on /machine directly or
wherever sensible.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Cornelia Huck]

On Mon, 27 Apr 2015 17:15:03 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 27.04.2015 um 16:19 schrieb Cornelia Huck:
> > On Mon, 27 Apr 2015 15:57:04 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> >> On 04/24/2015 11:07 AM, Cornelia Huck wrote:
> >>> On Wed, 22 Apr 2015 14:21:36 +0200
> >>> Alexander Graf <agraf@suse.de> wrote:
> >>>>> Am 22.04.2015 um 13:40 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
> >>>>> On Wed, 22 Apr 2015 11:14:40 +0200
> >>>>> Alexander Graf <agraf@suse.de> wrote:
> >>>>>>> On 04/22/2015 10:25 AM, Cornelia Huck wrote:
> >>>>>>> On Tue, 21 Apr 2015 21:06:42 +0200
> >>>>>>> Alexander Graf <agraf@suse.de> wrote:
> >>>>>>>>> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
> >>>>>>>>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
> >>>>>>>>>
> >>>>>>>>> We have to enable this flag to support dynamically adding devices to the
> >>>>>>>>> sysbus. This change is needed for the the upcoming diag288 watchdog.
> >>>>>>>> s390 doesn't have a "sysbus" per se. Please create a new bus type.
> >>>>>>> So what's wrong with the sysbus? I don't see why we should be different
> >>>>>>> than everyone else.
> >>>>>> The idea behind sysbus is that you have MMIO, PIO and IRQ pins
> >>>>>> connecting to a PIC. It provides a lot of infrastructure for those
> >>>>>> interfaces. S390 doesn't use any of them and instead wants registration
> >>>>>> on "diag" interfaces for example which I'd put on the same layer as PIO
> >>>>>> or MMIO registration.
> >>>>> I don't think a "diag" bus makes sense.
> >>>> You don't need a bus necessarily, just a parent class.
> >>>>
> >>>>> The individual diagnoses are
> >>>>> way too heterogenous beyond the fact that they use the same base
> >>>>> instruction.
> >>>>>
> >>>>> So where's the proper place for "misc" devices? My impression was that
> >>>>> they can go on the sysbus.
> >>>>>
> >>>> If you really don't want to create your own class, how about you inherit from the DeviceState class?
> >>> I tried that for the watchdog, and it certainly works, but some things
> >>> end up odd:
> >>>
> >>> - in 'info qtree', the watchdog device does not show up at all
> >>
> >> Please try "info qom-tree". Andreas also has a patch outstanding that 
> >> shows properties along the way with a verbose switch.
> > 
> > While it does show up in info qom-tree, is hiding it from info qtree a
> > good idea? I'd think that it is still widely used.
> 
> That's why I proposed to drop info qtree, so that people no longer
> mistakenly use it and do weird designs because of it. But there was
> opposition to it and its incomplete replacements at the time - in 2.3 we
> at least have the qom-tree display and an external qom-tree script to
> display all the properties.
> 
> In the end, the bus view and the composition view complement each other
> as long as we can't or don't want to get rid of qbuses completely.
> 
> >>> - in the list of devices printed by "-device help", diag288 is now the
> >>>    only device without any bus
> >>
> >> But it's not attached to a bus, so that's reasonable, no?
> > 
> > Hm. Are there bus-less devices on other platforms?
> 
> Take a look at the recent APIC patches, it's being converted from an ICC
> bus (for making hot-add work at the time) to bus-less device.
> 
> PCMCIA is a bus-less device already IIRC.

I'll take a look.

> 
> Just search for .parent = TYPE_DEVICE. :)
> 
> >>> I would have thought that any device not attached to a specialized bus
> >>> should end up on the main system bus, which brings me back to adding it
> >>> as a sysbus device ;)
> >>
> >> Not really, sysbus is QEMU's wording for what Linux calls "platform 
> >> bus". It's where devices go to that are attached to MMIO/PIO/IRQ lines 
> >> via some fabric that we don't model.
> > 
> > But in practice sysbus seems to be more like a catch-all: On s390x,
> > there are already things like the flic, various sclp-related devices,
> > the virtio bridges or the ipl device sitting on the sysbus. Should they
> > really be thrown out from the sysbus and dangle as bus-less devices? I
> > think there is a case to be made for a catch-all bus, even if it is not
> > the sysbus.
> 
> There's a difference between "dangling" and SysBus. There cannot be
> dangling QOM objects - that's part of the ongoing CPU discussion we're
> having (and that people seem to keep forgetting). They need to have a
> parent, i.e. a child<> property leading to them, recursively forming a
> canonical path. Internal devices are usually dangling and that is
> currently being caught be placing them in /machine/unattached at
> realization time - much too late. Devices added via -device or
> device_add are never dangling as they are placed in /machine/peripheral
> or /machine/peripheral-anon. A better question is whether that is
> actually desired for your PV devices or whether it should just be a
> -machine option that enabled a device sitting on /machine directly or
> wherever sensible.

Having the devices we currently have on the sysbus controlled via
machine options does not quite feel right to me at a first glance.
Placing them as "peripheral" somehow does not feel quite right either,
but that seems to be what we get from making it a device.

I think I'll try the s390 platform bus first. But should we perhaps
have something like /machine/infrastructure or so?

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Alexander Graf]

On 04/27/2015 04:19 PM, Cornelia Huck wrote:
> On Mon, 27 Apr 2015 15:57:04 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>> On 04/24/2015 11:07 AM, Cornelia Huck wrote:
>>> On Wed, 22 Apr 2015 14:21:36 +0200
>>> Alexander Graf <agraf@suse.de> wrote:
>>>
>>>>> Am 22.04.2015 um 13:40 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
>>>>>
>>>>> On Wed, 22 Apr 2015 11:14:40 +0200
>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>>> On 04/22/2015 10:25 AM, Cornelia Huck wrote:
>>>>>>> On Tue, 21 Apr 2015 21:06:42 +0200
>>>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>>>> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
>>>>>>>>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>>>>>>>>>
>>>>>>>>> We have to enable this flag to support dynamically adding devices to the
>>>>>>>>> sysbus. This change is needed for the the upcoming diag288 watchdog.
>>>>>>>> s390 doesn't have a "sysbus" per se. Please create a new bus type.
>>>>>>> So what's wrong with the sysbus? I don't see why we should be different
>>>>>>> than everyone else.
>>>>>> The idea behind sysbus is that you have MMIO, PIO and IRQ pins
>>>>>> connecting to a PIC. It provides a lot of infrastructure for those
>>>>>> interfaces. S390 doesn't use any of them and instead wants registration
>>>>>> on "diag" interfaces for example which I'd put on the same layer as PIO
>>>>>> or MMIO registration.
>>>>> I don't think a "diag" bus makes sense.
>>>> You don't need a bus necessarily, just a parent class.
>>>>
>>>>> The individual diagnoses are
>>>>> way too heterogenous beyond the fact that they use the same base
>>>>> instruction.
>>>>>
>>>>> So where's the proper place for "misc" devices? My impression was that
>>>>> they can go on the sysbus.
>>>>>
>>>> If you really don't want to create your own class, how about you inherit from the DeviceState class?
>>> I tried that for the watchdog, and it certainly works, but some things
>>> end up odd:
>>>
>>> - in 'info qtree', the watchdog device does not show up at all
>> Please try "info qom-tree". Andreas also has a patch outstanding that
>> shows properties along the way with a verbose switch.
> While it does show up in info qom-tree, is hiding it from info qtree a
> good idea? I'd think that it is still widely used.

It's not really about hiding it, but just about putting it at a 
different location. S390 won't be the only target that slowly moves to 
non-qdev'ed devices, so this is a problem that we'll need to solve 
regardless.

>
>>> - in the list of devices printed by "-device help", diag288 is now the
>>>     only device without any bus
>> But it's not attached to a bus, so that's reasonable, no?
> Hm. Are there bus-less devices on other platforms?

IIUC Andreas wants to move CPUs to bus-less devices. I'm sure there are 
more to come.

That said, if you feel more comfortable with a bus, just create an 
artificial s390 bus for "s390 platform devices".

>
>>> I would have thought that any device not attached to a specialized bus
>>> should end up on the main system bus, which brings me back to adding it
>>> as a sysbus device ;)
>> Not really, sysbus is QEMU's wording for what Linux calls "platform
>> bus". It's where devices go to that are attached to MMIO/PIO/IRQ lines
>> via some fabric that we don't model.
> But in practice sysbus seems to be more like a catch-all: On s390x,
> there are already things like the flic, various sclp-related devices,
> the virtio bridges or the ipl device sitting on the sysbus. Should they
> really be thrown out from the sysbus and dangle as bus-less devices? I
> think there is a case to be made for a catch-all bus, even if it is not
> the sysbus.

The problem is that before QOM sysbus was the lowest common denominator 
that we had. Now with QOM, we're only slowly starting to get the ability 
to have devices that are not attached to a bus.

>
>> The target for devices that are not the above we now have non-bus'ed
>> devices.
> I'm afraid I can't parse that sentence :)

FWIW you're supposed to use direct, non-bus'ed QOM devices for devices 
that don't sit on a bus now. Before this was not possible, now it is :).

I don't feel incredibly strongly about this, but I just consider it 
awkward to plug s390 specific devices into a bus that implements 
everything s390 doesn't do :).


Alex

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Cornelia Huck]

On Mon, 27 Apr 2015 17:28:29 +0200
Alexander Graf <agraf@suse.de> wrote:

> On 04/27/2015 04:19 PM, Cornelia Huck wrote:
> > On Mon, 27 Apr 2015 15:57:04 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> >
> >> On 04/24/2015 11:07 AM, Cornelia Huck wrote:
> >>> On Wed, 22 Apr 2015 14:21:36 +0200
> >>> Alexander Graf <agraf@suse.de> wrote:
> >>>
> >>>>> Am 22.04.2015 um 13:40 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
> >>>>>
> >>>>> On Wed, 22 Apr 2015 11:14:40 +0200
> >>>>> Alexander Graf <agraf@suse.de> wrote:
> >>>>>
> >>>>>>> On 04/22/2015 10:25 AM, Cornelia Huck wrote:
> >>>>>>> On Tue, 21 Apr 2015 21:06:42 +0200
> >>>>>>> Alexander Graf <agraf@suse.de> wrote:
> >>>>>>>
> >>>>>>>>> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
> >>>>>>>>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
> >>>>>>>>>
> >>>>>>>>> We have to enable this flag to support dynamically adding devices to the
> >>>>>>>>> sysbus. This change is needed for the the upcoming diag288 watchdog.
> >>>>>>>> s390 doesn't have a "sysbus" per se. Please create a new bus type.
> >>>>>>> So what's wrong with the sysbus? I don't see why we should be different
> >>>>>>> than everyone else.
> >>>>>> The idea behind sysbus is that you have MMIO, PIO and IRQ pins
> >>>>>> connecting to a PIC. It provides a lot of infrastructure for those
> >>>>>> interfaces. S390 doesn't use any of them and instead wants registration
> >>>>>> on "diag" interfaces for example which I'd put on the same layer as PIO
> >>>>>> or MMIO registration.
> >>>>> I don't think a "diag" bus makes sense.
> >>>> You don't need a bus necessarily, just a parent class.
> >>>>
> >>>>> The individual diagnoses are
> >>>>> way too heterogenous beyond the fact that they use the same base
> >>>>> instruction.
> >>>>>
> >>>>> So where's the proper place for "misc" devices? My impression was that
> >>>>> they can go on the sysbus.
> >>>>>
> >>>> If you really don't want to create your own class, how about you inherit from the DeviceState class?
> >>> I tried that for the watchdog, and it certainly works, but some things
> >>> end up odd:
> >>>
> >>> - in 'info qtree', the watchdog device does not show up at all
> >> Please try "info qom-tree". Andreas also has a patch outstanding that
> >> shows properties along the way with a verbose switch.
> > While it does show up in info qom-tree, is hiding it from info qtree a
> > good idea? I'd think that it is still widely used.
> 
> It's not really about hiding it, but just about putting it at a 
> different location. S390 won't be the only target that slowly moves to 
> non-qdev'ed devices, so this is a problem that we'll need to solve 
> regardless.

I'm just a bit uncomfortable with devices not showing up in info qtree.
info qom-tree is still too new :)

> 
> >
> >>> - in the list of devices printed by "-device help", diag288 is now the
> >>>     only device without any bus
> >> But it's not attached to a bus, so that's reasonable, no?
> > Hm. Are there bus-less devices on other platforms?
> 
> IIUC Andreas wants to move CPUs to bus-less devices. I'm sure there are 
> more to come.
> 
> That said, if you feel more comfortable with a bus, just create an 
> artificial s390 bus for "s390 platform devices".

Might make sense for now. I'm not really sure where I'd want to plug in
the devices alternatively.

> 
> >
> >>> I would have thought that any device not attached to a specialized bus
> >>> should end up on the main system bus, which brings me back to adding it
> >>> as a sysbus device ;)
> >> Not really, sysbus is QEMU's wording for what Linux calls "platform
> >> bus". It's where devices go to that are attached to MMIO/PIO/IRQ lines
> >> via some fabric that we don't model.
> > But in practice sysbus seems to be more like a catch-all: On s390x,
> > there are already things like the flic, various sclp-related devices,
> > the virtio bridges or the ipl device sitting on the sysbus. Should they
> > really be thrown out from the sysbus and dangle as bus-less devices? I
> > think there is a case to be made for a catch-all bus, even if it is not
> > the sysbus.
> 
> The problem is that before QOM sysbus was the lowest common denominator 
> that we had. Now with QOM, we're only slowly starting to get the ability 
> to have devices that are not attached to a bus.
> 
> >
> >> The target for devices that are not the above we now have non-bus'ed
> >> devices.
> > I'm afraid I can't parse that sentence :)
> 
> FWIW you're supposed to use direct, non-bus'ed QOM devices for devices 
> that don't sit on a bus now. Before this was not possible, now it is :).
> 
> I don't feel incredibly strongly about this, but I just consider it 
> awkward to plug s390 specific devices into a bus that implements 
> everything s390 doesn't do :).

Let me see if an s390 platform bus does it for us. Machine options or
so don't look particulary attractive to me right now.

Re: [Qemu-devel] [PATCH 1/6] s390x/virtio-ccw: enable has_dynamic_sysbus

[Markus Armbruster]

Cornelia Huck <cornelia.huck@de.ibm.com> writes:

> On Mon, 27 Apr 2015 17:28:29 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>> On 04/27/2015 04:19 PM, Cornelia Huck wrote:
>> > On Mon, 27 Apr 2015 15:57:04 +0200
>> > Alexander Graf <agraf@suse.de> wrote:
>> >
>> >> On 04/24/2015 11:07 AM, Cornelia Huck wrote:
>> >>> On Wed, 22 Apr 2015 14:21:36 +0200
>> >>> Alexander Graf <agraf@suse.de> wrote:
>> >>>
>> >>>>> Am 22.04.2015 um 13:40 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
>> >>>>>
>> >>>>> On Wed, 22 Apr 2015 11:14:40 +0200
>> >>>>> Alexander Graf <agraf@suse.de> wrote:
>> >>>>>
>> >>>>>>> On 04/22/2015 10:25 AM, Cornelia Huck wrote:
>> >>>>>>> On Tue, 21 Apr 2015 21:06:42 +0200
>> >>>>>>> Alexander Graf <agraf@suse.de> wrote:
>> >>>>>>>
>> >>>>>>>>> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
>> >>>>>>>>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>> >>>>>>>>>
>> >>>>>>>>> We have to enable this flag to support dynamically adding
>> >>>>>>>>> devices to the
>> >>>>>>>>> sysbus. This change is needed for the the upcoming diag288 watchdog.
>> >>>>>>>> s390 doesn't have a "sysbus" per se. Please create a new bus type.
>> >>>>>>> So what's wrong with the sysbus? I don't see why we should
>> >>>>>>> be different
>> >>>>>>> than everyone else.
>> >>>>>> The idea behind sysbus is that you have MMIO, PIO and IRQ pins
>> >>>>>> connecting to a PIC. It provides a lot of infrastructure for those
>> >>>>>> interfaces. S390 doesn't use any of them and instead wants registration
>> >>>>>> on "diag" interfaces for example which I'd put on the same layer as PIO
>> >>>>>> or MMIO registration.
>> >>>>> I don't think a "diag" bus makes sense.
>> >>>> You don't need a bus necessarily, just a parent class.
>> >>>>
>> >>>>> The individual diagnoses are
>> >>>>> way too heterogenous beyond the fact that they use the same base
>> >>>>> instruction.
>> >>>>>
>> >>>>> So where's the proper place for "misc" devices? My impression was that
>> >>>>> they can go on the sysbus.
>> >>>>>
>> >>>> If you really don't want to create your own class, how about
>> >>>> you inherit from the DeviceState class?
>> >>> I tried that for the watchdog, and it certainly works, but some things
>> >>> end up odd:
>> >>>
>> >>> - in 'info qtree', the watchdog device does not show up at all
>> >> Please try "info qom-tree". Andreas also has a patch outstanding that
>> >> shows properties along the way with a verbose switch.
>> > While it does show up in info qom-tree, is hiding it from info qtree a
>> > good idea? I'd think that it is still widely used.
>> 
>> It's not really about hiding it, but just about putting it at a 
>> different location. S390 won't be the only target that slowly moves to 
>> non-qdev'ed devices, so this is a problem that we'll need to solve 
>> regardless.
>
> I'm just a bit uncomfortable with devices not showing up in info qtree.
> info qom-tree is still too new :)

That feeling will pass :)

>> >>> - in the list of devices printed by "-device help", diag288 is now the
>> >>>     only device without any bus
>> >> But it's not attached to a bus, so that's reasonable, no?
>> > Hm. Are there bus-less devices on other platforms?
>> 
>> IIUC Andreas wants to move CPUs to bus-less devices. I'm sure there are 
>> more to come.
>> 
>> That said, if you feel more comfortable with a bus, just create an 
>> artificial s390 bus for "s390 platform devices".
>
> Might make sense for now. I'm not really sure where I'd want to plug in
> the devices alternatively.

I'd like to encourage you to embrace TYPE_DEVICE.

A qbus should model a real-world bus.  Stretching the "bus" concept
somewhat is okay.  Useful test: what have devices plugging into this bus
type in common?  If they share something interesting, such as the way
they connect to the rest of the machine, then modelling their
commonalities as a qbus can make sense.

What do sysbus devices have in common?  Anything regarding how they
connect?  Not really: devices expose pins, and to connect them you have
to wire them up, but that's true for *any* device.

Sysbus only exists because qdev's design required devices to plug into a
qbus.  We've since relaxed that silly design assumption.  Let's not keep
inventing silly buses just because we used to have to.

Whether an s390 platform bus would be silly I can't judge.  What would
s390 platform bus devices have in common?

>> >>> I would have thought that any device not attached to a specialized bus
>> >>> should end up on the main system bus, which brings me back to adding it
>> >>> as a sysbus device ;)
>> >> Not really, sysbus is QEMU's wording for what Linux calls "platform
>> >> bus". It's where devices go to that are attached to MMIO/PIO/IRQ lines
>> >> via some fabric that we don't model.
>> > But in practice sysbus seems to be more like a catch-all: On s390x,
>> > there are already things like the flic, various sclp-related devices,
>> > the virtio bridges or the ipl device sitting on the sysbus. Should they
>> > really be thrown out from the sysbus and dangle as bus-less devices? I
>> > think there is a case to be made for a catch-all bus, even if it is not
>> > the sysbus.
>> 
>> The problem is that before QOM sysbus was the lowest common denominator 
>> that we had. Now with QOM, we're only slowly starting to get the ability 
>> to have devices that are not attached to a bus.
>> 
>> >
>> >> The target for devices that are not the above we now have non-bus'ed
>> >> devices.
>> > I'm afraid I can't parse that sentence :)
>> 
>> FWIW you're supposed to use direct, non-bus'ed QOM devices for devices 
>> that don't sit on a bus now. Before this was not possible, now it is :).
>> 
>> I don't feel incredibly strongly about this, but I just consider it 
>> awkward to plug s390 specific devices into a bus that implements 
>> everything s390 doesn't do :).
>
> Let me see if an s390 platform bus does it for us. Machine options or
> so don't look particulary attractive to me right now.

Is it just due to unfamliarity?  That's temporary.

[Qemu-devel] [PATCH 2/6] watchdog: Add watchdog device diag288 to the sysbus

[Cornelia Huck]

From: Xu Wang <gesaint@linux.vnet.ibm.com>

A new sysbus device for diag288 watchdog, it monitors the kvm guest
status, and take corresponding actions when it detects that the guest
is not responding.

Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 default-configs/s390x-softmmu.mak |   1 +
 hw/watchdog/Makefile.objs         |   1 +
 hw/watchdog/wdt_diag288.c         | 110 ++++++++++++++++++++++++++++++++++++++
 include/hw/watchdog/wdt_diag288.h |  36 +++++++++++++
 qemu-options.hx                   |   6 ++-
 5 files changed, 152 insertions(+), 2 deletions(-)
 create mode 100644 hw/watchdog/wdt_diag288.c
 create mode 100644 include/hw/watchdog/wdt_diag288.h

diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
index f9e13f1..36e15de 100644
--- a/default-configs/s390x-softmmu.mak
+++ b/default-configs/s390x-softmmu.mak
@@ -4,3 +4,4 @@ CONFIG_VIRTIO=y
 CONFIG_SCLPCONSOLE=y
 CONFIG_S390_FLIC=y
 CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
+CONFIG_WDT_DIAG288=y
diff --git a/hw/watchdog/Makefile.objs b/hw/watchdog/Makefile.objs
index 4b0374a..72e3ffd 100644
--- a/hw/watchdog/Makefile.objs
+++ b/hw/watchdog/Makefile.objs
@@ -1,3 +1,4 @@
 common-obj-y += watchdog.o
 common-obj-$(CONFIG_WDT_IB6300ESB) += wdt_i6300esb.o
 common-obj-$(CONFIG_WDT_IB700) += wdt_ib700.o
+common-obj-$(CONFIG_WDT_DIAG288) += wdt_diag288.o
diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c
new file mode 100644
index 0000000..81a0e30
--- /dev/null
+++ b/hw/watchdog/wdt_diag288.c
@@ -0,0 +1,110 @@
+/*
+ * watchdog device diag288 support
+ *
+ * Copyright IBM, Corp. 2015
+ *
+ * Authors:
+ *  Xu Wang <gesaint@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at your
+ * option) any later version.  See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "sysemu/watchdog.h"
+#include "hw/sysbus.h"
+#include "qemu/timer.h"
+#include "hw/watchdog/wdt_diag288.h"
+
+static WatchdogTimerModel model = {
+    .wdt_name = TYPE_WDT_DIAG288,
+    .wdt_description = "diag288 device for s390x platform",
+};
+
+static void wdt_diag288_reset(DeviceState *dev)
+{
+    DIAG288State *diag288 = DIAG288(dev);
+
+    diag288->enabled = false;
+    timer_del(diag288->timer);
+}
+
+static void diag288_timer_expired(void *dev)
+{
+    qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n");
+    watchdog_perform_action();
+    wdt_diag288_reset(dev);
+}
+
+static int wdt_diag288_handle_timer(DIAG288State *diag288,
+                                     uint64_t func, uint64_t timeout)
+{
+    switch (func) {
+    case WDT_DIAG288_INIT:
+        diag288->enabled = true;
+        /* fall through */
+    case WDT_DIAG288_CHANGE:
+        if (!diag288->enabled) {
+            return -1;
+        }
+        timer_mod(diag288->timer,
+                  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+                  timeout * get_ticks_per_sec());
+        break;
+    case WDT_DIAG288_CANCEL:
+        if (!diag288->enabled) {
+            return -1;
+        }
+        diag288->enabled = false;
+        timer_del(diag288->timer);
+        break;
+    default:
+        return -1;
+    }
+
+    return 0;
+}
+
+static void wdt_diag288_realize(DeviceState *dev, Error **errp)
+{
+    DIAG288State *diag288 = DIAG288(dev);
+
+    diag288->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, diag288_timer_expired,
+                                  dev);
+}
+
+static void wdt_diag288_unrealize(DeviceState *dev, Error **errp)
+{
+    DIAG288State *diag288 = DIAG288(dev);
+
+    timer_del(diag288->timer);
+    timer_free(diag288->timer);
+}
+
+static void wdt_diag288_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    DIAG288Class *diag288 = DIAG288_CLASS(klass);
+
+    dc->realize = wdt_diag288_realize;
+    dc->unrealize = wdt_diag288_unrealize;
+    dc->reset = wdt_diag288_reset;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+    diag288->handle_timer = wdt_diag288_handle_timer;
+}
+
+static const TypeInfo wdt_diag288_info = {
+    .class_init = wdt_diag288_class_init,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .name  = TYPE_WDT_DIAG288,
+    .instance_size  = sizeof(DIAG288State),
+    .class_size = sizeof(DIAG288Class),
+};
+
+static void wdt_diag288_register_types(void)
+{
+    watchdog_add_model(&model);
+    type_register_static(&wdt_diag288_info);
+}
+
+type_init(wdt_diag288_register_types)
diff --git a/include/hw/watchdog/wdt_diag288.h b/include/hw/watchdog/wdt_diag288.h
new file mode 100644
index 0000000..3d076b0
--- /dev/null
+++ b/include/hw/watchdog/wdt_diag288.h
@@ -0,0 +1,36 @@
+#ifndef WDT_DIAG288_H
+#define WDT_DIAG288_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_WDT_DIAG288 "diag288"
+#define DIAG288(obj) \
+    OBJECT_CHECK(DIAG288State, (obj), TYPE_WDT_DIAG288)
+#define DIAG288_CLASS(klass) \
+    OBJECT_CLASS_CHECK(DIAG288Class, (klass), TYPE_WDT_DIAG288)
+#define DIAG288_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(DIAG288Class, (obj), TYPE_WDT_DIAG288)
+
+#define WDT_DIAG288_INIT      0
+#define WDT_DIAG288_CHANGE    1
+#define WDT_DIAG288_CANCEL    2
+
+typedef struct DIAG288State {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    QEMUTimer *timer;
+    bool enabled;
+
+    /*< public >*/
+} DIAG288State;
+
+typedef struct DIAG288Class {
+    /*< private >*/
+    SysBusDeviceClass parent_class;
+
+    /*< public >*/
+    int (*handle_timer)(DIAG288State *dev,
+                        uint64_t func, uint64_t timeout);
+} DIAG288Class;
+
+#endif  /* WDT_DIAG288_H */
diff --git a/qemu-options.hx b/qemu-options.hx
index 319d971..7ef8f50 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3115,7 +3115,7 @@ when the shift value is high (how high depends on the host machine).
 ETEXI
 
 DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
-    "-watchdog i6300esb|ib700\n" \
+    "-watchdog i6300esb|ib700|diag288\n" \
     "                enable virtual hardware watchdog [default=none]\n",
     QEMU_ARCH_ALL)
 STEXI
@@ -3129,7 +3129,9 @@ The @var{model} is the model of hardware watchdog to emulate.  Choices
 for model are: @code{ib700} (iBASE 700) which is a very simple ISA
 watchdog with a single timer, or @code{i6300esb} (Intel 6300ESB I/O
 controller hub) which is a much more featureful PCI-based dual-timer
-watchdog.  Choose a model for which your guest has drivers.
+watchdog. @code{diag288} is a watchdog device only available on s390x
+(for now only supported by KVM). Choose a model for which your guest
+has drivers.
 
 Use @code{-watchdog help} to list available hardware models.  Only one
 watchdog can be enabled for a guest.
-- 
2.3.5

Re: [Qemu-devel] [PATCH 2/6] watchdog: Add watchdog device diag288 to the sysbus

[Markus Armbruster]

Cornelia Huck <cornelia.huck@de.ibm.com> writes:

> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>
> A new sysbus device for diag288 watchdog, it monitors the kvm guest
> status, and take corresponding actions when it detects that the guest
> is not responding.
>
> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  default-configs/s390x-softmmu.mak |   1 +
>  hw/watchdog/Makefile.objs         |   1 +
>  hw/watchdog/wdt_diag288.c         | 110 ++++++++++++++++++++++++++++++++++++++
>  include/hw/watchdog/wdt_diag288.h |  36 +++++++++++++
>  qemu-options.hx                   |   6 ++-
>  5 files changed, 152 insertions(+), 2 deletions(-)
>  create mode 100644 hw/watchdog/wdt_diag288.c
>  create mode 100644 include/hw/watchdog/wdt_diag288.h
>
> diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
> index f9e13f1..36e15de 100644
> --- a/default-configs/s390x-softmmu.mak
> +++ b/default-configs/s390x-softmmu.mak
> @@ -4,3 +4,4 @@ CONFIG_VIRTIO=y
>  CONFIG_SCLPCONSOLE=y
>  CONFIG_S390_FLIC=y
>  CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
> +CONFIG_WDT_DIAG288=y
> diff --git a/hw/watchdog/Makefile.objs b/hw/watchdog/Makefile.objs
> index 4b0374a..72e3ffd 100644
> --- a/hw/watchdog/Makefile.objs
> +++ b/hw/watchdog/Makefile.objs
> @@ -1,3 +1,4 @@
>  common-obj-y += watchdog.o
>  common-obj-$(CONFIG_WDT_IB6300ESB) += wdt_i6300esb.o
>  common-obj-$(CONFIG_WDT_IB700) += wdt_ib700.o
> +common-obj-$(CONFIG_WDT_DIAG288) += wdt_diag288.o
> diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c
> new file mode 100644
> index 0000000..81a0e30
> --- /dev/null
> +++ b/hw/watchdog/wdt_diag288.c
> @@ -0,0 +1,110 @@
> +/*
> + * watchdog device diag288 support
> + *
> + * Copyright IBM, Corp. 2015
> + *
> + * Authors:
> + *  Xu Wang <gesaint@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at your
> + * option) any later version.  See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "sysemu/watchdog.h"
> +#include "hw/sysbus.h"
> +#include "qemu/timer.h"
> +#include "hw/watchdog/wdt_diag288.h"
> +
> +static WatchdogTimerModel model = {
> +    .wdt_name = TYPE_WDT_DIAG288,
> +    .wdt_description = "diag288 device for s390x platform",
> +};
> +
> +static void wdt_diag288_reset(DeviceState *dev)
> +{
> +    DIAG288State *diag288 = DIAG288(dev);
> +
> +    diag288->enabled = false;
> +    timer_del(diag288->timer);
> +}
> +
> +static void diag288_timer_expired(void *dev)
> +{
> +    qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n");
> +    watchdog_perform_action();
> +    wdt_diag288_reset(dev);
> +}
> +
> +static int wdt_diag288_handle_timer(DIAG288State *diag288,
> +                                     uint64_t func, uint64_t timeout)
> +{
> +    switch (func) {
> +    case WDT_DIAG288_INIT:
> +        diag288->enabled = true;
> +        /* fall through */
> +    case WDT_DIAG288_CHANGE:
> +        if (!diag288->enabled) {
> +            return -1;
> +        }
> +        timer_mod(diag288->timer,
> +                  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +                  timeout * get_ticks_per_sec());
> +        break;
> +    case WDT_DIAG288_CANCEL:
> +        if (!diag288->enabled) {
> +            return -1;
> +        }
> +        diag288->enabled = false;
> +        timer_del(diag288->timer);
> +        break;
> +    default:
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static void wdt_diag288_realize(DeviceState *dev, Error **errp)
> +{
> +    DIAG288State *diag288 = DIAG288(dev);
> +
> +    diag288->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, diag288_timer_expired,
> +                                  dev);
> +}
> +
> +static void wdt_diag288_unrealize(DeviceState *dev, Error **errp)
> +{
> +    DIAG288State *diag288 = DIAG288(dev);
> +
> +    timer_del(diag288->timer);
> +    timer_free(diag288->timer);
> +}
> +
> +static void wdt_diag288_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    DIAG288Class *diag288 = DIAG288_CLASS(klass);
> +
> +    dc->realize = wdt_diag288_realize;
> +    dc->unrealize = wdt_diag288_unrealize;
> +    dc->reset = wdt_diag288_reset;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +    diag288->handle_timer = wdt_diag288_handle_timer;
> +}
> +
> +static const TypeInfo wdt_diag288_info = {
> +    .class_init = wdt_diag288_class_init,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .name  = TYPE_WDT_DIAG288,
> +    .instance_size  = sizeof(DIAG288State),
> +    .class_size = sizeof(DIAG288Class),
> +};
> +
> +static void wdt_diag288_register_types(void)
> +{
> +    watchdog_add_model(&model);
> +    type_register_static(&wdt_diag288_info);
> +}
> +
> +type_init(wdt_diag288_register_types)
> diff --git a/include/hw/watchdog/wdt_diag288.h b/include/hw/watchdog/wdt_diag288.h
> new file mode 100644
> index 0000000..3d076b0
> --- /dev/null
> +++ b/include/hw/watchdog/wdt_diag288.h
> @@ -0,0 +1,36 @@
> +#ifndef WDT_DIAG288_H
> +#define WDT_DIAG288_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_WDT_DIAG288 "diag288"
> +#define DIAG288(obj) \
> +    OBJECT_CHECK(DIAG288State, (obj), TYPE_WDT_DIAG288)
> +#define DIAG288_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(DIAG288Class, (klass), TYPE_WDT_DIAG288)
> +#define DIAG288_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(DIAG288Class, (obj), TYPE_WDT_DIAG288)
> +
> +#define WDT_DIAG288_INIT      0
> +#define WDT_DIAG288_CHANGE    1
> +#define WDT_DIAG288_CANCEL    2
> +
> +typedef struct DIAG288State {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    QEMUTimer *timer;
> +    bool enabled;
> +
> +    /*< public >*/
> +} DIAG288State;
> +
> +typedef struct DIAG288Class {
> +    /*< private >*/
> +    SysBusDeviceClass parent_class;
> +
> +    /*< public >*/
> +    int (*handle_timer)(DIAG288State *dev,
> +                        uint64_t func, uint64_t timeout);
> +} DIAG288Class;
> +
> +#endif  /* WDT_DIAG288_H */
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 319d971..7ef8f50 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3115,7 +3115,7 @@ when the shift value is high (how high depends on the host machine).
>  ETEXI
>  
>  DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
> -    "-watchdog i6300esb|ib700\n" \
> +    "-watchdog i6300esb|ib700|diag288\n" \
>      "                enable virtual hardware watchdog [default=none]\n",
>      QEMU_ARCH_ALL)
>  STEXI

Preexisting: the help text assumes that all these watchdogs are actually
compiled in.  Generally not the case.  Instead of adding another one
that's generally not available, please change the help text in a
preliminary patch to something like

DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
    "-watchdog model\n" \
    "                enable virtual hardware watchdog [default none]\n",
    QEMU_ARCH_ALL)
STEXI
@item -watchdog @var{model}
@findex -watchdog
Create a virtual hardware watchdog device.  Once enabled (by a guest
action), the watchdog must be periodically polled by an agent inside
the guest or else the guest will be restarted.

The @var{model} is the model of hardware watchdog to emulate.  Use
@code{-watchdog help} to list available hardware models.  Only one
watchdog can be enabled for a guest.

The following models may be available:
@table @option
@item ib700
iBASE 700 is a very simple ISA watchdog with a single timer.
@item i6300esb
Intel 6300ESB I/O controller hub is a much more featureful PCI-based
dual-timer watchdog.
@end table
ETEXI

> @@ -3129,7 +3129,9 @@ The @var{model} is the model of hardware watchdog to emulate.  Choices
>  for model are: @code{ib700} (iBASE 700) which is a very simple ISA
>  watchdog with a single timer, or @code{i6300esb} (Intel 6300ESB I/O
>  controller hub) which is a much more featureful PCI-based dual-timer
> -watchdog.  Choose a model for which your guest has drivers.
> +watchdog. @code{diag288} is a watchdog device only available on s390x
> +(for now only supported by KVM). Choose a model for which your guest
> +has drivers.
>  
>  Use @code{-watchdog help} to list available hardware models.  Only one
>  watchdog can be enabled for a guest.

Recommend to split this patch: one patch to add the device model, and
another one to wire up -watchdog.

Re: [Qemu-devel] [PATCH 2/6] watchdog: Add watchdog device diag288 to the sysbus

[Cornelia Huck]

On Wed, 29 Apr 2015 14:43:56 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> 
> > From: Xu Wang <gesaint@linux.vnet.ibm.com>
> >
> > A new sysbus device for diag288 watchdog, it monitors the kvm guest
> > status, and take corresponding actions when it detects that the guest
> > is not responding.
> >
> > Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
> > Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> >  default-configs/s390x-softmmu.mak |   1 +
> >  hw/watchdog/Makefile.objs         |   1 +
> >  hw/watchdog/wdt_diag288.c         | 110 ++++++++++++++++++++++++++++++++++++++
> >  include/hw/watchdog/wdt_diag288.h |  36 +++++++++++++
> >  qemu-options.hx                   |   6 ++-
> >  5 files changed, 152 insertions(+), 2 deletions(-)
> >  create mode 100644 hw/watchdog/wdt_diag288.c
> >  create mode 100644 include/hw/watchdog/wdt_diag288.h

> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 319d971..7ef8f50 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -3115,7 +3115,7 @@ when the shift value is high (how high depends on the host machine).
> >  ETEXI
> >  
> >  DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
> > -    "-watchdog i6300esb|ib700\n" \
> > +    "-watchdog i6300esb|ib700|diag288\n" \
> >      "                enable virtual hardware watchdog [default=none]\n",
> >      QEMU_ARCH_ALL)
> >  STEXI
> 
> Preexisting: the help text assumes that all these watchdogs are actually
> compiled in.  Generally not the case.  Instead of adding another one
> that's generally not available, please change the help text in a
> preliminary patch to something like
> 
> DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
>     "-watchdog model\n" \
>     "                enable virtual hardware watchdog [default none]\n",
>     QEMU_ARCH_ALL)
> STEXI
> @item -watchdog @var{model}
> @findex -watchdog
> Create a virtual hardware watchdog device.  Once enabled (by a guest
> action), the watchdog must be periodically polled by an agent inside
> the guest or else the guest will be restarted.
> 
> The @var{model} is the model of hardware watchdog to emulate.  Use
> @code{-watchdog help} to list available hardware models.  Only one
> watchdog can be enabled for a guest.

Maybe keep "Choose a watchdog for which your guest has drivers."?

> 
> The following models may be available:
> @table @option
> @item ib700
> iBASE 700 is a very simple ISA watchdog with a single timer.
> @item i6300esb
> Intel 6300ESB I/O controller hub is a much more featureful PCI-based
> dual-timer watchdog.
> @end table
> ETEXI

I like that.

> 
> > @@ -3129,7 +3129,9 @@ The @var{model} is the model of hardware watchdog to emulate.  Choices
> >  for model are: @code{ib700} (iBASE 700) which is a very simple ISA
> >  watchdog with a single timer, or @code{i6300esb} (Intel 6300ESB I/O
> >  controller hub) which is a much more featureful PCI-based dual-timer
> > -watchdog.  Choose a model for which your guest has drivers.
> > +watchdog. @code{diag288} is a watchdog device only available on s390x
> > +(for now only supported by KVM). Choose a model for which your guest
> > +has drivers.

Hm... let's make that

@item diag288
A virtual watchdog for s390x backed by the diagnose 288 hypercall (currently
KVM only).

?

> >  
> >  Use @code{-watchdog help} to list available hardware models.  Only one
> >  watchdog can be enabled for a guest.
> 
> Recommend to split this patch: one patch to add the device model, and
> another one to wire up -watchdog.

So we end up with three patches:
- clean up help text
- add device (probably as a plain device)
- wire up -watchdog

Sounds sensible.

Re: [Qemu-devel] [PATCH 2/6] watchdog: Add watchdog device diag288 to the sysbus

[Cornelia Huck]

On Fri, 17 Apr 2015 09:52:37 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> From: Xu Wang <gesaint@linux.vnet.ibm.com>
> 
> A new sysbus device for diag288 watchdog, it monitors the kvm guest
> status, and take corresponding actions when it detects that the guest
> is not responding.
> 
> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  default-configs/s390x-softmmu.mak |   1 +
>  hw/watchdog/Makefile.objs         |   1 +
>  hw/watchdog/wdt_diag288.c         | 110 ++++++++++++++++++++++++++++++++++++++
>  include/hw/watchdog/wdt_diag288.h |  36 +++++++++++++
>  qemu-options.hx                   |   6 ++-
>  5 files changed, 152 insertions(+), 2 deletions(-)
>  create mode 100644 hw/watchdog/wdt_diag288.c
>  create mode 100644 include/hw/watchdog/wdt_diag288.h

OK, after some deliberation, I think it is best to model the diag288
watchdog as a bus-less device: The other watchdogs are modelled as
devices as well, and it is probably a good idea to stay with this model
as other parties may have made assumptions based on this.

Xu, can you please switch the diag288 device to TYPE_DEVICE and address
Markus' comments regarding the help text as well?

[Qemu-devel] [PATCH 3/6] s390/kvm: diag288 instruction interception and handling

[Cornelia Huck]

From: Xu Wang <gesaint@linux.vnet.ibm.com>

Intercept the diag288 requests from kvm guests, and hand the
requested command to the diag288 watchdog device for further
handling.

Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 target-s390x/cpu.h         |  1 +
 target-s390x/kvm.c         | 18 ++++++++++++++++++
 target-s390x/misc_helper.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index ba7d250..f5cafc3 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -1059,6 +1059,7 @@ uint32_t set_cc_nz_f128(float128 v);
 
 /* misc_helper.c */
 #ifndef CONFIG_USER_ONLY
+int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3);
 void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3);
 #endif
 void program_interrupt(CPUS390XState *env, uint32_t code, int ilen);
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index b8703b8..32ed4ab 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -97,6 +97,7 @@
 #define PRIV_E3_MPCIFC                  0xd0
 #define PRIV_E3_STPCIFC                 0xd4
 
+#define DIAG_TIMEREVENT                 0x288
 #define DIAG_IPL                        0x308
 #define DIAG_KVM_HYPERCALL              0x500
 #define DIAG_KVM_BREAKPOINT             0x501
@@ -1215,6 +1216,20 @@ static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
     return ret;
 }
 
+static void kvm_handle_diag_288(S390CPU *cpu, struct kvm_run *run)
+{
+    uint64_t r1, r3;
+    int rc;
+
+    cpu_synchronize_state(CPU(cpu));
+    r1 = (run->s390_sieic.ipa & 0x00f0) >> 4;
+    r3 = run->s390_sieic.ipa & 0x000f;
+    rc = handle_diag_288(&cpu->env, r1, r3);
+    if (rc == -1) {
+        enter_pgmcheck(cpu, PGM_SPECIFICATION);
+    }
+}
+
 static void kvm_handle_diag_308(S390CPU *cpu, struct kvm_run *run)
 {
     uint64_t r1, r3;
@@ -1254,6 +1269,9 @@ static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
      */
     func_code = decode_basedisp_rs(&cpu->env, ipb, NULL) & DIAG_KVM_CODE_MASK;
     switch (func_code) {
+    case DIAG_TIMEREVENT:
+        kvm_handle_diag_288(cpu, run);
+        break;
     case DIAG_IPL:
         kvm_handle_diag_308(cpu, run);
         break;
diff --git a/target-s390x/misc_helper.c b/target-s390x/misc_helper.c
index e1007fa..88d96bf 100644
--- a/target-s390x/misc_helper.c
+++ b/target-s390x/misc_helper.c
@@ -30,6 +30,7 @@
 #include <linux/kvm.h>
 #endif
 #include "exec/cpu_ldst.h"
+#include "hw/watchdog/wdt_diag288.h"
 
 #if !defined(CONFIG_USER_ONLY)
 #include "sysemu/cpus.h"
@@ -153,6 +154,38 @@ static int load_normal_reset(S390CPU *cpu)
     return 0;
 }
 
+int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
+{
+    uint64_t func = env->regs[r1];
+    uint64_t timeout = env->regs[r1 + 1];
+    uint64_t action = env->regs[r3];
+    Object *obj;
+    DIAG288State *diag288;
+    DIAG288Class *diag288_class;
+
+    if (r1 % 2) {
+        return -1;
+    }
+
+    if (action != 0) {
+        return -1;
+    }
+
+    /* Timeout must be more than 15 seconds except for timer deletion */
+    if (func != WDT_DIAG288_CANCEL && timeout < 15) {
+        return -1;
+    }
+
+    obj = object_resolve_path_type("", TYPE_WDT_DIAG288, NULL);
+    if (!obj) {
+        return -1;
+    }
+
+    diag288 = DIAG288(obj);
+    diag288_class = DIAG288_GET_CLASS(diag288);
+    return diag288_class->handle_timer(diag288, func, timeout);
+}
+
 #define DIAG_308_RC_OK              0x0001
 #define DIAG_308_RC_NO_CONF         0x0102
 #define DIAG_308_RC_INVALID         0x0402
-- 
2.3.5

Re: [Qemu-devel] [PATCH 3/6] s390/kvm: diag288 instruction interception and handling

[Alexander Graf]

On 04/17/2015 09:52 AM, Cornelia Huck wrote:
> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>
> Intercept the diag288 requests from kvm guests, and hand the
> requested command to the diag288 watchdog device for further
> handling.
>
> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

We're getting a lot of random devices allocating diag intercepts. Can't 
we make this an actual interface, similar to the hypercall registration 
on sPAPR?


Alex

> ---
>   target-s390x/cpu.h         |  1 +
>   target-s390x/kvm.c         | 18 ++++++++++++++++++
>   target-s390x/misc_helper.c | 33 +++++++++++++++++++++++++++++++++
>   3 files changed, 52 insertions(+)
>
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index ba7d250..f5cafc3 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -1059,6 +1059,7 @@ uint32_t set_cc_nz_f128(float128 v);
>   
>   /* misc_helper.c */
>   #ifndef CONFIG_USER_ONLY
> +int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3);
>   void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3);
>   #endif
>   void program_interrupt(CPUS390XState *env, uint32_t code, int ilen);
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index b8703b8..32ed4ab 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -97,6 +97,7 @@
>   #define PRIV_E3_MPCIFC                  0xd0
>   #define PRIV_E3_STPCIFC                 0xd4
>   
> +#define DIAG_TIMEREVENT                 0x288
>   #define DIAG_IPL                        0x308
>   #define DIAG_KVM_HYPERCALL              0x500
>   #define DIAG_KVM_BREAKPOINT             0x501
> @@ -1215,6 +1216,20 @@ static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
>       return ret;
>   }
>   
> +static void kvm_handle_diag_288(S390CPU *cpu, struct kvm_run *run)
> +{
> +    uint64_t r1, r3;
> +    int rc;
> +
> +    cpu_synchronize_state(CPU(cpu));
> +    r1 = (run->s390_sieic.ipa & 0x00f0) >> 4;
> +    r3 = run->s390_sieic.ipa & 0x000f;
> +    rc = handle_diag_288(&cpu->env, r1, r3);
> +    if (rc == -1) {
> +        enter_pgmcheck(cpu, PGM_SPECIFICATION);
> +    }
> +}
> +
>   static void kvm_handle_diag_308(S390CPU *cpu, struct kvm_run *run)
>   {
>       uint64_t r1, r3;
> @@ -1254,6 +1269,9 @@ static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
>        */
>       func_code = decode_basedisp_rs(&cpu->env, ipb, NULL) & DIAG_KVM_CODE_MASK;
>       switch (func_code) {
> +    case DIAG_TIMEREVENT:
> +        kvm_handle_diag_288(cpu, run);
> +        break;
>       case DIAG_IPL:
>           kvm_handle_diag_308(cpu, run);
>           break;
> diff --git a/target-s390x/misc_helper.c b/target-s390x/misc_helper.c
> index e1007fa..88d96bf 100644
> --- a/target-s390x/misc_helper.c
> +++ b/target-s390x/misc_helper.c
> @@ -30,6 +30,7 @@
>   #include <linux/kvm.h>
>   #endif
>   #include "exec/cpu_ldst.h"
> +#include "hw/watchdog/wdt_diag288.h"
>   
>   #if !defined(CONFIG_USER_ONLY)
>   #include "sysemu/cpus.h"
> @@ -153,6 +154,38 @@ static int load_normal_reset(S390CPU *cpu)
>       return 0;
>   }
>   
> +int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
> +{
> +    uint64_t func = env->regs[r1];
> +    uint64_t timeout = env->regs[r1 + 1];
> +    uint64_t action = env->regs[r3];
> +    Object *obj;
> +    DIAG288State *diag288;
> +    DIAG288Class *diag288_class;
> +
> +    if (r1 % 2) {
> +        return -1;
> +    }
> +
> +    if (action != 0) {
> +        return -1;
> +    }
> +
> +    /* Timeout must be more than 15 seconds except for timer deletion */
> +    if (func != WDT_DIAG288_CANCEL && timeout < 15) {
> +        return -1;
> +    }
> +
> +    obj = object_resolve_path_type("", TYPE_WDT_DIAG288, NULL);
> +    if (!obj) {
> +        return -1;
> +    }
> +
> +    diag288 = DIAG288(obj);
> +    diag288_class = DIAG288_GET_CLASS(diag288);
> +    return diag288_class->handle_timer(diag288, func, timeout);
> +}
> +
>   #define DIAG_308_RC_OK              0x0001
>   #define DIAG_308_RC_NO_CONF         0x0102
>   #define DIAG_308_RC_INVALID         0x0402

Re: [Qemu-devel] [PATCH 3/6] s390/kvm: diag288 instruction interception and handling

[Cornelia Huck]

On Tue, 21 Apr 2015 21:09:05 +0200
Alexander Graf <agraf@suse.de> wrote:

> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
> > From: Xu Wang <gesaint@linux.vnet.ibm.com>
> >
> > Intercept the diag288 requests from kvm guests, and hand the
> > requested command to the diag288 watchdog device for further
> > handling.
> >
> > Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
> > Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> We're getting a lot of random devices allocating diag intercepts. Can't 
> we make this an actual interface, similar to the hypercall registration 
> on sPAPR?

I've looked at the sPAPR hcall code, and it seems to basically provide
a table with a nice registration interface (we already use something
similar for the diagnose 500 virtio subcodes, btw.)

While we could move our basic diagnose handling over to a table-like
approach and registering new diagnoses, I think this is orthogonal to
introducing a diag288 watchdog device: It just makes sense to model the
watchdog as a device that just happens to be poked via a diagnose. If
we introduce any further diagnoses to manipulate timing etc., I agree
we don't want to add a device for each of these.

Re: [Qemu-devel] [PATCH 3/6] s390/kvm: diag288 instruction interception and handling

[Alexander Graf]

On 04/22/2015 10:32 AM, Cornelia Huck wrote:
> On Tue, 21 Apr 2015 21:09:05 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
>>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
>>>
>>> Intercept the diag288 requests from kvm guests, and hand the
>>> requested command to the diag288 watchdog device for further
>>> handling.
>>>
>>> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
>>> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> We're getting a lot of random devices allocating diag intercepts. Can't
>> we make this an actual interface, similar to the hypercall registration
>> on sPAPR?
> I've looked at the sPAPR hcall code, and it seems to basically provide
> a table with a nice registration interface (we already use something
> similar for the diagnose 500 virtio subcodes, btw.)
>
> While we could move our basic diagnose handling over to a table-like
> approach and registering new diagnoses, I think this is orthogonal to
> introducing a diag288 watchdog device: It just makes sense to model the
> watchdog as a device that just happens to be poked via a diagnose. If
> we introduce any further diagnoses to manipulate timing etc., I agree
> we don't want to add a device for each of these.

My thinking was in the opposite level. I really like the idea of having 
separate devices for each function. But I think that they should be 
completely self-contained with well defined interfaces.


Alex

Re: [Qemu-devel] [PATCH 3/6] s390/kvm: diag288 instruction interception and handling

[Cornelia Huck]

On Wed, 22 Apr 2015 11:16:00 +0200
Alexander Graf <agraf@suse.de> wrote:

> On 04/22/2015 10:32 AM, Cornelia Huck wrote:
> > On Tue, 21 Apr 2015 21:09:05 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> >
> >> On 04/17/2015 09:52 AM, Cornelia Huck wrote:
> >>> From: Xu Wang <gesaint@linux.vnet.ibm.com>
> >>>
> >>> Intercept the diag288 requests from kvm guests, and hand the
> >>> requested command to the diag288 watchdog device for further
> >>> handling.
> >>>
> >>> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
> >>> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> >>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> >> We're getting a lot of random devices allocating diag intercepts. Can't
> >> we make this an actual interface, similar to the hypercall registration
> >> on sPAPR?
> > I've looked at the sPAPR hcall code, and it seems to basically provide
> > a table with a nice registration interface (we already use something
> > similar for the diagnose 500 virtio subcodes, btw.)
> >
> > While we could move our basic diagnose handling over to a table-like
> > approach and registering new diagnoses, I think this is orthogonal to
> > introducing a diag288 watchdog device: It just makes sense to model the
> > watchdog as a device that just happens to be poked via a diagnose. If
> > we introduce any further diagnoses to manipulate timing etc., I agree
> > we don't want to add a device for each of these.
> 
> My thinking was in the opposite level. I really like the idea of having 
> separate devices for each function. But I think that they should be 
> completely self-contained with well defined interfaces.

I don't know... having a device for e.g. a hypercall that changes some
timing characteristics feels a bit artificial to me.

[Qemu-devel] [PATCH 4/6] watchdog: Add migration support to diag288 watchdog device

[Cornelia Huck]

From: Xu Wang <gesaint@linux.vnet.ibm.com>

Add vmstate structure to keep state and data during migration.

Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/watchdog/wdt_diag288.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c
index 81a0e30..d60431c 100644
--- a/hw/watchdog/wdt_diag288.c
+++ b/hw/watchdog/wdt_diag288.c
@@ -21,6 +21,17 @@ static WatchdogTimerModel model = {
     .wdt_description = "diag288 device for s390x platform",
 };
 
+static const VMStateDescription vmstate_diag288 = {
+    .name = "vmstate_diag288",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_TIMER_PTR(timer, DIAG288State),
+        VMSTATE_BOOL(enabled, DIAG288State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void wdt_diag288_reset(DeviceState *dev)
 {
     DIAG288State *diag288 = DIAG288(dev);
@@ -89,6 +100,7 @@ static void wdt_diag288_class_init(ObjectClass *klass, void *data)
     dc->realize = wdt_diag288_realize;
     dc->unrealize = wdt_diag288_unrealize;
     dc->reset = wdt_diag288_reset;
+    dc->vmsd = &vmstate_diag288;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     diag288->handle_timer = wdt_diag288_handle_timer;
 }
-- 
2.3.5

[Qemu-devel] [PATCH 5/6] nmi: Implement inject_nmi() for non-monitor context use

[Cornelia Huck]

From: Xu Wang <gesaint@linux.vnet.ibm.com>

Let's introduce a general "inject_nmi()" function that doesn't rely on the cpu
index of the monitor, but uses cpu index 0 as default (except for x86).
This function can then later be used from a non-monitor context.

Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/core/nmi.c    | 20 ++++++++++++++++++++
 include/hw/nmi.h |  1 +
 2 files changed, 21 insertions(+)

diff --git a/hw/core/nmi.c b/hw/core/nmi.c
index 3dff020..5260d6c 100644
--- a/hw/core/nmi.c
+++ b/hw/core/nmi.c
@@ -21,6 +21,7 @@
 
 #include "hw/nmi.h"
 #include "qapi/qmp/qerror.h"
+#include "monitor/monitor.h"
 
 struct do_nmi_s {
     int cpu_index;
@@ -70,6 +71,25 @@ void nmi_monitor_handle(int cpu_index, Error **errp)
     }
 }
 
+void inject_nmi(void)
+{
+#if defined(TARGET_I386)
+    CPUState *cs;
+
+    CPU_FOREACH(cs) {
+        X86CPU *cpu = X86_CPU(cs);
+
+        if (!cpu->apic_state) {
+            cpu_interrupt(cs, CPU_INTERRUPT_NMI);
+        } else {
+            apic_deliver_nmi(cpu->apic_state);
+        }
+    }
+#else
+    nmi_monitor_handle(0, NULL);
+#endif
+}
+
 static const TypeInfo nmi_info = {
     .name          = TYPE_NMI,
     .parent        = TYPE_INTERFACE,
diff --git a/include/hw/nmi.h b/include/hw/nmi.h
index b541772..f4cec62 100644
--- a/include/hw/nmi.h
+++ b/include/hw/nmi.h
@@ -45,5 +45,6 @@ typedef struct NMIClass {
 } NMIClass;
 
 void nmi_monitor_handle(int cpu_index, Error **errp);
+void inject_nmi(void);
 
 #endif /* NMI_H */
-- 
2.3.5

[Qemu-devel] [PATCH 6/6] watchdog: Add new Virtual Watchdog action INJECT-NMI

[Cornelia Huck]

From: Mao Chuan Li <maochuan@linux.vnet.ibm.com>

This patch allows QEMU to inject a NMI into a guest when the
watchdog expires.

Signed-off-by: Mao Chuan Li <maochuan@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/watchdog/watchdog.c | 10 ++++++++++
 qapi-schema.json       |  6 +++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 54440c9..8d4b0ee 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -27,6 +27,7 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/watchdog.h"
 #include "qapi-event.h"
+#include "hw/nmi.h"
 
 /* Possible values for action parameter. */
 #define WDT_RESET        1	/* Hard reset. */
@@ -35,6 +36,7 @@
 #define WDT_PAUSE        4	/* Pause. */
 #define WDT_DEBUG        5	/* Prints a message and continues running. */
 #define WDT_NONE         6	/* Do nothing. */
+#define WDT_NMI          7	/* Inject nmi into the guest */
 
 static int watchdog_action = WDT_RESET;
 static QLIST_HEAD(watchdog_list, WatchdogTimerModel) watchdog_list;
@@ -95,6 +97,8 @@ int select_watchdog_action(const char *p)
         watchdog_action = WDT_DEBUG;
     else if (strcasecmp(p, "none") == 0)
         watchdog_action = WDT_NONE;
+    else if (strcasecmp(p, "inject-nmi") == 0)
+        watchdog_action = WDT_NMI;
     else
         return -1;
 
@@ -138,5 +142,11 @@ void watchdog_perform_action(void)
     case WDT_NONE:
         qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_NONE, &error_abort);
         break;
+
+    case WDT_NMI:
+        qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_INJECT_NMI,
+                                 &error_abort);
+        inject_nmi();
+        break;
     }
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index ac9594d..2d3bce2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3606,10 +3606,14 @@
 #
 # @none: nothing is done
 #
+# @inject-nmi: a non-maskable interrupt is injected into the first VCPU (all
+#              VCPUS on x86)
+#
 # Since: 2.1
 ##
 { 'enum': 'WatchdogExpirationAction',
-  'data': [ 'reset', 'shutdown', 'poweroff', 'pause', 'debug', 'none' ] }
+  'data': [ 'reset', 'shutdown', 'poweroff', 'pause', 'debug', 'none',
+            'inject-nmi' ] }
 
 ##
 # @IoOperationType
-- 
2.3.5

Re: [Qemu-devel] [PATCH 6/6] watchdog: Add new Virtual Watchdog action INJECT-NMI

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 876 bytes --]

On 04/17/2015 01:52 AM, Cornelia Huck wrote:
> From: Mao Chuan Li <maochuan@linux.vnet.ibm.com>
> 
> This patch allows QEMU to inject a NMI into a guest when the
> watchdog expires.
> 
> Signed-off-by: Mao Chuan Li <maochuan@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  hw/watchdog/watchdog.c | 10 ++++++++++
>  qapi-schema.json       |  6 +++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 

> +++ b/qapi-schema.json
> @@ -3606,10 +3606,14 @@
>  #
>  # @none: nothing is done
>  #
> +# @inject-nmi: a non-maskable interrupt is injected into the first VCPU (all
> +#              VCPUS on x86)

Needs a '(since 2.4)' designation.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 6/6] watchdog: Add new Virtual Watchdog action INJECT-NMI

[Cornelia Huck]

On Fri, 17 Apr 2015 06:28:10 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 04/17/2015 01:52 AM, Cornelia Huck wrote:
> > From: Mao Chuan Li <maochuan@linux.vnet.ibm.com>
> > 
> > This patch allows QEMU to inject a NMI into a guest when the
> > watchdog expires.
> > 
> > Signed-off-by: Mao Chuan Li <maochuan@linux.vnet.ibm.com>
> > Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> >  hw/watchdog/watchdog.c | 10 ++++++++++
> >  qapi-schema.json       |  6 +++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> 
> > +++ b/qapi-schema.json
> > @@ -3606,10 +3606,14 @@
> >  #
> >  # @none: nothing is done
> >  #
> > +# @inject-nmi: a non-maskable interrupt is injected into the first VCPU (all
> > +#              VCPUS on x86)
> 
> Needs a '(since 2.4)' designation.
> 

Will add.