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

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.

>> @@ -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.

>> @@ -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.