Re: [Xen-devel] [PATCH V8 3/7] libxl: add pvusb API

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> On 11/11/2015 at 01:57 AM, in message
<CAFLBxZZU9O7jHm6fLvKZuvd4Lnt1AEcYBSBS_eO__fr0Yw_bOg@xxxxxxxxxxxxxx>, George
Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote: 
> On Tue, Nov 10, 2015 at 8:41 AM, Chun Yan Liu <cyliu@xxxxxxxx> wrote: 
> >> > +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid, 
> >> > +                               libxl_device_usbctrl *usbctrl, 
> >> > +                               libxl__ao_device *aodev) 
> >> > +{ 
> >> 
> >> Thanks for adjusting the error-handling patterns in these functions. 
> >> The new way is good, except that: 
> >> 
> >> > +out: 
> >> > +    aodev->rc = rc; 
> >> > +    if (rc) aodev->callback(egc, aodev); 
> >> 
> >> Here, rc is always set, and indeed the code would be wrong if it were 
> >> not.  So can you remove the conditional please ?  Ie: 
> > 
> > Reading the codes, libxl__wait_device_connection will call aodev->callback 
> > properly. So here, only if (rc != 0), that means error happens, then we  
> need to 
> > call aodev->callback to end the process. (Refer to current  
> libxl__device_disk_add, 
> > all current code does similar work.) So I think the code is not wrong (?) 
>  
> >> > +static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid, 
> >> > +                                  uint8_t *bus, uint8_t *addr) 
> >> > +{ 
> >> > +    char *filename; 
> >> > +    void *buf; 
> >> > + 
> >> > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid); 
> >> > +    if (!libxl__read_sysfs_file_contents(gc, filename, &buf, NULL)) 
> >> > +        *bus = atoi((char *)buf); 
> >> 
> >> I don't think this cast (and the one for addr) are necessary ? 
> > 
> > Which cast? Here, we want to get a uint_8 value, but buf is a string, 
> > we need to do atoi. 
>  
> atoi() isn't a cast, it's a function call.  The cast is the (char *) bit. 
>  
> I think probably you could get away with declaring buf as (char *). 
> &buf should be converted to void** automatically without any warnings, 
> I think. 

It will report warning if declaring char *buf.

>  
> >> > +/* Encode usb interface so that it could be written to xenstore as a 
> >> > key. 
> >> > + * 
> >> > + * Since xenstore key cannot include '.' or ':', we'll change '.' to 
> >> > '_', 
> >> > + * change ':' to '-'. For example, 3-1:2.1 will be encoded to 3-1-2_1. 
> >> > + * This will be used to save original driver of USB device to xenstore. 
> >> > + */ 
> >> 
> >> What is the syntax of the incoming busid ?  Could it contain _ or - ? 
> >> You should perhaps spot them and reject if it does. 
> > 
> > The busid is in syntax like 3-1:2.1. It does contain '-'. But since we only 
> >  
> use 
> > the single direction, never decode back, so it won't harm. 
>  
> So the only potential problem is that 3-1:2, 3-1-2, 3:1-2, and 3:1:2 
> all collapse down to 3-1-2.  Is there any risk of something like that 
> happening?  (Ian: NB these are all read from sysfs.) 
>  
> Since this isn't really being read by anyone,  maybe it would be 
> better to replace ':' with another character, just to be safe.  It 
> could even be something like 'c' if no other punctuation is available.

OK. Will update.
 
>  
> >> > +/* Bind USB device to "usbback" driver. 
> >> > + * 
> >> > + * If there are many interfaces under USB device, check each interface, 
> >> > + * unbind from original driver and bind to "usbback" driver. 
> >> > + */ 
> >> > +static int usbback_dev_assign(libxl__gc *gc, libxl_device_usb *usb) 
> >> > +{ 
> >> ... 
> >> > +            if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 
> >> > 0)  
> { 
> >> > +                LOG(WARN, "Write of %s to node %s failed", drvpath,  
> path); 
> >> > +            } 
> >> 
> >> One of the advantages of libxl__xs_write_checked is that it logs 
> >> errors.  So you can leave that log message out. 
> >> 
> >> However, if the xs write fails, you should presumably stop, rather 
> >> than carrying on. 
> >> 
> >> I think you probably do things in the wrong order here.  You should 
> >> write the old driver info to xenstore first, so that if the bind to 
> >> usbback fails, you have the old driver info. 
>  
> Ian, I don't understand what you're saying here.  The code I'm looking at  
> does: 
> 1. unbind + get old driver path in drvpath 
> 2. if(drvpath) write to xenstore 
> 3. bind to usbback 
>  
> If the bind fails, then we do have drvpath in xenstore.  Or am I 
> missing something? 

