[libvirt] [RFC PATCHv2 5/5] WIP: smartcard: turn on qemu support
Eric Blake
eblake at redhat.com
Fri Jan 14 18:25:36 UTC 2011
On 01/14/2011 05:41 AM, Daniel P. Berrange wrote:
> On Thu, Jan 13, 2011 at 05:34:37PM -0700, Eric Blake wrote:
>> Still to go - add .args files to match .xml files in testsuite
>>
>> * src/qemu/qemu_command.c (qemuBuildCommandLine): Emit smartcard
>> options.
>> (qemuAssignDeviceAliases): Assign an alias for smartcard passthrough.
>> * tests/qemuxml2argvtest.c (mymain): Three new tests.
>> * tests/qemuxml2argvdata/...arg: Three new files.
>> ---
>>
>> Well, as you can see, the tests/ part isn't done yet.
>>
>> src/qemu/qemu_command.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>
> What is the status of this support wrt upstream QEMU ? Has the
> smartcard stuff been accepted into GIT yet ?
Submitted, but not yet included (so another reason that I shouldn't
necessarily push this upstream yet).
http://patchwork.ozlabs.org/patch/78297/
>
> Also there is where the lack of security driver integration
> will cause problems, because QEMU won't start.
Yes, will address that in v3.
>> @@ -604,6 +604,11 @@ qemuAssignDeviceAliases(virDomainDefPtr def, unsigned long long qemuCmdFlags)
>> if (virAsprintf(&def->channels[i]->info.alias, "channel%d", i) < 0)
>> goto no_memory;
>> }
>> + for (i = 0; i < def->nsmartcards ; i++) {
>> + if (def->smartcards[i]->type == VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH &&
>> + virAsprintf(&def->smartcards[i]->info.alias, "smartcard%d", i) < 0)
>> + goto no_memory;
>> + }
>
> You've assigned device aliases here....
>
>> if (def->console) {
>> if (virAsprintf(&def->console->info.alias, "console%d", i) < 0)
>> goto no_memory;
>> @@ -3332,6 +3337,50 @@ qemuBuildCommandLine(virConnectPtr conn,
>> }
>> }
>>
>> + if (def->nsmartcards) {
>> + /* Requires -chardev and -device usb-ccid */
>> + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) ||
>> + !(qemuCmdFlags & QEMUD_CMD_FLAG_USB_CCID)) {
>> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("smartcard requires QEMU to support -usb-ccid"));
>> + goto error;
>> + }
>> + virCommandAddArgList(cmd, "-device", "usb-ccid", NULL);
>
> Ideally this would have an id= field too, eg if=ccid0
My understanding is that we only need one -device usb-ccid; that one
device is necessary to provide the bus shared by all other -device
ccid-card-* entries. So the alias defined above is for the ccid-card.
I guess that means you want a hard-coded alias here:
virCommandAddArgList(cmd, "-device", "usb-ccid,id=ccid0", NULL);
But with Cole's recent sound patch, we added -device hda-output without
any id= notation (another case where in addition to the real -device
intel-hda,id=sound4 to match the <sound> element, we also needed the
secondary bus -device hda-sound as behind-the-scenes glue).
>
>> + }
>> + for (i = 0 ; i < def->nsmartcards ; i++) {
>> + virDomainSmartcardDefPtr smartcard = def->smartcards[i];
>> + char *devstr;
>> + virBuffer smartcard_buf = VIR_BUFFER_INITIALIZER;
>> + int j;
>> +
>
> ....But in this following block of code never added the aliases to
> the -device options:
I assumed that the alias was only needed for the case of tying a chardev
to a -device ccid-card-passthru (the -device ccid-card-emulated does not
have an associated chardev). Which is why the DEVICE_TYPE_PASSTHROUGH
block _does_ use the alias (just the TYPE_HOST and
TYPE_HOST_CERTIFICATES did not use it, but it wasn't generated for those
cases).
>
>> + switch (smartcard->type) {
>> + case VIR_DOMAIN_SMARTCARD_TYPE_HOST:
>> + virCommandAddArgList(cmd, "-device", "ccid-card-emulated", NULL);
>> + break;
>> + case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES:
>> + virCommandAddArg(cmd, "-device");
>> + virBufferAddLit(&smartcard_buf,
>> + "ccid-card-emulated,backend=certificates");
>> + for (j = 0; j < VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES; j++)
>> + virBufferVSprintf(&smartcard_buf, ",cert%d=%s", j + 1,
>> + smartcard->data.cert.file[j]);
>
> I guess we need some kind of escaping of special chars in the filename
> here since we append 3 in a row, it might confuse QEMU's parser. Not
> sure offhand how QEMU expects that to work.
It's only confusing if the filename contains a literal comma, so v3 will
reject filenames with a literal comma.
>
> Or since we already depend on existance of -device, we might want to use
> '-set' for setting the filenames and thus avoid need to escape, eg
>
> -device ccid-card-emulated,backend=certificates,id=smartcard0
> -set ccid-card-emulated.smartcard0.cert0=$FILENAME
> -set ccid-card-emulated.smartcard0.cert1=$FILENAME
> -set ccid-card-emulated.smartcard0.cert2=$FILENAME
>
> albeit at the cost of much longer command lines.
Nah - a literal comma is rare enough to not go with this complexity
unless someone has a compelling reason why we should.
>
>
>> + virCommandAddArgBuffer(cmd, &smartcard_buf);
>> + break;
>> + case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
>> + virCommandAddArg(cmd, "-chardev");
>> + if (!(devstr = qemuBuildChrChardevStr(&smartcard->data.passthru,
>> + smartcard->info.alias)))
>> + goto error;
>
> The 'id' setting for the chardev should really have a prefix
> on it, because we won't want it to be the same as the 'id'
> setting used for the -device option.
But we don't do that for any of our other -chardevs. Oh, are you saying
that we first need to patch things to carry dual aliases to distinguish
between -chardev and its associated -device, and that every object
passed to qemu (and not just -chardev) should be given an alias?
Is that a prerequisite to getting this series in, or should it be a
separate cleanup patch that can come later?
>
>> + virCommandAddArg(cmd, devstr);
>> + VIR_FREE(devstr);
>> +
>> + virCommandAddArg(cmd, "-device");
>> + virCommandAddArgFormat(cmd, "ccid-card-passthru,chardev=%s",
>> + smartcard->info.alias);
>> + break;
>> + }
>> + }
>> +
>> if (!def->nserials) {
>> /* If we have -device, then we set -nodefault already */
>> if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE))
>
> We currently only provide a single '-device usb-ccid' device. It looks
> like we're relying on the ccid-card-XXX devices automagically associating
> themselves with that. It would be better to explicitly link them up
> by specifying the 'id' of the 'usb-ccid' device to which they must be
> associated. If QEMU doesn't have such an explicit link, it ought to
> be added.
That's how Alon implemented it. -device usb-ccid is instantiated
exactly once, and is not tied to any -chardev. -device
ccid-card-emulated automatically uses the services of -device usb-ccid,
and does not need any -chardev. And -device ccid-card-passthru
automatically uses the services of -device usb-ccid, and requires an
associated -chardev (Alon has only tested with a tcp chardev and with a
spicevmc,name=smartcard chardev). At this point, your complaints seem
to be more about whether Alon's qemu command line implementation makes
sense (which, given that the smartcard patches have been proposed but
are not yet upstream, may mean that the final qemu implementation will
change and my libvirt patch would have to equally change to match),
rather than whether I'm correctly mapping to the command line that Alon
documented:
http://cgit.freedesktop.org/~alon/qemu/commit/?h=usb_ccid.v15&id=024a37bf696ae3a8d148a2ecbecdd3d92d390b2a
Of course, providing some .args test files in v3 will help make it a
little easier to review.
>
> Also, does the order of '-device ccid-card-XXX' devices matter ie is
> the ordering a guest visible ABI ? If so, then we need to track an
> explicit address against each <smartcard> so that we can selectively
> hotplug/hotunplug devices, and have a stable ABI across migration.
That I'm not sure about. Alon said that the guest sees the smartcard as
a USB device, but didn't mention anything about whether -device
ccid-card-* can take additional USB addressing parameters to lock down
where the device appears on the guest's USB bus.
--
Eric Blake eblake at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110114/eb7ee4a4/attachment-0001.sig>
More information about the libvir-list
mailing list