[Qemu-devel] [RFC PATCH v1 0/5] Enable virtio-scsi boot from /dev/sgX

[Qemu-devel] [RFC PATCH v1 0/5] Enable virtio-scsi boot from /dev/sgX

[Eric Farman]

Today, trying to boot a guest from a SCSI LUN on s390x yields the following:

  virtio-blk               = OK
  virtio-scsi and /dev/sdX = OK
  virtio-scsi and /dev/sgX = FAIL

Example of the failing scenario:

  /usr/bin/qemu-system-s390x ...
    -device virtio-scsi-ccw,id=scsi0,devno=fe.0.0001
    -drive file=/dev/sg2,if=none,id=drive0,format=raw
    -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=0,drive=drive0,id=disk0
  LOADPARM=[........]
  Using virtio-scsi.
  Using SCSI scheme.
  ..
  ! virtio-scsi:read_many: STATUS=02 RSPN=70 KEY=0b CODE=00 QLFR=06, sure !

Why do we care?  Well, libvirt converts a virtio-scsi device from the host
SCSI address (host:bus:target:lun) into the associated /dev/sgX device,
which means we can't boot from virtio-scsi and have to rely on virtio-blk
for this action.

The short version of what happens is the host device driver rejects our
requests because the transfer lengths are too long for it to satisfy.
A virtio-scsi disk connected via scsi-generic is fine as a non-boot device
because the guest kernel is able to break up the requests for us.  So we just
need to handle this situation for the boot process.

Patches 2-N in this series do that, but rely on us to specify the max_sectors
parameter for the virtio-scsi-ccw device:

  /usr/bin/qemu-system-s390x ...
    -device virtio-scsi-ccw,id=scsi0,devno=fe.0.0001,max_sectors=2048

Without this, the default of xFFFF is used and we still end up exceeding
the limit in the host drivers.  Which is why we have Patch 1, and thus the
"RFC" tag on this series.  The backing block device has a maximum transfer
value calculated with the help of the BLKSECTGET ioctl[1][2].

With patch 1 applied, we are able to go through the children devices and
apply any known limit received from this IOCTL, and thus break up our requests
before submitting them to the host.  It is admittedly a little coarse, since
the max_sectors value is associated with the virtio-scsi controller and
multiple devices can be attached to it, not all of which are associated
with our boot device.  Meanwhile, each attached device can have its own
maximum transfer from the backing device.

[1] http://git.qemu-project.org/?p=qemu.git;a=commitdiff;h=482652502e98b1d570de0585f01dd55e35fdebfb
[2] http://git.qemu-project.org/?p=qemu.git;a=commitdiff;h=c4c41a0a65d650b241dec9efedf4ee3f00a16a30

Eric Farman (5):
  hw/scsi: Override the max_sectors value for virtio-scsi
  pc-bios/s390-ccw: Remove duplicate blk_factor adjustment
  pc-bios/s390-ccw: Move SCSI block factor to outer read
  pc-bios/s390-ccw: Break up virtio-scsi read into multiples
  pc-bios/s390-ccw.img: rebuild image

 hw/scsi/virtio-scsi.c          |  12 ++++++++++++
 pc-bios/s390-ccw.img           | Bin 26456 -> 26456 bytes
 pc-bios/s390-ccw/s390-ccw.h    |   4 ++++
 pc-bios/s390-ccw/virtio-scsi.c |  29 ++++++++++++++++++++---------
 4 files changed, 36 insertions(+), 9 deletions(-)

-- 
2.10.2

[Qemu-devel] [RFC PATCH v1 1/5] hw/scsi: Override the max_sectors value for virtio-scsi

[Eric Farman]

The virtio spec states that the max_sectors field is
"a hint to the driver for the maximum transfer size"
that would be used for a virtio-scsi request.

