Patchwork [PULL,00/14] SCSI updates for 2012-07-02

login
register
mail settings
Submitter Paolo Bonzini
Date July 2, 2012, 9:41 a.m.
Message ID <1341222087-24920-1-git-send-email-pbonzini@redhat.com>
Download mbox
Permalink /patch/168530/
State New
Headers show

Pull-request

git://github.com/bonzini/qemu.git scsi-next

Comments

Paolo Bonzini - July 2, 2012, 9:41 a.m.
Anthony,

The following changes since commit 71ea2e016131a9fcde6f1ffd3e0e34a64c21f593:

  bsd-user: fix build (2012-06-28 20:28:36 +0000)

are available in the git repository at:

  git://github.com/bonzini/qemu.git scsi-next

for you to fetch changes up to 9ce1bb2d36f24af79d2757497acbaf4dc4a2e302:

  scsi: Fix transfer length for READ POSITION commands. (2012-07-02 11:27:00 +0200)

The MegaSAS changes are rebased and it includes improved support for
SCSI tape and media changer commands.

----------------------------------------------------------------
Christian Hoff (4):
      scsi: Fix data length == SCSI_SENSE_BUF_SIZE
      scsi: Fix LOAD_UNLOAD
      scsi: Add basic support for SCSI media changer commands.
      scsi: Fix transfer length for READ POSITION commands.

Hannes Reinecke (2):
      megasas: Add header file
      megasas: LSI Megaraid SAS HBA emulation

Paolo Bonzini (6):
      scsi: simplify handling of the VPD page length field
      scsi: add a qdev property for the disk's WWN
      atapi: implement READ DISC INFORMATION
      scsi-disk: implement READ DISC INFORMATION
      virtio-scsi: do not crash on adding buffers to the event queue
      scsi: Ensure command and transfer lengths are set for all SCSI devices

Ronnie Sahlberg (2):
      ISCSI: Add SCSI passthrough via scsi-generic to libiscsi
      ISCSI: force use of sg for SMC and SSC devices

 block/iscsi.c           |  152 ++++
 default-configs/pci.mak |    1 +
 hw/Makefile.objs        |    1 +
 hw/ide/atapi.c          |   31 +
 hw/megasas.c            | 2198 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/mfi.h                | 1248 +++++++++++++++++++++++++++
 hw/pci_ids.h            |    2 +
 hw/scsi-bus.c           |   94 +-
 hw/scsi-defs.h          |   15 +-
 hw/scsi-disk.c          |   69 +-
 hw/scsi-generic.c       |   13 +-
 hw/virtio-scsi.c        |    6 +-
 trace-events            |   79 ++
 13 files changed, 3872 insertions(+), 37 deletions(-)
 create mode 100644 hw/megasas.c
 create mode 100644 hw/mfi.h
Anthony Liguori - July 9, 2012, 4:48 p.m.
On 07/02/2012 04:41 AM, Paolo Bonzini wrote:
> Anthony,
>
> The following changes since commit 71ea2e016131a9fcde6f1ffd3e0e34a64c21f593:
>
>    bsd-user: fix build (2012-06-28 20:28:36 +0000)

Pulled.  Thanks.

Regards,

Anthony Liguori

