[libvirt] [PATCH 1/2] Add domain support for virtio channel
Matthew Booth
mbooth at redhat.com
Thu Feb 18 15:29:13 UTC 2010
On 18/02/10 14:09, Daniel P. Berrange wrote:
> On Thu, Feb 18, 2010 at 01:16:02PM +0000, Matthew Booth wrote:
>> On 18/02/10 12:34, Daniel P. Berrange wrote:
>>> On Wed, Feb 17, 2010 at 05:11:00PM +0000, Matthew Booth wrote:
>>>> Add support for virtio-serial by defining a new 'virtio' channel target type and
>>>> a virtio-serial controller. Allows the following to be specified in a domain:
>>>>
>>>> <controller type='virtio-serial' index='0' max_ports='16' vectors='4'/>
>>>> <channel type='pty'>
>>>> <target type='virtio' name='org.linux-kvm.port.0'/>
>>>> <address type='virtio-serial' controller='0' bus='0'/>
>>>> </channel>
>>>>
>>>> * docs/schemas/domain.rng: Add virtio-serial controller and virtio channel type.
>>>> * src/conf/domain_conf.[ch]: Domain parsing/serialization for virtio-serial
>>>> controller and virtio channel.
>>>> * tests/qemuxml2xmltest.c
>>>> tests/qemuxml2argvdata/qemuxml2argv-channel-virtio.xml
>>>> : add domain xml parsing test
>>>> * src/libvirt_private.syms
>>>> src/qemu/qemu_conf.c: virDomainDefAddDiskControllers() renamed to
>>>> virDomainDefAddImplicitControllers()
>>>> ---
>>>> docs/schemas/domain.rng | 71 +++++-
>>>> src/conf/domain_conf.c | 234 +++++++++++++++++---
>>>> src/conf/domain_conf.h | 25 ++-
>>>> src/libvirt_private.syms | 2 +-
>>>> src/qemu/qemu_conf.c | 2 +-
>>>> .../qemuxml2argv-channel-virtio.xml | 35 +++
>>>> tests/qemuxml2xmltest.c | 1 +
>>>> 7 files changed, 320 insertions(+), 50 deletions(-)
>>>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-virtio.xml
>>>>
>>>> diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
>>>> index 53b82ce..85df8b8 100644
>>>> --- a/docs/schemas/domain.rng
>>>> +++ b/docs/schemas/domain.rng
>>>> @@ -523,16 +523,36 @@
>>>> </define>
>>>> <define name="controller">
>>>> <element name="controller">
>>>> - <optional>
>>>> - <attribute name="type">
>>>> - <choice>
>>>> - <value>fdc</value>
>>>> - <value>ide</value>
>>>> - <value>scsi</value>
>>>> - <value>sata</value>
>>>> - </choice>
>>>> - </attribute>
>>>> - </optional>
>>>> + <choice>
>>>> + <group>
>>>> + <optional>
>>>> + <attribute name="type">
>>>> + <choice>
>>>> + <value>fdc</value>
>>>> + <value>ide</value>
>>>> + <value>scsi</value>
>>>> + <value>sata</value>
>>>> + </choice>
>>>> + </attribute>
>>>> + </optional>
>>>> + </group>
>>>> + <!-- virtio-serial can have 2 additional attributes -->
>>>> + <group>
>>>> + <attribute name="type">
>>>> + <value>virtio-serial</value>
>>>> + </attribute>
>>>> + <optional>
>>>> + <attribute name="max_ports">
>>>> + <ref name="unsignedInt"/>
>>>> + </attribute>
>>>> + </optional>
>>>> + <optional>
>>>> + <attribute name="vectors">
>>>> + <ref name="unsignedInt"/>
>>>> + </attribute>
>>>> + </optional>
>>>
>>> What are these two new attributes doing ? Can't we just auto-assign
>>> those values based on the configured channels later int he XML.
>>
>> I'm not 100% sure what vectors does, however I believe this is a
>> resource usage tuning knob and therefore can't be inferred. max_ports we
>> could possibly default. However, virtio-serial also supports hot-plug,
>> although I haven't added libvirt support for it.
>
> Ok that's a good enough reason. Can we just call it 'ports' though.
> We don't use '_' in our XML attribute/element naming usually.
>
>>>> @@ -1269,6 +1302,16 @@
>>>> <ref name="driveUnit"/>
>>>> </attribute>
>>>> </define>
>>>> + <define name="virtioserialaddress">
>>>> + <attribute name="controller">
>>>> + <ref name="driveController"/>
>>>> + </attribute>
>>>> + <optional>
>>>> + <attribute name="bus">
>>>> + <ref name="driveBus"/>
>>>> + </attribute>
>>>> + </optional>
>>>> + </define>
>>>
>>> What is the "bus" in the content of virtio serial ?
>>
>> -device virtserialport,bus=channel0.0...
>>
>> I've called 'channel0' the controller, and '0' the bus.
>>
>>>> @@ -916,7 +930,8 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def)
>>>> */
>>>> static int virDomainDeviceInfoFormat(virBufferPtr buf,
>>>> virDomainDeviceInfoPtr info,
>>>> - int flags)
>>>> + int flags,
>>>> + const char *indent)
>>>
>>> I'm not seeing why we need to pass 'indent' through here? The device info
>>> data should always be appearing at exactly the same place in all devices,
>>> specifically at /domain/devices/[device type]/, so indent level should
>>> always be the same.
>>
>> I could remove this. I was originally putting <address> elsewhere, which
>> screwed up the indentation.
>
> Ok, your original code was definitely wrong then :-P
>
>>
>>>> @@ -1481,10 +1553,49 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>>>> if (virDomainDeviceInfoParseXML(node, &def->info, flags) < 0)
>>>> goto error;
>>>>
>>>> + switch (def->type) {
>>>> + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
>>>> + 0; /* C requires a statement immediately following a label */
>>>
>>> Just put a curly brace after the case
>>>
>>> case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: {
>>>
>>
>> Will do.
>>
>>>> +
>>>> + char *max_ports = virXMLPropString(node, "max_ports");
>>>> + if (max_ports) {
>>>> + int r = virStrToLong_i(max_ports, NULL, 10,
>>>> + &def->opts.vioserial.max_ports);
>>>> + if (r != 0 || def->opts.vioserial.max_ports < 0) {
>>>> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>>>> + _("Invalid max_ports: %s"), max_ports);
>>>> + VIR_FREE(max_ports);
>>>> + goto error;
>>>> + }
>>>> + } else {
>>>> + def->opts.vioserial.max_ports = -1;
>>>> + }
>>>> + VIR_FREE(max_ports);
>>>> +
>>>> + char *vectors = virXMLPropString(node, "vectors");
>>>> + if (vectors) {
>>>> + int r = virStrToLong_i(vectors, NULL, 10,
>>>> + &def->opts.vioserial.vectors);
>>>> + if (r != 0 || def->opts.vioserial.vectors < 0) {
>>>> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>>>> + _("Invalid vectors: %s"), vectors);
>>>> + VIR_FREE(vectors);
>>>> + goto error;
>>>> + }
>>>> + } else {
>>>> + def->opts.vioserial.vectors = -1;
>>>> + }
>>>
>>> I think '0' would be preferable as the not-initialized number here,
>>> since its not a valid value for either attribute AFAICT
>>
>> 0 has a special meaning for vectors. From
>> https://fedoraproject.org/wiki/Features/VirtioSerial#How_To_Test:
>>
>> If '0' is specified here, MSI will be disabled and a GSI interrupt will
>> be used.
>>
>> I used a signed int for both values for consistency.
>
> Ok, that makes sense.
Updated patch attached.
--
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team
M: +44 (0)7977 267231
GPG ID: D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Add-domain-support-for-virtio-channel.patch
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100218/bbab1347/attachment-0001.ksh>
More information about the libvir-list
mailing list