| Message ID | 20190409164038.25484-1-paul.durrant@citrix.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen-block: support feature-large-sector-size | expand |
On Tue, Apr 09, 2019 at 05:40:38PM +0100, Paul Durrant wrote: > A recent Xen commit [1] clarified the semantics of sector based quantities > used in the blkif protocol such that it is now safe to create a xen-block > device with a logical_block_size != 512, as long as the device only > connects to a frontend advertizing 'feature-large-block-size'. > > This patch modifies xen-block accordingly. It also uses a stack variable > for the BlockBackend in xen_block_realize() to avoid repeated dereferencing > of the BlockConf pointer, and changes the parameters of > xen_block_dataplane_create() so that the BlockBackend pointer and sector > size are passed expicitly rather than implicitly via the BlockConf. > > These modifications have been tested against a recent Windows PV XENVBD > driver [2] using a xen-disk device with a 4kB logical block size. > > [1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=67e1c050e36b2c9900cca83618e56189effbad98 > [2] https://winpvdrvbuild.xenproject.org:8080/job/XENVBD-master/126 > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > --- > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c > index ef635be4c2..05e890ad78 100644 > --- a/hw/block/xen-block.c > +++ b/hw/block/xen-block.c > @@ -51,11 +51,25 @@ static void xen_block_connect(XenDevice *xendev, Error **errp) [...] > + if (xen_device_frontend_scanf(xendev, "feature-large-sector-size", "%u", > + &feature_large_sector_size) != 1) { > + feature_large_sector_size = 0; > + } > + > + if (feature_large_sector_size != 1 && > + conf->logical_block_size != XEN_BLKIF_SECTOR_SIZE) { > + error_setg(errp, "logical_block_size != %u not supported", Maybe add "by frontend" to the error message? > + XEN_BLKIF_SECTOR_SIZE); > + return; > + } > + With the question answered: Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
> -----Original Message----- > From: Anthony PERARD [mailto:anthony.perard@citrix.com] > Sent: 10 April 2019 16:52 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; qemu-block@nongnu.org; Stefano Stabellini > <sstabellini@kernel.org>; Stefan Hajnoczi <stefanha@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Max > Reitz <mreitz@redhat.com> > Subject: Re: [PATCH] xen-block: support feature-large-sector-size > > On Tue, Apr 09, 2019 at 05:40:38PM +0100, Paul Durrant wrote: > > A recent Xen commit [1] clarified the semantics of sector based quantities > > used in the blkif protocol such that it is now safe to create a xen-block > > device with a logical_block_size != 512, as long as the device only > > connects to a frontend advertizing 'feature-large-block-size'. > > > > This patch modifies xen-block accordingly. It also uses a stack variable > > for the BlockBackend in xen_block_realize() to avoid repeated dereferencing > > of the BlockConf pointer, and changes the parameters of > > xen_block_dataplane_create() so that the BlockBackend pointer and sector > > size are passed expicitly rather than implicitly via the BlockConf. > > > > These modifications have been tested against a recent Windows PV XENVBD > > driver [2] using a xen-disk device with a 4kB logical block size. > > > > [1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=67e1c050e36b2c9900cca83618e56189effbad98 > > [2] https://winpvdrvbuild.xenproject.org:8080/job/XENVBD-master/126 > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > --- > > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c > > index ef635be4c2..05e890ad78 100644 > > --- a/hw/block/xen-block.c > > +++ b/hw/block/xen-block.c > > @@ -51,11 +51,25 @@ static void xen_block_connect(XenDevice *xendev, Error **errp) > [...] > > + if (xen_device_frontend_scanf(xendev, "feature-large-sector-size", "%u", > > + &feature_large_sector_size) != 1) { > > + feature_large_sector_size = 0; > > + } > > + > > + if (feature_large_sector_size != 1 && > > + conf->logical_block_size != XEN_BLKIF_SECTOR_SIZE) { > > + error_setg(errp, "logical_block_size != %u not supported", > > Maybe add "by frontend" to the error message? Yes, I'm fine with that addition. > > > + XEN_BLKIF_SECTOR_SIZE); > > + return; > > + } > > + > > With the question answered: > Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> > Thanks, Paul > Thanks, > > -- > Anthony PERARD
On 09.04.19 18:40, Paul Durrant wrote: > A recent Xen commit [1] clarified the semantics of sector based quantities > used in the blkif protocol such that it is now safe to create a xen-block > device with a logical_block_size != 512, as long as the device only > connects to a frontend advertizing 'feature-large-block-size'. > > This patch modifies xen-block accordingly. It also uses a stack variable > for the BlockBackend in xen_block_realize() to avoid repeated dereferencing > of the BlockConf pointer, and changes the parameters of > xen_block_dataplane_create() so that the BlockBackend pointer and sector > size are passed expicitly rather than implicitly via the BlockConf. > > These modifications have been tested against a recent Windows PV XENVBD > driver [2] using a xen-disk device with a 4kB logical block size. > > [1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=67e1c050e36b2c9900cca83618e56189effbad98 > [2] https://winpvdrvbuild.xenproject.org:8080/job/XENVBD-master/126 > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Anthony Perard <anthony.perard@citrix.com> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > Cc: Kevin Wolf <kwolf@redhat.com> > Cc: Max Reitz <mreitz@redhat.com> > --- > hw/block/dataplane/xen-block.c | 25 ++++++++++++---------- > hw/block/dataplane/xen-block.h | 3 ++- > hw/block/xen-block.c | 38 +++++++++++++++++++++------------- > 3 files changed, 40 insertions(+), 26 deletions(-) Thanks, added “by frontend” to the error message and applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max
On Wed, Jun 26, 2019 at 06:48:50PM +0200, Max Reitz wrote: > On 09.04.19 18:40, Paul Durrant wrote: > > A recent Xen commit [1] clarified the semantics of sector based quantities > > used in the blkif protocol such that it is now safe to create a xen-block > > device with a logical_block_size != 512, as long as the device only > > connects to a frontend advertizing 'feature-large-block-size'. > > > > This patch modifies xen-block accordingly. It also uses a stack variable > > for the BlockBackend in xen_block_realize() to avoid repeated dereferencing > > of the BlockConf pointer, and changes the parameters of > > xen_block_dataplane_create() so that the BlockBackend pointer and sector > > size are passed expicitly rather than implicitly via the BlockConf. > > > > These modifications have been tested against a recent Windows PV XENVBD > > driver [2] using a xen-disk device with a 4kB logical block size. > > > > [1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=67e1c050e36b2c9900cca83618e56189effbad98 > > [2] https://winpvdrvbuild.xenproject.org:8080/job/XENVBD-master/126 > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > --- > > Cc: Stefano Stabellini <sstabellini@kernel.org> > > Cc: Anthony Perard <anthony.perard@citrix.com> > > Cc: Stefan Hajnoczi <stefanha@redhat.com> > > Cc: Kevin Wolf <kwolf@redhat.com> > > Cc: Max Reitz <mreitz@redhat.com> > > --- > > hw/block/dataplane/xen-block.c | 25 ++++++++++++---------- > > hw/block/dataplane/xen-block.h | 3 ++- > > hw/block/xen-block.c | 38 +++++++++++++++++++++------------- > > 3 files changed, 40 insertions(+), 26 deletions(-) > > Thanks, added “by frontend” to the error message and applied to my block > branch: > > https://git.xanclic.moe/XanClic/qemu/commits/branch/block :(, I've just sent a pull request with that patch: https://patchew.org/QEMU/20190624153257.20163-1-anthony.perard@citrix.com/20190624153257.20163-2-anthony.perard@citrix.com/ I guess I need to start sending an email every time I've added a patch to my queue. Cheers,
On 26.06.19 19:19, Anthony PERARD wrote: > On Wed, Jun 26, 2019 at 06:48:50PM +0200, Max Reitz wrote: >> On 09.04.19 18:40, Paul Durrant wrote: >>> A recent Xen commit [1] clarified the semantics of sector based quantities >>> used in the blkif protocol such that it is now safe to create a xen-block >>> device with a logical_block_size != 512, as long as the device only >>> connects to a frontend advertizing 'feature-large-block-size'. >>> >>> This patch modifies xen-block accordingly. It also uses a stack variable >>> for the BlockBackend in xen_block_realize() to avoid repeated dereferencing >>> of the BlockConf pointer, and changes the parameters of >>> xen_block_dataplane_create() so that the BlockBackend pointer and sector >>> size are passed expicitly rather than implicitly via the BlockConf. >>> >>> These modifications have been tested against a recent Windows PV XENVBD >>> driver [2] using a xen-disk device with a 4kB logical block size. >>> >>> [1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=67e1c050e36b2c9900cca83618e56189effbad98 >>> [2] https://winpvdrvbuild.xenproject.org:8080/job/XENVBD-master/126 >>> >>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com> >>> --- >>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>> Cc: Anthony Perard <anthony.perard@citrix.com> >>> Cc: Stefan Hajnoczi <stefanha@redhat.com> >>> Cc: Kevin Wolf <kwolf@redhat.com> >>> Cc: Max Reitz <mreitz@redhat.com> >>> --- >>> hw/block/dataplane/xen-block.c | 25 ++++++++++++---------- >>> hw/block/dataplane/xen-block.h | 3 ++- >>> hw/block/xen-block.c | 38 +++++++++++++++++++++------------- >>> 3 files changed, 40 insertions(+), 26 deletions(-) >> >> Thanks, added “by frontend” to the error message and applied to my block >> branch: >> >> https://git.xanclic.moe/XanClic/qemu/commits/branch/block > > :(, I've just sent a pull request with that patch: > https://patchew.org/QEMU/20190624153257.20163-1-anthony.perard@citrix.com/20190624153257.20163-2-anthony.perard@citrix.com/ That’s just as well, then. :-) > I guess I need to start sending an email every time I've added a patch > to my queue. Well, it certainly won’t hurt. Although in this cases it’s just a bit of an unfortunate coincidence that I looked at this patch now when Peter seems to be away (otherwise I’d have seen it in master). Max
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c index 908bd27bbd..50094a886b 100644 --- a/hw/block/dataplane/xen-block.c +++ b/hw/block/dataplane/xen-block.c @@ -58,6 +58,7 @@ struct XenBlockDataPlane { int requests_inflight; unsigned int max_requests; BlockBackend *blk; + unsigned int sector_size; QEMUBH *bh; IOThread *iothread; AioContext *ctx; @@ -167,7 +168,7 @@ static int xen_block_parse_request(XenBlockRequest *request) goto err; } - request->start = request->req.sector_number * XEN_BLKIF_SECTOR_SIZE; + request->start = request->req.sector_number * dataplane->sector_size; for (i = 0; i < request->req.nr_segments; i++) { if (i == BLKIF_MAX_SEGMENTS_PER_REQUEST) { error_report("error: nr_segments too big"); @@ -177,14 +178,14 @@ static int xen_block_parse_request(XenBlockRequest *request) error_report("error: first > last sector"); goto err; } - if (request->req.seg[i].last_sect * XEN_BLKIF_SECTOR_SIZE >= + if (request->req.seg[i].last_sect * dataplane->sector_size >= XC_PAGE_SIZE) { error_report("error: page crossing"); goto err; } len = (request->req.seg[i].last_sect - - request->req.seg[i].first_sect + 1) * XEN_BLKIF_SECTOR_SIZE; + request->req.seg[i].first_sect + 1) * dataplane->sector_size; request->size += len; } if (request->start + request->size > blk_getlength(dataplane->blk)) { @@ -218,17 +219,17 @@ static int xen_block_copy_request(XenBlockRequest *request) if (to_domain) { segs[i].dest.foreign.ref = request->req.seg[i].gref; segs[i].dest.foreign.offset = request->req.seg[i].first_sect * - XEN_BLKIF_SECTOR_SIZE; + dataplane->sector_size; segs[i].source.virt = virt; } else { segs[i].source.foreign.ref = request->req.seg[i].gref; segs[i].source.foreign.offset = request->req.seg[i].first_sect * - XEN_BLKIF_SECTOR_SIZE; + dataplane->sector_size; segs[i].dest.virt = virt; } segs[i].len = (request->req.seg[i].last_sect - request->req.seg[i].first_sect + 1) * - XEN_BLKIF_SECTOR_SIZE; + dataplane->sector_size; virt += segs[i].len; } @@ -338,12 +339,12 @@ static bool xen_block_split_discard(XenBlockRequest *request, /* Wrap around, or overflowing byte limit? */ if (sec_start + sec_count < sec_count || - sec_start + sec_count > INT64_MAX / XEN_BLKIF_SECTOR_SIZE) { + sec_start + sec_count > INT64_MAX / dataplane->sector_size) { return false; } - byte_offset = sec_start * XEN_BLKIF_SECTOR_SIZE; - byte_remaining = sec_count * XEN_BLKIF_SECTOR_SIZE; + byte_offset = sec_start * dataplane->sector_size; + byte_remaining = sec_count * dataplane->sector_size; do { byte_chunk = byte_remaining > BDRV_REQUEST_MAX_BYTES ? @@ -626,13 +627,15 @@ static bool xen_block_dataplane_event(void *opaque) } XenBlockDataPlane *xen_block_dataplane_create(XenDevice *xendev, - BlockConf *conf, + BlockBackend *blk, + unsigned int sector_size, IOThread *iothread) { XenBlockDataPlane *dataplane = g_new0(XenBlockDataPlane, 1); dataplane->xendev = xendev; - dataplane->blk = conf->blk; + dataplane->blk = blk; + dataplane->sector_size = sector_size; QLIST_INIT(&dataplane->inflight); QLIST_INIT(&dataplane->freelist); diff --git a/hw/block/dataplane/xen-block.h b/hw/block/dataplane/xen-block.h index d6fa6d26dd..76dcd51c3d 100644 --- a/hw/block/dataplane/xen-block.h +++ b/hw/block/dataplane/xen-block.h @@ -15,7 +15,8 @@ typedef struct XenBlockDataPlane XenBlockDataPlane; XenBlockDataPlane *xen_block_dataplane_create(XenDevice *xendev, - BlockConf *conf, + BlockBackend *blk, + unsigned int sector_size, IOThread *iothread); void xen_block_dataplane_destroy(XenBlockDataPlane *dataplane); void xen_block_dataplane_start(XenBlockDataPlane *dataplane, diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c index ef635be4c2..05e890ad78 100644 --- a/hw/block/xen-block.c +++ b/hw/block/xen-block.c @@ -51,11 +51,25 @@ static void xen_block_connect(XenDevice *xendev, Error **errp) XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev); const char *type = object_get_typename(OBJECT(blockdev)); XenBlockVdev *vdev = &blockdev->props.vdev; + BlockConf *conf = &blockdev->props.conf; + unsigned int feature_large_sector_size; unsigned int order, nr_ring_ref, *ring_ref, event_channel, protocol; char *str; trace_xen_block_connect(type, vdev->disk, vdev->partition); + if (xen_device_frontend_scanf(xendev, "feature-large-sector-size", "%u", + &feature_large_sector_size) != 1) { + feature_large_sector_size = 0; + } + + if (feature_large_sector_size != 1 && + conf->logical_block_size != XEN_BLKIF_SECTOR_SIZE) { + error_setg(errp, "logical_block_size != %u not supported", + XEN_BLKIF_SECTOR_SIZE); + return; + } + if (xen_device_frontend_scanf(xendev, "ring-page-order", "%u", &order) != 1) { nr_ring_ref = 1; @@ -149,7 +163,7 @@ static void xen_block_set_size(XenBlockDevice *blockdev) const char *type = object_get_typename(OBJECT(blockdev)); XenBlockVdev *vdev = &blockdev->props.vdev; BlockConf *conf = &blockdev->props.conf; - int64_t sectors = blk_getlength(conf->blk) / XEN_BLKIF_SECTOR_SIZE; + int64_t sectors = blk_getlength(conf->blk) / conf->logical_block_size; XenDevice *xendev = XEN_DEVICE(blockdev); trace_xen_block_size(type, vdev->disk, vdev->partition, sectors); @@ -184,6 +198,7 @@ static void xen_block_realize(XenDevice *xendev, Error **errp) const char *type = object_get_typename(OBJECT(blockdev)); XenBlockVdev *vdev = &blockdev->props.vdev; BlockConf *conf = &blockdev->props.conf; + BlockBackend *blk = conf->blk; Error *local_err = NULL; if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID) { @@ -205,8 +220,8 @@ static void xen_block_realize(XenDevice *xendev, Error **errp) * The blkif protocol does not deal with removable media, so it must * always be present, even for CDRom devices. */ - assert(conf->blk); - if (!blk_is_inserted(conf->blk)) { + assert(blk); + if (!blk_is_inserted(blk)) { error_setg(errp, "device needs media, but drive is empty"); return; } @@ -223,26 +238,20 @@ static void xen_block_realize(XenDevice *xendev, Error **errp) blkconf_blocksizes(conf); - if (conf->logical_block_size != XEN_BLKIF_SECTOR_SIZE) { - error_setg(errp, "logical_block_size != %u not supported", - XEN_BLKIF_SECTOR_SIZE); - return; - } - if (conf->logical_block_size > conf->physical_block_size) { error_setg( errp, "logical_block_size > physical_block_size not supported"); return; } - blk_set_dev_ops(conf->blk, &xen_block_dev_ops, blockdev); - blk_set_guest_block_size(conf->blk, conf->logical_block_size); + blk_set_dev_ops(blk, &xen_block_dev_ops, blockdev); + blk_set_guest_block_size(blk, conf->logical_block_size); if (conf->discard_granularity == -1) { conf->discard_granularity = conf->physical_block_size; } - if (blk_get_flags(conf->blk) & BDRV_O_UNMAP) { + if (blk_get_flags(blk) & BDRV_O_UNMAP) { xen_device_backend_printf(xendev, "feature-discard", "%u", 1); xen_device_backend_printf(xendev, "discard-granularity", "%u", conf->discard_granularity); @@ -259,12 +268,13 @@ static void xen_block_realize(XenDevice *xendev, Error **errp) blockdev->device_type); xen_device_backend_printf(xendev, "sector-size", "%u", - XEN_BLKIF_SECTOR_SIZE); + conf->logical_block_size); xen_block_set_size(blockdev); blockdev->dataplane = - xen_block_dataplane_create(xendev, conf, blockdev->props.iothread); + xen_block_dataplane_create(xendev, blk, conf->logical_block_size, + blockdev->props.iothread); } static void xen_block_frontend_changed(XenDevice *xendev,
A recent Xen commit [1] clarified the semantics of sector based quantities used in the blkif protocol such that it is now safe to create a xen-block device with a logical_block_size != 512, as long as the device only connects to a frontend advertizing 'feature-large-block-size'. This patch modifies xen-block accordingly. It also uses a stack variable for the BlockBackend in xen_block_realize() to avoid repeated dereferencing of the BlockConf pointer, and changes the parameters of xen_block_dataplane_create() so that the BlockBackend pointer and sector size are passed expicitly rather than implicitly via the BlockConf. These modifications have been tested against a recent Windows PV XENVBD driver [2] using a xen-disk device with a 4kB logical block size. [1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=67e1c050e36b2c9900cca83618e56189effbad98 [2] https://winpvdrvbuild.xenproject.org:8080/job/XENVBD-master/126 Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Anthony Perard <anthony.perard@citrix.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> --- hw/block/dataplane/xen-block.c | 25 ++++++++++++---------- hw/block/dataplane/xen-block.h | 3 ++- hw/block/xen-block.c | 38 +++++++++++++++++++++------------- 3 files changed, 40 insertions(+), 26 deletions(-)