Re: [PATCH 3/3] virtio_scsi: Implement 'native LUN' feature

[Hannes Reinecke]

On 01/26/2018 03:15 PM, Steffen Maier wrote:
> On 12/18/2017 08:48 AM, Hannes Reinecke wrote:
>> On 12/15/2017 07:17 PM, Steffen Maier wrote:
>>> On 12/14/2017 11:11 AM, Hannes Reinecke wrote:
 The 'native LUN' feature allows virtio-scsi to pass in the LUN
 numbers from the underlying storage directly, without having
 to modify the LUN number itself.
 It works by shifting the existing LUN number down by 8 bytes,
 and add the virtio-specific 8-byte LUN steering header.
 With that virtio doesn't have to mangle the LUN number, allowing
 us to pass the 'real' LUN number to the guest.
>>>
>>> I only see shifts by 16 bits in the code below which would be 2 bytes.
>>> I had a quick look at the corresponding qemu code which looked the same
>>> to me.
>>> What's the relation to 8 byte shifting, which would be 64 bit shift and
>>> thus odd for a 64 bit LUN, mentioned in the description here?
>>>
>>> If the code keeps the LUN level 1 and 2 (dropping level 3 and 4) and I
>>> just don't understand it, it would be fine, I guess.
>>>
>> Yeah, messed that one up. It should be 8 _bits_, obviously.
> 
> Isn't it 16 bits or 2 bytes corresponding to one LUN level?
> See also below.
> 
Indeed, correct.

 Of course, we do cut off the last 8 bytes of the 'real' LUN number,
 but I'm not aware of any array utilizing that, so the impact should
 be negligible.
>>>
>>> Why did we do v3.17 commit 9cb78c16f5da ("scsi: use 64-bit LUNs")? ;-)
>>>
>> Because that patch just lifts the internal code to use 64-bit LUNs
>> without any changes to the behaviour.
>> This one uses the internal 64-bit LUNs and actually changes the
>> behaviour.
> 
> Sure, I was just being ironic, because your description sounded a bit as
> if all the LUN range extension is not even required because no storage
> array uses it.
> 
 Signed-off-by: Hannes Reinecke 
 ---
    drivers/scsi/virtio_scsi.c   | 62
 ++--
    include/uapi/linux/virtio_scsi.h |  1 +
    2 files changed, 48 insertions(+), 15 deletions(-)
