Re: [PATCHv2 1/3] conf: add 'model' attribute for panic device with values isa, pseries, hyperv

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 12, 2015 at 14:07:38 +0300, Dmitry Andreev wrote:
> Libvirt already has two types of panic devices - pvpanic and pSeries firmware.
> This patch introduces the 'model' attribute and a new type of panic device.
> 
> 'isa' model is for ISA pvpanic device.
> 'pseries' model is a default value for pSeries guests.
> 'hyperv' model is the new type. It's used for Hyper-V crash.
> 
> Schema, docs and tests are updated for the new attribute.
> ---
>  docs/formatdomain.html.in                          | 29 +++++++++++++++++--
>  docs/schemas/domaincommon.rng                      |  9 ++++++
>  src/conf/domain_conf.c                             | 33 ++++++++++++++++++----
>  src/conf/domain_conf.h                             | 10 +++++++
>  src/qemu/qemu_domain.c                             |  4 +++
>  .../qemuxml2argv-panic-no-address.xml              |  2 +-
>  tests/qemuxml2argvdata/qemuxml2argv-panic.xml      |  2 +-
>  .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml |  2 +-
>  .../qemuxml2argv-pseries-nvram.xml                 |  2 +-
>  .../qemuxml2argv-pseries-panic-address.xml         |  2 +-
>  .../qemuxml2argv-pseries-panic-no-address.xml      |  2 +-
>  .../qemuxml2xmlout-pseries-panic-missing.xml       |  2 +-
>  12 files changed, 85 insertions(+), 14 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index c88b032..93b9813 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6152,19 +6152,44 @@ qemu-kvm -net nic,model=? /dev/null
>  <pre>
>    ...
>    &lt;devices&gt;
> -    &lt;panic&gt;
> +    &lt;panic model='isa'&gt;
>        &lt;address type='isa' iobase='0x505'/&gt;
>      &lt;/panic&gt;
>    &lt;/devices&gt;
>    ...
>  </pre>
> +    <p>
> +      Example: usage of panic configuration for Hyper-V guests
> +    </p>
> +<pre>
> +  ...
> +  &lt;devices&gt;
> +    &lt;panic model='hyperv'/&gt;
> +  &lt;/devices&gt;
> +  ...
> +</pre>

I think it would be enough to add &lt;panic model='hyperv'/&gt; as
another device in the previous example. That is:

     <pre>
       ...
       &lt;devices&gt;
    -    &lt;panic&gt;
    +    &lt;panic model='isa'&gt;
           &lt;address type='isa' iobase='0x505'/&gt;
         &lt;/panic&gt;
    +    &lt;panic model='hyperv'/&gt;
       &lt;/devices&gt;
       ...
     </pre>

>    <dl>
> +    <dt><code>model</code></dt>
> +      <dd>
> +        <p>
> +          The required <code>model</code> attribute specifies what type
> +          of panic device is provided.

The attribute is optional. The panic model used when this attribute is
missing depends on the hypervisor and guest arch.

> +        </p>
> +        <ul>
> +          <li>'isa' &mdash; for ISA pvpanic device</li>
> +          <li>'pseries' &mdash; default and valid only for pSeries guests.</li>
> +          <li>'hyperv' &mdash; for Hyper-V crash CPU feature.
> +            <span class="since">Since 2.5.0, QEMU and KVM only</span>
> +          </li>
> +        </ul>

We use nested <dl> with values enclosed in <code/>.

> +      </dd>
> +
>      <dt><code>address</code></dt>
>      <dd>
>        <p>
>          address of panic. The default ioport is 0x505. Most users
>          don't need to specify an address, and doing so is forbidden
> -        altogether for pSeries guests.
> +        altogether for pSeries guests and for Hyper-V crash.

    +        altogether for pseries and hyperv models.