It's currently hardcoded to xFFFF unless one is established
by the max_sectors parameter on the command line, but let's
roll through the associated devices and set it to anything
lower if one is set for the underlying block device and
retrieved by the BLKSECTGET ioctl.

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 hw/scsi/virtio-scsi.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 46a3e3f..bca9461 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -640,7 +640,11 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
                                    uint8_t *config)
 {
     VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
+    VirtIOSCSI *vs = VIRTIO_SCSI(vdev);
     VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
+    SCSIDevice *d;
+    BusChild *kid;
+    unsigned int max_transfer;
 
     virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
     virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
@@ -652,6 +656,14 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
     virtio_stw_p(vdev, &scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
     virtio_stw_p(vdev, &scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
     virtio_stl_p(vdev, &scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
+
+    QTAILQ_FOREACH(kid, &vs->bus.qbus.children, sibling) {
+        d = SCSI_DEVICE(kid->child);
+        max_transfer = blk_get_max_transfer(d->conf.blk) / d->blocksize;
+        virtio_stl_p(vdev,
+                     &scsiconf->max_sectors,
+                     MIN_NON_ZERO(max_transfer, scsiconf->max_sectors));
+    }
 }
 
 static void virtio_scsi_set_config(VirtIODevice *vdev,
-- 
2.10.2

[Qemu-devel] [RFC PATCH v1 2/5] pc-bios/s390-ccw: Remove duplicate blk_factor adjustment

[Eric Farman]

When using virtio-scsi, we multiply the READ(10) data_size by
a block factor twice when building the I/O.  This is fine,
since it's only 1 for SCSI disks, but let's clean it up.

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/virtio-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index d850a8d..69b7a93 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -154,7 +154,7 @@ static bool scsi_read_10(VDev *vdev,
     VirtioCmd read_10[] = {
         { &req, sizeof(req), VRING_DESC_F_NEXT },
         { &resp, sizeof(resp), VRING_DESC_F_WRITE | VRING_DESC_F_NEXT },
-        { data, data_size * f, VRING_DESC_F_WRITE },
+        { data, data_size, VRING_DESC_F_WRITE },
     };
 
     debug_print_int("read_10  sector", sector);
-- 
2.10.2

[Qemu-devel] [RFC PATCH v1 3/5] pc-bios/s390-ccw: Move SCSI block factor to outer read

[Eric Farman]

Simple refactoring so that the blk_factor adjustment is
moved into virtio_scsi_read_many routine, in preparation
for another change.

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/virtio-scsi.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index 69b7a93..6d070e2 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -142,14 +142,13 @@ static bool scsi_report_luns(VDev *vdev, void *data, uint32_t data_size)
 }
 
 static bool scsi_read_10(VDev *vdev,
-                         ulong sector, int sectors, void *data)
+                         ulong sector, int sectors, void *data,
+                         unsigned int data_size)
 {
-    int f = vdev->blk_factor;
-    unsigned int data_size = sectors * virtio_get_block_size() * f;
     ScsiCdbRead10 cdb = {
         .command = 0x28,
-        .lba = sector * f,
-        .xfer_length = sectors * f,
+        .lba = sector,
+        .xfer_length = sectors,
     };
     VirtioCmd read_10[] = {
         { &req, sizeof(req), VRING_DESC_F_NEXT },
@@ -255,7 +254,10 @@ static void virtio_scsi_locate_device(VDev *vdev)
 int virtio_scsi_read_many(VDev *vdev,
                           ulong sector, void *load_addr, int sec_num)
 {
-    if (!scsi_read_10(vdev, sector, sec_num, load_addr)) {
+    int f = vdev->blk_factor;
+    unsigned int data_size = sec_num * virtio_get_block_size() * f;
+
+    if (!scsi_read_10(vdev, sector * f, sec_num * f, load_addr, data_size)) {
         virtio_scsi_verify_response(&resp, "virtio-scsi:read_many");
     }
 
-- 
2.10.2

[Qemu-devel] [RFC PATCH v1 4/5] pc-bios/s390-ccw: Break up virtio-scsi read into multiples

[Eric Farman]

A virtio-scsi request that goes through the host sd driver and
exceeds the maximum transfer size is automatically broken up
for us. But the equivalent request going to the sg driver
presumes that any length requirements have already been honored.
Let's use the max_sectors field from the device and break up
all virtio-scsi requests (both sd and sg) to avoid problem from
the host drivers.

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/s390-ccw.h    |  4 ++++
 pc-bios/s390-ccw/virtio-scsi.c | 19 ++++++++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index ded67bc..e1f3751 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -42,6 +42,10 @@ typedef unsigned long long __u64;
 #ifndef NULL
 #define NULL    0
 #endif
+#ifndef MIN
+#define MIN(a, b) (((a) < (b)) ? (a) : (b));
+#endif
+
 
 #include "cio.h"
 #include "iplb.h"
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index 6d070e2..90a8cc8 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -254,12 +254,21 @@ static void virtio_scsi_locate_device(VDev *vdev)
 int virtio_scsi_read_many(VDev *vdev,
                           ulong sector, void *load_addr, int sec_num)
 {
+    int sector_count;
     int f = vdev->blk_factor;
-    unsigned int data_size = sec_num * virtio_get_block_size() * f;
-
-    if (!scsi_read_10(vdev, sector * f, sec_num * f, load_addr, data_size)) {
-        virtio_scsi_verify_response(&resp, "virtio-scsi:read_many");
-    }
+    unsigned int data_size;
+
+    do {
+        sector_count = MIN(sec_num, vdev->config.scsi.max_sectors);
+        data_size = sector_count * virtio_get_block_size() * f;
+        if (!scsi_read_10(vdev, sector * f, sector_count * f, load_addr,
+                          data_size)) {
+            virtio_scsi_verify_response(&resp, "virtio-scsi:read_many");
+        }
+        load_addr += data_size;
+        sector += sector_count;
+        sec_num -= sector_count;
+    } while (sec_num > 0);
 
     return 0;
 }
-- 
2.10.2

[Qemu-devel] [RFC PATCH v1 5/5] pc-bios/s390-ccw.img: rebuild image

[Eric Farman]

Contains the following commits:
- pc-bios/s390-ccw: Remove duplicate blk_factor adjustment
- pc-bios/s390-ccw: Move SCSI block factor to outer read
- pc-bios/s390-ccw: Break up virtio-scsi read into multiples

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw.img | Bin 26456 -> 26456 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)

diff --git a/pc-bios/s390-ccw.img b/pc-bios/s390-ccw.img
index 2a4adfa654844040dbc84e90a9c2e0af56fdb9fa..0451d319e691544ded585846cfcb8f1799b3fbf5 100644
GIT binary patch
delta 4701
zcmZu#3sjWV7CvWYm<J3nFb`%15V(RuqC8a;6mXH54;Ty-6N`N0Gs7f%NDI4pOUnnb
z)vUb5O>??tg<=LpIo+#eYBnh#_(%#Qu9*Fkpmsz4yU#|*<+}e`%>MqpzrD{s`|NZ6
zvuHZWn@;lbM(>#gjGmbVueN*<I4!yF?N13+{5{hnRfcyq=@-p()oim5?H;Dh8vS){
z^)kgw1@DH#s`a*&u1k#hcDuqE3w@mMuL6f}gh#zT;;CR!Pvh5NzB(f6u&}Z(g*D`R
zVm?5)gYYzg!#b<?s1I{Rm=21y;UOV`tp)>Qs?LiZEmLVcogYaK7U^;$@fp~wE#u=L
zRu>g>q(SM?8kln}Gl?L<i$y@6PQsrOz9jIV7vNdl6x*TJ@FI-aT2*9as#7DRW&?8-
zqt|ixLANr<L;RJQ;Zif9<>aY{Mc&=Er9$KVi_qBJgnt$|^lz}r=RvzYfmGFG-b?rc
zGEafaKGDnVhRmX(a8j2OWewq%i4yI)B+Iw1IJ;z*c(?tRY^B7TNq80UOvGz)cJ*#0
zUOU+q5$_(tbBV`bjXs#KhaLJPpEqh%>{VA4=D8it>JM=jyy2UYWs4Vd-F^yln($tM
zLnhM@e%5GA=1H;!llAX}Y29?M5pyJ1{5JD<VYlDBn61PKBF;mE0|XBKNJQ&d>9jHC
zipS`Mkm$d_mMs`sH?h+RA0zf6sPeyNJ4%|#LSsEaI7?vrPf%ybv27uZg{*rB8_4=0
zL<YQWOBEW;=VZ+#{0Uj}p)p`6Ujn@WQ*7O&e~LJHgjW#9ALa(G;OC$|Fb02n14DT(
zgcwt8%LPX*2~N;u!j}cMt%G7?x@`h!4wLmO!e5ZJ5RMwp_02_1a|xP9R+@_y=VBUZ
z`v++TlP~QZb*RAB&%oQfH1lC$_4<*lUWCt)^%+sq-^bzlJS5)DQ^@wMz?KVyzb4)S
zIB0g-I!W^wSt|)IB&!#!vdp&aBTW)npCCM3;GpAh*)q#mAhz06T2Ivt;^e}Npd3CC
zb_Yc(EH-clnG_Zyv<GFBnbg8vwNB8?R|)@>LSfI#@{Km<7fgvewA5u_%#>At`+Ana
zUEIHCjX#seV)CYxRrAQ3llPIW++8xjr6yF-;bH!f{HD3Q#Oym^k9BZPHK|5Ys20L;
z0$aYNP_MSS-Ou5M5PgzkJ2en=J2CC#b&@b0CYJpWVjEU=tU>86u42v-`5G~b1w$zn
z7*N+GudYaxcA=zBG>^ya*&$fQ*~Gd)9%Y2j2yA(hG8uy&6~w!bY~K*?5yJl_9%3wA
zl(P%IvYGkwaNagP+o&hPCL+_)D5>U_A%cAUvZ${yyFpo5_~(fj_#63@kk1Ulbgo(O
zT(gJT&X8snS*e+neYN$#TlSo?on(z7D;^Fw?J&Zh(|FH|@yw0_H?F6<a^MUOTt`fA
zVrCIm5q^if9!0LIzx09Bc#1BvX9%w1D&Y-+Yq;(Zr$?q+s&qN9cC(>0INE9&LA-D9
zY#UC9HlQgQ>Vp507I+V-=sYtWB)q#n5iF4w>poLMPv_gxq`nV|`2tNO0PG=h)JF30
z7e1yXgmr|SBBAc3)WXYPd}wOa9AV`b$x5Z;&4iyKW-2l7hqpp)JRNF6SMvr)9yBY4
z&Rd=@m_|B{cn;xTgjd(Tf~s;RJOhkyaL_1)DF&_%3LTnq`&;<7)FHlwG)x#dln8S4
zbEJua#IRJp8kUE}@HMbKZ0tk>o%D=OdgCUMFQdvBaWQc;zgak1|56z2WZyyAyhwN(
z;bgFer%f;pBNZK&##q8j$*d#101Nwuc=ta??{mo3H~dV21G`~ccuX99lUdpw)=^}w
z7YSY4j!_QuEst1mQzTps&*!%wE8-LWZgo$@M6NJ;Rc8&3!y8C-Nz|JPezDpRy^{0q
zVQtJLtHEDH`85=y{Mva@TGY3^{$D~{%u9Uz&Sm#-g}(uo4_nAvpmA6?UkoQ=4=Vi3
zuy*)dg`cjvWB4oFRwQEF{7Gcx+lS8!?DH&ah?_83n@HvZWDX_#A(^KNe8qr62a0#?
zAm{rm!?}dfA$F6h>Wk&T&{S0)tZMvd8*Wb2(B||FButyr2RCPYLK&S}H>p2-HDtYC
z#P-GmQ<gj5$8*WBZ&LsIX|UTdR*GQgdkH5J+fD2(q?buQMBuQ#&0X4>;fcm;*5lM#
zv=Q~!$z!$Y|Cm07*o4Py(*)thcNCKDFE{7_^Ii!16Oy<D?FrBEIWRv_j<_WHQ*Uzq
z(JQrvOEoOA)gZZ8B<|gJgtz)8tWA2DAAz={h5R*`njFdjmLw<Yb$YDiPC@bd3f@c3
zG#|gc1<E{=`nQ1Bo6wt_%6G!pkqtg|$RF;?K)xT?N0pWp7$O$os4WhIREIh6QHycp
zCK|tvnfkKGG)N+=D$=?ss~1q$j@j+PzmHwz=of_Cqr|9UD(7P6U0?`Ev7R#rJ9jY+
z+E82I4oIQG-g`zTwM*^$r?ax*nemd8>5$ry(u7w|9P0MuRkeC%T@fzTRYaq;+`~{`
z=`o03sS<xPL`X-t$BL<U6kz%oF0|Ido@gzW-ouv0ns}Qt4AL)7D@#JoO_UsE56Wc@
zgGY-}S%yH5K!Gwu<R)jd8p^#{`Bql$<1At7S_KQh*3pV^x@&@Q>v*k|ZL|glhXw{@
zIxAVNiS2Vn$}c)&7{g8|7fs@EVDY=KS>pTlmhPXfO2$VvwpK<Iut?`0w~k`n_8$t1
zfcHnI`EG5E7mJNsnU9<WU855;t3*kYY9Tfyi4TUNl+VOHYs}WD3Th6ONaNZDk&>2n
zM$a~kfYm?^Pi%w6G0}>|GZka+<y9~de^<bUu|q<Veiv?kyoDnVbB<vOEMN_~psqU&
zPL7?P_}f{Eh~Ir#{;ghMuO|d<ALV?VapxYHc1734S}m&(`mVDuJ$0KR@dvn^8m;JQ
z08?6oZ&9nEQpw6Gll?FuZ63G6?z9j1NXQ#Elz$GT;~vhXV_4%WVtD9|sWJ=P){M7;
zb$LZ@lXE#!J4<?B!S^>s9xA+FBli)3H44JB1y<6OJoyki{w3dlKWpRh0oV8;c8M-z
z>T$wBqOXB+GvUsPbc<c0Pm(%`H1tVQ(kDqe4e{ybsH0*4^<c983oqt8;Q|dg=k{WW
zm<Owuq_6NTTZ^A#y)4)E;CF6|EPq;uvL0nU$}1?l(4H*IpXpIfMY#-R3Cb%dcggah
zLX>T?eB?aNs8E)_5psttpV)yC<DA4eDva;OII28ZK79Zs+Ur#)56E&^eH{WMHiV$;
zk>y4-c!kLFnG%%qWw{CUURzK`qTGuz8RcP=!j1_xV`6ILHzQA-jB-9oOtg6~O7uTl
zh_V#rE|eJOTo1}FSw4?`8ofMEK92@Xnk-+4MY#-R7fKxH2OLOq9_0>{=x|Ym9jiyV
z0cDmfw`8F#!Ij5>w3yJPNR-HLN2ycEa>o^vd9wVULP*Pw<3+G6d;IvC7KZnK{Of-(
zYlcBcZ)X!XsW*FN`MDJRW`@Do7Fqtd7?<NF6la^DC)>|nQ-f;L00~bQ#Pt}a1|oBY
zC~72_niIz}pfo3)7eignQ(jNwa`eKQ91CPSqIe3FI7TUsx?nGgV=icOOjMlM4zaoS
z+3+Z5*>^<71eI8w?<T>9+(C-(T3~PP&?qs34mR;+?^kq+mgC0;=vheKg^S&S*)FGS
z_0!|H7Sg82C9FY~c7RFgtCD5;6wZwK-J!>+sBryGErK1>hvFUQ@bnR7r#kK;;oMbq
zNNPbn4m$7%J=Wp0n8n@tE2z&v{hk3fCE<l@b-L*;ejzqj2zDtZ%nKN-c;G?2qvDx-
zhby*==*#+>(Dr~>UQ^x(MMDi_<)vfc*XG3~IFLN|4hPIlG-6}rUHas{#%z?JEpIgX
zhUAY>oH+_p^Rw;eT98|UfBg>@M>8LrHv<mj55?2ze0~luhu9g}gD+NLszq$_i+(Tp
zuG6pg5o@zSyHRtyqm)YSfH^bl)t}60(!l0<nw`B5sTHY?iUlLQGj1<Sh89v5jPkzY
VA4E?t7_Gy&;>oV6u31pY{|8Gx?qmP}

delta 4612
zcmZu#3sh9s6@B+H9~ge)3<L8JWQ-s~{2(6@1w~RcG@^`%iYZB~VkC+ND>bgD#iTT!
zMk{(ylN6I_wHc$vptw{LG-FDQ8b2e7qT(k-3_+4SDyV6c+j9?UNLJrk!`bibea^l2
z+}~R?o@9+DS@}7`G*zN!n!5JdK~vW8p0}SYRI#l=F+WtC2y2<0@kOhCv`u-;tMba{
zuIeIry37tio#ss?&D|nN{#|aHB!#CC{$60)40vAqG1Eh^ZX){}3UvLW$^|PO5^UHe
zQcfYfhwwy!BlcBq)73G#Ujr0-+rs)@X*L=qNptgRhqt8Bcij9~bx4tJ4+_V@4)0Rt
z1A~2{V%9czb$A;kSD6$flJp%e68hdG{2JjFf%|2{%RUp7x0-D;F=untOsk}+AF5t3
zO73Ee+6Y&C7Ka=l{o-_+dV$bF5gFbHg@!I=gzy-?6`s&a!e0|R8J74y9je?XS2fW~
z34cU%2(<ay7b;EZGiTb!T~3lL!haSxtkd1X`TcfRCwGf?<uGx}NjHG-64G5GU8AeB
zdp+rP6IV$1HsM*MI}R)T`m(7|>6hU9e2qq0>#o946u?El&)6z>*FQPKdQZrGc2ns4
zgm+MAFf|ciwPR;~FFe+-iQPm>nm3<~q+}2ru!b#y4+3V#{FXE!q<NfhAA$QW6WQ)9
zb}5qN9)<(8Lww*IB~&Q9J*4&}e2mnIP!)JfsUXi&!ef1#aE8F4JE7K?t;{4(FtJMr
z8;N}xBKy3f=!HjrfY@-t`-tri=lVpmB<SulK{-VJmq<esqF*GiatUUc7BLr`HO1iH
zZc{i5hcI)BGD_%ms?db&CEP~(TqriDDFNiEBX%9(gT&h5i1|{_SadX&kXT}AELI##
z&~Rk|c?M7{?HygXz}DBnV40sjNU++kgeUkk;Y-Ah6E*!|G|tbn;@uKL+zHZsNcaoV
z#lc>SOW8-B1;iE+o-1(3X;>2c8)XG~5{Ml`cnGl@p)GiZnYLQiv%;^rLz-}y7Lv_O
z@Ii=OwzPmJBuKVgg&QGbOU?Bpdqc=9pAdePLfS=l?_75sl)P|<E_WLxDae?I`+A1a
zQ~YqxnyV?|1&XFFYi3jQ35x#Ldgdv4q@_iup~J)S5yee(Yekjz!8YrF>{4<Kr&K!#
z$5N_yDAn3#k7osL2(eDGmHJ0X3rHDCQSTC_!zB0*5T*<&-O%9WDXx-SCHxjCQiZ}x
z%`>8}lP_BruWmt0E74-W?b#|+=4et~riclI&kG!!PCXfk5$i~2ChjQdh7kTI=@4V-
zqFkMDSh28)(4>r+WNIYAUr0<_&8w&8{}JNbZKA&B&;~DO{!b^O=^(|FP|Rb5>0Gnm
zxfU9(Y$nePVremX_0;wNyce2X`XaGW#Ny$A!;T>QCbjp9XwMRs=fU~(I3JzCrd(1Q
zNNFUjA-s^Ho=2_bVd+h@@Z?V7{e)Kjj&PpP8gDzq>5=YHoo)x#E*h5iwOh^qCEZax
z+o)hNZ9sD;)b@QWwa;xL={yU1mGB1-JAx(h#=1|}({uBEHM8eJv3y7i^D=~n@u(RT
z6DVSWk_h_{UMxD)HJ_BpFeW@DDn_vEDzQ{Lc7gD(NSQ)P3%nPuFkjdozKs1168p`F
zq4Sna6-qOmMl73fjfm>pl~+|RiAdvB*xN5jHg~~y{lcR)5AMSE)mCvAQov0LC=p`&
z3i5P7d_)R+5*9|puq=2xV$}FRI_V`k>CI1z{z_CCGfpP97BB-_dpH$=H;AvKR5^q<
z5SGAdOMSw0hg@`An(h*wFKAN(;W=2?cg1_4i{76huBZ9wgb%?6TTCq7$x>=BHi@uP
zbm-RGm}O7T^oR*JtMHv|Dmw}p{r9m&)%W_3XR_&Zb;f{L`~s;iiTbO|wpSbNiy7Ml
zWigr7z)K>__u-i3`<qwQ!k+03Tn*P_UT0G`7d_78fF@WtXfE3U=LU7L;c$HLUfKUE
zC>t_M_HU?e9r6}a28$eb|1Emu--DY3_8kwaVxM?Q-%a!$;qv>2@J^zw0=FBnX_I)@
zx4E`UV_owkMjEQBAC?1CQ&oMjs>4PoxH-{7o73+KVcMJq+?>PWO6k<POY6h$46(l!
zxea(=a(;6=o=e7EnGferkE<PXB~#Ci5soKyC8;aO|0Vec3LMe1^)s8tcG&SV>qS~x
zv=RM2r-)@i|A%@F2FEQ>6d|~CN~H0n@0UAtfEf~CcU%Iig&T1zSPT@z^ZvWU`t-f#
z`mtMWwyFE2$Y!JJmLhTQ{#``t>Yyy)IkpzAC(LEDAuBPQJqPm=<NdtxLx*n`674$J
znwW0c_+Sf^c4R)>0@}IIotVO2gi*sAeBVI*5cgQTtu8`n((+Q*kKCB&>0gK~ZNcAq
z{QVJsyLabEt6eWh1}emYW~b_uBG3D%d)2+j#JH?d0@_Uuqk06nYf_|RrMe$IL)O{V
zy<^MO1DIn%`Y_d<?x2YJnqbL<W7;Z3Vl2bB<7kT<rAD<8ffqWO(8_4r9Uiq6%fXT!
z6ahbG+3?n8kLtmNiX-*Gvx{>+zuD+9NagD#kFTpl($&aPADm-vQnU%h{oAe>Ny5pQ
zDdw%ihV|$an+E4_SkIF8`B3L}VnYUylPCHgZN~m0;lSorLw<6cew({oVhP_uMREe0
z4o8wdWq*RQk?W%l(;R6;Be&>l)SGUWd&e+Cl_XAamSBZ3qwI2mFBFa%$F9Rp{QD2+
z95pa3k*=&C>#;(ETq7m9c`VL1<?1Siq?DZaTLEGW-gy6FdK!=RcLLv-=XytCu5Fx)
zw=Q#yx3rE>0DO|NK~6A1PO4qL?*q$I`}<ck8=YQKx#&v#4cL}C8;`nx(I2rFpmuaL
z>wxQ{pHp^;cJ%%tN5^)7H5P$e{2_l#gtDGknb<V~d;LK8O{f_2y8pPJO3;x8_OyYe
z9fO2OH=0Cr7kaHB9Fb{r$_EDQn&hgxS4fFy!*-ZSpFkRa^1nn_k2kYsmjl()$V(em
zy+>;N(&t>gjUUP^z7Mg_JeW1DAlj<y(-!%cw&VTi$N47(c=1$l{%IuIvuIP%wxi8Q
zJ`rsR+AOqZ(Ux#tn~Qb}=bzP~z0UdJ3><Vm=f{Mt<otvNE#^6ec{G^+H0IIda(*@p
zE%IlJ(C*>9v>_4!3LDDM-sAin6524%&o`kh;Jgw2+6uI*(C$E6iM9@{;IV@juwy#Z
zUqGEM5$$ob*wKqYXfgg`9oprbU-CnXd728)cA~{Ny&o^-myyt?a(=l2Z4u{J^3h^L
zSFj;{6X#boXffbw5iT@8w4G=(IKQ?7Z3!*`Y{(ls)UpaK>TjU+(Qy7vF4|nQ*a4p{
zoVV4Yy)NN50RANQa*o871~70>GHZ{ZqZS<5LC`*FU}#MyzFLLo!4rlS8>-2K!Pyb4
z5wfyTWJ4!qdLW8Kdlp4DSd~3cuE~Qv*|FNKI0^H>=?I4I>?m!cs06)@aXiDWL8T*&
zIibmsBp<ay(BvffOFLvu9xtDepknfqBbQ>vNe{9MgWd4LI-ROQyk8o>S}&IwPGtH%
zQeY3n3h-n?P);;k38^{#<kOjupYtRuhAla<ajhuzek7;vYZ5uH$9_oxz5JL+gELs~
z0nL<X)(V5C3@tr_clXakICf1f=VwH*GI|9IKi1)l=>0GK?dZpnNZWp{*WzrQ9s3J?
zJ}$2?^cUk?<w<a1%F{|AN({Y12cx6{SME&6%8g}5V0rFP`J4@Q<fgH$(4HF`w;#o`
zdIezL#8NyjiqPAi*t0ax=Rwxg5g1!Gb*Qd!7!IU>X6xkC#Ly-&&XQiaut@p>T*g}=
zY+AH-CGNt*w1V=djnLjjT@i)uoR+A&X2sV|@fws7Q0^tSo;rEr_?O3y9{J*MfRjnp
zrs*wuD4t#jYi8>=cR#C>N42h-Gt`jg#4vo)oRB;x$<XWnI7{b@@Dbg?XHsVM{y9$e
EKL#z#%m4rY

-- 
2.10.2

Re: [Qemu-devel] [RFC PATCH v1 4/5] pc-bios/s390-ccw: Break up virtio-scsi read into multiples

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

On 04/26/2017 09:46 AM, Eric Farman wrote:
> A virtio-scsi request that goes through the host sd driver and
> exceeds the maximum transfer size is automatically broken up
> for us. But the equivalent request going to the sg driver
> presumes that any length requirements have already been honored.
> Let's use the max_sectors field from the device and break up
> all virtio-scsi requests (both sd and sg) to avoid problem from
> the host drivers.
> 
> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/s390-ccw.h    |  4 ++++
>  pc-bios/s390-ccw/virtio-scsi.c | 19 ++++++++++++++-----
>  2 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index ded67bc..e1f3751 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -42,6 +42,10 @@ typedef unsigned long long __u64;
>  #ifndef NULL
>  #define NULL    0
>  #endif
> +#ifndef MIN
> +#define MIN(a, b) (((a) < (b)) ? (a) : (b));
> +#endif

osdep.h defines MIN() for ALL files, so this hunk is spurious.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC PATCH v1 4/5] pc-bios/s390-ccw: Break up virtio-scsi read into multiples

[Eric Farman]

On 04/26/2017 11:17 AM, Eric Blake wrote:
> On 04/26/2017 09:46 AM, Eric Farman wrote:
>> A virtio-scsi request that goes through the host sd driver and
>> exceeds the maximum transfer size is automatically broken up
>> for us. But the equivalent request going to the sg driver
>> presumes that any length requirements have already been honored.
>> Let's use the max_sectors field from the device and break up
>> all virtio-scsi requests (both sd and sg) to avoid problem from
>> the host drivers.
>>
>> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
>> ---
>>  pc-bios/s390-ccw/s390-ccw.h    |  4 ++++
>>  pc-bios/s390-ccw/virtio-scsi.c | 19 ++++++++++++++-----
>>  2 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index ded67bc..e1f3751 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -42,6 +42,10 @@ typedef unsigned long long __u64;
>>  #ifndef NULL
>>  #define NULL    0
>>  #endif
>> +#ifndef MIN
>> +#define MIN(a, b) (((a) < (b)) ? (a) : (b));
>> +#endif
>
> osdep.h defines MIN() for ALL files, so this hunk is spurious.

Ah, so it does.  Okay, removed.  Thanks!

I guess it would make sense for the hunk that uses it to use 
MIN_NON_ZERO instead, which is also defined there.

>

Re: [Qemu-devel] [RFC PATCH v1 4/5] pc-bios/s390-ccw: Break up virtio-scsi read into multiples

[Cornelia Huck]

On Wed, 26 Apr 2017 10:17:36 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 04/26/2017 09:46 AM, Eric Farman wrote:
> > A virtio-scsi request that goes through the host sd driver and
> > exceeds the maximum transfer size is automatically broken up
> > for us. But the equivalent request going to the sg driver
> > presumes that any length requirements have already been honored.
> > Let's use the max_sectors field from the device and break up
> > all virtio-scsi requests (both sd and sg) to avoid problem from
> > the host drivers.
> > 
> > Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
> > ---
> >  pc-bios/s390-ccw/s390-ccw.h    |  4 ++++
> >  pc-bios/s390-ccw/virtio-scsi.c | 19 ++++++++++++++-----
> >  2 files changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> > index ded67bc..e1f3751 100644
> > --- a/pc-bios/s390-ccw/s390-ccw.h
> > +++ b/pc-bios/s390-ccw/s390-ccw.h
> > @@ -42,6 +42,10 @@ typedef unsigned long long __u64;
> >  #ifndef NULL
> >  #define NULL    0
> >  #endif
> > +#ifndef MIN
> > +#define MIN(a, b) (((a) < (b)) ? (a) : (b));
> > +#endif
> 
> osdep.h defines MIN() for ALL files, so this hunk is spurious.
> 

This is a separate binary, which does not include the normal qemu
headers, though.

Re: [Qemu-devel] [RFC PATCH v1 0/5] Enable virtio-scsi boot from /dev/sgX

[Christian Borntraeger]

On 04/26/2017 04:46 PM, Eric Farman wrote:
> Today, trying to boot a guest from a SCSI LUN on s390x yields the following:
> 
>   virtio-blk               = OK
>   virtio-scsi and /dev/sdX = OK
>   virtio-scsi and /dev/sgX = FAIL
> 
> Example of the failing scenario:
> 
>   /usr/bin/qemu-system-s390x ...
>     -device virtio-scsi-ccw,id=scsi0,devno=fe.0.0001
>     -drive file=/dev/sg2,if=none,id=drive0,format=raw
>     -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=0,drive=drive0,id=disk0
>   LOADPARM=[........]
>   Using virtio-scsi.
>   Using SCSI scheme.
>   ..
>   ! virtio-scsi:read_many: STATUS=02 RSPN=70 KEY=0b CODE=00 QLFR=06, sure !
> 
> Why do we care?  Well, libvirt converts a virtio-scsi device from the host
> SCSI address (host:bus:target:lun) into the associated /dev/sgX device,
> which means we can't boot from virtio-scsi and have to rely on virtio-blk
> for this action.
> 
> The short version of what happens is the host device driver rejects our
> requests because the transfer lengths are too long for it to satisfy.
> A virtio-scsi disk connected via scsi-generic is fine as a non-boot device
> because the guest kernel is able to break up the requests for us.  So we just
> need to handle this situation for the boot process.

Out of curiosity. Why is the guest kernel submitting "short enough" requests?
If it querying the device itself via scsi commands before issuing commands?

Re: [Qemu-devel] [RFC PATCH v1 4/5] pc-bios/s390-ccw: Break up virtio-scsi read into multiples

[Eric Farman]

On 04/26/2017 11:24 AM, Eric Farman wrote:
>
>
> On 04/26/2017 11:17 AM, Eric Blake wrote:
>> On 04/26/2017 09:46 AM, Eric Farman wrote:
>>> A virtio-scsi request that goes through the host sd driver and
>>> exceeds the maximum transfer size is automatically broken up
>>> for us. But the equivalent request going to the sg driver
>>> presumes that any length requirements have already been honored.
>>> Let's use the max_sectors field from the device and break up
>>> all virtio-scsi requests (both sd and sg) to avoid problem from
>>> the host drivers.
>>>
>>> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
>>> ---
>>>  pc-bios/s390-ccw/s390-ccw.h    |  4 ++++
>>>  pc-bios/s390-ccw/virtio-scsi.c | 19 ++++++++++++++-----
>>>  2 files changed, 18 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>>> index ded67bc..e1f3751 100644
>>> --- a/pc-bios/s390-ccw/s390-ccw.h
>>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>>> @@ -42,6 +42,10 @@ typedef unsigned long long __u64;
>>>  #ifndef NULL
>>>  #define NULL    0
>>>  #endif
>>> +#ifndef MIN
>>> +#define MIN(a, b) (((a) < (b)) ? (a) : (b));
>>> +#endif
>>
>> osdep.h defines MIN() for ALL files, so this hunk is spurious.
>
> Ah, so it does.  Okay, removed.  Thanks!

Er, not.  Changing this header file didn't force virtio-scsi.c to 
recompile, so it bombs out as Cornelia points out.

>
> I guess it would make sense for the hunk that uses it to use
> MIN_NON_ZERO instead, which is also defined there.
>
>>
>
>

Re: [Qemu-devel] [RFC PATCH v1 0/5] Enable virtio-scsi boot from /dev/sgX

[Eric Farman]

On 04/26/2017 11:48 AM, Christian Borntraeger wrote:
> On 04/26/2017 04:46 PM, Eric Farman wrote:
>> Today, trying to boot a guest from a SCSI LUN on s390x yields the following:
>>
>>   virtio-blk               = OK
>>   virtio-scsi and /dev/sdX = OK
>>   virtio-scsi and /dev/sgX = FAIL
>>
>> Example of the failing scenario:
>>
>>   /usr/bin/qemu-system-s390x ...
>>     -device virtio-scsi-ccw,id=scsi0,devno=fe.0.0001
>>     -drive file=/dev/sg2,if=none,id=drive0,format=raw
>>     -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=0,drive=drive0,id=disk0
>>   LOADPARM=[........]
>>   Using virtio-scsi.
>>   Using SCSI scheme.
>>   ..
>>   ! virtio-scsi:read_many: STATUS=02 RSPN=70 KEY=0b CODE=00 QLFR=06, sure !
>>
>> Why do we care?  Well, libvirt converts a virtio-scsi device from the host
>> SCSI address (host:bus:target:lun) into the associated /dev/sgX device,
>> which means we can't boot from virtio-scsi and have to rely on virtio-blk
>> for this action.
>>
>> The short version of what happens is the host device driver rejects our
>> requests because the transfer lengths are too long for it to satisfy.
>> A virtio-scsi disk connected via scsi-generic is fine as a non-boot device
>> because the guest kernel is able to break up the requests for us.  So we just
>> need to handle this situation for the boot process.
>
> Out of curiosity. Why is the guest kernel submitting "short enough" requests?
> If it querying the device itself via scsi commands before issuing commands?
>

Looking at some old traces I have, I think I misspoke that it's the 
guest kernel.  In the failing case, the host kernel is processing an 
ioctl system call via sg_ioctl, and just passing our I/O transaction to 
the block driver.  It blows up because the number of iovecs needed for 
the transaction exceeds UIO_MAXIOV.  In the working case (boot from 
virtio-scsi as /dev/sdX or virtio-blk), the host kernel goes through 
read system call, and the I/O is broken up by do_generic_file_read.

Re: [Qemu-devel] [RFC PATCH v1 2/5] pc-bios/s390-ccw: Remove duplicate blk_factor adjustment

[Christian Borntraeger]

On 04/26/2017 04:46 PM, Eric Farman wrote:
> When using virtio-scsi, we multiply the READ(10) data_size by
> a block factor twice when building the I/O.  This is fine,
> since it's only 1 for SCSI disks, but let's clean it up.
> 
> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>  pc-bios/s390-ccw/virtio-scsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
> index d850a8d..69b7a93 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.c
> +++ b/pc-bios/s390-ccw/virtio-scsi.c
> @@ -154,7 +154,7 @@ static bool scsi_read_10(VDev *vdev,
>      VirtioCmd read_10[] = {
>          { &req, sizeof(req), VRING_DESC_F_NEXT },
>          { &resp, sizeof(resp), VRING_DESC_F_WRITE | VRING_DESC_F_NEXT },
> -        { data, data_size * f, VRING_DESC_F_WRITE },
> +        { data, data_size, VRING_DESC_F_WRITE },
>      };
> 
>      debug_print_int("read_10  sector", sector);
> 

Re: [Qemu-devel] [RFC PATCH v1 1/5] hw/scsi: Override the max_sectors value for virtio-scsi

[Paolo Bonzini]

On 26/04/2017 16:46, Eric Farman wrote:
> The virtio spec states that the max_sectors field is
> "a hint to the driver for the maximum transfer size"
> that would be used for a virtio-scsi request.
> 
> It's currently hardcoded to xFFFF unless one is established
> by the max_sectors parameter on the command line, but let's
> roll through the associated devices and set it to anything
> lower if one is set for the underlying block device and
> retrieved by the BLKSECTGET ioctl.

Have you tried looking at the block limits VPD page in the firmware?
QEMU is passing BLKSECTGET down by patching that VPD page.  Your
solution would not work with hotplug.

Thanks,

Paolo

> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
> ---
>  hw/scsi/virtio-scsi.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 46a3e3f..bca9461 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -640,7 +640,11 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>                                     uint8_t *config)
>  {
>      VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
> +    VirtIOSCSI *vs = VIRTIO_SCSI(vdev);
>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
> +    SCSIDevice *d;
> +    BusChild *kid;
> +    unsigned int max_transfer;
>  
>      virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
>      virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
> @@ -652,6 +656,14 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>      virtio_stw_p(vdev, &scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
>      virtio_stw_p(vdev, &scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
>      virtio_stl_p(vdev, &scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
> +
> +    QTAILQ_FOREACH(kid, &vs->bus.qbus.children, sibling) {
> +        d = SCSI_DEVICE(kid->child);
> +        max_transfer = blk_get_max_transfer(d->conf.blk) / d->blocksize;
> +        virtio_stl_p(vdev,
> +                     &scsiconf->max_sectors,
> +                     MIN_NON_ZERO(max_transfer, scsiconf->max_sectors));
> +    }
>  }
>  
>  static void virtio_scsi_set_config(VirtIODevice *vdev,
> 

Re: [Qemu-devel] [RFC PATCH v1 0/5] Enable virtio-scsi boot from /dev/sgX

[Fam Zheng]

On Wed, 04/26 16:46, Eric Farman wrote:
> The short version of what happens is the host device driver rejects our
> requests because the transfer lengths are too long for it to satisfy.
> A virtio-scsi disk connected via scsi-generic is fine as a non-boot device
> because the guest kernel is able to break up the requests for us.  So we just
> need to handle this situation for the boot process.
> 
> Patches 2-N in this series do that, but rely on us to specify the max_sectors
> parameter for the virtio-scsi-ccw device:
> 
>   /usr/bin/qemu-system-s390x ...
>     -device virtio-scsi-ccw,id=scsi0,devno=fe.0.0001,max_sectors=2048

Can you instead do an INQUIRY from the bios code to check the Block Limits page?
The response is intercepted by hw/scsi/scsi-generic.c to merge in the host LUN's
limits. That's how Linux kernel finds the granularity for request splitting.

That way, patch 1 is not necessary too. I don't like it because it doesn't
always work considering LUN hotplug.

Fam

Re: [Qemu-devel] [RFC PATCH v1 1/5] hw/scsi: Override the max_sectors value for virtio-scsi

[Christian Borntraeger]

On 04/26/2017 04:46 PM, Eric Farman wrote:
> The virtio spec states that the max_sectors field is
> "a hint to the driver for the maximum transfer size"
> that would be used for a virtio-scsi request.
> 
> It's currently hardcoded to xFFFF unless one is established
> by the max_sectors parameter on the command line, but let's
> roll through the associated devices and set it to anything
> lower if one is set for the underlying block device and
> retrieved by the BLKSECTGET ioctl.
> 
> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>

So since nobody else commented, I looked into this (not knowing anything 
about scsi before)

Your patch basically limits the HBA limit to the lowest size of all child devices.
This will certainly work, as guests will use the minimum of per-device size
and hba size.
Now: I think this might have some impact on cases where a HBA will have different
scsi disks or even non disk type scsi devices (what does blk_get_max_transfer 
return for a scsi cdrom or old scsi-1 devices?)

So in essence I think what could be a better solution is to enhance the 
ccw bios to actually read the per-device config in addition to the hba config.

Linux seems to use the Block Limits VPD any maybe the ccw bios should do the
same. So basically keep patch 4 and add an additional call to read the block VPD
and use the minimum of HBA and device value?

> ---
>  hw/scsi/virtio-scsi.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 46a3e3f..bca9461 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -640,7 +640,11 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>                                     uint8_t *config)
>  {
>      VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
> +    VirtIOSCSI *vs = VIRTIO_SCSI(vdev);
>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
> +    SCSIDevice *d;
> +    BusChild *kid;
> +    unsigned int max_transfer;
> 
>      virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
>      virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
> @@ -652,6 +656,14 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>      virtio_stw_p(vdev, &scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
>      virtio_stw_p(vdev, &scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
>      virtio_stl_p(vdev, &scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
> +
> +    QTAILQ_FOREACH(kid, &vs->bus.qbus.children, sibling) {
> +        d = SCSI_DEVICE(kid->child);
> +        max_transfer = blk_get_max_transfer(d->conf.blk) / d->blocksize;
> +        virtio_stl_p(vdev,
> +                     &scsiconf->max_sectors,
> +                     MIN_NON_ZERO(max_transfer, scsiconf->max_sectors));
> +    }
>  }
> 
>  static void virtio_scsi_set_config(VirtIODevice *vdev,
> 

Re: [Qemu-devel] [RFC PATCH v1 0/5] Enable virtio-scsi boot from /dev/sgX

[Eric Farman]

On 05/05/2017 03:41 AM, Fam Zheng wrote:
> On Wed, 04/26 16:46, Eric Farman wrote:
>> The short version of what happens is the host device driver rejects our
>> requests because the transfer lengths are too long for it to satisfy.
>> A virtio-scsi disk connected via scsi-generic is fine as a non-boot device
>> because the guest kernel is able to break up the requests for us.  So we just
>> need to handle this situation for the boot process.
>>
>> Patches 2-N in this series do that, but rely on us to specify the max_sectors
>> parameter for the virtio-scsi-ccw device:
>>
>>   /usr/bin/qemu-system-s390x ...
>>     -device virtio-scsi-ccw,id=scsi0,devno=fe.0.0001,max_sectors=2048
>
> Can you instead do an INQUIRY from the bios code to check the Block Limits page?
> The response is intercepted by hw/scsi/scsi-generic.c to merge in the host LUN's
> limits. That's how Linux kernel finds the granularity for request splitting.

It's a good idea (Thanks Christian, Paolo, and Fam :), but this leads to 
other difficulties.

We get a value of x3fffff when sending that to a scsi-disk from bios 
code.  That's fully emulated though, in scsi_disk_emulate_inquiry.  And 
that's the scenario that already works.

While there is indeed code in hw/scsi/scsi-generic.c to wire that in, 
that only happens after the I/O goes to the device itself.  The Block 
Limits page isn't supported [1] and thus it gets rejected with "invalid 
field in cdb".  We never get to that fixup code you reference, since the 
returned len is zero.

Should I be refactoring this code to always patch in that block limit 
regardless of a response from the host/device?  (That is, when page xb0 
isn't supported by the hw.)

  - Eric

[1] If I issue an EVPD page x00 from the QEMU bios code, I only see 
pages xb1, xc0, and c1 are supported.  If I look at the supported pages 
from the host, I see a few more but still not xb0:

$ sg_inq --page=0 /dev/sda
VPD INQUIRY: Supported VPD pages page
    Supported VPD pages:
      0x0	Supported VPD pages
      0x80	Unit serial number
      0x83	Device identification
      0x86	Extended INQUIRY data
      0xb1	Block device characteristics (sbc3)
      0xc0	vendor: Firmware numbers (seagate); Unit path report (EMC)
      0xc1	vendor: Date code (seagate)
$ sg_inq --page=0 /dev/sg0
VPD INQUIRY: Supported VPD pages page
    Supported VPD pages:
      0x0	Supported VPD pages
      0x80	Unit serial number
      0x83	Device identification
      0x86	Extended INQUIRY data
      0xb1	Block device characteristics (sbc3)
      0xc0	vendor: Firmware numbers (seagate); Unit path report (EMC)
      0xc1	vendor: Date code (seagate)


>
> That way, patch 1 is not necessary too. I don't like it because it doesn't
> always work considering LUN hotplug.
>
> Fam
>

Re: [Qemu-devel] [RFC PATCH v1 0/5] Enable virtio-scsi boot from /dev/sgX

[Paolo Bonzini]

On 05/05/2017 17:03, Eric Farman wrote:
> We get a value of x3fffff when sending that to a scsi-disk from bios
> code.  That's fully emulated though, in scsi_disk_emulate_inquiry.  And
> that's the scenario that already works.
> 
> While there is indeed code in hw/scsi/scsi-generic.c to wire that in,
> that only happens after the I/O goes to the device itself.  The Block
> Limits page isn't supported [1] and thus it gets rejected with "invalid
> field in cdb".  We never get to that fixup code you reference, since the
> returned len is zero.
> 
> Should I be refactoring this code to always patch in that block limit
> regardless of a response from the host/device?  (That is, when page xb0
> isn't supported by the hw.)

What is the BLKSECTGET value you get?  Is there a sensible default value
that you can use when page 0xb0 isn't supported by the hardware?

Thanks,

Paolo

Re: [Qemu-devel] [RFC PATCH v1 0/5] Enable virtio-scsi boot from /dev/sgX

[Eric Farman]

On 05/05/2017 11:13 AM, Paolo Bonzini wrote:
>
>
> On 05/05/2017 17:03, Eric Farman wrote:
>> We get a value of x3fffff when sending that to a scsi-disk from bios
>> code.  That's fully emulated though, in scsi_disk_emulate_inquiry.  And
>> that's the scenario that already works.
>>
>> While there is indeed code in hw/scsi/scsi-generic.c to wire that in,
>> that only happens after the I/O goes to the device itself.  The Block
>> Limits page isn't supported [1] and thus it gets rejected with "invalid
>> field in cdb".  We never get to that fixup code you reference, since the
>> returned len is zero.
>>
>> Should I be refactoring this code to always patch in that block limit
>> regardless of a response from the host/device?  (That is, when page xb0
>> isn't supported by the hw.)
>
> What is the BLKSECTGET value you get?

x140000 bytes when using /dev/sg0 (xa00 sectors when using /dev/sda).

>  Is there a sensible default value
> that you can use when page 0xb0 isn't supported by the hardware?

I was setting max_sectors to x800 with good success, which was the 
power-of-2 floor that BLKSECTGET gave us.  That kept us within the 
limits of the host biovec code.  But it's a long way from the 
virtio-scsi value of xFFFF when max_sectors isn't specified, so don't 
know what side effects that may cause.

>
> Thanks,
>
> Paolo
>

Re: [Qemu-devel] [RFC PATCH v1 0/5] Enable virtio-scsi boot from /dev/sgX

[Paolo Bonzini]

On 05/05/2017 18:12, Eric Farman wrote:
> 
> 
> On 05/05/2017 11:13 AM, Paolo Bonzini wrote:
>>
>>
>> On 05/05/2017 17:03, Eric Farman wrote:
>>> We get a value of x3fffff when sending that to a scsi-disk from bios
>>> code.  That's fully emulated though, in scsi_disk_emulate_inquiry.  And
>>> that's the scenario that already works.
>>>
>>> While there is indeed code in hw/scsi/scsi-generic.c to wire that in,
>>> that only happens after the I/O goes to the device itself.  The Block
>>> Limits page isn't supported [1] and thus it gets rejected with "invalid
>>> field in cdb".  We never get to that fixup code you reference, since the
>>> returned len is zero.
>>>
>>> Should I be refactoring this code to always patch in that block limit
>>> regardless of a response from the host/device?  (That is, when page xb0
>>> isn't supported by the hw.)
>>
>> What is the BLKSECTGET value you get?
> 
> x140000 bytes when using /dev/sg0 (xa00 sectors when using /dev/sda).
> 
>>  Is there a sensible default value
>> that you can use when page 0xb0 isn't supported by the hardware?
> 
> I was setting max_sectors to x800 with good success, which was the
> power-of-2 floor that BLKSECTGET gave us.  That kept us within the
> limits of the host biovec code.  But it's a long way from the
> virtio-scsi value of xFFFF when max_sectors isn't specified, so don't
> know what side effects that may cause.

It's just slower, but 0x800 is already a megabyte worth of data so it's
not going to be that much slower.

Paolo

Re: [Qemu-devel] [RFC PATCH v1 0/5] Enable virtio-scsi boot from /dev/sgX

[Christian Borntraeger]

On 05/06/2017 10:24 AM, Paolo Bonzini wrote:
> 
> 
> On 05/05/2017 18:12, Eric Farman wrote:
>>
>>
>> On 05/05/2017 11:13 AM, Paolo Bonzini wrote:
>>>
>>>
>>> On 05/05/2017 17:03, Eric Farman wrote:
>>>> We get a value of x3fffff when sending that to a scsi-disk from bios
>>>> code.  That's fully emulated though, in scsi_disk_emulate_inquiry.  And
>>>> that's the scenario that already works.
>>>>
>>>> While there is indeed code in hw/scsi/scsi-generic.c to wire that in,
>>>> that only happens after the I/O goes to the device itself.  The Block
>>>> Limits page isn't supported [1] and thus it gets rejected with "invalid
>>>> field in cdb".  We never get to that fixup code you reference, since the
>>>> returned len is zero.
>>>>
>>>> Should I be refactoring this code to always patch in that block limit
>>>> regardless of a response from the host/device?  (That is, when page xb0
>>>> isn't supported by the hw.)
>>>
>>> What is thec value you get?
>>
>> x140000 bytes when using /dev/sg0 (xa00 sectors when using /dev/sda).
>>
>>>  Is there a sensible default value
>>> that you can use when page 0xb0 isn't supported by the hardware?
>>
>> I was setting max_sectors to x800 with good success, which was the
>> power-of-2 floor that BLKSECTGET gave us.  That kept us within the
>> limits of the host biovec code.  But it's a long way from the
>> virtio-scsi value of xFFFF when max_sectors isn't specified, so don't
>> know what side effects that may cause.
> 
> It's just slower, but 0x800 is already a megabyte worth of data so it's
> not going to be that much slower.
> 
> Paolo
> 

Eric, maybe that would be a safe variant. The bios boot code is not
performance optimized anyway. Lets just always use a maximum size that 
will always work.

Re: [Qemu-devel] [RFC PATCH v1 0/5] Enable virtio-scsi boot from /dev/sgX

[Eric Farman]

On 05/08/2017 03:00 AM, Christian Borntraeger wrote:
> On 05/06/2017 10:24 AM, Paolo Bonzini wrote:
>>
>>
>> On 05/05/2017 18:12, Eric Farman wrote:
>>>
>>>
>>> On 05/05/2017 11:13 AM, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 05/05/2017 17:03, Eric Farman wrote:
>>>>> We get a value of x3fffff when sending that to a scsi-disk from bios
>>>>> code.  That's fully emulated though, in scsi_disk_emulate_inquiry.  And
>>>>> that's the scenario that already works.
>>>>>
>>>>> While there is indeed code in hw/scsi/scsi-generic.c to wire that in,
>>>>> that only happens after the I/O goes to the device itself.  The Block
>>>>> Limits page isn't supported [1] and thus it gets rejected with "invalid
>>>>> field in cdb".  We never get to that fixup code you reference, since the
>>>>> returned len is zero.
>>>>>
>>>>> Should I be refactoring this code to always patch in that block limit
>>>>> regardless of a response from the host/device?  (That is, when page xb0
>>>>> isn't supported by the hw.)
>>>>
>>>> What is thec value you get?
>>>
>>> x140000 bytes when using /dev/sg0 (xa00 sectors when using /dev/sda).
>>>
>>>>  Is there a sensible default value
>>>> that you can use when page 0xb0 isn't supported by the hardware?
>>>
>>> I was setting max_sectors to x800 with good success, which was the
>>> power-of-2 floor that BLKSECTGET gave us.  That kept us within the
>>> limits of the host biovec code.  But it's a long way from the
>>> virtio-scsi value of xFFFF when max_sectors isn't specified, so don't
>>> know what side effects that may cause.
>>
>> It's just slower, but 0x800 is already a megabyte worth of data so it's
>> not going to be that much slower.
>>
>> Paolo
>>
>
> Eric, maybe that would be a safe variant. The bios boot code is not
> performance optimized anyway. Lets just always use a maximum size that
> will always work.
>

Sounds good to me.  Let me work on getting my very ugly code to be 
slightly less ugly, and I'll send a v2 here.  Thanks, everyone.

  - Eric