> 
 @@ -524,10 +532,16 @@ static void virtio_scsi_init_hdr(struct
 virtio_device *vdev,
     int target_id,
     struct scsi_cmnd *sc)
    {
 -    cmd->lun[0] = 1;
 -    cmd->lun[1] = target_id;
 -    cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
 -    cmd->lun[3] = sc->device->lun & 0xff;
 +    if (virtio_has_feature(vdev, VIRTIO_SCSI_F_NATIVE_LUN)) {
 +    u64 lun = sc->device->lun << 16;
 +    lun |= ((u64)1 << 8) | (u64)target_id;
 +    int_to_scsilun(lun, (struct scsi_lun *)>lun);
 +    } else {
 +    cmd->lun[0] = 1;
 +    cmd->lun[1] = target_id;
 +    cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
 +    cmd->lun[3] = sc->device->lun & 0xff;
 +    }
>>>
>>> Above 2 patterns seem to repeat. Have helper functions (similar to
>>> int_to_scsilun()) now that it's more than just 4 lines of filling in the
>>> virtio lun?
>>>
>> Yes, can do.
> 
> Meanwhile I think I realized why I had trouble understanding what the
> code does. I guess, I expected a conversion with int_to_scsilun() first,
> and then we would fill in the virtio-specific parts of magic-one and
> target-ID.
> You do it just the other way round, which is OK.
> 
> Say we have a 4 level 64-bit LUN and represent it in hex using
> placeholder hexdigits for the 4 levels like this:
> 0xL1L1L2L2L3L3L4L4
> Its decimal SCSI LUN representation (in hex) is:
> 0xL4L4L3L3L2L2L1L1
> Then you shift left by 16 bits (2 bytes, 1 LUN level), basically
> dropping the 4th level:
> 0xL3L3L2L2L1L1
> The steering header is 0x01TT where TT is the target ID.
> You bitwise or the virtio-specific parts into the SCSI LUN representation:
> 0xL3L3L2L2L1L101TT
> Finally you convert it into the 64-bit LUN representation:
> 0x01TTL1L1L2L2L3L3
>   0123456789abcdef [char array indexes]
> So we nicely have the virtio-specific parts at those array indexes where
> the virtio-scsi protocol expects them.
> The usage of the other bytes is now of course different from the
> original LUN encoding: It allows more than just peripheral and flat
> space addressing for the 1st level; and it now also uses levels 2 and 3
> which were previously always zero. The 3rd level really requires 64-bit
> support in the kvm host kernel.
> This also means that a 4-level LUN is not supported unless we would
> create a new virtio-scsi protocol version that would transfer the target
> ID in a separate field not as part of the LUN field.
> 
> Did I get that right?
> 
I couldn't have it phrased better.
Essentially we're moving the original LUN down by two bytes, and
stuffing the original virtio target and LUN addressing onto it.
With that we can keep (most) of the original LUN, _and_ keep compability
with virtio (of sorts).

And with that we have to rely on no-one using 4-level LUNs; but so far
I've yet to see an 

Re: [PATCH 3/3] virtio_scsi: Implement 'native LUN' feature

[Steffen Maier]

On 01/26/2018 03:15 PM, Steffen Maier wrote:

On 12/18/2017 08:48 AM, Hannes Reinecke wrote:

On 12/15/2017 07:17 PM, Steffen Maier wrote:

On 12/14/2017 11:11 AM, Hannes Reinecke wrote:



@@ -524,10 +532,16 @@ static void virtio_scsi_init_hdr(struct
virtio_device *vdev,
    int target_id,
    struct scsi_cmnd *sc)
   {
-    cmd->lun[0] = 1;
-    cmd->lun[1] = target_id;
-    cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
-    cmd->lun[3] = sc->device->lun & 0xff;
+    if (virtio_has_feature(vdev, VIRTIO_SCSI_F_NATIVE_LUN)) {
+    u64 lun = sc->device->lun << 16;
+    lun |= ((u64)1 << 8) | (u64)target_id;
+    int_to_scsilun(lun, (struct scsi_lun *)>lun);
+    } else {
+    cmd->lun[0] = 1;
+    cmd->lun[1] = target_id;
+    cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
+    cmd->lun[3] = sc->device->lun & 0xff;
+    }


Above 2 patterns seem to repeat. Have helper functions (similar to
int_to_scsilun()) now that it's more than just 4 lines of filling in the
virtio lun?


Yes, can do.


Meanwhile I think I realized why I had trouble understanding what the 
code does. I guess, I expected a conversion with int_to_scsilun() first, 
and then we would fill in the virtio-specific parts of magic-one and 
target-ID.

You do it just the other way round, which is OK.

Say we have a 4 level 64-bit LUN and represent it in hex using 
placeholder hexdigits for the 4 levels like this:

0xL1L1L2L2L3L3L4L4
Its decimal SCSI LUN representation (in hex) is:
0xL4L4L3L3L2L2L1L1
Then you shift left by 16 bits (2 bytes, 1 LUN level), basically 
dropping the 4th level:

0xL3L3L2L2L1L1
The steering header is 0x01TT where TT is the target ID.
You bitwise or the virtio-specific parts into the SCSI LUN representation:
0xL3L3L2L2L1L101TT
Finally you convert it into the 64-bit LUN representation:
0x01TTL1L1L2L2L3L3
   0123456789abcdef [char array indexes]

 0 1 2 3 4 5 6 7
of course
So we nicely have the virtio-specific parts at those array indexes where 
the virtio-scsi protocol expects them.
The usage of the other bytes is now of course different from the 
original LUN encoding: It allows more than just peripheral and flat 
space addressing for the 1st level; and it now also uses levels 2 and 3 
which were previously always zero. The 3rd level really requires 64-bit 
support in the kvm host kernel.
This also means that a 4-level LUN is not supported unless we would 
create a new virtio-scsi protocol version that would transfer the target 
ID in a separate field not as part of the LUN field.


Did I get that right?

A similar explanation in a kernel doc comment for the helper conversion 
function(s) might be helpful.


--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 3/3] virtio_scsi: Implement 'native LUN' feature

[Steffen Maier]

On 12/18/2017 08:48 AM, Hannes Reinecke wrote:

On 12/15/2017 07:17 PM, Steffen Maier wrote:

On 12/14/2017 11:11 AM, Hannes Reinecke wrote:

The 'native LUN' feature allows virtio-scsi to pass in the LUN
numbers from the underlying storage directly, without having
to modify the LUN number itself.
It works by shifting the existing LUN number down by 8 bytes,
and add the virtio-specific 8-byte LUN steering header.
With that virtio doesn't have to mangle the LUN number, allowing
us to pass the 'real' LUN number to the guest.


I only see shifts by 16 bits in the code below which would be 2 bytes.
I had a quick look at the corresponding qemu code which looked the same
to me.
What's the relation to 8 byte shifting, which would be 64 bit shift and
thus odd for a 64 bit LUN, mentioned in the description here?

If the code keeps the LUN level 1 and 2 (dropping level 3 and 4) and I
just don't understand it, it would be fine, I guess.


Yeah, messed that one up. It should be 8 _bits_, obviously.


Isn't it 16 bits or 2 bytes corresponding to one LUN level?
See also below.


Of course, we do cut off the last 8 bytes of the 'real' LUN number,
but I'm not aware of any array utilizing that, so the impact should
be negligible.


Why did we do v3.17 commit 9cb78c16f5da ("scsi: use 64-bit LUNs")? ;-)


