[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.

>> +        </group>
>> +      </choice>
>>        <attribute name="index">
>>          <ref name="unsignedInt"/>
>>        </attribute>
>> @@ -1139,12 +1159,25 @@
>>        <attribute name="port"/>
>>      </element>
>>    </define>
>> +  <define name="virtioTarget">
>> +    <element name="target">
>> +      <attribute name="type">
>> +          <value>virtio</value>
>> +      </attribute>
>> +      <optional>
>> +        <attribute name="name"/>
>> +      </optional>
>> +    </element>
>> +  </define>
>>    <define name="channel">
>>      <element name="channel">
>>        <ref name="qemucdevSrcType"/>
>>        <interleave>
>>          <ref name="qemucdevSrcDef"/>
>> -        <ref name="guestfwdTarget"/>
>> +        <choice>
>> +          <ref name="guestfwdTarget"/>
>> +          <ref name="virtioTarget"/>
>> +        </choice>
>>          <optional>
>>            <ref name="address"/>
>>          </optional>
>> @@ -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.

Matt
-- 
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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]