>
> are available in the git repository at:
>
>    git://github.com/bonzini/qemu.git scsi-next
>
> for you to fetch changes up to 9ce1bb2d36f24af79d2757497acbaf4dc4a2e302:
>
>    scsi: Fix transfer length for READ POSITION commands. (2012-07-02 11:27:00 +0200)
>
> The MegaSAS changes are rebased and it includes improved support for
> SCSI tape and media changer commands.
>
> ----------------------------------------------------------------
> Christian Hoff (4):
>        scsi: Fix data length == SCSI_SENSE_BUF_SIZE
>        scsi: Fix LOAD_UNLOAD
>        scsi: Add basic support for SCSI media changer commands.
>        scsi: Fix transfer length for READ POSITION commands.
>
> Hannes Reinecke (2):
>        megasas: Add header file
>        megasas: LSI Megaraid SAS HBA emulation
>
> Paolo Bonzini (6):
>        scsi: simplify handling of the VPD page length field
>        scsi: add a qdev property for the disk's WWN
>        atapi: implement READ DISC INFORMATION
>        scsi-disk: implement READ DISC INFORMATION
>        virtio-scsi: do not crash on adding buffers to the event queue
>        scsi: Ensure command and transfer lengths are set for all SCSI devices
>
> Ronnie Sahlberg (2):
>        ISCSI: Add SCSI passthrough via scsi-generic to libiscsi
>        ISCSI: force use of sg for SMC and SSC devices
>
>   block/iscsi.c           |  152 ++++
>   default-configs/pci.mak |    1 +
>   hw/Makefile.objs        |    1 +
>   hw/ide/atapi.c          |   31 +
>   hw/megasas.c            | 2198 +++++++++++++++++++++++++++++++++++++++++++++++
>   hw/mfi.h                | 1248 +++++++++++++++++++++++++++
>   hw/pci_ids.h            |    2 +
>   hw/scsi-bus.c           |   94 +-
>   hw/scsi-defs.h          |   15 +-
>   hw/scsi-disk.c          |   69 +-
>   hw/scsi-generic.c       |   13 +-
>   hw/virtio-scsi.c        |    6 +-
>   trace-events            |   79 ++
>   13 files changed, 3872 insertions(+), 37 deletions(-)
>   create mode 100644 hw/megasas.c
>   create mode 100644 hw/mfi.h
Alexander Graf - July 9, 2012, 11:09 p.m.
On 09.07.2012, at 18:48, Anthony Liguori wrote:

> On 07/02/2012 04:41 AM, Paolo Bonzini wrote:
>> Anthony,
>> 
>> The following changes since commit 71ea2e016131a9fcde6f1ffd3e0e34a64c21f593:
>> 
>>   bsd-user: fix build (2012-06-28 20:28:36 +0000)
> 
> Pulled.  Thanks.

Megasas? :)


http://buildbot.b1-systems.de/qemu/builders/default_i386_rhel61/builds/304/steps/compile/logs/stdio
Anthony Liguori - July 9, 2012, 11:19 p.m.
On 07/09/2012 06:09 PM, Alexander Graf wrote:
>
> On 09.07.2012, at 18:48, Anthony Liguori wrote:
>
>> On 07/02/2012 04:41 AM, Paolo Bonzini wrote:
>>> Anthony,
>>>
>>> The following changes since commit 71ea2e016131a9fcde6f1ffd3e0e34a64c21f593:
>>>
>>>    bsd-user: fix build (2012-06-28 20:28:36 +0000)
>>
>> Pulled.  Thanks.
>
> Megasas? :)

So this code is really broken:

     info.host.type = MFI_INFO_HOST_PCIX;
     info.device.type = MFI_INFO_DEV_SAS3G;
     info.device.port_count = 2;
     info.device.port_addr[0] = cpu_to_le64(megasas_gen_sas_addr((uint64_t)s));

This will make migration impossible not to mention the fact that casting a 
pointer to a uint64_t is really broken.