Because that patch just lifts the internal code to use 64-bit LUNs
without any changes to the behaviour.
This one uses the internal 64-bit LUNs and actually changes the behaviour.


Sure, I was just being ironic, because your description sounded a bit as 
if all the LUN range extension is not even required because no storage 
array uses it.



Signed-off-by: Hannes Reinecke 
---
   drivers/scsi/virtio_scsi.c   | 62
++--
   include/uapi/linux/virtio_scsi.h |  1 +
   2 files changed, 48 insertions(+), 15 deletions(-)



@@ -524,10 +532,16 @@ static void virtio_scsi_init_hdr(struct
virtio_device *vdev,
    int target_id,
    struct scsi_cmnd *sc)
   {
-    cmd->lun[0] = 1;
-    cmd->lun[1] = target_id;
-    cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
-    cmd->lun[3] = sc->device->lun & 0xff;
+    if (virtio_has_feature(vdev, VIRTIO_SCSI_F_NATIVE_LUN)) {
+    u64 lun = sc->device->lun << 16;
+    lun |= ((u64)1 << 8) | (u64)target_id;
+    int_to_scsilun(lun, (struct scsi_lun *)>lun);
+    } else {
+    cmd->lun[0] = 1;
+    cmd->lun[1] = target_id;
+    cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
+    cmd->lun[3] = sc->device->lun & 0xff;
+    }


Above 2 patterns seem to repeat. Have helper functions (similar to
int_to_scsilun()) now that it's more than just 4 lines of filling in the
virtio lun?


Yes, can do.


Meanwhile I think I realized why I had trouble understanding what the 
code does. I guess, I expected a conversion with int_to_scsilun() first, 
and then we would fill in the virtio-specific parts of magic-one and 
target-ID.

You do it just the other way round, which is OK.

Say we have a 4 level 64-bit LUN and represent it in hex using 
placeholder hexdigits for the 4 levels like this:

0xL1L1L2L2L3L3L4L4
Its decimal SCSI LUN representation (in hex) is:
0xL4L4L3L3L2L2L1L1
Then you shift left by 16 bits (2 bytes, 1 LUN level), basically 
dropping the 4th level:

0xL3L3L2L2L1L1
The steering header is 0x01TT where TT is the target ID.
You bitwise or the virtio-specific parts into the SCSI LUN representation:
0xL3L3L2L2L1L101TT
Finally you convert it into the 64-bit LUN representation:
0x01TTL1L1L2L2L3L3
  0123456789abcdef [char array indexes]
So we nicely have the virtio-specific parts at those array indexes where 
the virtio-scsi protocol expects them.
The usage of the other bytes is now of course different from the 
original LUN encoding: It allows more than just peripheral and flat 
space addressing for the 1st level; and it now also uses levels 2 and 3 
which were previously always zero. The 3rd level really requires 64-bit 
support in the kvm host kernel.
This also means that a 4-level LUN is not supported unless we would 
create a new virtio-scsi protocol version that would transfer the target 
ID in a separate field not as part of the LUN field.


Did I get that right?

A similar explanation in a kernel doc comment for the helper conversion 
function(s) might be helpful.


--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 3/3] virtio_scsi: Implement 'native LUN' feature