>        </p>
>      </dd>
>    </dl>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f196177..d67b1bf 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -5359,6 +5359,15 @@
>    <define name="panic">
>      <element name="panic">
>        <optional>
> +        <attribute name="model">
> +          <choice>
> +            <value>isa</value>
> +            <value>pseries</value>
> +            <value>hyperv</value>
> +          </choice>
> +        </attribute>
> +      </optional>
> +      <optional>
>          <ref name="address"/>
>        </optional>
>      </element>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index eb00444..935d429 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -525,6 +525,11 @@ VIR_ENUM_IMPL(virDomainWatchdogAction, VIR_DOMAIN_WATCHDOG_ACTION_LAST,
>                "none",
>                "inject-nmi")
>  
> +VIR_ENUM_IMPL(virDomainPanicModel, VIR_DOMAIN_PANIC_MODEL_LAST,

You need to add a "default" model.

> +              "isa",
> +              "pseries",
> +              "hyperv")
> +
>  VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
>                "vga",
>                "cirrus",
> @@ -10194,20 +10199,36 @@ virDomainTPMDefParseXML(xmlNodePtr node,
>  }
>  
>  static virDomainPanicDefPtr
> -virDomainPanicDefParseXML(xmlNodePtr node)
> +virDomainPanicDefParseXML(const virDomainDef *dom, xmlNodePtr node)

No need to change the prototype.

>  {
>      virDomainPanicDefPtr panic;
> +    char *model = NULL;
>  
>      if (VIR_ALLOC(panic) < 0)
>          return NULL;
>  
> +    if (!(model = virXMLPropString(node, "model"))) {
> +        if (ARCH_IS_PPC64(dom->os.arch) && STRPREFIX(dom->os.machine, "pseries"))
> +            panic->model = VIR_DOMAIN_PANIC_MODEL_PSERIES;
> +        else
> +            panic->model = VIR_DOMAIN_PANIC_MODEL_ISA;
> +    } else if ((panic->model = virDomainPanicModelTypeFromString(model)) < 0) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("unknown panic model '%s'"), model);
> +        goto error;
> +    }
> +

The only thing which should be done here is

    panic->model = virDomainPanicModelTypeFromString(model);
    if (panic->model < 0) {
        virReportError(...);
        goto error;
    }

The rest belongs to QEMU specific postparse callback [1].

