On Fri, Jan 14, 2011 at 07:23:17PM +0000, Daniel P. Berrange wrote:
On Fri, Jan 14, 2011 at 11:25:36AM -0700, Eric Blake wrote:
On 01/14/2011 05:41 AM, Daniel P. Berrange wrote:
On Thu, Jan 13, 2011 at 05:34:37PM -0700, Eric Blake wrote:
+ 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:
Alon's docs are showing the simplified syntax suitable for end users. This doesn't guarentee a stable guest visible ABI. Looking at the code, we need to set the 'slot' parameter on each ccid device we have. This means we need a new address type for smart card devices, and a corresponding <controller> instance. So in the XML we'd get (including libvirt generated aliases and addresses):
<devices> <controller type='ccid' index='0'> <alias id='ccid0'/> </controller> <controller type='ccid' index='1'> <alias id='ccid1'/> </controller> <smartcard mode='host'> <alias id='smartcard0'/> <address type='ccid' controller='0' slot='0'/> </smartcard> <smartcard mode='host-certificates'> <certificate>/path/to/cert1</certificate> <certificate>/path/to/cert2</certificate> <certificate>/path/to/cert3</certificate> <alias id='smartcard1'/> <address type='ccid' controller='0' slot='3'/> </smartcard> <smartcard mode='passthrough' type='tcp'> <source mode='connect' host='127.0.0.1' service='2001'/> <protocol type='raw'/> <alias id='smartcard2'/> <address type='ccid' controller='1' slot='0'/> </smartcard> </devices>
Which would end up mapping to
-device usb-ccid,id=ccid0 -device usb-ccid,id=ccid1
-device ccid-host,id=smartcard0,bus=ccid0,slot=0 -device ccid-certificates,id=smartcard1,bus=ccid0,slot=3... -device ccid-passthrough,id=smartcard2,bus=ccid1,slot=0...
In other words a hierarchy
USB bus 0 | +- ccid0 | | | +- smartcard0 (ccid slot 0) | +- smartcard1 (ccid slot 3) | +- ccid1 | +- smartcard2 (ccid slot 0)
This is a pretty similar setup to the way we link virtio-serial devices and vmchannels
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.
The 'usb-ccid' is on the USB bus, but we don't have stable addressing for that, because the guest OS controls USB addresses. We do need the stable slots for the <smartcards> though.
Ok, that saves me some code searching - was trying to figure out how to specify addresses from command line for usb devices :) Regarding usb-ccid bus: 1) the id is currently done by the qdev code when supplied a NULL bus parameter, i.e. it takes the device id and appends ".0", so you get for instance: -device usb-ccid,id=ccid0 -device usb-ccid,id=ccid1 -device ccid-card-emulated,bus=ccid0.0 -device ccid-card-emulated,bus=ccid1.0 Becomes: bus: ccid1.0 type ccid-bus dev: ccid-card-emulated, id "" dev-prop: backend = "nss-emulated" dev-prop: cert1 = <null> dev-prop: cert2 = <null> dev-prop: cert3 = <null> dev-prop: db = <null> dev-prop: debug = 0 bus-prop: slot = 0 dev: usb-ccid, id "ccid0" dev-prop: debug = 0 addr 0.1, speed 12, name QEMU USB CCID, attached bus: ccid0.0 type ccid-bus dev: ccid-card-emulated, id "" dev-prop: backend = "nss-emulated" dev-prop: cert1 = <null> dev-prop: cert2 = <null> dev-prop: cert3 = <null> dev-prop: db = <null> dev-prop: debug = 0 bus-prop: slot = 0 Tell me if this needs to be changed - for instance I could just reuse the id as the bus name, so it will lose the ".0" suffix. 2) usb-ccid device doesn't support more then one slot atm. This may be changed later, but atm to create two slots you need two busses. Also, this is a bug, two emulated slots will probably fail (haven't tested), since I'm pretty sure I'm doing all the initialization twice in that case, and even if not, my event quering loop doesn't have a concept of a target, so some events will end up at the first, some at the second - chaos. The good news is that this is totally opaque to libvirt, just an implementation bug.
Daniel