This code needs to be refactored to not do this.  It's quite pervasive though 
(there's a half a dozen instances like this).

I'm going to disable the build by default.  I don't want to see a rash fix like 
(uint64_t)(intptr_t).  This needs to be fixed by not making the pointer address 
guest visible.  It can then be re-enabled.  Should be easy enough to update your 
.mak config if you want to test between now and then.

Regards,

Anthony Liguori

>
>
> http://buildbot.b1-systems.de/qemu/builders/default_i386_rhel61/builds/304/steps/compile/logs/stdio
>
>
Hannes Reinecke - July 10, 2012, 5:57 a.m.
On 07/10/2012 01:19 AM, Anthony Liguori wrote:
> On 07/09/2012 06:09 PM, Alexander Graf wrote:
>>
>> On 09.07.2012, at 18:48, Anthony Liguori wrote:
>>
>>> On 07/02/2012 04:41 AM, Paolo Bonzini wrote:
>>>> Anthony,
>>>>
>>>> The following changes since commit
>>>> 71ea2e016131a9fcde6f1ffd3e0e34a64c21f593:
>>>>
>>>>    bsd-user: fix build (2012-06-28 20:28:36 +0000)
>>>
>>> Pulled.  Thanks.
>>
>> Megasas? :)
> 
> So this code is really broken:
> 
>     info.host.type = MFI_INFO_HOST_PCIX;
>     info.device.type = MFI_INFO_DEV_SAS3G;
>     info.device.port_count = 2;
>     info.device.port_addr[0] =
> cpu_to_le64(megasas_gen_sas_addr((uint64_t)s));
> 
> This will make migration impossible not to mention the fact that
> casting a pointer to a uint64_t is really broken.
> 
Hey, this is _NOT_ an address. It's a simple way of generating a
system-wide unique SAS address.

The whole thing is informational anyway, and can only be seen when
using the (proprietary) MegaCLI userspace command.

> This code needs to be refactored to not do this.  It's quite
> pervasive though (there's a half a dozen instances like this).
> 

Okay, so here's the challenge: We need to generate a system-wide
unique SAS address, one per SCSI device and one per megasas instance.
A simple counter won't work, as we might have several qemu instances
running. Which would result in all of them having the same SAS
address for the host.

> I'm going to disable the build by default.  I don't want to see a
> rash fix like (uint64_t)(intptr_t).  This needs to be fixed by not
> making the pointer address guest visible.  It can then be
> re-enabled.  Should be easy enough to update your .mak config if you
> want to test between now and then.
> 
As said, it's _not_ an address. The address it just use to seed the
SAS address.

But as you object, I see to use something else for seeding the SAS
address.

Cheers,

Hannes
Paolo Bonzini - July 10, 2012, 7:06 a.m.
Il 10/07/2012 07:57, Hannes Reinecke ha scritto:
>> > This will make migration impossible not to mention the fact that
>> > casting a pointer to a uint64_t is really broken.
>> > 
> Hey, this is _NOT_ an address. It's a simple way of generating a
> system-wide unique SAS address.
> 
> The whole thing is informational anyway, and can only be seen when
> using the (proprietary) MegaCLI userspace command.

So even on real hardware it is not exported to the VPD (in the case of
the per-LUN address)?  And the per-port address is also not visible in VPD?

I recently added a wwn property to scsi-{hd,cd}, a similar property
should perhaps be added to the megasas device.  We can do the same thing
and add a default.  Not the pointer value, though, because it is not
migratable.  A counter is also problematic for migration when you have
hotplug/hotunplug.  You can instead use something like a CRC32 of the
device id.

Once it's added, we can add support for it in SCSIBusInfo so that it is
exported via VPD.

> Okay, so here's the challenge: We need to generate a system-wide
> unique SAS address, one per SCSI device and one per megasas instance.
> A simple counter won't work, as we might have several qemu instances
> running. Which would result in all of them having the same SAS
> address for the host.

That's not a problem as long as we're not supporting things like
persistent reservations across guests (just like it's not a problem if
you give the same MAC address to network cards with slirp).

Paolo
Anthony Liguori - July 10, 2012, 12:52 p.m.
On 07/10/2012 12:57 AM, Hannes Reinecke wrote:
> On 07/10/2012 01:19 AM, Anthony Liguori wrote:
>> On 07/09/2012 06:09 PM, Alexander Graf wrote:
>>>
>>> On 09.07.2012, at 18:48, Anthony Liguori wrote:
>>>
>>>> On 07/02/2012 04:41 AM, Paolo Bonzini wrote:
>>>>> Anthony,
>>>>>
>>>>> The following changes since commit
>>>>> 71ea2e016131a9fcde6f1ffd3e0e34a64c21f593:
>>>>>
>>>>>     bsd-user: fix build (2012-06-28 20:28:36 +0000)
>>>>
>>>> Pulled.  Thanks.
>>>
>>> Megasas? :)
>>
>> So this code is really broken:
>>
>>      info.host.type = MFI_INFO_HOST_PCIX;
>>      info.device.type = MFI_INFO_DEV_SAS3G;
>>      info.device.port_count = 2;
>>      info.device.port_addr[0] =
>> cpu_to_le64(megasas_gen_sas_addr((uint64_t)s));
>>
>> This will make migration impossible not to mention the fact that
>> casting a pointer to a uint64_t is really broken.
>>
> Hey, this is _NOT_ an address. It's a simple way of generating a
> system-wide unique SAS address.

