* [PATCH 1/2] include: update Xen public headers io/blkif.h
2024-09-26 10:14 [PATCH 0/2] Xen: Update sector-size handling in block backend Anthony PERARD
@ 2024-09-26 10:14 ` Anthony PERARD
2024-09-26 10:14 ` [PATCH 2/2] hw/block/xen-block: Update sector-size handling Anthony PERARD
1 sibling, 0 replies; 3+ messages in thread
From: Anthony PERARD @ 2024-09-26 10:14 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony PERARD, Stefano Stabellini, Anthony PERARD, Paul Durrant,
Edgar E. Iglesias, xen-devel
Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
---
include/hw/xen/interface/io/blkif.h | 52 +++++++++++++++++++++--------
1 file changed, 39 insertions(+), 13 deletions(-)
diff --git a/include/hw/xen/interface/io/blkif.h b/include/hw/xen/interface/io/blkif.h
index 22f1eef0c0..9b00d633d3 100644
--- a/include/hw/xen/interface/io/blkif.h
+++ b/include/hw/xen/interface/io/blkif.h
@@ -237,12 +237,16 @@
* sector-size
* Values: <uint32_t>
*
- * The logical block size, in bytes, of the underlying storage. This
- * must be a power of two with a minimum value of 512.
+ * The logical block size, in bytes, of the underlying storage. This must
+ * be a power of two with a minimum value of 512. The sector size should
+ * only be used for request segment length and alignment.
*
- * NOTE: Because of implementation bugs in some frontends this must be
- * set to 512, unless the frontend advertizes a non-zero value
- * in its "feature-large-sector-size" xenbus node. (See below).
+ * When exposing a device that uses a logical sector size of 4096, the
+ * only difference xenstore wise will be that 'sector-size' (and possibly
+ * 'physical-sector-size' if supported by the backend) will be 4096, but
+ * the 'sectors' node will still be calculated using 512 byte units. The
+ * sector base units in the ring requests fields will all be 512 byte
+ * based despite the logical sector size exposed in 'sector-size'.
*
* physical-sector-size
* Values: <uint32_t>
@@ -254,9 +258,9 @@
* sectors
* Values: <uint64_t>
*
- * The size of the backend device, expressed in units of "sector-size".
- * The product of "sector-size" and "sectors" must also be an integer
- * multiple of "physical-sector-size", if that node is present.
+ * The size of the backend device, expressed in units of 512b. The
+ * product of "sectors" * 512 must also be an integer multiple of
+ * "physical-sector-size", if that node is present.
*
*****************************************************************************
* Frontend XenBus Nodes
@@ -338,6 +342,7 @@
* feature-large-sector-size
* Values: 0/1 (boolean)
* Default Value: 0
+ * Notes: DEPRECATED, 12
*
* A value of "1" indicates that the frontend will correctly supply and
* interpret all sector-based quantities in terms of the "sector-size"
@@ -411,6 +416,11 @@
*(10) The discard-secure property may be present and will be set to 1 if the
* backing device supports secure discard.
*(11) Only used by Linux and NetBSD.
+ *(12) Possibly only ever implemented by the QEMU Qdisk backend and the Windows
+ * PV block frontend. Other backends and frontends supported 'sector-size'
+ * values greater than 512 before such feature was added. Frontends should
+ * not expose this node, neither should backends make any decisions based
+ * on it being exposed by the frontend.
*/
/*
@@ -619,11 +629,14 @@
#define BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST 8
/*
- * NB. 'first_sect' and 'last_sect' in blkif_request_segment, as well as
- * 'sector_number' in blkif_request, blkif_request_discard and
- * blkif_request_indirect are sector-based quantities. See the description
- * of the "feature-large-sector-size" frontend xenbus node above for
- * more information.
+ * NB. 'first_sect' and 'last_sect' in blkif_request_segment are all in units
+ * of 512 bytes, despite the 'sector-size' xenstore node possibly having a
+ * value greater than 512.
+ *
+ * The value in 'first_sect' and 'last_sect' fields must be setup so that the
+ * resulting segment offset and size is aligned to the logical sector size
+ * reported by the 'sector-size' xenstore node, see 'Backend Device Properties'
+ * section.
*/
struct blkif_request_segment {
grant_ref_t gref; /* reference to I/O buffer frame */
@@ -634,6 +647,10 @@ struct blkif_request_segment {
/*
* Starting ring element for any I/O request.
+ *
+ * The 'sector_number' field is in units of 512b, despite the value of the
+ * 'sector-size' xenstore node. Note however that the offset in
+ * 'sector_number' must be aligned to 'sector-size'.
*/
struct blkif_request {
uint8_t operation; /* BLKIF_OP_??? */
@@ -648,6 +665,10 @@ typedef struct blkif_request blkif_request_t;
/*
* Cast to this structure when blkif_request.operation == BLKIF_OP_DISCARD
* sizeof(struct blkif_request_discard) <= sizeof(struct blkif_request)
+ *
+ * The 'sector_number' field is in units of 512b, despite the value of the
+ * 'sector-size' xenstore node. Note however that the offset in
+ * 'sector_number' must be aligned to 'sector-size'.
*/
struct blkif_request_discard {
uint8_t operation; /* BLKIF_OP_DISCARD */
@@ -660,6 +681,11 @@ struct blkif_request_discard {
};
typedef struct blkif_request_discard blkif_request_discard_t;
+/*
+ * The 'sector_number' field is in units of 512b, despite the value of the
+ * 'sector-size' xenstore node. Note however that the offset in
+ * 'sector_number' must be aligned to 'sector-size'.
+ */
struct blkif_request_indirect {
uint8_t operation; /* BLKIF_OP_INDIRECT */
uint8_t indirect_op; /* BLKIF_OP_{READ/WRITE} */
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH 2/2] hw/block/xen-block: Update sector-size handling
2024-09-26 10:14 [PATCH 0/2] Xen: Update sector-size handling in block backend Anthony PERARD
2024-09-26 10:14 ` [PATCH 1/2] include: update Xen public headers io/blkif.h Anthony PERARD
@ 2024-09-26 10:14 ` Anthony PERARD
1 sibling, 0 replies; 3+ messages in thread
From: Anthony PERARD @ 2024-09-26 10:14 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony PERARD, Stefano Stabellini, Anthony PERARD, Paul Durrant,
Edgar E. Iglesias, Stefan Hajnoczi, Kevin Wolf, Hanna Reitz,
xen-devel, qemu-block
The use of "feature-large-sector-size" have been removed from the
protocol, as it hasn't been evenly implemented across all backend and
frontend. Linux for example will happily expose "sector-size" != 512
even when "feature-large-sector-size" hasn't been set by the frontend.
The specification have been clarified regarding what "sector" is by
Xen commit 221f2748e8da ("blkif: reconcile protocol specification with
in-use implementations").
https://xenbits.xenproject.org/gitweb/?p=xen.git;a=commit;h=221f2748e8dabe8361b8cdfcffbeab9102c4c899
So change QEMU to follow the updated specification.
Frontends that exposes "feature-large-sector-size" will most certainly
misbehave if "sector-size" is different than 512, so don't even try.
(Windows driver is likely to be the only one having implemented it.)
Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
---
hw/block/dataplane/xen-block.c | 31 ++++++++++++++++++++++---------
hw/block/xen-block.c | 8 ++++----
2 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 98501e6885..43be97b4f3 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -176,7 +176,11 @@ static int xen_block_parse_request(XenBlockRequest *request)
goto err;
}
- request->start = request->req.sector_number * dataplane->sector_size;
+ request->start = request->req.sector_number * XEN_BLKIF_SECTOR_SIZE;
+ if (!QEMU_IS_ALIGNED(request->start, dataplane->sector_size)) {
+ error_report("error: sector_number missaligned with sector-size");
+ goto err;
+ }
for (i = 0; i < request->req.nr_segments; i++) {
if (i == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
error_report("error: nr_segments too big");
@@ -186,14 +190,23 @@ static int xen_block_parse_request(XenBlockRequest *request)
error_report("error: first > last sector");
goto err;
}
- if (request->req.seg[i].last_sect * dataplane->sector_size >=
+ if (request->req.seg[i].last_sect * XEN_BLKIF_SECTOR_SIZE >=
XEN_PAGE_SIZE) {
error_report("error: page crossing");
goto err;
}
+ if (!QEMU_IS_ALIGNED(request->req.seg[i].first_sect,
+ dataplane->sector_size / XEN_BLKIF_SECTOR_SIZE)) {
+ error_report("error: first_sect missaligned with sector-size");
+ goto err;
+ }
len = (request->req.seg[i].last_sect -
- request->req.seg[i].first_sect + 1) * dataplane->sector_size;
+ request->req.seg[i].first_sect + 1) * XEN_BLKIF_SECTOR_SIZE;
+ if (!QEMU_IS_ALIGNED(len, dataplane->sector_size)) {
+ error_report("error: segment size missaligned with sector-size");
+ goto err;
+ }
request->size += len;
}
if (request->start + request->size > blk_getlength(dataplane->blk)) {
@@ -227,17 +240,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 *
- dataplane->sector_size;
+ XEN_BLKIF_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 *
- dataplane->sector_size;
+ XEN_BLKIF_SECTOR_SIZE;
segs[i].dest.virt = virt;
}
segs[i].len = (request->req.seg[i].last_sect -
request->req.seg[i].first_sect + 1) *
- dataplane->sector_size;
+ XEN_BLKIF_SECTOR_SIZE;
virt += segs[i].len;
}
@@ -331,12 +344,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 / dataplane->sector_size) {
+ sec_start + sec_count > INT64_MAX / XEN_BLKIF_SECTOR_SIZE) {
return false;
}
- byte_offset = sec_start * dataplane->sector_size;
- byte_remaining = sec_count * dataplane->sector_size;
+ byte_offset = sec_start * XEN_BLKIF_SECTOR_SIZE;
+ byte_remaining = sec_count * XEN_BLKIF_SECTOR_SIZE;
do {
byte_chunk = byte_remaining > BDRV_REQUEST_MAX_BYTES ?
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index aed1d5c330..8c150c6c69 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -189,10 +189,10 @@ static void xen_block_connect(XenDevice *xendev, Error **errp)
feature_large_sector_size = 0;
}
- if (feature_large_sector_size != 1 &&
+ if (feature_large_sector_size == 1 &&
conf->logical_block_size != XEN_BLKIF_SECTOR_SIZE) {
- error_setg(errp, "logical_block_size != %u not supported by frontend",
- XEN_BLKIF_SECTOR_SIZE);
+ error_setg(errp,
+ "\"feature-large-sector-size\" not supported by backend");
return;
}
@@ -294,7 +294,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) / conf->logical_block_size;
+ int64_t sectors = blk_getlength(conf->blk) / XEN_BLKIF_SECTOR_SIZE;
XenDevice *xendev = XEN_DEVICE(blockdev);
trace_xen_block_size(type, vdev->disk, vdev->partition, sectors);
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply related [flat|nested] 3+ messages in thread