Re: [libvirt] [PATCH 1/2] Add domain support for virtio channel

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.