But it's not stable across migration.  That's the problem.

> The whole thing is informational anyway, and can only be seen when
> using the (proprietary) MegaCLI userspace command.

Nonetheless, it's still guest visible.

>> This code needs to be refactored to not do this.  It's quite
>> pervasive though (there's a half a dozen instances like this).
>>
>
> Okay, so here's the challenge: We need to generate a system-wide
> unique SAS address, one per SCSI device and one per megasas instance.
> A simple counter won't work, as we might have several qemu instances
> running. Which would result in all of them having the same SAS
> address for the host.

You could used a hashed uuid.

Regards,

Anthony Liguori

>
>> I'm going to disable the build by default.  I don't want to see a
>> rash fix like (uint64_t)(intptr_t).  This needs to be fixed by not
>> making the pointer address guest visible.  It can then be
>> re-enabled.  Should be easy enough to update your .mak config if you
>> want to test between now and then.
>>
> As said, it's _not_ an address. The address it just use to seed the
> SAS address.
>
> But as you object, I see to use something else for seeding the SAS
> address.
>
> Cheers,
>
> Hannes
Hannes Reinecke - July 10, 2012, 1:01 p.m.
On 07/10/2012 02:52 PM, Anthony Liguori wrote:
> On 07/10/2012 12:57 AM, Hannes Reinecke wrote:
>> On 07/10/2012 01:19 AM, Anthony Liguori wrote:
>>> On 07/09/2012 06:09 PM, Alexander Graf wrote:
>>>>
>>>> On 09.07.2012, at 18:48, Anthony Liguori wrote:
>>>>
>>>>> On 07/02/2012 04:41 AM, Paolo Bonzini wrote:
>>>>>> Anthony,
>>>>>>
>>>>>> The following changes since commit
>>>>>> 71ea2e016131a9fcde6f1ffd3e0e34a64c21f593:
>>>>>>
>>>>>>     bsd-user: fix build (2012-06-28 20:28:36 +0000)
>>>>>
>>>>> Pulled.  Thanks.
>>>>
>>>> Megasas? :)
>>>
>>> So this code is really broken:
>>>
>>>      info.host.type = MFI_INFO_HOST_PCIX;
>>>      info.device.type = MFI_INFO_DEV_SAS3G;
>>>      info.device.port_count = 2;
>>>      info.device.port_addr[0] =
>>> cpu_to_le64(megasas_gen_sas_addr((uint64_t)s));
>>>
>>> This will make migration impossible not to mention the fact that
>>> casting a pointer to a uint64_t is really broken.
>>>
>> Hey, this is _NOT_ an address. It's a simple way of generating a
>> system-wide unique SAS address.
> 
> But it's not stable across migration.  That's the problem.
> 
>> The whole thing is informational anyway, and can only be seen when
>> using the (proprietary) MegaCLI userspace command.
> 
> Nonetheless, it's still guest visible.
> 

Okay, I see your point. I'll be reworking things to not use the
pointer here.

>>> This code needs to be refactored to not do this.  It's quite
>>> pervasive though (there's a half a dozen instances like this).
>>>
>>
>> Okay, so here's the challenge: We need to generate a system-wide
>> unique SAS address, one per SCSI device and one per megasas instance.
>> A simple counter won't work, as we might have several qemu instances
>> running. Which would result in all of them having the same SAS
>> address for the host.
> 
> You could used a hashed uuid.
> 
Right. Will see how that'll work out.

Cheers,

Hannes