[Hannes Reinecke]

On 12/15/2017 07:17 PM, Steffen Maier wrote:
> Just a few very early view-only review comments.
> Haven't run the code.
> 
> On 12/14/2017 11:11 AM, Hannes Reinecke wrote:
>> The 'native LUN' feature allows virtio-scsi to pass in the LUN
>> numbers from the underlying storage directly, without having
>> to modify the LUN number itself.
>> It works by shifting the existing LUN number down by 8 bytes,
>> and add the virtio-specific 8-byte LUN steering header.
>> With that virtio doesn't have to mangle the LUN number, allowing
>> us to pass the 'real' LUN number to the guest.
> 
> I only see shifts by 16 bits in the code below which would be 2 bytes.
> I had a quick look at the corresponding qemu code which looked the same
> to me.
> What's the relation to 8 byte shifting, which would be 64 bit shift and
> thus odd for a 64 bit LUN, mentioned in the description here?
> 
> If the code keeps the LUN level 1 and 2 (dropping level 3 and 4) and I
> just don't understand it, it would be fine, I guess.
> 
Yeah, messed that one up. It should be 8 _bits_, obviously.

>> Of course, we do cut off the last 8 bytes of the 'real' LUN number,
>> but I'm not aware of any array utilizing that, so the impact should
>> be negligible.
> 
> Why did we do v3.17 commit 9cb78c16f5da ("scsi: use 64-bit LUNs")? ;-)
> 
Because that patch just lifts the internal code to use 64-bit LUNs
without any changes to the behaviour.
This one uses the internal 64-bit LUNs and actually changes the behaviour.

