Re: xbd and large sectors

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Mon, Jul 24, 2023 at 02:11:49PM +0200, Christian Kujau wrote:
> On Mon, 24 Jul 2023, Manuel Bouyer wrote:
> > OK this is probaly because the domU tries an I/O smaller than 4k, or
> > not aligned to 4k. The xbd driver should really report 4k block size and not
> > 512.
> > 
> > Please try the attached patch (or use binaries at
> > ftp://asim.lip6.fr/outgoing/bouyer/rel/HEAD/amd64 which were built with the
> > patch).
> > I tested that it still works with a 512-bytes backend but I have to way to
> > test 4k. 
> 
> Uh, that (netbsd-INSTALL_XEN3_DOMU in PV mode) failed early. I don't have 
> much experience with ddb(4), please let me know if you need more details.
> 
> [   1.1400572] event 14 bound to VCPU  vcpu1 1
> [   1.1400572] fatal integer divide fault in supervisor mode
> [   1.1500605] trap type 8 code 0 rip 0xffffffff80213569 cs 0xe030 rflags 0x10246 cr2 0 ilevel 0 rsp 0xffffa901386dde30
> [   1.1500605] curlwp 0xffffa90007f0fb80 pid 0.109 lowest kstack 0xffffa901386d92c0
> kernel: integer divide fault trap, code=0
> Stopped in pid 0.109 (system) at        netbsd:xbd_backend_changed+0x2e2:       

This points to
		dg->dg_nsectors = 1024 * (1024 / dg->dg_secsize);
		dg->dg_ncylinders = dg->dg_secperunit / dg->dg_nsectors;

and of course if dg->dg_secsize is 4096, dg->dg_nsectors ends up being 0.

changing to
		dg->dg_nsectors = (1024 * 1024) / dg->dg_secsize;

should fix it.

updated diff attached, and I also updated the binaries on asim.lip6.fr


-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Index: sys/arch/xen/xen/xbd_xenbus.c
===================================================================
RCS file: /cvsroot/src/sys/arch/xen/xen/xbd_xenbus.c,v
retrieving revision 1.133
diff -u -p -u -r1.133 xbd_xenbus.c
--- sys/arch/xen/xen/xbd_xenbus.c	21 Jul 2023 11:28:50 -0000	1.133
+++ sys/arch/xen/xen/xbd_xenbus.c	24 Jul 2023 12:26:06 -0000
@@ -169,7 +169,7 @@ struct xbd_xenbus_softc {
 #define BLKIF_SHUTDOWN_REMOTE 1 /* backend-initiated shutdown in progress */
 #define BLKIF_SHUTDOWN_LOCAL  2 /* locally-initiated shutdown in progress */
 
-	uint64_t sc_sectors; /* number of XEN_BSIZE sectors for this device */
+	uint64_t sc_sectors; /* number of sc_secsize sectors for this device */
 	u_long sc_secsize; /* sector size */
 	uint64_t sc_xbdsize; /* size of disk in DEV_BSIZE */
 	u_long sc_info; /* VDISK_* */
@@ -674,15 +674,14 @@ xbd_backend_changed(void *arg, XenbusSta
 		xbd_connect(sc);
 		sc->sc_shutdown = BLKIF_SHUTDOWN_RUN;
 		sc->sc_xbdsize =
-		    sc->sc_sectors * (uint64_t)XEN_BSIZE / DEV_BSIZE;
+		    sc->sc_sectors * (uint64_t)sc->sc_secsize / DEV_BSIZE;
 		dg = &sc->sc_dksc.sc_dkdev.dk_geom;
 		memset(dg, 0, sizeof(*dg));
 
-		dg->dg_secperunit = sc->sc_xbdsize;
-		dg->dg_secsize = DEV_BSIZE;
+		dg->dg_secperunit = sc->sc_sectors;
+		dg->dg_secsize = sc->sc_secsize;
 		dg->dg_ntracks = 1;
-		// XXX: Ok to hard-code DEV_BSIZE?
-		dg->dg_nsectors = 1024 * (1024 / dg->dg_secsize);
+		dg->dg_nsectors = (1024 * 1024) / dg->dg_secsize;
 		dg->dg_ncylinders = dg->dg_secperunit / dg->dg_nsectors;
 
 		bufq_alloc(&sc->sc_dksc.sc_bufq, "fcfs", 0);
@@ -739,14 +738,6 @@ xbd_connect(struct xbd_xenbus_softc *sc)
 		panic("%s: can't read number from %s/virtual-device\n",
 		    device_xname(sc->sc_dksc.sc_dev),
 		    sc->sc_xbusd->xbusd_otherend);
-	err = xenbus_read_ull(NULL,
-	    sc->sc_xbusd->xbusd_otherend, "sectors", &sectors, 10);
-	if (err)
-		panic("%s: can't read number from %s/sectors\n",
-		    device_xname(sc->sc_dksc.sc_dev),
-		    sc->sc_xbusd->xbusd_otherend);
-	sc->sc_sectors = sectors;
-
 	err = xenbus_read_ul(NULL,
 	    sc->sc_xbusd->xbusd_otherend, "info", &sc->sc_info, 10);
 	if (err)
@@ -760,6 +751,14 @@ xbd_connect(struct xbd_xenbus_softc *sc)
 		    device_xname(sc->sc_dksc.sc_dev),
 		    sc->sc_xbusd->xbusd_otherend);
 
+	err = xenbus_read_ull(NULL,
+	    sc->sc_xbusd->xbusd_otherend, "sectors", &sectors, 10);
+	if (err)
+		panic("%s: can't read number from %s/sectors\n",
+		    device_xname(sc->sc_dksc.sc_dev),
+		    sc->sc_xbusd->xbusd_otherend);
+	sc->sc_sectors = sectors * (uint64_t)XEN_BSIZE / sc->sc_secsize;
+
 	xenbus_switch_state(sc->sc_xbusd, NULL, XenbusStateConnected);
 }
 
@@ -1274,7 +1273,8 @@ xbd_diskstart_submit(struct xbd_xenbus_s
 	req->id = req_id;
 	req->operation =
 	    bp->b_flags & B_READ ? BLKIF_OP_READ : BLKIF_OP_WRITE;
-	req->sector_number = bp->b_rawblkno + (start >> XEN_BSHIFT);
+	req->sector_number = (bp->b_rawblkno * DEV_BSIZE / XEN_BSIZE) +
+	    (start >> XEN_BSHIFT);
 	req->handle = sc->sc_handle;
 
 	size = uimin(bp->b_bcount - start, XBD_MAX_CHUNK);
@@ -1331,7 +1331,7 @@ xbd_diskstart_submit_indirect(struct xbd
 	req->operation = BLKIF_OP_INDIRECT;
 	req->indirect_op =
 	    bp->b_flags & B_READ ? BLKIF_OP_READ : BLKIF_OP_WRITE;
-	req->sector_number = bp->b_rawblkno;
+	req->sector_number = bp->b_rawblkno * DEV_BSIZE / XEN_BSIZE;
 	req->handle = sc->sc_handle;
 
 	xbdreq->req_indirect = SLIST_FIRST(&sc->sc_indirect_head);