Re: [Qemu-devel] [SeaBIOS] [PATCH 1/2] usb attached scsi boot support

[Kevin O'Connor]

On Thu, Jul 19, 2012 at 06:15:02PM +0200, Gerd Hoffmann wrote:
> This patch adds support for booting from UAS (usb
> attached scsi) devices.
> 
> For now only usb 2.0 support is there.  On usb 3.0 the
> UAS protocol uses streams, so changes will be required
> to make usb 3.0 devices fly once we have a xhci host
> controller driver.
> 
> So far the driver has been tested on qemu-emulated
> virtual hardware only.  In theory should just work on
> bare metal too.

Thanks.  Looks good to me.  I have a few minor comments below.

[...]
> --- a/src/block.c
> +++ b/src/block.c
> @@ -280,7 +280,7 @@ map_floppy_drive(struct drive_s *drive_g)
>  static int
>  process_scsi_op(struct disk_op_s *op)
>  {
> -    if (!CONFIG_VIRTIO_SCSI && !CONFIG_USB_MSC)
> +    if (!CONFIG_VIRTIO_SCSI && !CONFIG_USB_MSC && !CONFIG_USB_UAS)

I think I'll submit a patch to drop this config check - it's not
really needed.

[...]
> --- a/src/blockcmd.c
> +++ b/src/blockcmd.c
[...]
>  int
>  scsi_init_drive(struct drive_s *drive, const char *s, int prio)
>  {
> -    if (!CONFIG_USB_MSC && !CONFIG_VIRTIO_SCSI)
> +    if (!CONFIG_USB_UAS && !CONFIG_USB_MSC && !CONFIG_VIRTIO_SCSI)
>          return 0;

Same here.

[...]
> +int
> +usb_uas_init(struct usbdevice_s *usbdev)
> +{
> +    if (!CONFIG_USB_UAS)
> +        return -1;
> +
> +    // Verify right kind of device
> +    struct usb_interface_descriptor *iface = usbdev->iface;
> +    if (iface->bInterfaceSubClass != 0x06 ||
> +        iface->bInterfaceProtocol != 0x62) {

It would be preferable to #define constants for these numbers.

[...]
> +    while (desc) {
> +        desc += desc[0];
> +        switch (desc[1]) {
> +        case USB_DT_ENDPOINT:
> +            ep = (void*)desc;
> +            break;
> +        case 0x24:

Same here.

[...]
> --- /dev/null
> +++ b/src/usb-uas.h
> @@ -0,0 +1,2 @@
> +int uas_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize);
> +int usb_uas_init(struct usbdevice_s *usbdev);

Due to SeaBIOS' use of -fcombine during the build, all headers really
should have #ifdef guards on them.

[...]
> +    else if (iface->bInterfaceClass == USB_CLASS_MASS_STORAGE) {
> +        if (iface->bInterfaceProtocol == 0x50)
> +            ret = usb_msc_init(usbdev);
> +        if (iface->bInterfaceProtocol == 0x62)
> +            ret = usb_uas_init(usbdev);

Would prefer #define names.

-Kevin