Re: [PATCH-for-9.0 v2 19/19] hw/xen: Have most of Xen files become target-agnostic
On Tue, Nov 14, 2023 at 03:38:15PM +0100, Philippe Mathieu-Daudé wrote:
> Previous commits re-organized the target-specific bits
> from Xen files. We can now build the common files once
> instead of per-target.
>
> Only 4 files call libxen API (thus its CPPFLAGS):
> - xen-hvm-common.c,
> - xen_pt.c, xen_pt_graphics.c, xen_pt_msi.c
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> Reworked since v1 so dropping David's R-b tag.
> ---
> accel/xen/meson.build | 2 +-
> hw/block/dataplane/meson.build | 2 +-
> hw/xen/meson.build | 21 ++---
> 3 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/accel/xen/meson.build b/accel/xen/meson.build
> index 002bdb03c6..455ad5d6be 100644
> --- a/accel/xen/meson.build
> +++ b/accel/xen/meson.build
> @@ -1 +1 @@
> -specific_ss.add(when: 'CONFIG_XEN', if_true: files('xen-all.c'))
> +system_ss.add(when: 'CONFIG_XEN', if_true: files('xen-all.c'))
> diff --git a/hw/block/dataplane/meson.build b/hw/block/dataplane/meson.build
> index 025b3b061b..4d8bcb0bb9 100644
> --- a/hw/block/dataplane/meson.build
> +++ b/hw/block/dataplane/meson.build
> @@ -1,2 +1,2 @@
> system_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c'))
> -specific_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen-block.c'))
> +system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen-block.c'))
> diff --git a/hw/xen/meson.build b/hw/xen/meson.build
> index d887fa9ba4..403cab49cf 100644
> --- a/hw/xen/meson.build
> +++ b/hw/xen/meson.build
> @@ -7,26 +7,25 @@ system_ss.add(when: ['CONFIG_XEN_BUS'], if_true: files(
>'xen_pvdev.c',
> ))
>
> -system_ss.add(when: ['CONFIG_XEN', xen], if_true: files(
> +system_ss.add(when: ['CONFIG_XEN'], if_true: files(
>'xen-operations.c',
> -))
> -
> -xen_specific_ss = ss.source_set()
> -xen_specific_ss.add(files(
>'xen-mapcache.c',
> +))
> +system_ss.add(when: ['CONFIG_XEN', xen], if_true: files(
>'xen-hvm-common.c',
> ))
> +
> if have_xen_pci_passthrough
> - xen_specific_ss.add(files(
> + system_ss.add(when: ['CONFIG_XEN'], if_true: files(
> 'xen-host-pci-device.c',
> -'xen_pt.c',
> 'xen_pt_config_init.c',
> -'xen_pt_graphics.c',
> 'xen_pt_load_rom.c',
> + ))
> + system_ss.add(when: ['CONFIG_XEN', xen], if_true: files(
> +'xen_pt.c',
> +'xen_pt_graphics.c',
How is it useful to separate those source files? In the commit
description, there's a talk about "CPPFLAGS", but having `when: [xen]`
doesn't change the flags used to build those objects, so the talk about
"CPPFLAGS" is confusing.
Second, if for some reason the dependency `xen` is false, but
`CONFIG_XEN` is true, then we wouldn't be able to build QEMU. Try
linking a binary with "xen_pt_config_init.o" but without "xen_pt.o",
that's not going to work. So even if that first source file doesn't
directly depend on the Xen libraries, it depends on "xen_pt.o" which
depends on the Xen libraries. So ultimately, I think all those source
files should have the same condition: ['CONFIG_XEN', xen].
I've only checked the xen_pt* source files, I don't know if the same
applies to "xen-operations.c" or "xen-mapcache.c".
Beside this, QEMU built with Xen support still seems to works fine, so
adding the objects to `system_ss` instead of `specific_ss` seems
alright.
Thanks,
--
Anthony PERARD
Re: [PATCH-for-9.0 v2 16/19] hw/xen/xen_pt: Add missing license
On Tue, Nov 14, 2023 at 03:38:12PM +0100, Philippe Mathieu-Daudé wrote:
> Commit eaab4d60d3 ("Introduce Xen PCI Passthrough, qdevice")
> introduced both xen_pt.[ch], but only added the license to
> xen_pt.c. Use the same license for xen_pt.h.
>
> Suggested-by: David Woodhouse
> Signed-off-by: Philippe Mathieu-Daudé
Fine by me. Looks like there was a license header before:
https://xenbits.xen.org/gitweb/?p=qemu-xen-unstable.git;a=blob;f=hw/pass-through.h;h=0b5822414e24d199a064abccc4d378dcaf569bd6;hb=HEAD
I don't know why I didn't copied it over here.
Acked-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH-for-9.0 v2 15/19] hw/xen: Reduce inclusion of 'cpu.h' to target-specific sources
On Tue, Nov 14, 2023 at 03:38:11PM +0100, Philippe Mathieu-Daudé wrote: > We rarely need to include "cpu.h" in headers. Including it > 'taint' headers to be target-specific. Here only the i386/arm > implementations requires "cpu.h", so include it there and > remove from the "hw/xen/xen-hvm-common.h" *common* header. > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: Richard Henderson > Reviewed-by: David Woodhouse Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [RFC PATCH-for-9.0 v2 13/19] hw/xen: Remove use of 'target_ulong' in handle_ioreq()
On Tue, Nov 14, 2023 at 03:38:09PM +0100, Philippe Mathieu-Daudé wrote:
> Per commit f17068c1c7 ("xen-hvm: reorganize xen-hvm and move common
> function to xen-hvm-common"), handle_ioreq() is expected to be
> target-agnostic. However it uses 'target_ulong', which is a target
> specific definition.
>
> Per xen/include/public/hvm/ioreq.h header:
>
> struct ioreq {
> uint64_t addr; /* physical address */
> uint64_t data; /* data (or paddr of data) */
> uint32_t count; /* for rep prefixes */
> uint32_t size; /* size in bytes */
> uint32_t vp_eport; /* evtchn for notifications to/from device model
> */
> uint16_t _pad0;
> uint8_t state:4;
> uint8_t data_is_ptr:1; /* if 1, data above is the guest paddr
> * of the real data to use. */
> uint8_t dir:1; /* 1=read, 0=write */
> uint8_t df:1;
> uint8_t _pad1:1;
> uint8_t type; /* I/O type */
> };
> typedef struct ioreq ioreq_t;
>
> If 'data' is not a pointer, it is a u64.
>
> - In PIO / VMWARE_PORT modes, only 32-bit are used.
>
> - In MMIO COPY mode, memory is accessed by chunks of 64-bit
Looks like it could also be 8, 16, or 32 as well, depending on
req->size.
> - In PCI_CONFIG mode, access is u8 or u16 or u32.
>
> - None of TIMEOFFSET / INVALIDATE use 'req'.
>
> - Fallback is only used in x86 for VMWARE_PORT.
>
> Masking the upper bits of 'data' to keep 'req->size' low bits
> is irrelevant of the target word size. Remove the word size
> check and always extract the relevant bits.
When building QEMU for Xen, we tend to build the target "i386-softmmu",
which looks like to have target_ulong == uint32_t. So the `data`
clamping would only apply to size 8 and 16. The clamping with
target_ulong was introduce long time ago, here:
https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=b4a663b87df3954557434a2d31bff7f6b2706ec1
and they were more IOREQ types.
So my guess is it isn't relevant anymore, but extending the clamping to
32-bits request should be fine, when using qemu-system-i386 that is, as
it is already be done if one use qemu-system-x86_64.
So I think the patch is fine, and the tests I've ran so far worked fine.
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH-for-9.0 v2 12/19] hw/xen: Merge 'hw/xen/arch_hvm.h' in 'hw/xen/xen-hvm-common.h'
On Tue, Nov 14, 2023 at 03:38:08PM +0100, Philippe Mathieu-Daudé wrote: > We don't need a target-specific header for common target-specific > prototypes. Declare xen_arch_handle_ioreq() and xen_arch_set_memory() > in "hw/xen/xen-hvm-common.h". > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: David Woodhouse > Reviewed-by: Richard Henderson Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH-for-9.0 v2 11/19] hw/xen/xen_arch_hvm: Rename prototypes using 'xen_arch_' prefix
On Tue, Nov 14, 2023 at 03:38:07PM +0100, Philippe Mathieu-Daudé wrote: > Use a common 'xen_arch_' prefix for architecture-specific functions. > Rename xen_arch_set_memory() and xen_arch_handle_ioreq(). > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: David Woodhouse > Reviewed-by: Richard Henderson Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [RFC PATCH-for-9.0 v2 09/19] hw/block/xen_blkif: Align structs with QEMU_ALIGNED() instead of #pragma
On Tue, Nov 14, 2023 at 03:38:05PM +0100, Philippe Mathieu-Daudé wrote:
> Except imported source files, QEMU code base uses
> the QEMU_ALIGNED() macro to align its structures.
This patch only convert the alignment, but discard pack. We need both or
the struct is changed.
> ---
> hw/block/xen_blkif.h | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/hw/block/xen_blkif.h b/hw/block/xen_blkif.h
> index 99733529c1..c1d154d502 100644
> --- a/hw/block/xen_blkif.h
> +++ b/hw/block/xen_blkif.h
> @@ -18,7 +18,6 @@ struct blkif_common_response {
> };
>
> /* i386 protocol version */
> -#pragma pack(push, 4)
> struct blkif_x86_32_request {
> uint8_toperation;/* BLKIF_OP_???
> */
> uint8_tnr_segments; /* number of segments
> */
> @@ -26,7 +25,7 @@ struct blkif_x86_32_request {
> uint64_t id; /* private guest value, echoed in resp
> */
> blkif_sector_t sector_number;/* start sector idx on disk (r/w only)
> */
> struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> -};
> +} QEMU_ALIGNED(4);
E.g. for this one, I've compare the output of
`pahole --class_name=blkif_x86_32_request build/qemu-system-i386`:
--- before
+++ after
@@ -1,11 +1,15 @@
struct blkif_x86_32_request {
uint8_toperation;/* 0 1 */
uint8_tnr_segments; /* 1 1 */
uint16_t handle; /* 2 2 */
- uint64_t id; /* 4 8 */
- uint64_t sector_number;/*12 8 */
- struct blkif_request_segment seg[11];/*2088 */
- /* size: 108, cachelines: 2, members: 6 */
- /* last cacheline: 44 bytes */
-} __attribute__((__packed__));
+ /* XXX 4 bytes hole, try to pack */
+
+ uint64_t id; /* 8 8 */
+ uint64_t sector_number;/*16 8 */
+ struct blkif_request_segment seg[11];/*2488 */
+
+ /* size: 112, cachelines: 2, members: 6 */
+ /* sum members: 108, holes: 1, sum holes: 4 */
+ /* last cacheline: 48 bytes */
+} __attribute__((__aligned__(8)));
Thanks,
--
Anthony PERARD
Re: [PATCH-for-9.0 v2 05/19] hw/display: Restrict xen_register_framebuffer() call to Xen
On Tue, Nov 14, 2023 at 03:38:01PM +0100, Philippe Mathieu-Daudé wrote: > Only call xen_register_framebuffer() when Xen is enabled. > > Signed-off-by: Philippe Mathieu-Daudé I don't think this patch is very useful but it's fine, so: Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH-for-9.0 v2 08/19] hw/xen: Remove unused Xen stubs
On Tue, Nov 14, 2023 at 03:38:04PM +0100, Philippe Mathieu-Daudé wrote:
> All these stubs are protected by a 'if (xen_enabled())' check.
Are you sure? There's still nothing that prevent a compiler from wanting
those, I don't think.
Sure, often compilers will remove dead code in `if(0){...}`, but there's
no guaranty, is there?
Cheers,
--
Anthony PERARD
Re: [PATCH v4 2/6] xen: backends: don't overwrite XenStore nodes created by toolstack
On Sat, Dec 02, 2023 at 01:41:21AM +, Volodymyr Babchuk wrote: > Xen PV devices in QEMU can be created in two ways: either by QEMU > itself, if they were passed via command line, or by Xen toolstack. In > the latter case, QEMU scans XenStore entries and configures devices > accordingly. > > In the second case we don't want QEMU to write/delete front-end > entries for two reasons: it might have no access to those entries if > it is running in un-privileged domain and it is just incorrect to > overwrite entries already provided by Xen toolstack, because toolstack > manages those nodes. For example, it might read backend- or frontend- > state to be sure that they are both disconnected and it is safe to > destroy a domain. > > This patch checks presence of xendev->backend to check if Xen PV > device was configured by Xen toolstack to decide if it should touch > frontend entries in XenStore. Also, when we need to remove XenStore > entries during device teardown only if they weren't created by Xen > toolstack. If they were created by toolstack, then it is toolstack's > job to do proper clean-up. > > Suggested-by: Paul Durrant > Suggested-by: David Woodhouse > Co-Authored-by: Oleksandr Tyshchenko > Signed-off-by: Volodymyr Babchuk > Reviewed-by: David Woodhouse > Hi Volodymyr, There's something wrong with this patch. The block backend doesn't work when creating a guest via libxl, an x86 hvm guest with qdisk. Error from guest kernel: "2 reading backend fields at /local/domain/0/backend/qdisk/23/768" It seems that "sector-size" is missing for the disk. Thanks, -- Anthony PERARD
Re: [PATCH 6/7] block: Clean up local variable shadowing
On Thu, Aug 31, 2023 at 03:25:45PM +0200, Markus Armbruster wrote:
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 3906b9058b..a07cd7eb5d 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -369,7 +369,7 @@ static void xen_block_get_vdev(Object *obj, Visitor *v,
> const char *name,
> case XEN_BLOCK_VDEV_TYPE_XVD:
> case XEN_BLOCK_VDEV_TYPE_HD:
> case XEN_BLOCK_VDEV_TYPE_SD: {
> -char *name = disk_to_vbd_name(vdev->disk);
> +char *vbd_name = disk_to_vbd_name(vdev->disk);
>
> str = g_strdup_printf("%s%s%lu",
>(vdev->type == XEN_BLOCK_VDEV_TYPE_XVD) ?
> @@ -377,8 +377,8 @@ static void xen_block_get_vdev(Object *obj, Visitor *v,
> const char *name,
>(vdev->type == XEN_BLOCK_VDEV_TYPE_HD) ?
>"hd" :
>"sd",
> - name, vdev->partition);
> -g_free(name);
> + vbd_name, vdev->partition);
> + g_free(vbd_name);
> break;
> }
> default:
Acked-by: Anthony PERARD
Thanks,
--
Anthony PERARD
[PATCH] xen-block: Avoid leaks on new error path
From: Anthony PERARD
Commit 189829399070 ("xen-block: Use specific blockdev driver")
introduced a new error path, without taking care of allocated
resources.
So only allocate the qdicts after the error check, and free both
`filename` and `driver` when we are about to return and thus taking
care of both success and error path.
Coverity only spotted the leak of qdicts (*_layer variables).
Reported-by: Peter Maydell
Fixes: Coverity CID 1508722, 1398649
Fixes: 189829399070 ("xen-block: Use specific blockdev driver")
Signed-off-by: Anthony PERARD
---
hw/block/xen-block.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index f099914831..3906b9058b 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -781,14 +781,15 @@ static XenBlockDrive *xen_block_drive_create(const char
*id,
drive = g_new0(XenBlockDrive, 1);
drive->id = g_strdup(id);
-file_layer = qdict_new();
-driver_layer = qdict_new();
-
rc = stat(filename, );
if (rc) {
error_setg_errno(errp, errno, "Could not stat file '%s'", filename);
goto done;
}
+
+file_layer = qdict_new();
+driver_layer = qdict_new();
+
if (S_ISBLK(st.st_mode)) {
qdict_put_str(file_layer, "driver", "host_device");
} else {
@@ -796,7 +797,6 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
}
qdict_put_str(file_layer, "filename", filename);
-g_free(filename);
if (mode && *mode != 'w') {
qdict_put_bool(file_layer, "read-only", true);
@@ -831,7 +831,6 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
qdict_put_str(file_layer, "locking", "off");
qdict_put_str(driver_layer, "driver", driver);
-g_free(driver);
qdict_put(driver_layer, "file", file_layer);
@@ -842,6 +841,8 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
qobject_unref(driver_layer);
done:
+g_free(filename);
+g_free(driver);
if (*errp) {
xen_block_drive_destroy(drive, NULL);
return NULL;
--
Anthony PERARD
Re: [PATCH v5 12/21] xen-block: implement BlockDevOps->drained_begin()
On Thu, May 04, 2023 at 03:53:18PM -0400, Stefan Hajnoczi wrote:
> @@ -819,11 +841,9 @@ void xen_block_dataplane_start(XenBlockDataPlane
> *dataplane,
> blk_set_aio_context(dataplane->blk, dataplane->ctx, NULL);
> aio_context_release(old_context);
>
> -/* Only reason for failure is a NULL channel */
> -aio_context_acquire(dataplane->ctx);
> -xen_device_set_event_channel_context(xendev, dataplane->event_channel,
> - dataplane->ctx, _abort);
> -aio_context_release(dataplane->ctx);
> +if (!blk_in_drain(dataplane->blk)) {
There's maybe something missing in the patch.
xen_block_dataplane_start() calls xen_device_bind_event_channel() just
before xen_block_dataplane_attach().
And xen_device_bind_event_channel() sets the event context to
qemu_get_aio_context() instead of NULL.
So, even if we don't call xen_block_dataplane_attach() while in drain,
there's already a fd_handler attach to the fd. So should
xen_device_bind_event_channel() be changed as well? Or maybe a call to
xen_block_dataplane_detach() would be enough.
(There's only one user of xen_device_bind_event_channel() at the moment
so I don't know if other implementation making use of this API will want
to call set_event_channel_context or not.)
> +xen_block_dataplane_attach(dataplane);
> +}
>
> return;
>
--
Anthony PERARD
Re: [PATCH v5 13/21] hw/xen: do not set is_external=true on evtchn fds
On Thu, May 04, 2023 at 03:53:19PM -0400, Stefan Hajnoczi wrote: > is_external=true suspends fd handlers between aio_disable_external() and > aio_enable_external(). The block layer's drain operation uses this > mechanism to prevent new I/O from sneaking in between > bdrv_drained_begin() and bdrv_drained_end(). > > The previous commit converted the xen-block device to use BlockDevOps > .drained_begin/end() callbacks. It no longer relies on is_external=true > so it is safe to pass is_external=false. > > This is part of ongoing work to remove the aio_disable_external() API. > > Signed-off-by: Stefan Hajnoczi Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v5 12/21] xen-block: implement BlockDevOps->drained_begin()
On Thu, May 04, 2023 at 03:53:18PM -0400, Stefan Hajnoczi wrote: > Detach event channels during drained sections to stop I/O submission > from the ring. xen-block is no longer reliant on aio_disable_external() > after this patch. This will allow us to remove the > aio_disable_external() API once all other code that relies on it is > converted. > > Extend xen_device_set_event_channel_context() to allow ctx=NULL. The > event channel still exists but the event loop does not monitor the file > descriptor. Event channel processing can resume by calling > xen_device_set_event_channel_context() with a non-NULL ctx. > > Factor out xen_device_set_event_channel_context() calls in > hw/block/dataplane/xen-block.c into attach/detach helper functions. > Incidentally, these don't require the AioContext lock because > aio_set_fd_handler() is thread-safe. > > It's safer to register BlockDevOps after the dataplane instance has been > created. The BlockDevOps .drained_begin/end() callbacks depend on the > dataplane instance, so move the blk_set_dev_ops() call after > xen_block_dataplane_create(). > > Signed-off-by: Stefan Hajnoczi Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH 2/6] hw/xen: use G_GNUC_PRINTF/SCANF for various functions
On Mon, Dec 19, 2022 at 08:02:01AM -0500, Daniel P. Berrangé wrote: > Signed-off-by: Daniel P. Berrangé Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
[PULL 3/3] include/hw/ide: Unexport pci_piix3_xen_ide_unplug()
From: Bernhard Beschow
This function was declared in a generic and public header, implemented
in a device-specific source file but only used in xen_platform. Given its
'aux' parameter, this function is more xen-specific than piix-specific.
Also, the hardcoded magic constants seem to be generic and related to
PCIIDEState and IDEBus rather than piix.
Therefore, move this function to xen_platform, unexport it, and drop the
"piix3" in the function name as well.
Signed-off-by: Bernhard Beschow
Reviewed-by: Paul Durrant
Acked-by: Anthony PERARD
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20220513180957.90514-4-shen...@gmail.com>
Signed-off-by: Anthony PERARD
---
hw/i386/xen/xen_platform.c | 48 +-
hw/ide/piix.c | 46
include/hw/ide.h | 3 ---
3 files changed, 47 insertions(+), 50 deletions(-)
diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index 72028449ba..a64265cca0 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -26,6 +26,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "hw/ide.h"
+#include "hw/ide/pci.h"
#include "hw/pci/pci.h"
#include "hw/xen/xen_common.h"
#include "migration/vmstate.h"
@@ -134,6 +135,51 @@ static void pci_unplug_nics(PCIBus *bus)
pci_for_each_device(bus, 0, unplug_nic, NULL);
}
+/*
+ * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
+ * request unplug of 'aux' disks (which is stated to mean all IDE disks,
+ * except the primary master).
+ *
+ * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
+ * is simultaneously requested is not clear. The implementation assumes
+ * that an 'all' request overrides an 'aux' request.
+ *
+ * [1]
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
+ */
+static void pci_xen_ide_unplug(DeviceState *dev, bool aux)
+{
+PCIIDEState *pci_ide;
+int i;
+IDEDevice *idedev;
+IDEBus *idebus;
+BlockBackend *blk;
+
+pci_ide = PCI_IDE(dev);
+
+for (i = aux ? 1 : 0; i < 4; i++) {
+idebus = _ide->bus[i / 2];
+blk = idebus->ifs[i % 2].blk;
+
+if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
+if (!(i % 2)) {
+idedev = idebus->master;
+} else {
+idedev = idebus->slave;
+}
+
+blk_drain(blk);
+blk_flush(blk);
+
+blk_detach_dev(blk, DEVICE(idedev));
+idebus->ifs[i % 2].blk = NULL;
+idedev->conf.blk = NULL;
+monitor_remove_blk(blk);
+blk_unref(blk);
+}
+}
+qdev_reset_all(dev);
+}
+
static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
{
uint32_t flags = *(uint32_t *)opaque;
@@ -147,7 +193,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void
*opaque)
switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) {
case PCI_CLASS_STORAGE_IDE:
-pci_piix3_xen_ide_unplug(DEVICE(d), aux);
+pci_xen_ide_unplug(DEVICE(d), aux);
break;
case PCI_CLASS_STORAGE_SCSI:
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index bc1b37512a..9a9b28078e 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -173,52 +173,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
**errp)
}
}
-/*
- * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
- * request unplug of 'aux' disks (which is stated to mean all IDE disks,
- * except the primary master).
- *
- * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
- * is simultaneously requested is not clear. The implementation assumes
- * that an 'all' request overrides an 'aux' request.
- *
- * [1]
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
- */
-int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
-{
-PCIIDEState *pci_ide;
-int i;
-IDEDevice *idedev;
-IDEBus *idebus;
-BlockBackend *blk;
-
-pci_ide = PCI_IDE(dev);
-
-for (i = aux ? 1 : 0; i < 4; i++) {
-idebus = _ide->bus[i / 2];
-blk = idebus->ifs[i % 2].blk;
-
-if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
-if (!(i % 2)) {
-idedev = idebus->master;
-} else {
-idedev = idebus->slave;
-}
-
-blk_drain(blk);
-blk_flush(blk);
-
-blk_detach_dev(blk, DEVICE(idedev));
-idebus->ifs[i % 2].blk = NULL;
-idedev->conf.blk = NULL;
-monitor_remove_blk(blk);
-blk_unref(blk);
-}
-}
-qdev_reset_all(dev);
-return 0;
-}
-
static void pci_piix_ide_exitfn(PCIDevice *dev)
{
[PULL 1/3] hw/ide/piix: Remove redundant "piix3-ide-xen" device class
From: Bernhard Beschow
Commit 0f8445820f11a69154309863960328dda3dc1ad4 'xen: piix reuse pci
generic class init function' already resolved redundant code which in
turn rendered piix3-ide-xen redundant.
Signed-off-by: Bernhard Beschow
Reviewed-by: Anthony PERARD
Message-Id: <20220513180957.90514-2-shen...@gmail.com>
Signed-off-by: Anthony PERARD
---
hw/i386/pc_piix.c | 3 +--
hw/ide/piix.c | 7 ---
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 578e537b35..0e45521e74 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -246,8 +246,7 @@ static void pc_init1(MachineState *machine,
if (pcmc->pci_enabled) {
PCIDevice *dev;
-dev = pci_create_simple(pci_bus, piix3_devfn + 1,
-xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
+dev = pci_create_simple(pci_bus, piix3_devfn + 1, "piix3-ide");
pci_ide_create_devs(dev);
idebus[0] = qdev_get_child_bus(>qdev, "ide.0");
idebus[1] = qdev_get_child_bus(>qdev, "ide.1");
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ce89fd0aa3..2345fe9e1d 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -241,12 +241,6 @@ static const TypeInfo piix3_ide_info = {
.class_init= piix3_ide_class_init,
};
-static const TypeInfo piix3_ide_xen_info = {
-.name = "piix3-ide-xen",
-.parent= TYPE_PCI_IDE,
-.class_init= piix3_ide_class_init,
-};
-
/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
static void piix4_ide_class_init(ObjectClass *klass, void *data)
{
@@ -272,7 +266,6 @@ static const TypeInfo piix4_ide_info = {
static void piix_ide_register_types(void)
{
type_register_static(_ide_info);
-type_register_static(_ide_xen_info);
type_register_static(_ide_info);
}
--
Anthony PERARD
[PULL 2/3] hw/ide/piix: Add some documentation to pci_piix3_xen_ide_unplug()
From: Bernhard Beschow
The comment is based on commit message
ae4d2eb273b167dad748ea4249720319240b1ac2 'xen-platform: add missing disk
unplug option'. Since it seems to describe design decisions and
limitations that still apply it seems worth having.
Signed-off-by: Bernhard Beschow
Reviewed-by: Anthony PERARD
Message-Id: <20220513180957.90514-3-shen...@gmail.com>
Signed-off-by: Anthony PERARD
---
hw/ide/piix.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 2345fe9e1d..bc1b37512a 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -173,6 +173,17 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
**errp)
}
}
+/*
+ * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
+ * request unplug of 'aux' disks (which is stated to mean all IDE disks,
+ * except the primary master).
+ *
+ * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
+ * is simultaneously requested is not clear. The implementation assumes
+ * that an 'all' request overrides an 'aux' request.
+ *
+ * [1]
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
+ */
int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
{
PCIIDEState *pci_ide;
--
Anthony PERARD
Re: [PATCH v2 3/3] include/hw/ide: Unexport pci_piix3_xen_ide_unplug()
On Fri, May 13, 2022 at 08:09:57PM +0200, Bernhard Beschow wrote: > This function was declared in a generic and public header, implemented > in a device-specific source file but only used in xen_platform. Given its > 'aux' parameter, this function is more xen-specific than piix-specific. > Also, the hardcoded magic constants seem to be generic and related to > PCIIDEState and IDEBus rather than piix. > > Therefore, move this function to xen_platform, unexport it, and drop the > "piix3" in the function name as well. > > Signed-off-by: Bernhard Beschow > Reviewed-by: Paul Durrant Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v2 2/3] hw/ide/piix: Add some documentation to pci_piix3_xen_ide_unplug()
On Fri, May 13, 2022 at 08:09:56PM +0200, Bernhard Beschow wrote:
> The comment is based on commit message
> ae4d2eb273b167dad748ea4249720319240b1ac2 'xen-platform: add missing disk
> unplug option'. Since it seems to describe design decisions and
> limitations that still apply it seems worth having.
>
> Signed-off-by: Bernhard Beschow
> ---
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 2345fe9e1d..bc1b37512a 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -173,6 +173,17 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
> **errp)
> }
> }
>
> +/*
> + * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
> + * request unplug of 'aux' disks (which is stated to mean all IDE disks,
> + * except the primary master).
> + *
> + * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
> + * is simultaneously requested is not clear. The implementation assumes
> + * that an 'all' request overrides an 'aux' request.
> + *
> + * [1]
> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
> + */
> int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
> {
> PCIIDEState *pci_ide;
That comments seems to focus on 'aux', but it also gives some pointer on
what calls the function. So it looks fine.
Reviewed-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH v2 1/3] hw/ide/piix: Remove redundant "piix3-ide-xen" device class
On Fri, May 13, 2022 at 08:09:55PM +0200, Bernhard Beschow wrote: > Commit 0f8445820f11a69154309863960328dda3dc1ad4 'xen: piix reuse pci > generic class init function' already resolved redundant code which in > turn rendered piix3-ide-xen redundant. > > Signed-off-by: Bernhard Beschow Creating a guest and migrating a guest seems to work fine without "piix3-ide-xen", and I can't find this name used outside of QEMU. So I guess it's fine to remove it. Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
[PATCH] xen-block: Use specific blockdev driver
From: Anthony PERARD
... when a xen-block backend instance is created via xenstore.
Following 8d17adf34f50 ("block: remove support for using "file" driver
with block/char devices"), using the "file" blockdev driver for
everything doesn't work anymore, we need to use the "host_device"
driver when the disk image is a block device and "file" driver when it
is a regular file.
Signed-off-by: Anthony PERARD
---
hw/block/xen-block.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 83754a434481..674953f1adee 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -728,6 +728,8 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
XenBlockDrive *drive = NULL;
QDict *file_layer;
QDict *driver_layer;
+struct stat st;
+int rc;
if (params) {
char **v = g_strsplit(params, ":", 2);
@@ -761,7 +763,17 @@ static XenBlockDrive *xen_block_drive_create(const char
*id,
file_layer = qdict_new();
driver_layer = qdict_new();
-qdict_put_str(file_layer, "driver", "file");
+rc = stat(filename, );
+if (rc) {
+error_setg_errno(errp, errno, "Could not stat file '%s'", filename);
+goto done;
+}
+if (S_ISBLK(st.st_mode)) {
+qdict_put_str(file_layer, "driver", "host_device");
+} else {
+qdict_put_str(file_layer, "driver", "file");
+}
+
qdict_put_str(file_layer, "filename", filename);
g_free(filename);
--
Anthony PERARD
Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
Hi Paul, Stefano,
Could one of you could give a Ack to this patch?
Thanks,
On Mon, Mar 08, 2021 at 02:32:32PM +, Anthony PERARD wrote:
> From: Anthony PERARD
>
> Whenever a Xen block device is detach via xenstore, the image
> associated with it remained open by the backend QEMU and an error is
> logged:
> qemu-system-i386: failed to destroy drive: Node xvdz-qcow2 is in use
>
> This happened since object_unparent() doesn't immediately frees the
> object and thus keep a reference to the node we are trying to free.
> The reference is hold by the "drive" property and the call
> xen_block_drive_destroy() fails.
>
> In order to fix that, we call drain_call_rcu() to run the callback
> setup by bus_remove_child() via object_unparent().
>
> Fixes: 2d24a6466154 ("device-core: use RCU for list of children of a bus")
>
> Signed-off-by: Anthony PERARD
> ---
> CCing people whom introduced/reviewed the change to use RCU to give
> them a chance to say if the change is fine.
> ---
> hw/block/xen-block.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index a3b69e27096f..fe5f828e2d25 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -972,6 +972,15 @@ static void xen_block_device_destroy(XenBackendInstance
> *backend,
>
> object_unparent(OBJECT(xendev));
>
> +/*
> + * Drall all pending RCU callbacks as object_unparent() frees `xendev'
> + * in a RCU callback.
> + * And due to the property "drive" still existing in `xendev', we
> + * cann't destroy the XenBlockDrive associated with `xendev' with
> + * xen_block_drive_destroy() below.
> + */
> + drain_call_rcu();
> +
> if (iothread) {
> xen_block_iothread_destroy(iothread, errp);
> if (*errp) {
> --
> Anthony PERARD
>
--
Anthony PERARD
Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
On Mon, Mar 08, 2021 at 06:37:38PM +0100, Paolo Bonzini wrote:
> On 08/03/21 18:29, Anthony PERARD wrote:
> > > If nothing else works then I guess it's okay, but why can't you do the
> > > xen_block_drive_destroy from e.g. an unrealize callback?
> >
> > I'm not sure if that's possible.
> >
> > xen_block_device_create/xen_block_device_destroy() is supposed to be
> > equivalent to do those qmp commands:
> > blockdev-add node-name=xvdz-qcow2 driver=qcow2
> > file={"driver":"file","filename":"disk.qcow2","locking":"off"}
> > device_add id=xvdz driver=xen-disk vdev=xvdz drive=xvdz-qcow2
> >
> > But I tried to add a call xen_block_drive_destroy from
> > xen_block_unrealize, but that still is called too early, it's called
> > before object_property_del_all() which would delete "drive" and call
> > release_drive() which would free the node.
>
> Can you use blockdev_mark_auto_del? Then you don't have to call
> xen_block_drive_destroy at all.
There is no legacy_dinfo, so blockdev_mark_auto_del doesn't work.
--
Anthony PERARD
Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
On Mon, Mar 08, 2021 at 03:38:49PM +0100, Paolo Bonzini wrote:
> On 08/03/21 15:32, Anthony PERARD wrote:
> > From: Anthony PERARD
> >
> > Whenever a Xen block device is detach via xenstore, the image
> > associated with it remained open by the backend QEMU and an error is
> > logged:
> > qemu-system-i386: failed to destroy drive: Node xvdz-qcow2 is in use
> >
> > This happened since object_unparent() doesn't immediately frees the
> > object and thus keep a reference to the node we are trying to free.
> > The reference is hold by the "drive" property and the call
> > xen_block_drive_destroy() fails.
> >
> > In order to fix that, we call drain_call_rcu() to run the callback
> > setup by bus_remove_child() via object_unparent().
> >
> > Fixes: 2d24a6466154 ("device-core: use RCU for list of children of a bus")
> >
> > Signed-off-by: Anthony PERARD
> > ---
> > CCing people whom introduced/reviewed the change to use RCU to give
> > them a chance to say if the change is fine.
>
> If nothing else works then I guess it's okay, but why can't you do the
> xen_block_drive_destroy from e.g. an unrealize callback?
I'm not sure if that's possible.
xen_block_device_create/xen_block_device_destroy() is supposed to be
equivalent to do those qmp commands:
blockdev-add node-name=xvdz-qcow2 driver=qcow2
file={"driver":"file","filename":"disk.qcow2","locking":"off"}
device_add id=xvdz driver=xen-disk vdev=xvdz drive=xvdz-qcow2
But I tried to add a call xen_block_drive_destroy from
xen_block_unrealize, but that still is called too early, it's called
before object_property_del_all() which would delete "drive" and call
release_drive() which would free the node.
So, no, I don't think we can use an unrealized callback.
I though of trying to delete the "drive" property ahead of calling
object_unparent() but I didn't figure out how to do so and it's maybe
not possible.
So either drain_call_rcu or adding call_rcu(xen_block_drive_destroy)
seems to be the way, but since xen_block_drive_destroy uses
qmp_blockdev_del, it seems better to drain_call_rcu.
Cheers,
--
Anthony PERARD
[PATCH] xen-block: Fix removal of backend instance via xenstore
From: Anthony PERARD
Whenever a Xen block device is detach via xenstore, the image
associated with it remained open by the backend QEMU and an error is
logged:
qemu-system-i386: failed to destroy drive: Node xvdz-qcow2 is in use
This happened since object_unparent() doesn't immediately frees the
object and thus keep a reference to the node we are trying to free.
The reference is hold by the "drive" property and the call
xen_block_drive_destroy() fails.
In order to fix that, we call drain_call_rcu() to run the callback
setup by bus_remove_child() via object_unparent().
Fixes: 2d24a6466154 ("device-core: use RCU for list of children of a bus")
Signed-off-by: Anthony PERARD
---
CCing people whom introduced/reviewed the change to use RCU to give
them a chance to say if the change is fine.
---
hw/block/xen-block.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index a3b69e27096f..fe5f828e2d25 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -972,6 +972,15 @@ static void xen_block_device_destroy(XenBackendInstance
*backend,
object_unparent(OBJECT(xendev));
+/*
+ * Drall all pending RCU callbacks as object_unparent() frees `xendev'
+ * in a RCU callback.
+ * And due to the property "drive" still existing in `xendev', we
+ * cann't destroy the XenBlockDrive associated with `xendev' with
+ * xen_block_drive_destroy() below.
+ */
+drain_call_rcu();
+
if (iothread) {
xen_block_iothread_destroy(iothread, errp);
if (*errp) {
--
Anthony PERARD
Re: [PATCH v2] xen: rework pci_piix3_xen_ide_unplug
On Tue, Oct 27, 2020 at 01:33:32PM -0400, John Snow wrote: > On 10/27/20 11:40 AM, Anthony PERARD wrote: > > From: Anthony PERARD > > > > This is to allow IDE disks to be unplugged when adding to QEMU via: > > -drive file=/root/disk_file,if=none,id=ide-disk0,format=raw > > -device ide-hd,drive=ide-disk0,bus=ide.0,unit=0 > > > > as the current code only works for disk added with: > > -drive file=/root/disk_file,if=ide,index=0,media=disk,format=raw > > > > Since the code already have the IDE controller as `dev`, we don't need > > to use the legacy DriveInfo to find all the drive we want to unplug. > > We can simply use `blk` from the controller, as it kind of was already > > assume to be the same, by setting it to NULL. > > > > Signed-off-by: Anthony PERARD > > Acked-by: John Snow > > Do you need me to send a PR for this? No, that's fine, I can do the PR since it's all xen code. Thanks, -- Anthony PERARD
[PATCH v2] xen: rework pci_piix3_xen_ide_unplug
From: Anthony PERARD
This is to allow IDE disks to be unplugged when adding to QEMU via:
-drive file=/root/disk_file,if=none,id=ide-disk0,format=raw
-device ide-hd,drive=ide-disk0,bus=ide.0,unit=0
as the current code only works for disk added with:
-drive file=/root/disk_file,if=ide,index=0,media=disk,format=raw
Since the code already have the IDE controller as `dev`, we don't need
to use the legacy DriveInfo to find all the drive we want to unplug.
We can simply use `blk` from the controller, as it kind of was already
assume to be the same, by setting it to NULL.
Signed-off-by: Anthony PERARD
---
v2: coding style
CC: Paul Durrant
CC: Stefano Stabellini
---
hw/ide/piix.c | 27 +--
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index b402a936362b..b9860e35a5c4 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -164,30 +164,29 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
**errp)
int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
{
PCIIDEState *pci_ide;
-DriveInfo *di;
int i;
IDEDevice *idedev;
+IDEBus *idebus;
+BlockBackend *blk;
pci_ide = PCI_IDE(dev);
for (i = aux ? 1 : 0; i < 4; i++) {
-di = drive_get_by_index(IF_IDE, i);
-if (di != NULL && !di->media_cd) {
-BlockBackend *blk = blk_by_legacy_dinfo(di);
-DeviceState *ds = blk_get_attached_dev(blk);
+idebus = _ide->bus[i / 2];
+blk = idebus->ifs[i % 2].blk;
-blk_drain(blk);
-blk_flush(blk);
-
-if (ds) {
-blk_detach_dev(blk, ds);
-}
-pci_ide->bus[di->bus].ifs[di->unit].blk = NULL;
+if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
if (!(i % 2)) {
-idedev = pci_ide->bus[di->bus].master;
+idedev = idebus->master;
} else {
-idedev = pci_ide->bus[di->bus].slave;
+idedev = idebus->slave;
}
+
+blk_drain(blk);
+blk_flush(blk);
+
+blk_detach_dev(blk, DEVICE(idedev));
+idebus->ifs[i % 2].blk = NULL;
idedev->conf.blk = NULL;
monitor_remove_blk(blk);
blk_unref(blk);
--
Anthony PERARD
[PATCH] xen: rework pci_piix3_xen_ide_unplug
This is to allow IDE disks to be unplugged when adding to QEMU via:
-drive file=/root/disk_file,if=none,id=ide-disk0,format=raw
-device ide-hd,drive=ide-disk0,bus=ide.0,unit=0
as the current code only works for disk added with:
-drive file=/root/disk_file,if=ide,index=0,media=disk,format=raw
Since the code already have the IDE controller as `dev`, we don't need
to use the legacy DriveInfo to find all the drive we want to unplug.
We can simply use `blk` from the controller, as it kind of was already
assume to be the same, by setting it to NULL.
Signed-off-by: Anthony PERARD
---
hw/ide/piix.c | 27 +--
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index b402a936362b..16fcbe58d6fe 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -164,30 +164,29 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
**errp)
int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
{
PCIIDEState *pci_ide;
-DriveInfo *di;
int i;
IDEDevice *idedev;
+IDEBus *idebus;
+BlockBackend *blk;
pci_ide = PCI_IDE(dev);
for (i = aux ? 1 : 0; i < 4; i++) {
-di = drive_get_by_index(IF_IDE, i);
-if (di != NULL && !di->media_cd) {
-BlockBackend *blk = blk_by_legacy_dinfo(di);
-DeviceState *ds = blk_get_attached_dev(blk);
+idebus = _ide->bus[i / 2];
+blk = idebus->ifs[i % 2].blk;
-blk_drain(blk);
-blk_flush(blk);
-
-if (ds) {
-blk_detach_dev(blk, ds);
-}
-pci_ide->bus[di->bus].ifs[di->unit].blk = NULL;
+if (blk && idebus->ifs[i%2].drive_kind != IDE_CD) {
if (!(i % 2)) {
-idedev = pci_ide->bus[di->bus].master;
+idedev = idebus->master;
} else {
-idedev = pci_ide->bus[di->bus].slave;
+idedev = idebus->slave;
}
+
+blk_drain(blk);
+blk_flush(blk);
+
+blk_detach_dev(blk, DEVICE(idedev));
+idebus->ifs[i % 2].blk = NULL;
idedev->conf.blk = NULL;
monitor_remove_blk(blk);
blk_unref(blk);
--
Anthony PERARD
Re: [PATCH for-5.0] xen-block: Fix uninitialized variable
On Mon, Apr 06, 2020 at 06:50:41PM +0200, Philippe Mathieu-Daudé wrote:
> On 4/6/20 6:42 PM, Anthony PERARD wrote:
> > Since 7f5d9b206d1e ("object-add: don't create return value if
> > failed"), qmp_object_add() don't write any value in 'ret_data', thus
> > has random data. Then qobject_unref() fails and abort().
> >
> > Fix by initialising 'ret_data' properly.
>
> Or move qobject_unref() after the error check?
>
> -- >8 --
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 07bb32e22b..f3f1cbef65 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -869,7 +869,6 @@ static XenBlockIOThread *xen_block_iothread_create(const
> char *id,
> qdict_put_str(opts, "id", id);
> qmp_object_add(opts, _data, _err);
> qobject_unref(opts);
> -qobject_unref(ret_data);
>
> if (local_err) {
> error_propagate(errp, local_err);
> @@ -878,6 +877,7 @@ static XenBlockIOThread *xen_block_iothread_create(const
> char *id,
> g_free(iothread);
> return NULL;
> }
> +qobject_unref(ret_data);
That won't help, qmp_object_add() doesn't change the value of ret_data
at all. The other users of qmp_object_add() passes an initialised
'ret_data', so we should do the same I think.
Thanks,
--
Anthony PERARD
[PATCH for-5.0] xen-block: Fix uninitialized variable
Since 7f5d9b206d1e ("object-add: don't create return value if
failed"), qmp_object_add() don't write any value in 'ret_data', thus
has random data. Then qobject_unref() fails and abort().
Fix by initialising 'ret_data' properly.
Fixes: 5f07c4d60d09 ("qapi: Flatten object-add")
Signed-off-by: Anthony PERARD
---
hw/block/xen-block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 07bb32e22b51..99cb4c67cb09 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -860,7 +860,7 @@ static XenBlockIOThread *xen_block_iothread_create(const
char *id,
XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1);
Error *local_err = NULL;
QDict *opts;
-QObject *ret_data;
+QObject *ret_data = NULL;
iothread->id = g_strdup(id);
--
Anthony PERARD
[PATCH v2 for-5.0] xen-block: Fix double qlist remove and request leak
Commit a31ca6801c02 ("qemu/queue.h: clear linked list pointers on
remove") revealed that a request was removed twice from a list, once
in xen_block_finish_request() and a second time in
xen_block_release_request() when both function are called from
xen_block_complete_aio(). But also, the `requests_inflight' counter is
decreased twice, and thus became negative.
This is a bug that was introduced in bfd0d6366043, where a `finished'
list was removed.
That commit also introduced a leak of request in xen_block_do_aio().
That function calls xen_block_finish_request() but the request is
never released after that.
To fix both issue, we do two changes:
- we squash finish_request() and release_request() together as we want
to remove a request from 'inflight' list to add it to 'freelist'.
- before releasing a request, we need to let now the result to the
other end, thus we should call xen_block_send_response() before
releasing a request.
The first change fix the double QLIST_REMOVE() as we remove the extra
call. The second change makes the leak go away because if we want to
call finish_request(), we need to call a function that do all of
finish, send response, and release.
Fixes: bfd0d6366043 ("xen-block: improve response latency")
Signed-off-by: Anthony PERARD
---
hw/block/dataplane/xen-block.c | 48 --
1 file changed, 16 insertions(+), 32 deletions(-)
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 288a87a814ad..5f8f15778ba5 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -64,6 +64,8 @@ struct XenBlockDataPlane {
AioContext *ctx;
};
+static int xen_block_send_response(XenBlockRequest *request);
+
static void reset_request(XenBlockRequest *request)
{
memset(>req, 0, sizeof(request->req));
@@ -115,23 +117,26 @@ static XenBlockRequest
*xen_block_start_request(XenBlockDataPlane *dataplane)
return request;
}
-static void xen_block_finish_request(XenBlockRequest *request)
+static void xen_block_complete_request(XenBlockRequest *request)
{
XenBlockDataPlane *dataplane = request->dataplane;
-QLIST_REMOVE(request, list);
-dataplane->requests_inflight--;
-}
+if (xen_block_send_response(request)) {
+Error *local_err = NULL;
-static void xen_block_release_request(XenBlockRequest *request)
-{
-XenBlockDataPlane *dataplane = request->dataplane;
+xen_device_notify_event_channel(dataplane->xendev,
+dataplane->event_channel,
+_err);
+if (local_err) {
+error_report_err(local_err);
+}
+}
QLIST_REMOVE(request, list);
+dataplane->requests_inflight--;
reset_request(request);
request->dataplane = dataplane;
QLIST_INSERT_HEAD(>freelist, request, list);
-dataplane->requests_inflight--;
}
/*
@@ -246,7 +251,6 @@ static int xen_block_copy_request(XenBlockRequest *request)
}
static int xen_block_do_aio(XenBlockRequest *request);
-static int xen_block_send_response(XenBlockRequest *request);
static void xen_block_complete_aio(void *opaque, int ret)
{
@@ -286,7 +290,6 @@ static void xen_block_complete_aio(void *opaque, int ret)
}
request->status = request->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY;
-xen_block_finish_request(request);
switch (request->req.operation) {
case BLKIF_OP_WRITE:
@@ -306,17 +309,8 @@ static void xen_block_complete_aio(void *opaque, int ret)
default:
break;
}
-if (xen_block_send_response(request)) {
-Error *local_err = NULL;
-xen_device_notify_event_channel(dataplane->xendev,
-dataplane->event_channel,
-_err);
-if (local_err) {
-error_report_err(local_err);
-}
-}
-xen_block_release_request(request);
+xen_block_complete_request(request);
if (dataplane->more_work) {
qemu_bh_schedule(dataplane->bh);
@@ -420,8 +414,8 @@ static int xen_block_do_aio(XenBlockRequest *request)
return 0;
err:
-xen_block_finish_request(request);
request->status = BLKIF_RSP_ERROR;
+xen_block_complete_request(request);
return -1;
}
@@ -575,17 +569,7 @@ static bool xen_block_handle_requests(XenBlockDataPlane
*dataplane)
break;
};
-if (xen_block_send_response(request)) {
-Error *local_err = NULL;
-
-xen_device_notify_event_channel(dataplane->xendev,
-dataplane->event_channel,
-_err);
-if (local_err) {
-error_report_err(local_err);
-}
-}
-xen_block_release_reques
Re: [PATCH for-5.0] xen-block: Fix double qlist remove
On Thu, Apr 02, 2020 at 03:27:22PM +0100, Paul Durrant wrote:
> > -Original Message-
> > From: Anthony PERARD
> > Sent: 02 April 2020 14:08
> > To: qemu-de...@nongnu.org
> > Cc: qemu-sta...@nongnu.org; Anthony PERARD ;
> > Stefano Stabellini
> > ; Paul Durrant ; Stefan Hajnoczi
> > ; Kevin
> > Wolf ; Max Reitz ;
> > xen-de...@lists.xenproject.org; qemu-
> > bl...@nongnu.org
> > Subject: [PATCH for-5.0] xen-block: Fix double qlist remove
> >
> > Commit a31ca6801c02 ("qemu/queue.h: clear linked list pointers on
> > remove") revealed that a request was removed twice from a list, once
> > in xen_block_finish_request() and a second time in
> > xen_block_release_request() when both function are called from
> > xen_block_complete_aio(). But also, the `requests_inflight' counter is
> > decreased twice, and thus became negative.
> >
> > This is a bug that was introduced in bfd0d6366043, where a `finished'
> > list was removed.
> >
> > This patch simply re-add the `finish' parameter of
> > xen_block_release_request() so that we can distinguish when we need to
> > remove a request from the inflight list and when not.
> >
> > Fixes: bfd0d6366043 ("xen-block: improve response latency")
> > Signed-off-by: Anthony PERARD
>
> It looks to me like it would just be more straightforward to simply drop the
> QLIST_REMOVE and requests_inflight-- from
> xen_block_release_request() and simply insist that xen_block_finish_request()
> is called in all cases (which I think means adding one
> extra call to it in xen_block_handle_requests()).
I'm thinking of going further than that. I've notice another bug, in
case of error in xen_block_do_aio(), xen_block_finish_request() is
called without ever calling send_response() or release_request(). I
think that mean a leak of request.
So, I'm thinking of creating a function that would do finish_request(),
send_response(), release_request(), has I believe those operations needs
to be done together anyway.
I'll rework the patch.
--
Anthony PERARD
[PATCH for-5.0] xen-block: Fix double qlist remove
Commit a31ca6801c02 ("qemu/queue.h: clear linked list pointers on
remove") revealed that a request was removed twice from a list, once
in xen_block_finish_request() and a second time in
xen_block_release_request() when both function are called from
xen_block_complete_aio(). But also, the `requests_inflight' counter is
decreased twice, and thus became negative.
This is a bug that was introduced in bfd0d6366043, where a `finished'
list was removed.
This patch simply re-add the `finish' parameter of
xen_block_release_request() so that we can distinguish when we need to
remove a request from the inflight list and when not.
Fixes: bfd0d6366043 ("xen-block: improve response latency")
Signed-off-by: Anthony PERARD
---
hw/block/dataplane/xen-block.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 288a87a814ad..6cc089fc561f 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -123,15 +123,19 @@ static void xen_block_finish_request(XenBlockRequest
*request)
dataplane->requests_inflight--;
}
-static void xen_block_release_request(XenBlockRequest *request)
+static void xen_block_release_request(XenBlockRequest *request, bool finish)
{
XenBlockDataPlane *dataplane = request->dataplane;
-QLIST_REMOVE(request, list);
+if (!finish) {
+QLIST_REMOVE(request, list);
+}
reset_request(request);
request->dataplane = dataplane;
QLIST_INSERT_HEAD(>freelist, request, list);
-dataplane->requests_inflight--;
+if (!finish) {
+dataplane->requests_inflight--;
+}
}
/*
@@ -316,7 +320,7 @@ static void xen_block_complete_aio(void *opaque, int ret)
error_report_err(local_err);
}
}
-xen_block_release_request(request);
+xen_block_release_request(request, true);
if (dataplane->more_work) {
qemu_bh_schedule(dataplane->bh);
@@ -585,7 +589,7 @@ static bool xen_block_handle_requests(XenBlockDataPlane
*dataplane)
error_report_err(local_err);
}
}
-xen_block_release_request(request);
+xen_block_release_request(request, false);
continue;
}
--
Anthony PERARD
Re: [PATCH 08/20] hw/xen/xen_pt_load_rom: Remove unused includes
On Mon, Oct 14, 2019 at 03:29:42PM +0100, Paul Durrant wrote: > On Mon, 14 Oct 2019 at 15:27, Philippe Mathieu-Daudé > wrote: > > > > xen_pt_load_rom.c does not use any of these includes, remove them. > > > > Signed-off-by: Philippe Mathieu-Daudé > > Reviewed-by: Paul Durrant Hi, I've added this patch to a pull requests for the xen. Cheers, -- Anthony PERARD
Re: [PATCH] xen-bus/block: explicitly assign event channels to an AioContext
On Wed, Jan 29, 2020 at 10:22:14PM +, Julien Grall wrote: > Hi Anthony, > > On 19/12/2019 17:11, Anthony PERARD wrote: > > On Mon, Dec 16, 2019 at 02:34:51PM +, Paul Durrant wrote: > > > It is not safe to close an event channel from the QEMU main thread when > > > that channel's poller is running in IOThread context. > > > > > > This patch adds a new xen_device_set_event_channel_context() function > > > to explicitly assign the channel AioContext, and modifies > > > xen_device_bind_event_channel() to initially assign the channel's poller > > > to the QEMU main thread context. The code in xen-block's dataplane is > > > then modified to assign the channel to IOThread context during > > > xen_block_dataplane_start() and de-assign it during in > > > xen_block_dataplane_stop(), such that the channel is always assigned > > > back to main thread context before it is closed. aio_set_fd_handler() > > > already deals with all the necessary synchronization when moving an fd > > > between AioContext-s so no extra code is needed to manage this. > > > > > > Reported-by: Julien Grall > > > Signed-off-by: Paul Durrant > > > > Reviewed-by: Anthony PERARD > > I can't find the patch in QEMU upstream. Are we missing any ack/review for > this patch? No, I just need to prepare a pull request. It's in my list of patch for upstream, so there will be a pull request at some point before the next QEMU release. Cheers, -- Anthony PERARD
Re: [PATCH] xen-bus/block: explicitly assign event channels to an AioContext
On Mon, Dec 16, 2019 at 02:34:51PM +, Paul Durrant wrote: > It is not safe to close an event channel from the QEMU main thread when > that channel's poller is running in IOThread context. > > This patch adds a new xen_device_set_event_channel_context() function > to explicitly assign the channel AioContext, and modifies > xen_device_bind_event_channel() to initially assign the channel's poller > to the QEMU main thread context. The code in xen-block's dataplane is > then modified to assign the channel to IOThread context during > xen_block_dataplane_start() and de-assign it during in > xen_block_dataplane_stop(), such that the channel is always assigned > back to main thread context before it is closed. aio_set_fd_handler() > already deals with all the necessary synchronization when moving an fd > between AioContext-s so no extra code is needed to manage this. > > Reported-by: Julien Grall > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD > Tested against an HVM debian guest with a QCOW2 image as system disk, and > as a hot-plugged/unplgged secondary disk. And I've run an osstest flight with the patch. Thanks, -- Anthony PERARD
Re: [RFC v5 031/126] xen: introduce ERRP_AUTO_PROPAGATE
On Fri, Oct 11, 2019 at 07:04:17PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -915,15 +903,15 @@ static void xen_block_device_create(XenBackendInstance
> *backend,
> goto fail;
> }
>
> -drive = xen_block_drive_create(vdev, device_type, opts, _err);
> +drive = xen_block_drive_create(vdev, device_type, opts, errp);
> if (!drive) {
> -error_propagate_prepend(errp, local_err, "failed to create drive: ");
> +error_prepend(errp, "failed to create drive: ");
> goto fail;
> }
>
> -iothread = xen_block_iothread_create(vdev, _err);
> -if (local_err) {
> -error_propagate_prepend(errp, local_err,
> +iothread = xen_block_iothread_create(vdev, errp);
> +if (*errp) {
> +error_prepend(errp,
> "failed to create iothread: ");
These two line could be joined now.
> goto fail;
> }
And there are more indentation issues like that in the patch. It would be
nice to fix, but otherwise the patch looks fine:
Acked-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH] xen-block: treat XenbusStateUnknown the same as XenbusStateClosed
On Wed, Sep 18, 2019 at 12:57:02PM +0100, Paul Durrant wrote: > When a frontend gracefully disconnects from an offline backend, it will > set its own state to XenbusStateClosed. The code in xen-block.c correctly > deals with this and sets the backend into XenbusStateClosed. Unfortunately > it is possible for toolstack to actually delete the frontend area > before the state key has been read, leading to an apparent frontend state > of XenbusStateUnknown. This prevents the backend state from transitioning > to XenbusStateClosed and hence leaves it limbo. > > This patch simply treats a frontend state of XenbusStateUnknown the same > as XenbusStateClosed, which will unblock the backend in these circumstances. > > Reported-by: Mark Syms > Signed-off-by: Paul Durrant Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH] xen-block: support feature-large-sector-size
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 > > --- > > Cc: Stefano Stabellini > > Cc: Anthony Perard > > Cc: Stefan Hajnoczi > > Cc: Kevin Wolf > > Cc: Max Reitz > > --- > > 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.per...@citrix.com/20190624153257.20163-2-anthony.per...@citrix.com/ I guess I need to start sending an email every time I've added a patch to my queue. Cheers, -- Anthony PERARD
Re: [Qemu-block] [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
> ---
> 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",
> + _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
Thanks,
--
Anthony PERARD
Re: [Qemu-block] [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
On Wed, Apr 10, 2019 at 04:20:05PM +0100, Paul Durrant wrote: > > -Original Message- > > From: Anthony PERARD [mailto:anthony.per...@citrix.com] > > Sent: 10 April 2019 13:57 > > To: Paul Durrant > > Cc: qemu-de...@nongnu.org; qemu-block@nongnu.org; > > xen-de...@lists.xenproject.org; Stefano Stabellini > > ; Stefan Hajnoczi ; Kevin Wolf > > ; Max > > Reitz > > Subject: Re: [PATCH 2/3] xen-bus: allow AioContext to be specified for each > > event channel > > > > On Mon, Apr 08, 2019 at 04:16:16PM +0100, Paul Durrant wrote: > > > This patch adds an AioContext parameter to xen_device_bind_event_channel() > > > and then uses aio_set_fd_handler() to set the callback rather than > > > qemu_set_fd_handler(). > > > > > > Signed-off-by: Paul Durrant > > > --- > > > @@ -943,6 +944,7 @@ static void xen_device_event(void *opaque) > > > } > > > > > > XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev, > > > + AioContext *ctx, > > > unsigned int port, > > > XenEventHandler handler, > > > void *opaque, Error > > > **errp) > > > @@ -968,8 +970,9 @@ XenEventChannel > > > *xen_device_bind_event_channel(XenDevice *xendev, > > > channel->handler = handler; > > > channel->opaque = opaque; > > > > > > -qemu_set_fd_handler(xenevtchn_fd(channel->xeh), xen_device_event, > > > NULL, > > > -channel); > > > +channel->ctx = ctx; > > > +aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false, > > > + xen_device_event, NULL, NULL, channel); > > > > I wonder if the `'is_external' parameter of aio_set_fd_handler shoud be > > `true' here, instead. That flag seems to be used when making a snapshot > > of a blockdev, for example. > > > > That was introduced by: > > dca21ef23ba48f6f1428c59f295a857e5dc203c8^..c07bc2c1658fffeee08eb46402b2f66d55b07586 > > > > What do you think? > > Interesting. I admit I was merely transcribing what qemu_set_fd_handler() > passes without really looking into the values. Looking at the arguments that > virtio-blk passes to aio_set_event_notifier() though, and what 'is_external' > means, it would appear that setting it to true is probably the right thing to > do. Do you want me to send a v2 of the series or can you fix it up? I'll fix that up. Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH 3/3] xen-bus / xen-block: add support for event channel polling
On Mon, Apr 08, 2019 at 04:16:17PM +0100, Paul Durrant wrote: > This patch introduces a poll callback for event channel fd-s and uses > this to invoke the channel callback function. > > To properly support polling, it is necessary for the event channel callback > function to return a boolean saying whether it has done any useful work or > not. Thus xen_block_dataplane_event() is modified to directly invoke > xen_block_handle_requests() and the latter only returns true if it actually > processes any requests. This also means that the call to qemu_bh_schedule() > is moved into xen_block_complete_aio(), which is more intuitive since the > only reason for doing a deferred poll of the shared ring should be because > there were previously insufficient resources to fully complete a previous > poll. > > Signed-off-by: Paul Durrant > --- Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
On Mon, Apr 08, 2019 at 04:16:16PM +0100, Paul Durrant wrote: > This patch adds an AioContext parameter to xen_device_bind_event_channel() > and then uses aio_set_fd_handler() to set the callback rather than > qemu_set_fd_handler(). > > Signed-off-by: Paul Durrant > --- > @@ -943,6 +944,7 @@ static void xen_device_event(void *opaque) > } > > XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev, > + AioContext *ctx, > unsigned int port, > XenEventHandler handler, > void *opaque, Error **errp) > @@ -968,8 +970,9 @@ XenEventChannel *xen_device_bind_event_channel(XenDevice > *xendev, > channel->handler = handler; > channel->opaque = opaque; > > -qemu_set_fd_handler(xenevtchn_fd(channel->xeh), xen_device_event, NULL, > -channel); > +channel->ctx = ctx; > +aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false, > + xen_device_event, NULL, NULL, channel); I wonder if the `'is_external' parameter of aio_set_fd_handler shoud be `true' here, instead. That flag seems to be used when making a snapshot of a blockdev, for example. That was introduced by: dca21ef23ba48f6f1428c59f295a857e5dc203c8^..c07bc2c1658fffeee08eb46402b2f66d55b07586 What do you think? -- Anthony PERARD
Re: [Qemu-block] [PATCH 1/3] xen-bus: use a separate fd for each event channel
On Mon, Apr 08, 2019 at 04:16:15PM +0100, Paul Durrant wrote: > To better support use of IOThread-s it will be necessary to be able to set > the AioContext for each XenEventChannel and hence it is necessary to open a > separate handle to libxenevtchan for each channel. > > This patch stops using NotifierList for event channel callbacks, replacing > that construct by a list of complete XenEventChannel structures. Each of > these now has a xenevtchn_handle pointer in place of the single pointer > previously held in the XenDevice structure. The individual handles are > opened/closed in xen_device_bind/unbind_event_channel(), replacing the > single open/close in xen_device_realize/unrealize(). > > NOTE: This patch does not add an AioContext parameter to > xen_device_bind_event_channel(). That will be done in a subsequent > patch. > > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD There are a few places were I would have like to add an assert, but they can't be compiled-out in QEMU :-(. Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH v3] xen-block: scale sector based quantities correctly
On Mon, Apr 01, 2019 at 01:17:19PM +0100, Paul Durrant wrote: > The Xen blkif protocol requires that sector based quantities should be > interpreted strictly as multiples of 512 bytes. Specifically: > > "first_sect and last_sect in blkif_request_segment, as well as > sector_number in blkif_request, are always expressed in 512-byte units." > > Commit fcab2b464e06 "xen: add header and build dataplane/xen-block.c" > incorrectly modified behaviour to use the block device logical_block_size > property as the scale, instead of correctly shifting values by the > hardcoded BDRV_SECTOR_BITS (and hence scaling them to 512 byte units). > This patch undoes that change and restores compliance with the spec. > > Furthermore, this patch also restores the original xen_disk behaviour > of advertizing a hardcoded 'sector-size' value of 512 in xenstore and > scaling 'sectors' accordingly. The realize() method is also modified to > fail if logical_block_size is set to anything other than 512. > > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [Xen-devel] [PATCH v2 0/2] xen-block: fix sector size confusion
On Wed, Mar 27, 2019 at 08:32:28PM +, Paul Durrant wrote: > > -Original Message- > > From: Andrew Cooper > > Sent: 27 March 2019 18:20 > > To: Paul Durrant ; xen-de...@lists.xenproject.org; > > qemu-block@nongnu.org; > > qemu-de...@nongnu.org > > Cc: Kevin Wolf ; Stefano Stabellini > > ; Max Reitz > > ; Stefan Hajnoczi ; Anthony Perard > > > > Subject: Re: [Xen-devel] [PATCH v2 0/2] xen-block: fix sector size confusion > > > > On 27/03/2019 17:32, Paul Durrant wrote: > > > The Xen blkif protocol is confusing but discussion with the maintainer > > > has clarified that sector based quantities in requests and the 'sectors' > > > value advertized in xenstore should always be in terms of 512-byte > > > units and not the advertised logical 'sector-size' value. > > > > > > This series fixes xen-block to adhere to the spec. > > > > I thought we agreed that hardcoding things to 512 bytes was the wrong > > thing to do. > > To some extent we decided it was the *only* thing to do. > > > > > I was expecting something like: > > > > 1) Clarify the spec with the intended meaning, (which is what some > > implementations actually use already) and wont cripple 4k datapaths. > > 2) Introduce a compatibility key for "I don't rely on sector-size being > > 512", which fixed implementations should advertise. > > 3) Specify that because of bugs in the spec which got out into the wild, > > drivers which don't find the key being advertised by the other end > > should emulate sector-size=512 for compatibility with broken > > implementations. > > Yes, that's how we are going to fix things. > > > > > Whatever the eventual way out, the first thing which needs to happen is > > an update to the spec, before actions are taken to alter existing > > implementations. > > Well the implementation is currently wrong w.r.t. the spec and these patches > fix that. As long as sector-size remains at 512 then no existing frontend > should break, so I guess you could argue that patch #2 should also make sure > that sector-size is also 512... but that is not yet in the spec. > I guess I'm ok to defer patch #2 until a revised spec. is agreed, but the > ship has already sailed as far as patch #1 goes. > > Anthony, thoughts? So QEMU used to always set "sector-size" to 512, and used that for request. The new implementation (not released yet) doesn't do that anymore, and may set "sector-size" to a different value and used that for requests. patch #1 is one way to fix the requests (and avoid regression) and more clearly spell out the weird thing about the spec. I also think patch #2 is too soon and should point to a commit in xen.git instead of a thread on xen-devel. In the meantime, we should probably set "sector-size" to 512, like QEMU used to do anyway, with a comment about the fact that different implementations uses sector-size differently and a value of 512 would work fine. -- Anthony PERARD
Re: [Qemu-block] [PATCH] xen-block: only advertize discard to the frontend when it is enabled...
On Wed, Mar 20, 2019 at 02:28:25PM +, Paul Durrant wrote: > ...and properly enable it when synthesizing a drive. > > The Xen toolstack sets 'discard-enable' to '1' in xenstore when it wants > to enable discard on a specified image. The code in > xen_block_driver_create() correctly parses this and uses it to set typo: It's xen_block_drive_create (s/driver/drive/), otherwise my IDE can't find it :-(. > 'discard' to 'unamp' for the file_layer, but fails to do the same for the Looks like s/unamp/unmap/ > driver_layer (which effectively disables it). Meanwhile the code in > xen_block_realize() advertizes discard support to the frontend in the > default case (because conf->discard_granularity defaults to -1), even when That doesn't match the code I'm reading, before the patch apply. if (discard_granularity > 0) feature-discard=1 Nothing seems to set discard_granularity, so it keeps it's default to -1, so wait, discard_granularity is unsigned :-( The description is fine then. > the underlying image may not handle it. > > This patch adds the missing option to the driver_layer in > xen_block_driver_create() and checks whether BDRV_O_UNMAP is actually > set on the block device before advertizing discard to the frontend. > In the case that discard is supported it also makes sure that the > granularity is set to the physical block size. > > Signed-off-by: Paul Durrant With the two typos fixed (which I can try to remember to do on commit): Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [Qemu-devel] [PATCH] xen-block: Replace qdict_put_obj() by qdict_put() where appropriate
On Thu, Mar 14, 2019 at 08:04:00PM +0100, Markus Armbruster wrote: > Kevin Wolf writes: > > > Am 13.03.2019 um 18:44 hat Markus Armbruster geschrieben: > >> Patch created mechanically by rerunning: > >> > >> $ spatch --sp-file scripts/coccinelle/qobject.cocci \ > >> --macro-file scripts/cocci-macro-file.h \ > >> --dir hw/block --in-place > >> > >> Signed-off-by: Markus Armbruster > > > > Reviewed-by: Kevin Wolf > > Thanks! > > > Which tree should this go through? The Xen one? > > Fine with me. I could also include it in a "miscellaneous cleanup" pull > request along with other cleanup patches I got in flight. Markus, I don't have any other Xen patches, so could you include this one in your pull request? Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH] xen-block: stop leaking memory in xen_block_drive_create()
On Tue, Feb 19, 2019 at 04:36:28PM +, Paul Durrant wrote:
> > The locally allocated QDict-s need to be freed. ('file_layer' will be
> > freed implicitly since it is added as an object to 'driver_layer').
> >
> > Spotted by Coverity: CID 1398649
> >
> > While in the neighbourhood free 'driver' and 'filename' as soon as they
> > are
> > added to the QDicts. Freeing after the 'done' label doesn't make that much
> > sense as, if the error path jumps to that label, the values would be NULL
> > anyway.
> >
> > This patch also makes that more obvious by taking the error path if
> > 'params' is NULL and then asserting that both driver and filename are
> > non-NULL in the normal path.
> >
> > Reported-by: Peter Maydell
> > Signed-off-by: Paul Durrant
Acked-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [Qemu-block] [PATCH 3/3] xen-block: report error condition from vbd_name_to_disk()
On Fri, Feb 15, 2019 at 04:25:33PM +, Paul Durrant wrote: > The function needs to make sure it is passed a valid disk name. This is > easily done by making sure that the parsing loop results in a non-zero > value. > > Spotted by Coverity: CID 1398640 > > Reported-by: Peter Maydell > Signed-off-by: Paul Durrant Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH 2/3] xen-block: remove redundant assignment
On Fri, Feb 15, 2019 at 04:25:32PM +, Paul Durrant wrote: > The assignment to 'p' is unnecessary as the code will either goto 'invalid' > or p will get overwritten. > > Spotted by Coverity: CID 1398638 > > Reported-by: Peter Maydell > Signed-off-by: Paul Durrant Acked-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [Qemu-devel] [PATCH 1/3] dataplane/xen-block: remove dead code
On Fri, Feb 15, 2019 at 09:38:59PM +0100, Philippe Mathieu-Daudé wrote: > On 2/15/19 5:25 PM, Paul Durrant wrote: > > The if() statement is clearly bogus (dead code which should have been > > cleaned up when grant mapping was removed). > > "... was removed in 06454c24ad)." Actually, it looks like c6025bd197 should have remove the if statement. > > > > Spotted by Coverity: CID 1398635 > > > > While in the neighbourhood, add a missing 'fall through' annotation. > > > > Reported-by: Peter Maydell > > Signed-off-by: Paul Durrant Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH v3] xen-block: handle resize callback
On Thu, Jan 31, 2019 at 03:33:16PM +, Paul Durrant wrote: > Some frontend drivers will handle dynamic resizing of PV disks, so set up > the BlockDevOps resize_cb() method during xen_block_realize() to allow > this to be done. > > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH v2] xen-block: handle resize callback
On Thu, Jan 31, 2019 at 03:22:18PM +, Paul Durrant wrote: > > -Original Message- > > From: Anthony PERARD [mailto:anthony.per...@citrix.com] > > Sent: 31 January 2019 15:21 > > To: Paul Durrant > > Cc: qemu-de...@nongnu.org; qemu-block@nongnu.org; xen- > > de...@lists.xenproject.org; Stefan Hajnoczi ; Stefano > > Stabellini ; Kevin Wolf ; Max > > Reitz > > Subject: Re: [PATCH v2] xen-block: handle resize callback > > > > On Wed, Jan 30, 2019 at 04:19:48PM +, Paul Durrant wrote: > > > Some frontend drivers will handle dynamic resizing of PV disks, so set > > up > > > the BlockDevOps resize_cb() method during xen_block_realize() to allow > > > this to be done. > > > > > > Signed-off-by: Paul Durrant > > > --- > > > > > > > +/* > > > + * Mimic the behaviour of Linux xen-blkback and re-write the state > > > + * to trigger the frontend watch. > > > + */ > > > +xen_device_backend_set_state(xendev, backend_state); > > > > :(, that function doesn't write the state again if it hasn't changed. > > So in my testing, Linux never did anything. > > Gah! I forgot about that. Alright, it's going to have to be a bit more crude. more crude > Yes, I tried to ignore the check in _set_state and end-up with an infinit loop. -- Anthony PERARD
Re: [Qemu-block] [PATCH] xen-block: handle resize callback
On Wed, Jan 23, 2019 at 09:08:49AM +, Paul Durrant wrote:
> Some frontend drivers will handle dynamic resizing of PV disks, so set up
> the BlockDevOps resize_cb() method during xen_block_realize() to allow
> this to be done.
"will": which drivers are you thinking about? The Linux one seems to be
able to handle resize already.
About the Linux one, it check the new size only when the backend set
its "state" to "connected" again.
It's frontend seems to implement resize with
1fa73be6be65028a7543bba8f14474b42e064a1b.
There is this is the source code:
static void blkfront_connect(struct blkfront_info *info)
{
// ...
switch (info->connected) {
case BLKIF_STATE_CONNECTED:
/*
* Potentially, the back-end may be signalling
* a capacity change; update the capacity.
*/
In the backend, Linux does this:
xenbus_printf(xbt, dev->nodename, "sectors", "%llu", ...
/*
* Write the current state; we will use this to synchronize
* the front-end. If the current state is "connected" the
* front-end will get the new size information online.
*/
xenbus_printf(xbt, dev->nodename, "state", "%d", dev->state);
Maybe the QEMU backend needs do to the same thing, and write its current
state again?
FreeBSD doesn't seems to care about resize.
And there is nothing in blkif.h about resizing :(.
Thanks,
--
Anthony PERARD
Re: [Qemu-block] [PATCH v9 16/18] xen: automatically create XenBlockDevice-s
On Tue, Jan 08, 2019 at 02:49:01PM +, Paul Durrant wrote:
> This patch adds create and destroy function for XenBlockDevice-s so that
> they can be created automatically when the Xen toolstack instantiates a new
> PV backend via xenstore. When the XenBlockDevice is created this way it is
> also necessary to create a 'drive' which matches the configuration that the
> Xen toolstack has written into xenstore. This is done by formulating the
> parameters necessary for each 'blockdev' layer of the drive and then using
> qmp_blockdev_add() to create the layers. Also, for compatibility with the
> legacy 'xen_disk' implementation, an iothread is automatically created for
> the new XenBlockDevice. This, like the driver layers, will be destroyed
> after the XenBlockDevice is unrealized.
>
> The legacy backend scan for 'qdisk' is removed by this patch, which makes
> the 'xen_disk' code is redundant. The code will be removed by a subsequent
> patch.
>
> Signed-off-by: Paul Durrant
> Signed-off-by: Anthony Perard
> ---
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index a7c37c185a..c6ec1d9543 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -7,12 +7,20 @@
...
> +static XenBlockDrive *xen_block_drive_create(const char *id,
> + const char *device_type,
> + QDict *opts, Error **errp)
> +{
...
> +Error *local_err = NULL;
...
> +if (!filename) {
> +error_setg(errp, "no filename");
> +goto done;
> +}
...
> +drive->node_name = xen_block_blockdev_add(drive->id, driver_layer,
> + _err);
> +
> +done:
> +g_free(driver);
> +g_free(filename);
> +
> +if (local_err) {
When xen_block_blockdev_add failed, it sets local_err, but nothing after
sets errp. I'm going to add this while preparing the pull request:
error_propagate(errp, local_err);
Is this fine with you?
With that fix, I think the series is ready, so:
Reviewed-by: Anthony PERARD
> +xen_block_drive_destroy(drive, NULL);
> +return NULL;
> +}
> +
> +return drive;
> +}
There is still the warning about using the 'file' driver when
'host_device' should be use, but I think we can try to fix that later.
Thanks,
--
Anthony PERARD
Re: [Qemu-block] [PATCH v7 16/18] xen: automatically create XenBlockDevice-s
> I've tested your patch and it does seem like the best way forward. I'll > squash it in. Do you want me to put your S-o-b on the combined patch? You can, I'll have to add it anyway when I'll prepare the pull request. Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH v7 16/18] xen: automatically create XenBlockDevice-s
On Tue, Jan 08, 2019 at 01:07:49PM +, Paul Durrant wrote:
> > -Original Message-
> > From: Kevin Wolf [mailto:kw...@redhat.com]
> > Sent: 08 January 2019 12:53
> > To: Paul Durrant
> > Cc: Anthony Perard ; qemu-de...@nongnu.org;
> > qemu-block@nongnu.org; xen-de...@lists.xenproject.org; Max Reitz
> > ; Stefano Stabellini
> > Subject: Re: [PATCH v7 16/18] xen: automatically create XenBlockDevice-s
> >
> > Am 04.01.2019 um 17:40 hat Paul Durrant geschrieben:
> > > > -Original Message-
> > > > From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> > > > Sent: 04 January 2019 16:31
> > > > To: Paul Durrant
> > > > Cc: qemu-de...@nongnu.org; qemu-block@nongnu.org; xen-
> > > > de...@lists.xenproject.org; Kevin Wolf ; Max Reitz
> > > > ; Stefano Stabellini
> > > > Subject: Re: [PATCH v7 16/18] xen: automatically create
> > XenBlockDevice-s
> > > >
> > > > Almost done, there is one thing left which I believe is an issue.
> > > > Whenever I attach a raw file to QEMU, it print:
> > > > qemu-system-i386: warning: Opening a block device as a file using
> > the
> > > > 'file' driver is deprecated
> > >
> > > Oh, I'd not noticed that... but then I only use raw files occasionally.
> >
> > Strictly speaking, this is not about raw (regular) files, but raw block
> > devices. 'file' is fine for actual regular files, but the protocol
> > driver for block devices is 'host_device'.
> >
> > > > raw files should use the "raw" driver, so we aren't done yet.
> > >
> > > Ok. Having a strictly 2-layer stack actually makes things simpler anyway
> > :-)
> >
> > Using 'raw' there will make the block layer auto-detect the right
> > protocol layer, so this works. If you want to avoid the second layer,
> > you'd have to figure out manually whether to use 'file' or
> > 'host_device'.
>
> Thanks for the explanation. I'll give it a spin using a device... I've posted
> v8 but, given what you say, I'm still not sure I have it right.
Indeed, in v8, even with the extra 'raw' layer, the warning is still
there. I was trying to understand why, and I only found out today that
we would need to use the 'host_device' driver as explain by Kevin.
BTW Paul, we can simplify the code that manage layers, by not managing
them.
Instead of (in JSON / QMP term):
{ 'driver': 'file', 'filename': '/file', 'node-name': 'node-file' }
{ 'driver': 'qcow2', 'file': 'node-file', 'node-name': 'node-qcow2' }
we can have:
{ 'driver': 'qcow2', 'node-name': 'node-qcow2',
'file': { 'driver': 'file', 'filename': '/file' } }
Here is the patch I have, fill free to review and squash it, or not:
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 91f5b58993..c6ec1d9543 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -653,43 +653,23 @@ static char *xen_block_blockdev_add(const char *id, QDict
*qdict,
static void xen_block_drive_destroy(XenBlockDrive *drive, Error **errp)
{
-while (drive->layers-- != 0) {
-char *node_name = drive->node_name[drive->layers];
+char *node_name = drive->node_name;
+
+if (node_name) {
Error *local_err = NULL;
xen_block_blockdev_del(node_name, _err);
if (local_err) {
error_propagate(errp, local_err);
-drive->layers++;
return;
}
+g_free(node_name);
+drive->node_name = NULL;
}
g_free(drive->id);
g_free(drive);
}
-static void xen_block_drive_layer_add(XenBlockDrive *drive, QDict *qdict,
- Error **errp)
-{
-unsigned int i = drive->layers;
-char *node_name;
-
-g_assert(drive->layers < ARRAY_SIZE(drive->node_name));
-
-if (i != 0) {
-/* Link to the lower layer */
-qdict_put_str(qdict, "file", drive->node_name[i - 1]);
-}
-
-node_name = xen_block_blockdev_add(drive->id, qdict, errp);
-if (!node_name) {
-return;
-}
-
-drive->node_name[i] = node_name;
-drive->layers++;
-}
-
static XenBlockDrive *xen_block_drive_create(const char *id,
const char *device_type,
QDict *opts, Error **errp)
@@ -702,7 +682,8 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
char *filename = NULL;
XenBlockDrive *drive = NULL;
Error *local_err = NULL;
-QDict *qdict;
+QDict *file_layer;
+QDict *driver_layer;
if (params) {
char **v = g_strsplit(params, ":", 2);
@@ -733,13 +714,13 @@ static XenBlockDrive *x
Re: [Qemu-block] [PATCH v7 16/18] xen: automatically create XenBlockDevice-s
Almost done, there is one thing left which I believe is an issue.
Whenever I attach a raw file to QEMU, it print:
qemu-system-i386: warning: Opening a block device as a file using the
'file' driver is deprecated
So, I think the comment below isn't true. We should create a "raw"
driver for "raw" files.
On Thu, Dec 20, 2018 at 05:14:37PM +, Paul Durrant wrote:
> +static XenBlockDrive *xen_block_drive_create(const char *id,
> + const char *device_type,
> + QDict *opts, Error **errp)
> +{
...
> +if (params) {
> +char **v = g_strsplit(params, ":", 2);
> +
> +if (v[1] == NULL) {
> +filename = g_strdup(v[0]);
> +driver = g_strdup("file");
> +} else {
> +if (strcmp(v[0], "aio") == 0) {
> +driver = g_strdup("file");
> +} else if (strcmp(v[0], "vhd") == 0) {
> +driver = g_strdup("vpc");
> +} else {
> +driver = g_strdup(v[0]);
> +}
> +filename = g_strdup(v[1]);
> +}
> +
> +g_strfreev(v);
> +}
> +
...
> +/* If the image is a raw file then we are done */
raw files should use the "raw" driver, so we aren't done yet.
> +if (!strcmp(driver, "file")) {
> +goto done;
> +}
--
Anthony PERARD
Re: [Qemu-block] [PATCH v7 09/18] xen: remove unnecessary code from dataplane/xen-block.c
On Thu, Dec 20, 2018 at 05:14:30PM +, Paul Durrant wrote: > Not all of the code duplicated from xen_disk.c is required as the basis for > the new dataplane implementation so this patch removes extraneous code, > along with the legacy #includes and calls to the legacy xen_pv_printf() > function. Error messages are changed to be reported using error_report(). > > NOTE: The code is still not yet built. Further transformations will be > required to make it correctly interface to the new XenBus/XenDevice > framework. They will be delivered in a subsequent patch. > > Signed-off-by: Paul Durrant Acked-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [PATCH v6 16/18] xen: automatically create XenBlockDevice-s
On Wed, Dec 19, 2018 at 12:43:24PM +, Paul Durrant wrote: > > Kevin seems to say that this could be done without the _flat_confused > > version. The flat_confused version seems to be useful just because > > the key "cache.direct" is used earlier, and because everything in qdict > > is a string. > > It could be, but there's a good reason for wanting everything as a string, > and that is so that I can do a qdict_iter to generate my trace message. Also > I really don't want to get too elaborate here... this is supposed to be > mimicking what would normally come via a json blob, and that would start out > as strings. Mimic JSON ? Not really. JSON has types. If the toolstack wanted cache.direct or read-only option on a blockdev, it will need to use the bool type as string type will be rejected. The expected types when issuing a QMP request can be found in "qapi/block-core.json", for the command "blockdev-add". Also, there is a comment on the qobject_input_visitor_new_flat_confused function, it reads: The block subsystem uses this function to visit its flat QDict with possibly confused scalar types. It should not be used for anything else, and it should go away once the block subsystem has been cleaned up. We might as well avoid using it right now, as it's easy to do so. -- Anthony PERARD
Re: [Qemu-block] [PATCH v6 15/18] xen: add a mechanism to automatically create XenDevice-s...
On Mon, Dec 17, 2018 at 01:30:08PM +, Paul Durrant wrote: > ...that maintains compatibility with existing Xen toolstacks. > > Xen toolstacks instantiate PV backends by simply writing information into > xenstore and expecting a backend implementation to be watching for this. > > This patch adds a new 'xen-backend' module to allow individual XenDevice > implementations to register create and destroy functions. The creator > will be called when a tool-stack instantiates a new backend in this way, > and the destructor will then be called after the resulting XenDevice > object is unrealized. > > To support this it is also necessary to add new watchers into the XenBus > implementation to handle enumeration of new backends and also destruction > of XenDevice-s when the toolstack sets the backend 'online' key to 0. > > NOTE: This patch only adds the framework. A subsequent patch will add a > creator function for xen-block devices. > > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [PATCH v6 16/18] xen: automatically create XenBlockDevice-s
& !!value) {
> +qdict_put_str(qdict, "discard", "unmap");
> +}
> +}
> +
> +/*
> + * It is necessary to turn file locking off as an emulated device
> + * may have already opened the same image file.
> + */
> +qdict_put_str(qdict, "locking", "off");
> +
> +xen_block_drive_layer_add(drive, qdict, _err);
> +qobject_unref(qdict);
> +
> + if (local_err) {
> +error_propagate(errp, local_err);
> +goto done;
> +}
> +
> +/* If the image is a raw file then we are done */
I don't think that is true, as I have this warning in QEMU:
qemu-system-i386: warning: Opening a block device as a file using the 'file'
driver is deprecated
We would need a "raw" driver.
> +if (!strcmp(driver, "file")) {
> +goto done;
> +}
> +
> +qdict = qdict_new();
> +
> +qdict_put_str(qdict, "driver", driver);
> +
> +xen_block_drive_layer_add(drive, qdict, _err);
> +qobject_unref(qdict);
> +
> +done:
> +g_free(driver);
> +g_free(filename);
> +
> +if (local_err) {
> +xen_block_drive_destroy(drive, NULL);
> +return NULL;
> +}
> +
> +return drive;
> +}
--
Anthony PERARD
Re: [Qemu-block] [Xen-devel] [PATCH v5 09/18] xen: remove unnecessary code from dataplane/xen-block.c
On Mon, Dec 17, 2018 at 11:40:39AM +, Paul Durrant wrote: > Not all of the code duplicated from xen_disk.c is required as the basis for > the new dataplane implementation so this patch removes extraneous code, > along with the legacy #includes and calls to the legacy xen_pv_printf() > function. Error messages are changed to be reported using error_report(). > > NOTE: The code is still not yet built. Further transformations will be > required to make it correctly interface to the new XenBus/XenDevice > framework. They will be delivered in a subsequent patch. > > Signed-off-by: Paul Durrant > Acked-by: Anthony Perard > --- > Cc: Stefano Stabellini > Cc: Stefan Hajnoczi > Cc: Kevin Wolf > Cc: Max Reitz > > v2: > - Leave existing boilerplate alone, other than removing the now-incorrect >description > --- > hw/block/dataplane/xen-block.c | 409 > ++--- > 1 file changed, 16 insertions(+), 393 deletions(-) > > diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c > index 9fae505..98f987d 100644 > --- a/hw/block/dataplane/xen-block.c > +++ b/hw/block/dataplane/xen-block.c > @@ -1,6 +1,4 @@ > /* > - * xen paravirt block device backend > - * > * (c) Gerd Hoffmann > * > * This program is free software; you can redistribute it and/or modify > @@ -19,26 +17,12 @@ > * GNU GPL, version 2 or (at your option) any later version. > */ > > -#include "qemu/osdep.h" > -#include "qemu/units.h" > -#include > -#include > - > -#include "hw/hw.h" > -#include "hw/xen/xen_backend.h" > -#include "xen_blkif.h" > -#include "sysemu/blockdev.h" > -#include "sysemu/iothread.h" > -#include "sysemu/block-backend.h" > -#include "qapi/error.h" > -#include "qapi/qmp/qdict.h" > -#include "qapi/qmp/qstring.h" > -#include "trace.h" > - > -/* - */ > - > -#define BLOCK_SIZE 512 > -#define IOCB_COUNT (BLKIF_MAX_SEGMENTS_PER_REQUEST + 2) > +/* > + * Copyright (c) 2018 Citrix Systems Inc. > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + */ This patch obviously comes from v3 of the patch series. v4 was fine. Please check comments on v1 v2 and v3. Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH v3 0/3] Performance improvements for xen_disk^Wxen-block
On Wed, Dec 12, 2018 at 11:16:23AM +, Paul Durrant wrote: > This series is a re-base of Tim's v2 series [1] on top of my series [2]. > > [1] https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00243.html > [2] https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02271.html For the series: Acked-by: Anthony PERARD And I've pushed that here: https://xenbits.xen.org/gitweb/?p=people/aperard/qemu-dm.git;a=shortlog;h=refs/heads/xen-next -- Anthony PERARD
Re: [Qemu-block] [PATCH v1] xen_disk: fix memory leak
On Tue, Dec 11, 2018 at 05:02:24PM +0100, Olaf Hering wrote:
> There are some code paths that clobber ioreq->buf, which leads to a huge
> memory leak after a few hours of runtime. One code path is
> qemu_aio_complete, which might be called recursive. Another one is
I think it's s/recursive/recursively/.
> ioreq_reset, which might clobber ioreq->buf as well.
>
> Add wrappers to free ioreq->buf before reassignment.
>
> Signed-off-by: Olaf Hering
That patch seems fine, with a few coding style issues, and is going to
be needed to be forward ported to Paul's reimplementation (not yet
merged).
> ---
> hw/block/xen_disk.c | 22 +-
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 36eff94f84..e15eefe625 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -103,12 +103,24 @@ struct XenBlkDev {
>
> /* - */
>
> +static void ioreq_buf_alloc(struct ioreq *ioreq, size_t alignment)
You have the parameter `alignment` but don't actually use it, I don't
think it's needed.
> +{
> +if (ioreq->buf)
> +qemu_vfree(ioreq->buf);
You could call ioreq_buf_free here instead of duplicating the code.
> +ioreq->buf = qemu_memalign(XC_PAGE_SIZE, ioreq->size);
> +}
> +static void ioreq_buf_free(struct ioreq *ioreq)
> +{
> +if (ioreq->buf)
> +qemu_vfree(ioreq->buf);
> +ioreq->buf = NULL;
> +}
Thanks,
--
Anthony PERARD
Re: [Qemu-block] [PATCH v4 15/18] xen: add a mechanism to automatically create XenDevice-s...
On Tue, Dec 11, 2018 at 03:57:39PM +, Paul Durrant wrote: > ...that maintains compatibility with existing Xen toolstacks. > > Xen toolstacks instantiate PV backends by simply writing information into > xenstore and expecting a backend implementation to be watching for this. > > This patch adds a new 'xen-backend' module to allow individual XenDevice > implementations to register a creator function to be called when a tool- > stack instantiates a new backend in this way. > > To support this it is also necessary to add new watchers into the XenBus > implementation to handle enumeration of new backends and also destruction > of XenDevice-s when the toolstack sets the backend 'online' key to 0. > > NOTE: This patch only adds the framework. A subsequent patch will add a > creator function for xen-block devices. > > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [PATCH v4 14/18] xen: add implementations of xen-block connect and disconnect functions...
On Tue, Dec 11, 2018 at 03:57:38PM +, Paul Durrant wrote: > ...and wire in the dataplane. > > This patch adds the remaining code to make the xen-block XenDevice > functional. The parameters that a block frontend expects to find are > populated in the backend xenstore area, and the 'ring-ref' and > 'event-channel' values specified in the frontend xenstore area are > mapped/bound and used to set up the dataplane. > > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [PATCH v3 14/18] xen: add implementations of xen-block connect and disconnect functions...
On Tue, Dec 11, 2018 at 10:47:14AM +, Paul Durrant wrote: > ...and wire in the dataplane. > > This patch adds the remaining code to make the xen-block XenDevice > functional. The parameters that a block frontend expects to find are > populated in the backend xenstore area, and the 'ring-ref' and > 'event-channel' values specified in the frontend xenstore area are > mapped/bound and used to set up the dataplane. > > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [PATCH v3 07/18] xen: add event channel interface for XenDevice-s
On Tue, Dec 11, 2018 at 10:47:07AM +, Paul Durrant wrote: > The legacy PV backend infrastructure provides functions to bind, unbind > and send notifications to event channnels. Similar functionality will be > required by XenDevice implementations so this patch adds the necessary > support. > > Signed-off-by: Paul Durrant > Reviewed-by: Stefano Stabellini When and where did this review happend? I can only find my review-by tag on v2, which is missing here. -- Anthony PERARD
Re: [Qemu-block] [PATCH v3 05/18] xen: add xenstore watcher infrastructure
On Tue, Dec 11, 2018 at 10:47:05AM +, Paul Durrant wrote: > A Xen PV frontend communicates its state to the PV backend by writing to > the 'state' key in the frontend area in xenstore. It is therefore > necessary for a XenDevice implementation to be notified whenever the > value of this key changes. > > This patch adds code to do this as follows: > > - an 'fd handler' is registered on the libxenstore handle which will be > triggered whenever a 'watch' event occurs > - primitives are added to xen-bus-helper to add or remove watch events > - a list of Notifier objects is added to XenBus to provide a mechanism > to call the appropriate 'watch handler' when its associated event > occurs > > The xen-block implementation is extended with a 'frontend_changed' method, > which calls as-yet stub 'connect' and 'disconnect' functions when the > relevant frontend state transitions occur. A subsequent patch will supply > a full implementation for these functions. > > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [Xen-devel] [PATCH v3 09/18] xen: remove unnecessary code from dataplane/xen-block.c
On Tue, Dec 11, 2018 at 10:47:09AM +, Paul Durrant wrote: > v2: > - Leave existing boilerplate alone, other than removing the now-incorrect >description > --- > hw/block/dataplane/xen-block.c | 409 > ++--- > 1 file changed, 16 insertions(+), 393 deletions(-) > > diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c > index 9fae505..98f987d 100644 > --- a/hw/block/dataplane/xen-block.c > +++ b/hw/block/dataplane/xen-block.c > @@ -1,6 +1,4 @@ > /* > - * xen paravirt block device backend > - * > * (c) Gerd Hoffmann > * > * This program is free software; you can redistribute it and/or modify > @@ -19,26 +17,12 @@ > * GNU GPL, version 2 or (at your option) any later version. > */ > > +/* > + * Copyright (c) 2018 Citrix Systems Inc. Can you move this copyright line to the existing license boilerplate as I've asked on v2? > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. And this isn't needed as it just duplicate the already existing one. > + */ > Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH v3 04/18] xen: create xenstore areas for XenDevice-s
On Tue, Dec 11, 2018 at 10:47:04AM +, Paul Durrant wrote: > This patch adds a new source module, xen-bus-helper.c, which builds on > basic libxenstore primitives to provide functions to create (setting > permissions appropriately) and destroy xenstore areas, and functions to > 'printf' and 'scanf' nodes therein. The main xen-bus code then uses > these primitives [1] to initialize and destroy the frontend and backend > areas for a XenDevice during realize and unrealize respectively. > > The 'xen-block' implementation is extended with a 'get_name' method that > returns the VBD number. This number is required to 'name' the xenstore > areas. > > NOTE: An exit handler is also added to make sure the xenstore areas are > cleaned up if QEMU terminates without devices being unrealized. > > [1] The 'scanf' functions are actually not yet needed, but they will be > needed by code delivered in subsequent patches. > > Signed-off-by: Paul Durrant > --- > > v3: > - Add transaction id parameters to xen-bus-helper functions > - Not added Anthony's R-b because of change > Reviewed-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [PATCH v3 03/18] xen: introduce 'xen-block', 'xen-disk' and 'xen-cdrom'
On Tue, Dec 11, 2018 at 10:47:03AM +, Paul Durrant wrote: > This patch adds new XenDevice-s: 'xen-disk' and 'xen-cdrom', both derived > from a common 'xen-block' parent type. These will eventually replace the > 'xen_disk' (note the underscore rather than hyphen) legacy PV backend but > it is illustrative to build up the implementation incrementally, along with > the XenBus/XenDevice framework. Subsequent patches will therefore add to > these devices' implementation as new features are added to the framework. > > After this patch has been applied it is possible to instantiate new > 'xen-disk' or 'xen-cdrom' devices with a single 'vdev' parameter, which > accepts values adhering to the Xen VBD naming scheme [1]. For example, a > command-line instantiation of a xen-disk can be done with an argument > similar to the following: > > -device xen-disk,vdev=hda > > The implementation of the vdev parameter formulates the appropriate VBD > number for use in the PV protocol. > > [1] https://xenbits.xen.org/docs/unstable/man/xen-vbd-interface.7.html > > Signed-off-by: Paul Durrant Now we can add a xen-disk with vdev=xvdbgqcv :) Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH v2 18/18] xen: remove the legacy 'xen_disk' backend
On Thu, Dec 06, 2018 at 03:08:44PM +, Paul Durrant wrote: > This backend has now been replaced by the 'xen-qdisk' XenDevice. > > Signed-off-by: Paul Durrant Acked-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [PATCH v2 16/18] xen: automatically create XenBlockDevice-s
On Thu, Dec 06, 2018 at 03:08:42PM +, Paul Durrant wrote: > This patch adds a creator function for XenBlockDevice-s so that they can > be created automatically when the Xen toolstack instantiates a new > PV backend. When the XenBlockDevice is created this way it is also > necessary to create a drive which matches the configuration that the Xen > toolstack has written into xenstore. This drive is marked 'auto_del' so > that it will be removed when the XenBlockDevice is destroyed. Also, for > compatibility with the legacy 'xen_disk' implementation, an iothread > is automatically created for the new XenBlockDevice. This will also be > removed when the XenBlockDevice is destroyed. > > Correspondingly the legacy backend scan for 'qdisk' is removed. > > After this patch is applied the legacy 'xen_disk' code is redundant. It > will be removed by a subsequent patch. > > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH v2 15/18] xen: add a mechanism to automatically create XenDevice-s...
On Thu, Dec 06, 2018 at 03:08:41PM +, Paul Durrant wrote: > ...that maintains compatibility with existing Xen toolstacks. > > Xen toolstacks instantiate PV backends by simply writing information into > xenstore and expecting a backend implementation to be watching for this. > > This patch adds a new 'xen-backend' module to allow individual XenDevice > implementations to register a creator function to be called when a tool- > stack instantiates a new backend in this way. > > To support this it is also necessary to add new watchers into the XenBus > implementation to handle enumeration of new backends and also destruction > of XenDevice-s when the toolstack sets the backend 'online' key to 0. > > NOTE: This patch only adds the framework. A subsequent patch will add a > creator function for xen-block devices. > > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH v2 14/18] xen: add implementations of xen-block connect and disconnect functions...
On Thu, Dec 06, 2018 at 03:08:40PM +, Paul Durrant wrote:
> ...and wire in the dataplane.
>
> This patch adds the remaining code to make the xen-block XenDevice
> functional. The parameters that a block frontend expects to find are
> populated in the backend xenstore area, and the 'ring-ref' and
> 'event-channel' values specified in the frontend xenstore area are
> mapped/bound and used to set up the dataplane.
>
> Signed-off-by: Paul Durrant
With this patch, we should be able to have QEMU instantiate a new
backend for a guest, right ? (via command line or QMP)
I've tried, and that doesn't work, the xenstore path for the frontend is
wrong. In the qemu trace, I have:
xs_node_create /local/domain/0/backend/xen-disk/23/268572709
Which is probably fine, even if not described in xenstore-paths.markdown.
xs_node_create /local/domain/23/device/xen-disk/268572709
Which is not, instead of "xen-disk", we should have "vbd".
I know that this is fixed in "xen: automatically create
XenBlockDevice-s", but at least the "vbd" type couldn't be added in this
patch, or maybe a previous one.
Another issue seems to be error handling. I've done a very simple test,
I've added '-device xen-disk,vdev=d536p37,id=mydisk' to the command
line (which is obvious wrong), and QEMU abort with:
qemu-system-i386: hw/block/xen-block.c:174: xen_block_realize: Assertion
`conf->blk' failed.
But I've pointed out the error in the code below.
And just for fun, adding then removing a xen-disk via QMP. Adding works
fine (once I've fixed the frontend name). I've run the following with
./scripts/qmp/qmp-shell:
blockdev-add driver=file filename=/root/vm/disk/testing-disk.qcow2
node-name=emptyfile
blockdev-add driver=qcow2 node-name=emptyqcow2 file=emptyfile
device_add driver=xen-disk vdev=xvdn id=fromqmp drive=emptyqcow2
But, then, remove doesn't work, running "device_del id=fromqmp" doesn't
do anything. I guess we can try to fix that later if you don't find
what's missing.
> @@ -76,6 +151,7 @@ static void xen_block_realize(XenDevice *xendev, Error
> **errp)
> const char *type = object_get_typename(OBJECT(blockdev));
> XenBlockVdev *vdev = >vdev;
> Error *local_err = NULL;
> +BlockConf *conf = >conf;
>
> if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> error_setg(errp, "vdev property not set");
> @@ -90,6 +166,59 @@ static void xen_block_realize(XenDevice *xendev, Error
> **errp)
> error_propagate(errp, local_err);
You probably want to add a return here, this is when
`blockdev_class->realize' fails.
> }
> }
> +
> +/*
> + * The blkif protocol does not deal with removable media, so it must
> + * always be present, even for CDRom devices.
> + */
> +assert(conf->blk);
That assert should probably not be there, as a missing conf->blk isn't a
programming error, but a user error, I think.
Actually, the issue is the missing return abrove, and the assert is
probably fine.
--
Anthony PERARD
Re: [Qemu-block] [PATCH v2 13/18] xen: purge 'blk' and 'ioreq' from function names in dataplane/xen-block.c
On Thu, Dec 06, 2018 at 03:08:39PM +, Paul Durrant wrote: > This is a purely cosmetic patch that purges remaining use of 'blk' and > 'ioreq' in local function names, and then makes sure all functions are > prefixed with 'xen_block_'. > > No functional change. > > Signed-off-by: Paul Durrant Acked-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [PATCH v2 10/18] xen: add header and build dataplane/xen-block.c
On Thu, Dec 06, 2018 at 03:08:36PM +, Paul Durrant wrote: > This patch adds the transformations necessary to get dataplane/xen-block.c > to build against the new XenBus/XenDevice framework. MAINTAINERS is also > updated due to the introduction of dataplane/xen-block.h. > > NOTE: Existing data structure names are retained for the moment. These will > be modified by subsequent patches. A typedef for XenBlockDataPlane > has been added to the header (based on the old struct XenBlkDev name > for the moment) so that the old names don't need to leak out of the > dataplane code. > > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [Xen-devel] [PATCH v2 09/18] xen: remove unnecessary code from dataplane/xen-block.c
On Thu, Dec 06, 2018 at 03:08:35PM +, Paul Durrant wrote: > v2: > - Leave existing boilerplate alone, other than removing the now-incorrect >description > --- > hw/block/dataplane/xen-block.c | 409 > ++--- > 1 file changed, 16 insertions(+), 393 deletions(-) > > diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c > index 9fae505..98f987d 100644 > --- a/hw/block/dataplane/xen-block.c > +++ b/hw/block/dataplane/xen-block.c > @@ -1,6 +1,4 @@ > /* > - * xen paravirt block device backend > - * > * (c) Gerd Hoffmann > * > * This program is free software; you can redistribute it and/or modify > @@ -19,26 +17,12 @@ > * GNU GPL, version 2 or (at your option) any later version. > */ > > +/* > + * Copyright (c) 2018 Citrix Systems Inc. You can add this copyright line just after Gerd's one abrove. > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. And I'm pretty sure this should not be added. The boilerplate at the top of the file already state that your contributions is going to be GPLv2+. -- Anthony PERARD
Re: [Qemu-block] [PATCH v2 07/18] xen: add event channel interface for XenDevice-s
On Thu, Dec 06, 2018 at 03:08:33PM +, Paul Durrant wrote: > The legacy PV backend infrastructure provides functions to bind, unbind > and send notifications to event channnels. Similar functionality will be > required by XenDevice implementations so this patch adds the necessary > support. > > Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH v2 05/18] xen: add xenstore watcher infrastructure
On Thu, Dec 06, 2018 at 03:08:31PM +, Paul Durrant wrote:
> @@ -36,6 +54,12 @@ static void xen_block_unrealize(XenDevice *xendev, Error
> **errp)
>
> trace_xen_block_unrealize(type, vdev->disk, vdev->partition);
>
> +/* Disconnect from the frontend in case this has not already happened */
> +xen_block_disconnect(xendev, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
If xen_block_disconnect fails, local_err is going to be reuse below. If
it's fine to try unrealize, then `local_err=NULL` is probably enough.
> +}
> +
> if (blockdev_class->unrealize) {
> blockdev_class->unrealize(blockdev, _err);
> if (local_err) {
[...]
> +static void xen_bus_remove_watch(XenBus *xenbus, XenWatch *watch,
> + Error **errp)
> +{
> +Error *local_err = NULL;
> +
> +trace_xen_bus_remove_watch(watch->node, watch->key, watch->token);
> +
> +xs_node_unwatch(xenbus->xsh, watch->node, watch->key, watch->token,
> +_err);
You could simply pass `errp' directly instead of having `local_err'.
> +
> +notifier_remove(>notifier);
> +free_watch(watch);
> +
> +if (local_err) {
> +error_propagate(errp, local_err);
> +}
> +}
> +
--
Anthony PERARD
Re: [Qemu-block] [PATCH v2 03/18] xen: introduce 'xen-block', 'xen-disk' and 'xen-cdrom'
On Fri, Dec 07, 2018 at 02:39:40PM +, Paul Durrant wrote:
> > -Original Message-
> > From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> > Sent: 07 December 2018 14:35
> > To: Paul Durrant
> > Cc: qemu-de...@nongnu.org; qemu-block@nongnu.org; xen-
> > de...@lists.xenproject.org; Kevin Wolf ; Max Reitz
> > ; Stefano Stabellini
> > Subject: Re: [PATCH v2 03/18] xen: introduce 'xen-block', 'xen-disk' and
> > 'xen-cdrom'
> >
> > On Thu, Dec 06, 2018 at 03:08:29PM +, Paul Durrant wrote:
> > > +static char *disk_to_vbd_name(unsigned int disk)
> > > +{
> > > +char *name, *prefix = (disk >= 26) ?
> > > +disk_to_vbd_name((disk / 26) - 1) : g_strdup("");
> > > +
> > > +name = g_strdup_printf("%s%c", prefix, 'a' + disk);
> >
> > I don't think that works, if disk is 27, we do ('a' + 27) here. It's
> > probably missing a `disk % 26`.
>
> Damn, yes I was not allowing the >2 letters.
>
> >
> > > +g_free(prefix);
> > > +
> > > +return name;
> > > +}
> >
> > [...]
> >
> > > +static unsigned int vbd_name_to_disk(const char *name, const char
> > **endp)
> > > +{
> > > +unsigned int disk = 0;
> > > +
> > > +while (*name != '\0') {
> > > +if (!g_ascii_isalpha(*name) || !g_ascii_islower(*name)) {
> > > +break;
> > > +}
> > > +
> > > +disk *= 26;
> > > +disk += *name++ - 'a';
> > > +}
> > > +*endp = name;
> > > +
> > > +return disk;
> > > +}
> > > +
> > > +static void xen_block_set_vdev(Object *obj, Visitor *v, const char
> > *name,
> > > + void *opaque, Error **errp)
> > > +{
> >
> > Setting vdev doesn't work. I've tried to add a disk `xvdaa', and it
> > result in `xvda', or `d0p0' (in the trace). (Same result with `xvdaaa',
> > and 'xvdba' gives 'xvdaa'/d26p0)
> >
>
> Ok, that's weird. I'll have to figure that out.
It's probably because 'a' is somtime 0 and sometime is 1.
'a' should be 0
'aa' should be 26,
'aaa' Seems to be 702.
'xvda': 0 -> 0 * 1
'xvdz': 25->25 * 1
'xvdaa': 26 ->1 * 26 + 0 * 1
'xvdaaa': 702 -> 1 * 26^2 + 1 * 26 + 0 * 1
So, it's weird. Have fun fixing the algorithm for that.
--
Anthony PERARD
Re: [Qemu-block] [PATCH v2 04/18] xen: create xenstore areas for XenDevice-s
On Thu, Dec 06, 2018 at 03:08:30PM +, Paul Durrant wrote: > This patch adds a new source module, xen-bus-helper.c, which builds on > basic libxenstore primitives to provide functions to create (setting > permissions appropriately) and destroy xenstore areas, and functions to > 'printf' and 'scanf' nodes therein. The main xen-bus code then uses > these primitives [1] to initialize and destroy the frontend and backend > areas for a XenDevice during realize and unrealize respectively. > > The 'xen-block' implementation is extended with a 'get_name' method that > returns the VBD number. This number is required to 'name' the xenstore > areas. > > NOTE: An exit handler is also added to make sure the xenstore areas are > cleaned up if QEMU terminates without devices being unrealized. > > [1] The 'scanf' functions are actually not yet needed, but they will be > needed by code delivered in subsequent patches. > > Signed-off-by: Paul Durrant Looks good, Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH v2 03/18] xen: introduce 'xen-block', 'xen-disk' and 'xen-cdrom'
On Thu, Dec 06, 2018 at 03:08:29PM +, Paul Durrant wrote:
> +static char *disk_to_vbd_name(unsigned int disk)
> +{
> +char *name, *prefix = (disk >= 26) ?
> +disk_to_vbd_name((disk / 26) - 1) : g_strdup("");
> +
> +name = g_strdup_printf("%s%c", prefix, 'a' + disk);
I don't think that works, if disk is 27, we do ('a' + 27) here. It's
probably missing a `disk % 26`.
> +g_free(prefix);
> +
> +return name;
> +}
[...]
> +static unsigned int vbd_name_to_disk(const char *name, const char **endp)
> +{
> +unsigned int disk = 0;
> +
> +while (*name != '\0') {
> +if (!g_ascii_isalpha(*name) || !g_ascii_islower(*name)) {
> +break;
> +}
> +
> +disk *= 26;
> +disk += *name++ - 'a';
> +}
> +*endp = name;
> +
> +return disk;
> +}
> +
> +static void xen_block_set_vdev(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
Setting vdev doesn't work. I've tried to add a disk `xvdaa', and it
result in `xvda', or `d0p0' (in the trace). (Same result with `xvdaaa',
and 'xvdba' gives 'xvdaa'/d26p0)
--
Anthony PERARD
Re: [Qemu-block] [PATCH v2 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
On Thu, Dec 06, 2018 at 03:08:28PM +, Paul Durrant wrote:
> This patch adds the basic boilerplate for a 'XenBus' object that will act
> as a parent to 'XenDevice' PV backends.
> A new 'XenBridge' object is also added to connect XenBus to the system bus.
>
> The XenBus object is instantiated by a new xen_bus_init() function called
> from the same sites as the legacy xen_be_init() function.
>
> Subsequent patches will flesh-out the functionality of these objects.
>
> Signed-off-by: Paul Durrant
> ---
> Cc: Stefano Stabellini
> Cc: Anthony Perard
> Cc: "Michael S. Tsirkin"
> Cc: Marcel Apfelbaum
> Cc: Paolo Bonzini
> Cc: Richard Henderson
> Cc: Eduardo Habkost
>
> v2:
> - Fix boilerplate
> - Make xen-bus hotplug capable
> ---
> hw/i386/xen/xen-hvm.c | 3 ++
> hw/xen/Makefile.objs | 2 +-
> hw/xen/trace-events | 6 +++
> hw/xen/xen-bus.c | 131
> ++
> hw/xenpv/xen_machine_pv.c | 3 ++
> include/hw/xen/xen-bus.h | 55 +++
> 6 files changed, 199 insertions(+), 1 deletion(-)
> create mode 100644 hw/xen/xen-bus.c
> create mode 100644 include/hw/xen/xen-bus.h
>
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index 1d63763..4497f75 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -17,6 +17,7 @@
> #include "hw/i386/apic-msidef.h"
> #include "hw/xen/xen_common.h"
> #include "hw/xen/xen-legacy-backend.h"
> +#include "hw/xen/xen-bus.h"
> #include "qapi/error.h"
> #include "qapi/qapi-commands-misc.h"
> #include "qemu/error-report.h"
> @@ -1479,6 +1480,8 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion
> **ram_memory)
> QLIST_INIT(>dev_list);
> device_listener_register(>device_listener);
>
> +xen_bus_init();
> +
> /* Initialize backend core & drivers */
> if (xen_be_init() != 0) {
> error_report("xen backend core setup failed");
> diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
> index 3f64a44..d9d6d7b 100644
> --- a/hw/xen/Makefile.objs
> +++ b/hw/xen/Makefile.objs
> @@ -1,5 +1,5 @@
> # xen backend driver support
> -common-obj-$(CONFIG_XEN) += xen-legacy-backend.o xen_devconfig.o xen_pvdev.o
> xen-common.o
> +common-obj-$(CONFIG_XEN) += xen-legacy-backend.o xen_devconfig.o xen_pvdev.o
> xen-common.o xen-bus.o
>
> obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
> obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o
> xen_pt_graphics.o xen_pt_msi.o
> diff --git a/hw/xen/trace-events b/hw/xen/trace-events
> index c7e7a3b..0172cd4 100644
> --- a/hw/xen/trace-events
> +++ b/hw/xen/trace-events
> @@ -12,3 +12,9 @@ xen_unmap_portio_range(uint32_t id, uint64_t start_addr,
> uint64_t end_addr) "id:
> xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: %u
> bdf: %02x.%02x.%02x"
> xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id:
> %u bdf: %02x.%02x.%02x"
> xen_domid_restrict(int err) "err: %u"
> +
> +# include/hw/xen/xen-bus.c
> +xen_bus_realize(void) ""
> +xen_bus_unrealize(void) ""
> +xen_device_realize(const char *type) "type: %s"
> +xen_device_unrealize(const char *type) "type: %s"
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> new file mode 100644
> index 000..1385bab
> --- /dev/null
> +++ b/hw/xen/xen-bus.c
> @@ -0,0 +1,131 @@
> +/*
> + * Copyright (c) 2018 Citrix Systems Inc.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/hw.h"
> +#include "hw/sysbus.h"
> +#include "hw/xen/xen-bus.h"
> +#include "qapi/error.h"
> +#include "trace.h"
> +
> +static void xen_bus_unrealize(BusState *bus, Error **errp)
> +{
> +trace_xen_bus_unrealize();
> +}
> +
> +static void xen_bus_realize(BusState *bus, Error **errp)
> +{
> +trace_xen_bus_realize();
> +}
> +
> +static void xen_bus_class_init(ObjectClass *class, void *data)
> +{
> +BusClass *bus_class = BUS_CLASS(class);
> +
> +bus_class->realize = xen_bus_realize;
> +bus_class->unrealize = xen_bus_unrealize;
> +}
> +
> +static const TypeInfo xen_bus_type_info = {
> +.name = TYPE_XEN_BUS,
> +.parent = TYPE_BUS,
> +.instance_size = sizeof(XenBus),
> +.class_size = sizeof(XenBusClass),
> +
Re: [Qemu-block] [PATCH 15/18] xen: add a mechanism to automatically create XenDevice-s...
On Thu, Dec 06, 2018 at 12:36:52PM +, Paul Durrant wrote:
> > -Original Message-
> > From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> > Sent: 04 December 2018 15:35
> >
> > On Wed, Nov 21, 2018 at 03:12:08PM +, Paul Durrant wrote:
> > > +xenbus->backend_watch =
> > > +xen_bus_add_watch(xenbus, "", /* domain root node */
> > > + "backend", xen_bus_enumerate, xenbus,
> > _err);
> > > +if (local_err) {
> > > +error_propagate(errp, local_err);
> > > +error_prepend(errp, "failed to set up enumeration watch: ");
> >
> > You should use error_propagate_prepend instead
> > error_propagate;error_prepend. And it looks like there is the same
> > mistake in other patches that I haven't notice.
> >
>
> Oh, I didn't know about that one either... I've only seen the separate calls
> used elsewhere.
That information is all in "include/qapi/error.h", if you which to know
more on how to use Error.
> > Also you probably want goto fail here.
> >
>
> Not sure about that. Whilst the bus scan won't happen, it doesn't mean
> devices can't be added via QMP.
In that case, don't modify errp, and use error_reportf_err instead, or
warn_reportf_err (then local_err = NULL, in case it is reused in a
future modification of the function).
Setting errp (with error_propagate) means that the function failed, and
QEMU is going to exit(1), because of qdev_init_nofail call in
xen_bus_init.
> > > +static void xen_device_backend_changed(void *opaque)
> > > +{
> > > +XenDevice *xendev = opaque;
> > > +const char *type = object_get_typename(OBJECT(xendev));
> > > +enum xenbus_state state;
> > > +unsigned int online;
> > > +
> > > +trace_xen_device_backend_changed(type, xendev->name);
> > > +
> > > +if (xen_device_backend_scanf(xendev, "state", "%u", ) != 1) {
> > > +state = XenbusStateUnknown;
> > > +}
> > > +
> > > +xen_device_backend_set_state(xendev, state);
> >
> > It's kind of weird to set the internal state base on the external one
> > that something else may have modified. Shouldn't we check that it is
> > fine for something else to modify the state and that it is a correct
> > transition?
>
> The only thing (apart from this code) that's going to have perms to write the
> backend state is the toolstack... which is, of course, be definition trusted.
"trusted" doesn't mean that there isn't a bug somewhere else :-). But I
guess it's good enough for now.
> > Also aren't we going in a loop by having QEMU set the state, then the
> > watch fires again? (Not really a loop since the function _set_state
> > check for changes.
>
> No. It's de-bounced inside the set_state function.
>
> >
> > Also maybe we should watch for the state changes only when something
> > else like libxl creates (ask for) the backend, and ignore changes when
> > QEMU did it itself.
>
> I don't think it's necessary to add that complexity.
Ok.
--
Anthony PERARD
Re: [Qemu-block] [PATCH 04/18] xen: create xenstore areas for XenDevice-s
On Wed, Dec 05, 2018 at 12:05:23PM +, Paul Durrant wrote: > > > +value = xs_read(xsh, XBT_NULL, path, NULL); > > > > The xenstore.h isn't clear about failure of this function, it is > > supposed to return a malloced value. Do we actually need to check if value > > is NULL? > > The comment above xs_read() in xs.c is: > > /* Get the value of a single file, nul terminated. > * Returns a malloced value: call free() on it after use. > * len indicates length in bytes, not including the nul. > */ > > and I think we should check it for NULL before passing it to vsscanf(). I've sent a patch against xenstore.h to document that xs_read, and some other functions, can return NULL. <20181205162603.25788-1-anthony.per...@citrix.com> -- Anthony PERARD
Re: [Qemu-block] [PATCH 04/18] xen: create xenstore areas for XenDevice-s
On Wed, Dec 05, 2018 at 12:43:57PM +, Paul Durrant wrote: > > > > +value = g_strdup_vprintf(fmt, ap); > > > > > > Looks like g_vasprintf() would be better, since it returns the lenght as > > > well. > > > > > > > Yes. > > I tried this and it appears not to exist in the version of glib in my > environment so I guess I'll stick with g_strdup_printf(). It's probably because you need to include "glib/gprintf.h", I've suggested it because I've seen the function use elsewhere in QEMU. But g_strdup_printf is fine too. https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-vasprintf -- Anthony PERARD
Re: [Qemu-block] [PATCH 16/18] xen: automatically create XenQdiskDevice-s
On Wed, Nov 21, 2018 at 03:12:09PM +, Paul Durrant wrote:
> This patch adds a creator function for XenQdiskDevice-s so that they can
> be created automatically when the Xen toolstack instantiates a new
> PV backend. When the XenQdiskDevice is created this way it is also
> necessary to create a drive which matches the configuration that the Xen
> toolstack has written into xenstore. This drive is marked 'auto_del' so
> that it will be removed when the XenQdiskDevice is destroyed. Also, for
> compatibilitye with the legacy 'xen_disk' implementation, an iothread
> is automatically created for the new XenQdiskDevice. This will also be
> removed when he XenQdiskDevice is destroyed.
"the XenQdiskDevice"
[...]
> +qemu_opt_set(drive_opts, "file.locking", "off", _err);
That looks new, compared to the xen_disk.c implementation. Maybe that
should be mention in the commit message.
[..]
> +static void xen_qdisk_device_create(BusState *bus, const char *name,
> +QDict *opts, Error **errp)
> +{
[...]
> +iothread = iothread_create(vdev, _abort);
I would just propagate the error, since iothread could fail for external
reason. No need to crash qemu while a VM is running.
> +
> +dev = qdev_create(bus, TYPE_XEN_QDISK_DEVICE);
> +
> +qdev_prop_set_string(dev, "vdev", vdev);
> +
> +if (XEN_QDISK_DEVICE(dev)->vdev.number != number) {
> +error_setg(errp, "invalid dev parameter '%s'", vdev);
> +goto unref;
> +}
> +
> +qdev_prop_set_drive(dev, "drive", blk, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +error_prepend(errp, "failed to set 'drive': ");
> +goto unref;
> +}
> +
> +XEN_QDISK_DEVICE(dev)->auto_iothread = iothread;
> +
> +qdev_init_nofail(dev);
That function shouldn't be use during hotplug. But I'm not sure what
should be done instead, probably object_property_set_bool(..., true
"realized") and check for error.
Thanks,
--
Anthony PERARD
Re: [Qemu-block] [PATCH 17/18] MAINTAINERS: add myself as a Xen maintainer
On Wed, Nov 21, 2018 at 03:12:10PM +, Paul Durrant wrote: > I have made many significant contributions to the Xen code in QEMU, > particularly the recent patches introducing a new PV device framework. > I intend to make further significant contributions, porting other PV back- > ends to the new framework with the intent of eventually removing the > legacy code. It therefore seems reasonable that I become a maintiner of > the Xen code. > > Signed-off-by: Paul Durrant With the typo fixed: Acked-by: Anthony PERARD -- Anthony PERARD
Re: [Qemu-block] [PATCH 03/18] xen: introduce 'xen-qdisk'
On Tue, Dec 04, 2018 at 03:20:04PM +, Paul Durrant wrote:
> > > +static char *disk_to_vbd_name(unsigned int disk)
> > > +{
> > > +unsigned int len = DIV_ROUND_UP(disk, 26);
> > > +char *name = g_malloc0(len + 1);
> > > +
> > > +do {
> > > +name[len--] = 'a' + (disk % 26);
> > > +disk /= 26;
> > > +} while (disk != 0);
> > > +assert(len == 0);
> > > +
> > > +return name;
> > > +}
> >
> > That function doesn't work.
> >
> > For a simple xvdp, (so disk==15), it return "", I mean "\0p".
> >
> > For a more complicated 'xvdbhwza', we have len == 22901. And the assert
> > failed.
> >
> > Maybe the recursing algo in libxl would be fine, with a buffer that is
> > big enough, and could probably be on the stack (in _get_vdev).
>
> I used libxl__device_disk_dev_number() as my model (as well as cross-checking
> with the spec), but I guess a recursing algorithm would be neater.
There is libxl__devid_to_vdev() actually just after ;-) which calls
encode_disk_name which is recursing.
> > > diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> > > new file mode 100644
> > > index 00..ade0866037
> > > --- /dev/null
> > > +++ b/include/hw/xen/xen-qdisk.h
> > > @@ -0,0 +1,38 @@
> > > +/*
> > > + * Copyright (c) Citrix Systems Inc.
> > > + * All rights reserved.
> > > + */
> > > +
> > > +#ifndef HW_XEN_QDISK_H
> > > +#define HW_XEN_QDISK_H
> > > +
> > > +#include "hw/xen/xen-bus.h"
> > > +
> > > +typedef enum XenQdiskVdevType {
> > > +XEN_QDISK_VDEV_TYPE_DP,
> >
> > Maybe we could set type_dp value to 1, so that, when vdev->type isn't
> > set, we can detect it later.
>
> Rather than having the 'valid' bool? Yes, that would work.
Well, the 'valid' bool doesn't seems to always be check so it would be
better. e.g. xen_qdisk_get_vdev() doesn't check `valid` before
genereting a string. Then xen_qdisk_set_vdev could set `type` to invalid
when it is invalid.
--
Anthony PERARD
Re: [Qemu-block] [PATCH 15/18] xen: add a mechanism to automatically create XenDevice-s...
xendev, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +error_prepend(errp, "failed to watch backend state: ");
You should return here, as local_err mustn't be reused.
> +}
> +
> +xendev->backend_online_watch =
> +xen_bus_add_watch(xenbus, xendev->backend_path,
> + "online", xen_device_backend_changed,
> + xendev, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +error_prepend(errp, "failed to watch backend online: ");
You probably want a return here, in case there is more code added after.
> +}
Other instances of error_propagate;error_prepend to be replaced by
error_propagate_prepend.
> }
>
Thanks,
--
Anthony PERARD
Re: [Qemu-block] [PATCH 07/18] xen: add event channel interface for XenDevice-s
On Mon, Dec 03, 2018 at 04:24:24PM +, Anthony PERARD wrote:
> On Wed, Nov 21, 2018 at 03:12:00PM +, Paul Durrant wrote:
> > +static void xen_device_event(void *opaque)
> > +{
> > +XenDevice *xendev = opaque;
> > +unsigned long port = xenevtchn_pending(xendev->xeh);
> > +
> > +notifier_list_notify(>event_notifiers, (void *)port);
>
> I wonder if a Notifier is a good fit for XenDevice, like here for the
> events or the xenstore watches in previous patches, as NotifierLists are
> normaly used when every Notifiers want to do something, but here there
> is only one that is going to do something. But I guess it might not be
> much better to write a loop in here rather than use the one in
> notifier_list_notify.
I've seen that you use GHashTable in a following patch, wouldn't that be
useful to use for xenstore watches and evtchn events as well?
--
Anthony PERARD
Re: [Qemu-block] [PATCH 14/18] xen: add implementations of xen-qdisk connect and disconnect functions...
On Wed, Nov 21, 2018 at 03:12:07PM +, Paul Durrant wrote:
> diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> index 35f7b70480..8c88393832 100644
> --- a/hw/block/xen-qdisk.c
> +++ b/hw/block/xen-qdisk.c
> static void xen_qdisk_connect(XenQdiskDevice *qdiskdev, Error **errp)
> {
> XenQdiskVdev *vdev = >vdev;
> +XenDevice *xendev = XEN_DEVICE(qdiskdev);
> +unsigned int order, nr_ring_ref, *ring_ref, event_channel, protocol;
> +char *str;
>
> trace_xen_qdisk_connect(vdev->disk, vdev->partition);
> +
> +if (xen_device_frontend_scanf(xendev, "ring-page-order", "%u",
> + ) != 1) {
> +nr_ring_ref = 1;
> +ring_ref = g_new(unsigned int, nr_ring_ref);
> +
> +if (xen_device_frontend_scanf(xendev, "ring-ref", "%u",
> + _ref[0]) != 1) {
> +error_setg(errp, "failed to read ring-ref");
Don't you need to free `ring_ref`?
> +return;
> +}
[...]
> diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> index ade0866037..d7dd2bf0ee 100644
> --- a/include/hw/xen/xen-qdisk.h
> +++ b/include/hw/xen/xen-qdisk.h
> @@ -6,7 +6,15 @@
> #ifndef HW_XEN_QDISK_H
> #define HW_XEN_QDISK_H
>
> +#include "hw/xen/xen.h"
> #include "hw/xen/xen-bus.h"
> +#include "hw/block/block.h"
> +#include "hw/block/xen_blkif.h"
> +#include "hw/block/dataplane/xen-qdisk.h"
> +#include "sysemu/blockdev.h"
> +#include "sysemu/iothread.h"
> +#include "sysemu/block-backend.h"
> +#include "sysemu/iothread.h"
You don't need that many includes, especially not iothread.h twice ;-).
I think those new includes would be enough:
#include "hw/block/block.h"; for BlockConf
#include "sysemu/iothread.h"
#include "hw/block/dataplane/xen-qdisk.h"
>
> typedef enum XenQdiskVdevType {
> XEN_QDISK_VDEV_TYPE_DP,
> @@ -33,6 +41,10 @@ typedef struct XenQdiskDevice XenQdiskDevice;
> struct XenQdiskDevice {
> XenDevice xendev;
> XenQdiskVdev vdev;
> +BlockConf conf;
> +unsigned int max_ring_page_order;
> +IOThread *iothread;
> +XenQdiskDataPlane *dataplane;
> };
>
> #endif /* HW_XEN_QDISK_H */
--
Anthony PERARD
Re: [Qemu-block] [PATCH 13/18] xen: purge 'blk' and 'ioreq' from function names in dataplane/xen-qdisk.c
On Wed, Nov 21, 2018 at 03:12:06PM +, Paul Durrant wrote: > This is a purely cosmetic patch that purges remaining use of 'blk' and > 'ioreq' in local function names. > > No functional change. > > Signed-off-by: Paul Durrant I don't think it's a good idee to use function names that could be use elsewhere, don't have a namespace. It makes it more difficult to figure out which function is called by just searching for the function name. Could you had a prefix? Maybe xendisk_ or xen_disk or xen_qdisk or xen_block or ..., so we can have xendisk_start_request, or xendisk_request_start. I don't have a preference beside staying away from generic names. Thanks, -- Anthony PERARD
Re: [Qemu-block] [PATCH 12/18] xen: remove 'ioreq' struct/varable/field names from dataplane/xen-qdisk.c
On Wed, Nov 21, 2018 at 03:12:05PM +, Paul Durrant wrote: > This is a purely cosmetic patch that purges the name 'ioreq' from struct, > variable and field names. (This name has been problematic for a long time > as 'ioreq' is the name used for generic I/O requests coming from Xen). > The patch replaces 'struct ioreq' with a new 'XenQdiskRequest' type and > 'ioreq' field/variable names with 'request', and then does necessary > fix-up to adhere to coding style. > > Function names are not modified by this patch. Yhey will be dealt with in s/Yhey/They/ > a subsequent patch. > > No functional change. > > Signed-off-by: Paul Durrant Acked-by: Anthony PERARD -- Anthony PERARD