>> Signed-off-by: Hannes Reinecke 
>> ---
>>   drivers/scsi/virtio_scsi.c   | 62
>> ++--
>>   include/uapi/linux/virtio_scsi.h |  1 +
>>   2 files changed, 48 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
>> index f925fbd..63c2c85 100644
>> --- a/drivers/scsi/virtio_scsi.c
>> +++ b/drivers/scsi/virtio_scsi.c
>> @@ -356,8 +356,12 @@ static void
>> virtscsi_handle_transport_reset(struct virtio_scsi *vscsi,
>>   struct scsi_device *sdev;
>>   struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
>>   unsigned int target = event->lun[1];
>> -    unsigned int lun = (event->lun[2] << 8) | event->lun[3];
>> +    u64 lun;
>>
>> +    if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_NATIVE_LUN))
>> +    lun = scsilun_to_int((struct scsi_lun *)event->lun) >> 16;
>> +    else
>> +    lun = (event->lun[2] << 8) | event->lun[3];
> 
> 
>> @@ -524,10 +532,16 @@ static void virtio_scsi_init_hdr(struct
>> virtio_device *vdev,
>>    int target_id,
>>    struct scsi_cmnd *sc)
>>   {
>> -    cmd->lun[0] = 1;
>> -    cmd->lun[1] = target_id;
>> -    cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
>> -    cmd->lun[3] = sc->device->lun & 0xff;
>> +    if (virtio_has_feature(vdev, VIRTIO_SCSI_F_NATIVE_LUN)) {
>> +    u64 lun = sc->device->lun << 16;
>> +    lun |= ((u64)1 << 8) | (u64)target_id;
>> +    int_to_scsilun(lun, (struct scsi_lun *)>lun);
>> +    } else {
>> +    cmd->lun[0] = 1;
>> +    cmd->lun[1] = target_id;
>> +    cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
>> +    cmd->lun[3] = sc->device->lun & 0xff;
>> +    }
> 
> Above 2 patterns seem to repeat. Have helper functions (similar to
> int_to_scsilun()) now that it's more than just 4 lines of filling in the
> virtio lun?
> 
Yes, can do.

>> @@ -851,10 +871,18 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
>>   .subtype = VIRTIO_SCSI_T_TMF_ABORT_TASK,
> 
>>   .lun[0] = 1,
>>   .lun[1] = target_id,
> 
> drop those 2 superfluous lines, too?
> 
Hmm. Will be checking.

>> -    .lun[2] = (sc->device->lun >> 8) | 0x40,
>> -    .lun[3] = sc->device->lun & 0xff,
>>   .tag = cpu_to_virtio64(vscsi->vdev, (unsigned long)sc),
>>   };
>> +    if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_NATIVE_LUN)) {
>> +    u64 lun = sc->device->lun << 16;
>> +    lun |= ((u64)1 << 8) | (u64)target_id;
>> +    int_to_scsilun(lun, (struct scsi_lun *)>req.tmf.lun);
>> +    } else {
>> +    cmd->req.tmf.lun[0] = 1;
>> +    cmd->req.tmf.lun[1] = target_id;
>> +    cmd->req.tmf.lun[2] = (sc->device->lun >> 8) | 0x40;
>> +    cmd->req.tmf.lun[3] = sc->device->lun & 0xff;
>> +    }
> 
>>   return virtscsi_tmf(vscsi, cmd);
>>   }
>>
>> @@ -1429,7 +1457,10 @@ static int virtscsi_probe(struct virtio_device
>> *vdev)
>>   /* LUNs > 256 are reported with format 1, so they go in the range
>>    * 16640-32767.
>>    */
> 
> Above old comment now only seems to apply to the then case of the
> following if statement, not to the else case.
> 
Yes.
Will be doing a respin.

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/3] virtio_scsi: Implement 'native LUN' feature

[Steffen Maier]

Just a few very early view-only review comments.
Haven't run the code.

On 12/14/2017 11:11 AM, Hannes Reinecke wrote:

The 'native LUN' feature allows virtio-scsi to pass in the LUN
numbers from the underlying storage directly, without having
to modify the LUN number itself.
It works by shifting the existing LUN number down by 8 bytes,
and add the virtio-specific 8-byte LUN steering header.
With that virtio doesn't have to mangle the LUN number, allowing
us to pass the 'real' LUN number to the guest.


I only see shifts by 16 bits in the code below which would be 2 bytes.
I had a quick look at the corresponding qemu code which looked the same 
to me.
What's the relation to 8 byte shifting, which would be 64 bit shift and 
thus odd for a 64 bit LUN, mentioned in the description here?


If the code keeps the LUN level 1 and 2 (dropping level 3 and 4) and I 
just don't understand it, it would be fine, I guess.



Of course, we do cut off the last 8 bytes of the 'real' LUN number,
but I'm not aware of any array utilizing that, so the impact should
be negligible.


Why did we do v3.17 commit 9cb78c16f5da ("scsi: use 64-bit LUNs")? ;-)


Signed-off-by: Hannes Reinecke 
---
  drivers/scsi/virtio_scsi.c   | 62 ++--
  include/uapi/linux/virtio_scsi.h |  1 +
  2 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index f925fbd..63c2c85 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -356,8 +356,12 @@ static void virtscsi_handle_transport_reset(struct 
virtio_scsi *vscsi,
struct scsi_device *sdev;
struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
unsigned int target = event->lun[1];
-   unsigned int lun = (event->lun[2] << 8) | event->lun[3];
+   u64 lun;

+   if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_NATIVE_LUN))
+   lun = scsilun_to_int((struct scsi_lun *)event->lun) >> 16;
+   else
+   lun = (event->lun[2] << 8) | event->lun[3];




