Re: [PATCH v7 RFC 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2014-06-18 at 14:58 +0100, George Dunlap wrote:
> On 06/18/2014 02:49 PM, Ian Campbell wrote:
> >>>> +        ("backend_domid",    libxl_domid),
> >>>> +        ("backend_domname",  string),
> >>>> +        ("u", KeyedUnion(None, libxl_device_usb_type, "type",
> >>>> +                         [("hostdev", Struct(None, [
> >>>> +                                ("hostbus",   integer),
> >>>> +                                ("hostaddr",  integer) ]))
> >>> No need to express the host topology I think (because you can build that
> >>> from the bus,addr tuples)?
> >> I don't really follow.  You mean, we can drop 'host' from the last two
> >> elements, and just call them "bus" and "addr"?
> > Gah, I started writing one thing and then reunderstood usb and wrote
> > half another.
> >
> > What I was trying to say is that you don't need hostaddr to describe the
> > full USB topology path to the device because the (bus,addr) tuple you've
> > given already does so (because each hub effectively creates a new bus
> > number, so they all look like toplevel buses in this representation).
> 
> You seem to be saying that something is redundant, or that there's extra 
> information somewhere; but as there are only two bits of data (bus and 
> addr), and agree that I need both, I'm having a hard time telling what 
> you think is redundant / could be removed..

I'm actually saying the opposite (or rather asking for confirmation that
this is the case). i.e. one might expect that *more* information would
be needed here (i.e. the full bus topology/path to a device via multiple
hubs etc) but in fact this is not needed because the topology is not
visible in this numbering scheme (each potential fork in the path
results in a whole new bus number).

Sorry, I could have been clearer about this.

> "hostdev" is an element of the union; so the structure should unpack 
> like this:
> struct {
>    libxl_domid backend_domid;
>    char * backend_domname;
>    libxl_usb_device_type type;
>    union u {
>     struct {
>      int hostbus, hostaddr;
>     } hostdev;
>    };
> };
> 
> At the moment, "type" can only be "hostdev"; but I'm envisioning in the 
> future that "type" might be "tablet", "mouse", "keyboard", maybe 
> "mass-storage", and that the union would have more elements.

This seems sensible. You brought up dropping the hostdev prefix from the
bus and addr (because you thought I was ;-)), which seemed sensible when
you think of the code which accesses these fields i.e.
dev->u.hostdev.hostbus vs dev->u.hostdev.bus. Up to you whether you want
to repeat "host" in that.

> Does that clear things up?  Or am I totally confused? :-)

I think you are remarkably unconfused given how confusing I'm being...

Ian.