Re: [libvirt] [PATCH v2 1/3] qemu: add panic device support for S390

On Fri, 2016-04-15 at 10:20 +0200, Boris Fiuczynski wrote: > If a panic device is being defined without a model in a domain > the default value is always overwritten with model ISA. An ISA > bus does not exist on S390 and therefore specifying a panic device > results in an unsupported configuration. > Since the S390 architecture inherently provides a crash detection > capability the panic device should be defined in the domain xml. > > This patch adds an s390 panic device model and prevents setting a > device address on it. > > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; > Reviewed-by: Cornelia Huck <cornelia.huck(a)de.ibm.com&gt; > --- > docs/formatdomain.html.in | 7 ++++++- > docs/schemas/domaincommon.rng | 1 + > src/conf/domain_conf.c | 3 ++- > src/conf/domain_conf.h | 1 + > src/qemu/qemu_command.c | 21 ++++++++++++++++++++- > src/qemu/qemu_domain.c | 2 ++ > 6 files changed, 32 insertions(+), 3 deletions(-) Sorry for taking so long to reply. > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index c2955eb..10c27fb 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -6242,6 +6242,9 @@ qemu-kvm -net nic,model=? /dev/null > For pSeries guests, this feature is always enabled since it's > implemented by the guest firmware, thus libvirt automatically > adds the <code>panic</code> element to the domain XML. > + For S390 guests, this feature is always enabled since it's an > + integral part of the S390 architecture, thus libvirt automatically > + adds the <code>panic</code> element to the domain XML. Shouldn't that be "S/390" instead of "S390"?

Just like we use "pSeries". Same for the commit message and in the remaining patches. This paragraph should also be merged with the existing one, as they're basically identical. Tweak the existing text as needed.

> @@ -9034,7 +9053,7 @@ qemuBuildPanicCommandLine(virCommandPtr cmd, > if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PANIC)) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > _("the QEMU binary does not support the " > - "panic device")); > + "ISA panic device")); This should be a separate commit. A very tiny one ;)

Everything else looks good. -- Andrea Bolognani Software Engineer - Virtualization Team