On 05/03/2016 11:16 AM, John Ferlan wrote:
>
>
> On 05/03/2016 09:50 AM, Cole Robinson wrote:
>> On 05/02/2016 06:30 PM, John Ferlan wrote:
>>> Add the ability to add an 'iothread' to the controller which will be how
>>> virtio-scsi-pci and virtio-scsi-ccw iothreads have been implemented in qemu.
>>>
>>> Describe the new functionality and add tests to parse/validate that the
>>> new attribute can be added.
>>>
>>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>>> ---
>>> docs/formatdomain.html.in | 33 +++++++++++---
>>> docs/schemas/domaincommon.rng | 3 ++
>>> src/conf/domain_conf.c | 25 ++++++++++-
>>> src/conf/domain_conf.h | 1 +
>>> .../qemuxml2argv-iothreads-virtio-scsi-ccw.xml | 39 +++++++++++++++++
>>> .../qemuxml2argv-iothreads-virtio-scsi-pci.xml | 47 ++++++++++++++++++++
>>> .../qemuxml2xmlout-iothreads-virtio-scsi-ccw.xml | 40 +++++++++++++++++
>>> .../qemuxml2xmlout-iothreads-virtio-scsi-pci.xml | 51 ++++++++++++++++++++++
>>> tests/qemuxml2xmltest.c | 7 +++
>>> 9 files changed, 240 insertions(+), 6 deletions(-)
>>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-virtio-scsi-ccw.xml
>>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-virtio-scsi-pci.xml
>>> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-virtio-scsi-ccw.xml
>>> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-iothreads-virtio-scsi-pci.xml
>>>
>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>> index 5781dba..5e27fca 100644
>>> --- a/docs/formatdomain.html.in
>>> +++ b/docs/formatdomain.html.in
>>> @@ -565,11 +565,13 @@
>>> <dt><code>iothreads</code></dt>
>>> <dd>
>>> The content of this optional element defines the number
>>> - of IOThreads to be assigned to the domain for use by
>>> - virtio-blk-pci and virtio-blk-ccw target storage devices. There
>>> - should be only 1 or 2 IOThreads per host CPU. There may be more
>>> - than one supported device assigned to each IOThread.
>>> - <span class="since">Since 1.2.8</span>
>>> + of IOThreads to be assigned to the domain for use by supported
>>> + storage disks or controllers. There should be only 1 or 2 IOThreads
>>> + per host CPU. There may be more than one supported device assigned
>>> + to each IOThread. Supported for virtio-blk-pci and virtio-blk-ccw
>>> + disk devices <span class="since">since 1.2.8 (QEMU 2.1)</span>.
>>> + Supported for virtio-scsi-pci and virtio-scsi-ccw controller
>>> + devices <span class="since">since 1.3.4 (QEMU 2.4)</span>.
>>> </dd>
>>> <dt><code>iothreadids</code></dt>
>>> <dd>
>>
>> This is the bit that should be made generic IMO
>>
>
> I'll remove this hunk...
>
>>> @@ -3004,6 +3006,10 @@
>>> <controller type='virtio-serial' index='1'>
>>> <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/>
>>> </controller>
>>> + <controller type='scsi' index='0' model='virtio-scsi'>
>>> + <driver iothread='4'>
>>> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/>
>>> + </controller>
>>> ...
>>> </devices>
>>> ...</pre>
>>> @@ -3084,6 +3090,23 @@
>>> I/O asynchronous handling</a> or not. Accepted values are
>>> "on" and "off". <span class="since">Since 1.2.18</span>
>>> </dd>
>>> + <dt><code>iothread</code></dt>
>>> + <dd>
>>> + Supported for controller type <code>scsi</code> using model
>>> + <code>virtio-scsi</code> for <code>address</code> types
>>> + <code>pci</code> and <code>ccw</code>
>>> + <span class="since">since 1.3.4 (QEMU 2.4)</span>.
>>> +
>>> + The optional <code>iothread</code> attribute assigns the controller
>>> + to an IOThread as defined by the range for the domain
>>> + <a href="#elementsIOThreadsAllocation"><code>iothreads</code></a>
>>> + value. Each SCSI <code>disk</code> assigned to use the specified
>>> + <code>controller</code> will utilize the same IOThread. If a specific
>>> + IOThread is desired for a specific SCSI <code>disk</code>, then
>>> + multiple controllers must be defined each having a specific
>>> + <code>iothread</code> value. The <code>iothread</code> value
>>> + must be within the range 1 to the domain iothreads value.
>>> + </dd>
>>> </dl>
>>> <p>
>>> USB companion controllers have an optional
>>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>>> index b82f8c8..ceced6b 100644
>>> --- a/docs/schemas/domaincommon.rng
>>> +++ b/docs/schemas/domaincommon.rng
>>> @@ -1893,6 +1893,9 @@
>>> <optional>
>>> <ref name="ioeventfd"/>
>>> </optional>
>>> + <optional>
>>> + <ref name="driverIOThread"/>
>>> + </optional>
>>> </element>
>>> </optional>
>>> </interleave>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 6375b2b..f571466 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -7853,6 +7853,7 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>>> char *busNr = NULL;
>>> int numaNode = -1;
>>> char *ioeventfd = NULL;
>>> + char *iothread = NULL;
>>> xmlNodePtr saved = ctxt->node;
>>> int rc;
>>>
>>> @@ -7897,6 +7898,7 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>>> cmd_per_lun = virXMLPropString(cur, "cmd_per_lun");
>>> max_sectors = virXMLPropString(cur, "max_sectors");
>>> ioeventfd = virXMLPropString(cur, "ioeventfd");
>>> + iothread = virXMLPropString(cur, "iothread");
>>> } else if (xmlStrEqual(cur->name, BAD_CAST "model")) {
>>> if (processedModel) {
>>> virReportError(VIR_ERR_XML_ERROR, "%s",
>>> @@ -7958,6 +7960,21 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>>> goto error;
>>> }
>>>
>>> + if (iothread) {
>>> + if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) {
>>> + virReportError(VIR_ERR_XML_ERROR,
>>> + _("'iothread' attribute only supported for "
>>> + "controller model '%s'"),
>>> + virDomainControllerModelSCSITypeToString(VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI));
>>> + goto error;
>>> + }
>>
>> I think this particular check should be moved to one of the generic PostParse
>> validation functions, since it's not specific to parse time.
>>
>
> I guess I was under the impression that post parse callbacks were to be
> used when we didn't have all the information about a relationship
> between say a disk and controller.
>
For driver specific bits, maybe. But in general I think as much validation as
possible should move out of the XML parsing routines, and into the generic
PostParse function, so drivers that manually populate virDomainDef hit the
checks as well.
> In this case, the attribute is only supported on the controller that has
> been defined using "virtio-scsi" or "virtio-ccw". Since those weren't
> "discover-able" based on some other relationship, thus checking at parse
> time was better.
>
> IDC either way, but I think (without too much digging), that would mean
> a change to qemuDomainDeviceDefPostParse in the following if:
>
I would put it in generic code instead. Roughly what
virDomainDefPostParseTimer does, but probably tied into the device iteration.
Really the check being in done in DomainDefParse* isn't impactful in this
case, but it's a pattern we should try to break out of IMO
- Cole
> if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
>
> in order to add a check such as
>
> if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
> cont->iothread &&
> cont->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)
>
> Is there anyone else that has a preference/thought where the check
> should go?
>
> Tks -
>
> John
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list