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@xxxxxxxx>
---
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;
--
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