>      if (virDomainDeviceInfoParseXML(node, NULL, &panic->info, 0) < 0)
>          goto error;
>  
> + cleanup:
> +    VIR_FREE(model);
>      return panic;
> +
>   error:
>      virDomainPanicDefFree(panic);
> -    return NULL;
> +    panic = NULL;
> +    goto cleanup;
>  }
>  
>  /* Parse the XML definition for an input device */
> @@ -12751,7 +12772,7 @@ virDomainDeviceDefParse(const char *xmlStr,
>              goto error;
>          break;
>      case VIR_DOMAIN_DEVICE_PANIC:
> -        if (!(dev->data.panic = virDomainPanicDefParseXML(node)))
> +        if (!(dev->data.panic = virDomainPanicDefParseXML(def, node)))
>              goto error;
>          break;
>      case VIR_DOMAIN_DEVICE_MEMORY:
> @@ -16398,7 +16419,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>      }
>      if (n > 0) {
>          virDomainPanicDefPtr panic =
> -            virDomainPanicDefParseXML(nodes[0]);
> +            virDomainPanicDefParseXML(def, nodes[0]);
>          if (!panic)
>              goto error;
>  
> @@ -20627,10 +20648,12 @@ virDomainWatchdogDefFormat(virBufferPtr buf,
>  static int virDomainPanicDefFormat(virBufferPtr buf,
>                                     virDomainPanicDefPtr def)
>  {
> +    const char *model = virDomainPanicModelTypeToString(def->model);
> +
>      virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
>      int indent = virBufferGetIndent(buf, false);
>  
> -    virBufferAddLit(buf, "<panic");
> +    virBufferAsprintf(buf, "<panic model='%s'", model);

Only print model attribute if panic->model is set.

>      virBufferAdjustIndent(&childrenBuf, indent + 2);
>      if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0)
>          return -1;
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index f10b534..00f3679 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2045,7 +2045,16 @@ struct _virDomainIdMapDef {
>  };
>  
>  
> +typedef enum {

Add VIR_DOMAIN_PANIC_MODEL_DEFAULT here so that you can distinguish
whether the model was specified or not.

> +    VIR_DOMAIN_PANIC_MODEL_ISA,
> +    VIR_DOMAIN_PANIC_MODEL_PSERIES,
> +    VIR_DOMAIN_PANIC_MODEL_HYPERV,
> +
> +    VIR_DOMAIN_PANIC_MODEL_LAST
> +} virDomainPanicModel;
> +
>  struct _virDomainPanicDef {
> +    int model;
>      virDomainDeviceInfo info;
>  };
>  
> @@ -3060,6 +3069,7 @@ VIR_ENUM_DECL(virDomainMemballoonModel)
>  VIR_ENUM_DECL(virDomainSmbiosMode)
>  VIR_ENUM_DECL(virDomainWatchdogModel)
>  VIR_ENUM_DECL(virDomainWatchdogAction)
> +VIR_ENUM_DECL(virDomainPanicModel)
>  VIR_ENUM_DECL(virDomainVideo)
>  VIR_ENUM_DECL(virDomainHostdevMode)
>  VIR_ENUM_DECL(virDomainHostdevSubsys)
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3d2b7a0..8ec3e8e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1180,6 +1180,10 @@ qemuDomainDefPostParse(virDomainDefPtr def,

This is the callback referenced by [1].

>          if (VIR_ALLOC(panic) < 0)
>              goto cleanup;
>  
> +        if (ARCH_IS_PPC64(def->os.arch) &&
> +            STRPREFIX(def->os.machine, "pseries"))
> +            panic->model = VIR_DOMAIN_PANIC_MODEL_PSERIES;
> +
>          def->panic = panic;
>      }
>  
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-panic-no-address.xml b/tests/qemuxml2argvdata/qemuxml2argv-panic-no-address.xml
> index 79f8a1e..f3f3fbb 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-panic-no-address.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-panic-no-address.xml
> @@ -24,6 +24,6 @@
>      <controller type='ide' index='0'/>
>      <controller type='pci' index='0' model='pci-root'/>
>      <memballoon model='virtio'/>
> -    <panic/>
> +    <panic model='isa'/>
>    </devices>
>  </domain>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-panic.xml b/tests/qemuxml2argvdata/qemuxml2argv-panic.xml
> index e354511..b9595a8 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-panic.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-panic.xml
> @@ -24,7 +24,7 @@
>      <controller type='ide' index='0'/>
>      <controller type='pci' index='0' model='pci-root'/>
>      <memballoon model='virtio'/>
> -    <panic>
> +    <panic model='isa'>
>        <address type='isa' iobase='0x505'/>
>      </panic>
>    </devices>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml
> index 3a96209..39f4a1f 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml
> @@ -37,6 +37,6 @@
>        <model type='cirrus' vram='16384' heads='1'/>
>      </video>
>      <memballoon model='none'/>
> -    <panic/>
> +    <panic model='pseries'/>
>    </devices>
>  </domain>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml
> index 619186a..2da2832 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml
> @@ -20,6 +20,6 @@
>      <nvram>
>        <address type='spapr-vio' reg='0x4000'/>
>      </nvram>
> -    <panic/>
> +    <panic model='pseries'/>
>    </devices>
>  </domain>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-address.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-address.xml
> index e62ead8..be5b862 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-address.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-address.xml
> @@ -25,7 +25,7 @@
>        <address type='spapr-vio'/>
>      </console>
>      <memballoon model='none'/>
> -    <panic>
> +    <panic model='isa'>
>          <address type='isa' iobase='0x505'/>
>      </panic>
>    </devices>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.xml
> index 9312975..8fcd644 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-panic-no-address.xml
> @@ -25,6 +25,6 @@
>        <address type='spapr-vio'/>
>      </console>
>      <memballoon model='none'/>
> -    <panic/>
> +    <panic model='pseries'/>
>    </devices>
>  </domain>
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml
> index 9312975..8fcd644 100644
> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml
> @@ -25,6 +25,6 @@
>        <address type='spapr-vio'/>
>      </console>
>      <memballoon model='none'/>
> -    <panic/>
> +    <panic model='pseries'/>
>    </devices>
>  </domain>

You should add tests with <panic/> and a corresponding <panic
model='...'/> in xml2xmloutdata for both isa and pseries models to make
sure we autogenerate the right model.

Jirka

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]