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

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



On Thu, Feb 18, 2010 at 01:23:57PM +0000, Matthew Booth wrote:
> 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?

Yep, because you'll want this method to exist if you ever add the
hot-plug support, since that uses the exact same syntax as the 
main -device arg these days. Which is nice :-)

> 
> >> +
> >> +            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?

Yep, we do for disks. If you want though, you could add more #defines to
src/qemu/qemu_conf.h for that, alongside the existing

  #define QEMU_DRIVE_HOST_PREFIX "drive-"

eg,

  #define QEMU_VIRTIO_SERIAL_PREFIX "virtio-serial"

and reference that constant in both places

> 
> >> +            }
> >> +
> >> +            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;
> >>          }
> >>      }
> >>  


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


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