@@ -524,10 +532,16 @@ static void virtio_scsi_init_hdr(struct virtio_device 
*vdev,
 int target_id,
 struct scsi_cmnd *sc)
  {
-   cmd->lun[0] = 1;
-   cmd->lun[1] = target_id;
-   cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
-   cmd->lun[3] = sc->device->lun & 0xff;
+   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_NATIVE_LUN)) {
+   u64 lun = sc->device->lun << 16;
+   lun |= ((u64)1 << 8) | (u64)target_id;
+   int_to_scsilun(lun, (struct scsi_lun *)>lun);
+   } else {
+   cmd->lun[0] = 1;
+   cmd->lun[1] = target_id;
+   cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
+   cmd->lun[3] = sc->device->lun & 0xff;
+   }


Above 2 patterns seem to repeat. Have helper functions (similar to 
int_to_scsilun()) now that it's more than just 4 lines of filling in the 
virtio lun?



@@ -851,10 +871,18 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
.subtype = VIRTIO_SCSI_T_TMF_ABORT_TASK,



.lun[0] = 1,
.lun[1] = target_id,


drop those 2 superfluous lines, too?


-   .lun[2] = (sc->device->lun >> 8) | 0x40,
-   .lun[3] = sc->device->lun & 0xff,
.tag = cpu_to_virtio64(vscsi->vdev, (unsigned long)sc),
};
+   if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_NATIVE_LUN)) {
+   u64 lun = sc->device->lun << 16;
+   lun |= ((u64)1 << 8) | (u64)target_id;
+   int_to_scsilun(lun, (struct scsi_lun *)>req.tmf.lun);
+   } else {
+   cmd->req.tmf.lun[0] = 1;
+   cmd->req.tmf.lun[1] = target_id;
+   cmd->req.tmf.lun[2] = (sc->device->lun >> 8) | 0x40;
+   cmd->req.tmf.lun[3] = sc->device->lun & 0xff;
+   }



return virtscsi_tmf(vscsi, cmd);
  }

@@ -1429,7 +1457,10 @@ static int virtscsi_probe(struct virtio_device *vdev)
/* LUNs > 256 are reported with format 1, so they go in the range
 * 16640-32767.
 */


Above old comment now only seems to apply to the then case of the 
following if statement, not to the else case.



-   shost->max_lun = virtscsi_config_get(vdev, max_lun) + 1 + 0x4000;
+   if (!virtio_has_feature(vdev, VIRTIO_SCSI_F_NATIVE_LUN))
+   shost->max_lun = virtscsi_config_get(vdev, max_lun) + 1 + 
0x4000;
+   else
+   shost->max_lun = (u64)-1;
shost->max_id = num_targets;
shost->max_channel = 0;
shost->max_cmd_len = VIRTIO_SCSI_CDB_SIZE;



--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht 

[PATCH 3/3] virtio_scsi: Implement 'native LUN' feature

[Hannes Reinecke]

The 'native LUN' feature allows virtio-scsi to pass in the LUN
numbers from the underlying storage directly, without having
to modify the LUN number itself.
It works by shifting the existing LUN number down by 8 bytes,
and add the virtio-specific 8-byte LUN steering header.
With that virtio doesn't have to mangle the LUN number, allowing
us to pass the 'real' LUN number to the guest.
Of course, we do cut off the last 8 bytes of the 'real' LUN number,
but I'm not aware of any array utilizing that, so the impact should
be negligible.

