| Message ID | 1513246316-56019-4-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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 <hare@suse.com> > --- > 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 *)&cmd->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 *)&cmd->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;
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 <hare@suse.com> >> --- >> 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 *)&cmd->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 *)&cmd->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
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 <hare@suse.com> >>> --- >>> 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 *)&cmd->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: 0xL3L3L2L2L1L10000 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.
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 <hare@suse.com> >>>> --- >>>> 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 *)&cmd->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: > 0xL3L3L2L2L1L10000 > 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 array even using 3-level LUNs. > A similar explanation in a kernel doc comment for the helper conversion > function(s) might be helpful. > Okay, I can update it. However, I'm waiting for some life signals from Paolo; a review would be pretty much appreciated... Cheers, Hannes
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 *)&cmd->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: > 0xL3L3L2L2L1L10000 > 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.
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 *)&cmd->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 *)&cmd->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 virtscsi_abort(struct scsi_cmnd *sc) .subtype = VIRTIO_SCSI_T_TMF_ABORT_TASK, .lun[0] = 1, .lun[1] = target_id, - .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 *)&cmd->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. */ - 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; @@ -1522,6 +1553,7 @@ static int virtscsi_restore(struct virtio_device *vdev) VIRTIO_SCSI_F_T10_PI, #endif VIRTIO_SCSI_F_RESCAN, + VIRTIO_SCSI_F_NATIVE_LUN, }; static struct virtio_driver virtio_scsi_driver = { diff --git a/include/uapi/linux/virtio_scsi.h b/include/uapi/linux/virtio_scsi.h index 762622e..30d275f 100644 --- a/include/uapi/linux/virtio_scsi.h +++ b/include/uapi/linux/virtio_scsi.h @@ -134,6 +134,7 @@ struct virtio_scsi_config { #define VIRTIO_SCSI_F_CHANGE 2 #define VIRTIO_SCSI_F_T10_PI 3 #define VIRTIO_SCSI_F_RESCAN 4 +#define VIRTIO_SCSI_F_NATIVE_LUN 5 /* Response codes */ #define VIRTIO_SCSI_S_OK 0
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 <hare@suse.com> --- drivers/scsi/virtio_scsi.c | 62 ++++++++++++++++++++++++++++++---------- include/uapi/linux/virtio_scsi.h | 1 + 2 files changed, 48 insertions(+), 15 deletions(-)