If bind to usbback fails, it will goto 'out_rebind', that is we'll try to
rebind it to its original driver and remove xenstore info. 

>  
> Bailing out if the above xenstore write fails seems sensible though. 
>  
> >> > +/* Operation to remove usb device. 
> >> > + * 
> >> > + * Generally, it does: 
> >> > + * 1) check if the usb device is assigned to the domain 
> >> > + * 2) remove the usb device from xenstore controller/port. 
> >> > + * 3) unbind usb device from usbback and rebind to its original driver. 
> >> > + *    If usb device has many interfaces, do it to each interface. 
> >> > + */ 
> >> > +static int libxl__device_usb_remove(libxl__gc *gc, uint32_t domid, 
> >> > +                                    libxl_device_usb *usb) 
> >> > +{ 
> >> > +    int rc; 
> >> > + 
> >> > +    if (usb->ctrl < 0 || usb->port < 1) { 
> >> > +        LOG(ERROR, "Invalid USB device"); 
> >> > +        rc = ERROR_FAIL; 
> >> > +        goto out; 
> >> > +    } 
> >> > + 
> >> > +    if (usb->devtype == LIBXL_USBDEV_TYPE_HOSTDEV && 
> >> > +        (usb->u.hostdev.hostbus < 1 || usb->u.hostdev.hostaddr < 1)) { 
> >> > +        LOG(ERROR, "Invalid USB device of hostdev"); 
> >> > +        rc = ERROR_FAIL; 
> >> > +        goto out; 
> >> > +    } 
> >> 
> >> Why are these checks here rather than in do_usb_remove ? 
> >> 
> >> For that matter, why is this function not the same as do_usb_remove ? 
> >> 
> >> I might ask the same question about do_usb_add and 
> >> libxl__device_usb_add ? 
> > 
> > Using libxl__device_usb_add and within it extracting do_usb_add is to: 
> > * doing some initialization work and necessary check work in 
> >   libxl__device_usb_add (this part are common to PVUSB and QEMU USB) 
> > * doing actual adding usb operations in do_usb_add (this part will diverge 
> >   for PVUSB and QEMU USB) . 
> > 
> > Same to libxl__device_usb_remove and do_usb_remove. 
> > 
> > Certainly, we could merge into one function if that is better. George may 
> > have some opinions on this? 
>  
> I think it would be nice to have things broken down that way, but it's 
> likely I'll have to do some adjustments when I come to add in 
> devicemodel usb anyway.  So making it one big function wouldn't hurt. 
> (Nor would leaving it the way it is, as far as I'm concerned.)

Got it. Will update.
 
>  
> The big thing though is with the removal interface here, where we 
> again have somewhat of a confusion between naming things based on the 
> *host* name (as we do in pci pass-through) vs the *guest* names (as we 
> do for disks and nics). 
>  
> If you have the controller and virtual port, there's no need for the 
> caller to *also* go and look up the host bus and host address; or vice 
> versa -- if you have the hostbus and hostaddr, you shouldn't also need 
> to go look up the usbctrl and virtual port. 
>  
> It looks like for libxl_device_{disk,nic,vtpm}_remove(), it gets a 
> libxl_device struct using libxl__device_from_{disk,nic,vtpm}; and in 
> all cases, the information used to construct the "device" is the 
> backend + either a devid (for nic and vtpm) or vdev, for disks.  It 
> looks like for those devices, any conversion from other labels (mac 
> address for nic, uuid for vtpm) is done in xl_cmdimpli.c. 
>  
> I think we want to follow suit here -- libxl_device_usb_remove() 
> should take a usbctrl and port number only, and should look up 
> whatever information it needs to do the removal from that. 
>  
> All we really need for the re-assignment is the busid, which is 
> already stored in a xenstore location we can find using domid, ctrl, 
> and port.  Something like the attached patch (compile-tested only). 
> What do you think? 

Thanks. Much better. Change interface to use 'busid' instead of
'struct usb' usbback_dev_assign and usbback_dev_unassign makes things
much cleaner.

>  
> (NB the patch doesn't fix the gc or aliasing issues in 
> usb_interface_xenstore_encode() -- those still need to be addressed.)

I'll take care of the left.

- Chunyan
 
>  
>  -George 
>  


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel