[libvirt] [PATCH 2/2] Add QEMU support for virtio channel

[Matthew Booth]

On 18/02/10 12:39, Daniel P. Berrange wrote:
> On Wed, Feb 17, 2010 at 05:11:01PM +0000, Matthew Booth wrote:
>> Support virtio-serial controller and virtio channel in QEMU backend. Will output
>> the following for virtio-serial controller:
>>
>> -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4,max_ports=16,vectors=4
>>
>> and the following for a virtio channel:
>>
>> -chardev pty,id=channel0 \
>> -device virtserialport,bus=virtio-serial0.0,chardev=channel0,name=org.linux-kvm.port.0
>>
>> * src/qemu/qemu_conf.c: Add argument output for virtio
>> * tests/qemuxml2argvtest.c
>>   tests/qemuxml2argvdata/qemuxml2argv-channel-virtio.args
>>                       : Add test for QEMU command line generation
>> ---
>>  src/qemu/qemu_conf.c                               |   91 +++++++++++++++++++-
>>  .../qemuxml2argv-channel-virtio.args               |    1 +
>>  tests/qemuxml2argvtest.c                           |    1 +
>>  3 files changed, 92 insertions(+), 1 deletions(-)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-virtio.args
>>
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index 456ee34..110409d 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -2168,7 +2168,6 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs)
>>      }
>>      for (i = 0; i < def->nchannels ; i++) {
>>          /* Nada - none are PCI based (yet) */
>> -        /* XXX virtio-serial will need one */
>>      }
>>      if (def->watchdog &&
>>          def->watchdog->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
>> @@ -2465,6 +2464,23 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def)
>>          virBufferVSprintf(&buf, ",id=scsi%d", def->idx);
>>          break;
>>  
>> +    case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
>> +        if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
>> +            virBufferAddLit(&buf, "virtio-serial-pci");
>> +        } else {
>> +            virBufferAddLit(&buf, "virtio-serial");
>> +        }
>> +        virBufferVSprintf(&buf, ",id=virtio-serial%d", def->idx);
>> +        if (def->opts.vioserial.max_ports != -1) {
>> +            virBufferVSprintf(&buf, ",max_ports=%d",
>> +                              def->opts.vioserial.max_ports);
>> +        }
>> +        if (def->opts.vioserial.vectors != -1) {
>> +            virBufferVSprintf(&buf, ",vectors=%d",
>> +                              def->opts.vioserial.vectors);
>> +        }
>> +        break;
>> +
>>      /* We always get an IDE controller, whether we want it or not. */
>>      case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
>>      default:
>> @@ -3830,6 +3846,79 @@ int qemudBuildCommandLine(virConnectPtr conn,
>>              }
>>              VIR_FREE(addr);
>>              ADD_ARG(devstr);
>> +            break;
>> +
>> +        case VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO:
>> +            if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) {
>> +                qemuReportError(VIR_ERR_NO_SUPPORT, "%s",
>> +                    _("virtio channel requires QEMU to support -device"));
>> +                goto error;
>> +            }
>> +
>> +            ADD_ARG_LIT("-chardev");
>> +            if (!(devstr = qemuBuildChrChardevStr(channel)))
>> +                goto error;
>> +            ADD_ARG(devstr);
> 
> It would be desirable to put the stuff that follows this point, into
> a  qemuBuildVirtioSerialPortDevStr()  method, because the main 
> qemudBuildCommandLine() method is far too large & unwieldly these days.

Would this comment still stand if the code below was simplified to
hard-code alias?

>> +
>> +            virBuffer virtiodev = VIR_BUFFER_INITIALIZER;
>> +            virBufferAddLit(&virtiodev, "virtserialport");
>> +
>> +            if (channel->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
>> +                /* Check it's a virtio-serial address */
>> +                if (channel->info.type !=
>> +                    VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL)
>> +                {
>> +                    qemuReportError(VIR_ERR_INTERNAL_ERROR,
>> +                                    _("virtio serial device has invalid "
>> +                                      "address type"));
>> +                    virBufferFreeAndReset(&virtiodev);
>> +                    goto error;
>> +                }
>> +
>> +                /* Look for the virtio-serial controller */
>> +                const char *vsalias = NULL;
>> +                int vs = 0;
>> +                int c;
>> +                for (c = 0; c < def->ncontrollers; c++) {
>> +                    if (def->controllers[c]->type !=
>> +                        VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
>> +                    {
>> +                        continue;
>> +                    }
>> +
>> +                    if (vs == channel->info.addr.vioserial.controller) {
>> +                        vsalias = def->controllers[c]->info.alias;
>> +                        break;
>> +                    }
>> +
>> +                    vs++;
>> +                }
>> +
>> +                if (!vsalias) {
>> +                    qemuReportError(VIR_ERR_INTERNAL_ERROR,
>> +                                    _("virtio serial device address controller "
>> +                                      "does not exist"));
>> +                    virBufferFreeAndReset(&virtiodev);
>> +                    goto error;
>> +                }
> 
> We don't really need this loop / check there, since the XML parser routine
> has already guarenteed that we have a controller associated with the port.
> Just 
> 
>> +
>> +                virBufferVSprintf(&virtiodev, ",bus=%s.%d",
>> +                                  vsalias, channel->info.addr.vioserial.bus);
> 
> This can just be
> 
>                virBufferVSprintf(&virtiodev, ",bus=virtio-serial%d.%d",
>                                  channel->info.addr.vioserial.controller,
>                                  channel->info.addr.vioserial.bus);

Actually, this code is really to look for the controller's alias. Is it
safe to hard-code it?

>> +            }
>> +
>> +            virBufferVSprintf(&virtiodev, ",chardev=%s", channel->info.alias);
>> +            if (channel->target.name) {
>> +                virBufferVSprintf(&virtiodev, ",name=%s", channel->target.name);
>> +            }
>> +            if (virBufferError(&virtiodev)) {
>> +                virReportOOMError();
>> +                goto error;
>> +            }
>> +
>> +            ADD_ARG_LIT("-device");
>> +            ADD_ARG(virBufferContentAndReset(&virtiodev));
>> +
>> +            break;
>>          }
>>      }
>>  
-- 
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