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. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|