Signed-off-by: Hannes Reinecke 
---
 drivers/scsi/virtio_scsi.c   | 62 ++--
 include/uapi/linux/virtio_scsi.h |  1 +
 2 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index f925fbd..63c2c85 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -356,8 +356,12 @@ static void virtscsi_handle_transport_reset(struct 
virtio_scsi *vscsi,
struct scsi_device *sdev;
struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
unsigned int target = event->lun[1];
-   unsigned int lun = (event->lun[2] << 8) | event->lun[3];
+   u64 lun;
 
+   if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_NATIVE_LUN))
+   lun = scsilun_to_int((struct scsi_lun *)event->lun) >> 16;
+   else
+   lun = (event->lun[2] << 8) | event->lun[3];
switch (virtio32_to_cpu(vscsi->vdev, event->reason)) {
case VIRTIO_SCSI_EVT_RESET_RESCAN:
scsi_add_device(shost, 0, target, lun);
@@ -368,7 +372,7 @@ static void virtscsi_handle_transport_reset(struct 
virtio_scsi *vscsi,
scsi_remove_device(sdev);
scsi_device_put(sdev);
} else {
-   pr_err("SCSI device %d 0 %d %d not found\n",
+   pr_err("SCSI device %d 0 %d %llu not found\n",
shost->host_no, target, lun);
}
break;
@@ -383,13 +387,17 @@ static void virtscsi_handle_param_change(struct 
virtio_scsi *vscsi,
struct scsi_device *sdev;
struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
unsigned int target = event->lun[1];
-   unsigned int lun = (event->lun[2] << 8) | event->lun[3];
+   u64 lun;
u8 asc = virtio32_to_cpu(vscsi->vdev, event->reason) & 255;
u8 ascq = virtio32_to_cpu(vscsi->vdev, event->reason) >> 8;
 
+   if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_NATIVE_LUN))
+   lun = scsilun_to_int((struct scsi_lun *)event->lun) >> 16;
+   else
+   lun = (event->lun[2] << 8) | event->lun[3];
sdev = scsi_device_lookup(shost, 0, target, lun);
if (!sdev) {
-   pr_err("SCSI device %d 0 %d %d not found\n",
+   pr_err("SCSI device %d 0 %d %llu not found\n",
shost->host_no, target, lun);
return;
}
@@ -524,10 +532,16 @@ static void virtio_scsi_init_hdr(struct virtio_device 
*vdev,
 int target_id,
 struct scsi_cmnd *sc)
 {
-   cmd->lun[0] = 1;
-   cmd->lun[1] = target_id;
-   cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
-   cmd->lun[3] = sc->device->lun & 0xff;
+   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_NATIVE_LUN)) {
+   u64 lun = sc->device->lun << 16;
+   lun |= ((u64)1 << 8) | (u64)target_id;
+   int_to_scsilun(lun, (struct scsi_lun *)>lun);
+   } else {
+   cmd->lun[0] = 1;
+   cmd->lun[1] = target_id;
+   cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
+   cmd->lun[3] = sc->device->lun & 0xff;
+   }
cmd->tag = cpu_to_virtio64(vdev, (unsigned long)sc);
cmd->task_attr = VIRTIO_SCSI_S_SIMPLE;
cmd->prio = 0;
@@ -776,11 +790,17 @@ static int virtscsi_device_reset(struct scsi_cmnd *sc)
.type = VIRTIO_SCSI_T_TMF,
.subtype = cpu_to_virtio32(vscsi->vdev,
 
VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET),
-   .lun[0] = 1,
-   .lun[1] = target_id,
-   .lun[2] = (sc->device->lun >> 8) | 0x40,
-   .lun[3] = sc->device->lun & 0xff,
};
+   if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_NATIVE_LUN)) {
+   u64 lun = sc->device->lun << 16;
+   lun |= ((u64)1 << 8) | (u64)target_id;
+   int_to_scsilun(lun, (struct scsi_lun *)>req.tmf.lun);
+   } else {
+   cmd->req.tmf.lun[0] = 1;
+   cmd->req.tmf.lun[1] = target_id;
+   cmd->req.tmf.lun[2] = (sc->device->lun >> 8) | 0x40;
+   cmd->req.tmf.lun[3] = sc->device->lun & 0xff;
+   }
return virtscsi_tmf(vscsi, cmd);
 }
 
@@ -851,10 +871,18 @@ static int