Re: [Qemu-block] [PATCH v5 4/6] nbd/server: implement dirty bitmap export
On 6/9/18 10:17 AM, Vladimir Sementsov-Ogievskiy wrote:
Handle new NBD meta namespace: "qemu", and corresponding queries:
"qemu:dirty-bitmap:".
With new metadata context negotiated, BLOCK_STATUS query will reply
with dirty-bitmap data, converted to extents. New public function
nbd_export_bitmap selects bitmap to export. For now, only one bitmap
may be exported.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/nbd.h | 6 ++
nbd/server.c| 271
2 files changed, 259 insertions(+), 18 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index fcdcd54502..a653d0fba7 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -234,6 +234,10 @@ enum {
#define NBD_STATE_HOLE (1 << 0)
#define NBD_STATE_ZERO (1 << 1)
+/* Flags for extents (NBDExtent.flags) of NBD_REPLY_TYPE_BLOCK_STATUS,
+ * for qemu:dirty-bitmap:* meta contexts */
+#define NBD_STATE_DIRTY (1 << 0)
Hindsight is 20:20, so there's nothing we can do about this definition
now; commit 3d068aff is already released in 3.0. However, if I had been
more careful when this was first introduced, I would have negated the
sense of this bit. The NBD protocol recommends that a status of all 0
represent the default state, and when it comes to dirty bitmaps, the
safest fallback path is to declare the entire image as dirty (the way
that we have a bit named NBD_STATE_HOLE that is 1 for a hole, but 0 for
data, because the safest fallback path is to treat the entire image as
data).
Why does this matter? Because with the 3.0 hack of qemu-img map with
x-dirty-bitmap=qemu:dirty-bitmap:FOO (commit 216ee365), if the client
requests a particular bitmap name but the server does NOT have that name
present, the client does NOT refuse to connect, but rather acts as
though bdrv_block_status() treats the entire image as data. Since the
hack relies on treating "data":false sections of the disk as dirty, but
the fallback causes qemu-img map to report "data":true for the entire
image, relying on the hack will end up seeing NO clusters as dirty, and
with NO indication that the x-dirty-bitmap= failed to work.
Had we used NBD_STATE_CLEAN (1 for clean, 0 for dirty), we would have at
least had the safe fallback of treating the entire image as dirty, which
would result in a full backup (a bit wasteful, but better than not
backing up any data on the incorrect assumption that the image is clean).
I'm in the process of implementing 'qemu-nbd --list' which will show
exactly which meta contexts an export supports, so that we can use that
as a fallback mechanism for a client to check whether the NBD server is
actually advertising the desired qemu:dirty-bitmap: context prior to
blindly requesting that context via x-dirty-bitmap. But even that hits
a slight snag:
@@ -924,6 +987,16 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
}
}
+if (meta->dirty_bitmap) {
+ret = nbd_negotiate_send_meta_context(client,
+ meta->exp->export_bitmap_context,
+ NBD_META_ID_DIRTY_BITMAP,
+ errp);
+if (ret < 0) {
+return ret;
+}
+}
+
The NBD spec says a client is allowed to issue NBD_OPT_LIST_META_CONTEXT
with 0 queries in order to learn about all supported contexts, but this
hunk forgot to advertise qemu:dirty-bitmap:FOO in that case. My full
patch series for 'qemu-nbd --list' is coming up soon, and includes a
patch that works around the 3.0/3.1 flaw by following up with a second
LIST_META_CONTEXT with 1 query of "qemu:' if the 0-query version didn't
get any "qemu:..." responses.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[Qemu-block] [PATCH] mirror dead-lock
Hi all!
We've faced the following mirror bug:
Just run mirror on qcow2 image more than 1G, and qemu is in dead lock.
Note: I've decided to send this as a patch with reproducer, to make it
easier to reproduce). No needs to commit this before mirror fix, but
after, commit message may be a bit shortened, may be even up to just
"iotests: add simple mirror test".
Note2: if drop 'kvm' option from the test it still reproduces, but if
use iotests.VM() - does not (may be, because of qtest?). If add
iothread, it doesn't reproduce too. Also, it doesn't reproduce with raw
instead of qcow2 and doesn't reproduce for small images.
So, here is the dead-lock:
(gdb) info thr
Id Target Id Frame
3Thread 0x7fd7fd4ea700 (LWP 145412) "qemu-system-x86" syscall () at
../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
2Thread 0x7fd7fc205700 (LWP 145416) "qemu-system-x86" __lll_lock_wait ()
at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
* 1Thread 0x7fd8102cfcc0 (LWP 145411) "qemu-system-x86" __lll_lock_wait ()
at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
(gdb) bt
#0 0x7fd80e8864cd in __lll_lock_wait () at
../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1 0x7fd80e881dcb in _L_lock_812 () at /lib64/libpthread.so.0
#2 0x7fd80e881c98 in __GI___pthread_mutex_lock (mutex=0x561032dce420
) at ../nptl/pthread_mutex_lock.c:79
#3 0x561032654c3b in qemu_mutex_lock_impl (mutex=0x561032dce420
, file=0x5610327d8654 "util/main-loop.c", line=236) at
util/qemu-thread-posix.c:66
#4 0x5610320cb327 in qemu_mutex_lock_iothread_impl (file=0x5610327d8654
"util/main-loop.c", line=236) at /work/src/qemu/up-mirror-dead-lock/cpus.c:1849
#5 0x56103265038f in os_host_main_loop_wait (timeout=480116000) at
util/main-loop.c:236
#6 0x561032650450 in main_loop_wait (nonblocking=0) at
util/main-loop.c:497
#7 0x5610322575c9 in main_loop () at vl.c:1892
#8 0x56103225f3c7 in main (argc=13, argv=0x7ffcc8bb15a8,
envp=0x7ffcc8bb1618) at vl.c:4648
(gdb) p qemu_global_mutex
$1 = {lock = {__data = {__lock = 2, __count = 0, __owner = 145416, __nusers =
1, __kind = 0, __spins = 0, __elision = 0, __list = {__prev = 0x0, __next =
0x0}},
__size = "\002\000\000\000\000\000\000\000\b8\002\000\001", '\000'
, __align = 2}, file = 0x56103267bcb0
"/work/src/qemu/up-mirror-dead-lock/exec.c", line = 3197, initialized = true}
So, we see, that thr1 is main loop, and now it wants BQL, which is
owned by thr2.
(gdb) thr 2
[Switching to thread 2 (Thread 0x7fd7fc205700 (LWP 145416))]
#0 __lll_lock_wait () at
../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
135 2: movl%edx, %eax
(gdb) bt
#0 0x7fd80e8864cd in __lll_lock_wait () at
../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1 0x7fd80e881de6 in _L_lock_870 () at /lib64/libpthread.so.0
#2 0x7fd80e881cdf in __GI___pthread_mutex_lock (mutex=0x561034d25dc0) at
../nptl/pthread_mutex_lock.c:114
#3 0x561032654c3b in qemu_mutex_lock_impl (mutex=0x561034d25dc0,
file=0x5610327d81d1 "util/async.c", line=511) at util/qemu-thread-posix.c:66
#4 0x56103264d840 in aio_context_acquire (ctx=0x561034d25d60) at
util/async.c:511
#5 0x561032254554 in dma_blk_cb (opaque=0x7fd7f41463b0, ret=0) at
dma-helpers.c:169
#6 0x56103225479d in dma_blk_io (ctx=0x561034d25d60, sg=0x561035a80210,
offset=0, align=512, io_func=0x5610322547a3 ,
io_func_opaque=0x561034d40870, cb=0x561032340b35 ,
opaque=0x561035a7fee8, dir=DMA_DIRECTION_FROM_DEVICE) at dma-helpers.c:227
#7 0x561032254855 in dma_blk_read (blk=0x561034d40870, sg=0x561035a80210,
offset=0, align=512, cb=0x561032340b35 , opaque=0x561035a7fee8) at
dma-helpers.c:245
#8 0x561032340e6e in ide_dma_cb (opaque=0x561035a7fee8, ret=0) at
hw/ide/core.c:910
#9 0x56103234a912 in bmdma_cmd_writeb (bm=0x561035a81030, val=9) at
hw/ide/pci.c:240
#10 0x56103234b5bd in bmdma_write (opaque=0x561035a81030, addr=0, val=9,
size=1) at hw/ide/piix.c:76
#11 0x5610320e5763 in memory_region_write_accessor (mr=0x561035a81180,
addr=0, value=0x7fd7fc204678, size=1, shift=0, mask=255, attrs=...) at
/work/src/qemu/up-mirror-dead-lock/memory.c:504
#12 0x5610320e596d in access_with_adjusted_size (addr=0,
value=0x7fd7fc204678, size=1, access_size_min=1, access_size_max=4, access_fn=
0x5610320e5683 , mr=0x561035a81180,
attrs=...) at /work/src/qemu/up-mirror-dead-lock/memory.c:570
#13 0x5610320e86ce in memory_region_dispatch_write (mr=0x561035a81180,
addr=0, data=9, size=1, attrs=...) at
/work/src/qemu/up-mirror-dead-lock/memory.c:1452
#14 0x561032083770 in flatview_write_continue (fv=0x7fd7f4108090,
addr=49216, attrs=..., buf=0x7fd810309000 "\t\371\061", len=1, addr1=0, l=1,
mr=0x561035a81180)
at /work/src/qemu/up-mirror-dead-lock/exec.c:3233
#15 0x5610320838cf in flatview_write (fv=0x7fd7f4108090, addr=49216,
attrs=..., buf=0x7fd810309000
Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
On Wed, Nov 28, 2018 at 05:17:10PM +, Paul Durrant wrote: > > Will we need a hotplug handler for this bus, like it is done with > > TYPE_XENSYSBUS? > > I didn't seem to need one even doing 'xl block-attach' after the VM had > booted. I'm really not sure what that does. Indeed, that works fine, and I think I've tested block-detach as well. Maybe we will need something if we initiate attach of a block device via QMP instead of xenstore, but we can fix that later. -- Anthony PERARD
Re: [Qemu-block] [Qemu-devel] [PULL 12/15] Acceptance tests: add make rule for running them
On Wed, Nov 07, 2018 at 12:24:55AM +0100, Paolo Bonzini wrote: > On 31/10/2018 01:31, Eduardo Habkost wrote: > > From: Cleber Rosa > > > > The acceptance (aka functional, aka Avocado-based) tests are > > Python files located in "tests/acceptance" that need to be run > > with the Avocado libs and test runner. > > > > Let's provide a convenient way for QEMU developers to run them, > > by making use of the tests-venv with the required setup. > > > > Also, while the Avocado test runner will take care of creating a > > location to save test results to, it was understood that it's better > > if the results are kept within the build tree. > > > > Signed-off-by: Cleber Rosa > > Acked-by: Stefan Hajnoczi > > Acked-by: Wainer dos Santos Moschetta > > Reviewed-by: Caio Carrara > > Message-Id: <20181018153134.8493-3-cr...@redhat.com> > > Signed-off-by: Eduardo Habkost > > --- > > docs/devel/testing.rst | 43 +- > > tests/requirements.txt | 1 + > > tests/Makefile.include | 21 +++-- > > 3 files changed, 58 insertions(+), 7 deletions(-) > > > > diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst > > index a227754f86..18e2c0868a 100644 > > --- a/docs/devel/testing.rst > > +++ b/docs/devel/testing.rst > > @@ -545,10 +545,39 @@ Tests based on ``avocado_qemu.Test`` can easily: > > - > > http://avocado-framework.readthedocs.io/en/latest/api/test/avocado.html#avocado.Test > > - > > http://avocado-framework.readthedocs.io/en/latest/api/utils/avocado.utils.html > > > > -Installation > > - > > +Running tests > > +- > > + > > +You can run the acceptance tests simply by executing: > > + > > +.. code:: > > + > > + make check-acceptance > > + > > +This involves the automatic creation of Python virtual environment > > +within the build tree (at ``tests/venv``) which will have all the > > +right dependencies, and will save tests results also within the > > +build tree (at ``tests/results``). > > > > -To install Avocado and its dependencies, run: > > +Note: the build environment must be using a Python 3 stack, and have > > +the ``venv`` and ``pip`` packages installed. If necessary, make sure > > +``configure`` is called with ``--python=`` and that those modules are > > +available. On Debian and Ubuntu based systems, depending on the > > +specific version, they may be on packages named ``python3-venv`` and > > +``python3-pip``. > > + > > +The scripts installed inside the virtual environment may be used > > +without an "activation". For instance, the Avocado test runner > > +may be invoked by running: > > + > > + .. code:: > > + > > + tests/venv/bin/avocado run $OPTION1 $OPTION2 tests/acceptance/ > > + > > +Manual Installation > > +--- > > + > > +To manually install Avocado and its dependencies, run: > > > > .. code:: > > > > @@ -689,11 +718,15 @@ The exact QEMU binary to be used on QEMUMachine. > > Uninstalling Avocado > > > > > > -If you've followed the installation instructions above, you can easily > > -uninstall Avocado. Start by listing the packages you have installed:: > > +If you've followed the manual installation instructions above, you can > > +easily uninstall Avocado. Start by listing the packages you have > > +installed:: > > > >pip list --user > > > > And remove any package you want with:: > > > >pip uninstall > > + > > +If you've used ``make check-acceptance``, the Python virtual environment > > where > > +Avocado is installed will be cleaned up as part of ``make check-clean``. > > diff --git a/tests/requirements.txt b/tests/requirements.txt > > index d39f9d1576..64c6e27a94 100644 > > --- a/tests/requirements.txt > > +++ b/tests/requirements.txt > > @@ -1,3 +1,4 @@ > > # Add Python module requirements, one per line, to be installed > > # in the tests/venv Python virtual environment. For more info, > > # refer to: https://pip.pypa.io/en/stable/user_guide/#id1 > > +avocado-framework==65.0 > > diff --git a/tests/Makefile.include b/tests/Makefile.include > > index eabc1da2f3..d2e577eabb 100644 > > --- a/tests/Makefile.include > > +++ b/tests/Makefile.include > > @@ -11,6 +11,7 @@ check-help: > > @echo " $(MAKE) check-qapi-schemaRun QAPI schema tests" > > @echo " $(MAKE) check-block Run block tests" > > @echo " $(MAKE) check-tcgRun TCG tests" > > + @echo " $(MAKE) check-acceptance Run all acceptance (functional) > > tests" > > @echo " $(MAKE) check-report.htmlGenerates an HTML test report" > > @echo " $(MAKE) check-venv Creates a Python venv for tests" > > @echo " $(MAKE) check-clean Clean the tests" > > @@ -902,10 +903,15 @@ check-decodetree: > > > > # Python venv for running tests > > > > -.PHONY: check-venv > > +.PHONY: check-venv check-acceptance > > > > TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv > > TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt > >
Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
> -Original Message-
> From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> Sent: 28 November 2018 17:10
> To: Paul Durrant
> Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen-
> de...@lists.xenproject.org; Stefano Stabellini ;
> Michael S. Tsirkin ; Marcel Apfelbaum
> ; Paolo Bonzini ; Richard
> Henderson ; Eduardo Habkost
> Subject: Re: [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice'
> object hierarchy
>
> On Wed, Nov 21, 2018 at 03:11:55PM +, Paul Durrant wrote:
> > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> > new file mode 100644
> > index 00..dede2d914a
> > --- /dev/null
> > +++ b/hw/xen/xen-bus.c
> > @@ -0,0 +1,125 @@
> > +/*
> > + * Copyright (c) Citrix Systems Inc.
> > + * All rights reserved.
>
> You probably should include:
> This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> See the COPYING file in the top-level directory.
>
> As this seems to be a boilerplate used in recent new files, there are
> other similair boilerplates.
>
> Also, I think the copyright line should include a year.
Ok.
>
> > +void xen_bus_init(void)
> > +{
> > +DeviceState *dev = qdev_create(NULL, TYPE_XEN_BRIDGE);
> > +
> > +qbus_create(TYPE_XEN_BUS, dev, NULL);
> > +qdev_init_nofail(dev);
>
> Will we need a hotplug handler for this bus, like it is done with
> TYPE_XENSYSBUS?
I didn't seem to need one even doing 'xl block-attach' after the VM had booted.
I'm really not sure what that does.
Paul
>
>
> The rest looks good,
> Thanks,
>
> --
> Anthony PERARD
Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
On Wed, Nov 21, 2018 at 03:11:55PM +, Paul Durrant wrote:
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> new file mode 100644
> index 00..dede2d914a
> --- /dev/null
> +++ b/hw/xen/xen-bus.c
> @@ -0,0 +1,125 @@
> +/*
> + * Copyright (c) Citrix Systems Inc.
> + * All rights reserved.
You probably should include:
This work is licensed under the terms of the GNU GPL, version 2 or later.
See the COPYING file in the top-level directory.
As this seems to be a boilerplate used in recent new files, there are
other similair boilerplates.
Also, I think the copyright line should include a year.
> +void xen_bus_init(void)
> +{
> +DeviceState *dev = qdev_create(NULL, TYPE_XEN_BRIDGE);
> +
> +qbus_create(TYPE_XEN_BUS, dev, NULL);
> +qdev_init_nofail(dev);
Will we need a hotplug handler for this bus, like it is done with
TYPE_XENSYSBUS?
The rest looks good,
Thanks,
--
Anthony PERARD
Re: [Qemu-block] [Qemu-devel] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
> -Original Message- > From: Eric Blake [mailto:ebl...@redhat.com] > Sent: 28 November 2018 17:01 > To: Paul Durrant ; 'Kevin Wolf' > > Cc: Stefano Stabellini ; Eduardo Habkost > ; qemu-block@nongnu.org; Michael S. Tsirkin > ; qemu-de...@nongnu.org; Paolo Bonzini > ; Anthony Perard ; xen- > de...@lists.xenproject.org; Richard Henderson > Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 02/18] xen: introduce new > 'XenBus' and 'XenDevice' object hierarchy > > On 11/28/18 10:26 AM, Paul Durrant wrote: > > >>> +++ b/hw/xen/xen-bus.c > >>> @@ -0,0 +1,125 @@ > >>> +/* > >>> + * Copyright (c) Citrix Systems Inc. > >>> + * All rights reserved. > >>> + */ > >> > >> This doesn't look very compatible with the GPL. In fact it might even > >> make it illegal for the QEMU project to distribute this code. :-) > >> > >> Other files you add throughout the series seem to have the same > problem. > >> > > > > I was working on the assumption that a lack of explicit license meant > that the overall project license as described in item 2 in LICENSE. Did I > misinterpret that text? > > No, but you missed the fact that "All rights reserved" is an explicit > license (or rather, an explicit anti-license that states you are not > granting rights that the GPL would normally grant), and an implicit > license does not apply when an explicit (anti-)license is present. > > What's more, relying on implicit licenses is prone to misinterpretation, > so even though the overall project documentation tries to cover what > will happen, it's much nicer if you DO use an explicit license mention > in your file so that we don't HAVE to rely on the implicit license. > > Yes, the phrase "All rights reserved" exists in several existing files: > > $ git grep -il 'all rights reserved' |wc > 138 1383557 > > but we should be striving to clean those up, not adding to the mess. Ok. I'll send a v2 with the "All rights reserved" removed from and a GPL statement added to all the new files. Paul > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
> -Original Message- > From: Kevin Wolf [mailto:kw...@redhat.com] > Sent: 28 November 2018 16:19 > To: Paul Durrant > Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen- > de...@lists.xenproject.org; Stefano Stabellini ; > Eduardo Habkost ; Michael S. Tsirkin > ; Marcel Apfelbaum ; Anthony > Perard ; Paolo Bonzini ; > Richard Henderson > Subject: Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and > 'XenDevice' object hierarchy > > Am 21.11.2018 um 16:11 hat Paul Durrant geschrieben: > > 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 > > > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c > > new file mode 100644 > > index 00..dede2d914a > > --- /dev/null > > +++ b/hw/xen/xen-bus.c > > @@ -0,0 +1,125 @@ > > +/* > > + * Copyright (c) Citrix Systems Inc. > > + * All rights reserved. > > + */ > > This doesn't look very compatible with the GPL. In fact it might even > make it illegal for the QEMU project to distribute this code. :-) > > Other files you add throughout the series seem to have the same problem. > I was working on the assumption that a lack of explicit license meant that the overall project license as described in item 2 in LICENSE. Did I misinterpret that text? Paul > Kevin
Re: [Qemu-block] [Qemu-devel] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
On 11/28/18 10:26 AM, Paul Durrant wrote: +++ b/hw/xen/xen-bus.c @@ -0,0 +1,125 @@ +/* + * Copyright (c) Citrix Systems Inc. + * All rights reserved. + */ This doesn't look very compatible with the GPL. In fact it might even make it illegal for the QEMU project to distribute this code. :-) Other files you add throughout the series seem to have the same problem. I was working on the assumption that a lack of explicit license meant that the overall project license as described in item 2 in LICENSE. Did I misinterpret that text? No, but you missed the fact that "All rights reserved" is an explicit license (or rather, an explicit anti-license that states you are not granting rights that the GPL would normally grant), and an implicit license does not apply when an explicit (anti-)license is present. What's more, relying on implicit licenses is prone to misinterpretation, so even though the overall project documentation tries to cover what will happen, it's much nicer if you DO use an explicit license mention in your file so that we don't HAVE to rely on the implicit license. Yes, the phrase "All rights reserved" exists in several existing files: $ git grep -il 'all rights reserved' |wc 138 1383557 but we should be striving to clean those up, not adding to the mess. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
> -Original Message- > From: Stefano Stabellini [mailto:sstabell...@kernel.org] > Sent: 28 November 2018 16:28 > To: Paul Durrant > Cc: 'Kevin Wolf' ; qemu-block@nongnu.org; qemu- > de...@nongnu.org; xen-de...@lists.xenproject.org; Stefano Stabellini > ; Eduardo Habkost ; Michael > S. Tsirkin ; Marcel Apfelbaum > ; Anthony Perard ; > Paolo Bonzini ; Richard Henderson > Subject: RE: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and > 'XenDevice' object hierarchy > > On Wed, 28 Nov 2018, Paul Durrant wrote: > > > -Original Message- > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > Sent: 28 November 2018 16:19 > > > To: Paul Durrant > > > Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen- > > > de...@lists.xenproject.org; Stefano Stabellini > ; > > > Eduardo Habkost ; Michael S. Tsirkin > > > ; Marcel Apfelbaum ; > Anthony > > > Perard ; Paolo Bonzini > ; > > > Richard Henderson > > > Subject: Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' > and > > > 'XenDevice' object hierarchy > > > > > > Am 21.11.2018 um 16:11 hat Paul Durrant geschrieben: > > > > 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 > > > > > > > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c > > > > new file mode 100644 > > > > index 00..dede2d914a > > > > --- /dev/null > > > > +++ b/hw/xen/xen-bus.c > > > > @@ -0,0 +1,125 @@ > > > > +/* > > > > + * Copyright (c) Citrix Systems Inc. > > > > + * All rights reserved. > > > > + */ > > > > > > This doesn't look very compatible with the GPL. In fact it might even > > > make it illegal for the QEMU project to distribute this code. :-) > > > > > > Other files you add throughout the series seem to have the same > problem. > > > > > > > I was working on the assumption that a lack of explicit license meant > that the overall project license as described in item 2 in LICENSE. Did I > misinterpret that text? > > It's "All rights reserved." the problem Oh, I see. I'm happy to remove that. Paul
Re: [Qemu-block] [PATCH 14/18] xen: add implementations of xen-qdisk connect and disconnect functions...
Am 21.11.2018 um 16:12 hat Paul Durrant geschrieben:
> ...and wire in the dataplane.
>
> This patch adds the remaining code to make the xen-qdisk 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
> ---
> Cc: Stefano Stabellini
> Cc: Anthony Perard
> Cc: Kevin Wolf
> Cc: Max Reitz
> ---
> hw/block/xen-qdisk.c | 140
> +
> hw/xen/xen-bus.c | 12 ++--
> include/hw/xen/xen-bus.h | 8 +++
> include/hw/xen/xen-qdisk.h | 12
> 4 files changed, 166 insertions(+), 6 deletions(-)
>
> 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
> @@ -9,6 +9,10 @@
> #include "qapi/visitor.h"
> #include "hw/hw.h"
> #include "hw/xen/xen-qdisk.h"
> +#include "sysemu/blockdev.h"
> +#include "sysemu/block-backend.h"
> +#include "sysemu/iothread.h"
> +#include "dataplane/xen-qdisk.h"
> #include "trace.h"
>
> static char *xen_qdisk_get_name(XenDevice *xendev, Error **errp)
> @@ -23,6 +27,11 @@ static void xen_qdisk_realize(XenDevice *xendev, Error
> **errp)
> {
> XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
> XenQdiskVdev *vdev = >vdev;
> +BlockConf *conf = >conf;
> +DriveInfo *dinfo;
> +bool is_cdrom;
> +unsigned int info;
> +int64_t size;
>
> if (!vdev->valid) {
> error_setg(errp, "vdev property not set");
> @@ -30,13 +39,134 @@ static void xen_qdisk_realize(XenDevice *xendev, Error
> **errp)
> }
>
> trace_xen_qdisk_realize(vdev->disk, vdev->partition);
> +
> +if (!conf->blk) {
> +error_setg(errp, "drive property not set");
> +return;
> +}
> +
> +if (!blk_is_inserted(conf->blk)) {
> +error_setg(errp, "device needs media, but drive is empty");
> +return;
> +}
Hm, the code below suggests that you support CD-ROMs. Don't you want to
support media change as well then? Which would mean that you need to
support empty drives.
> +if (!blkconf_apply_backend_options(conf, blk_is_read_only(conf->blk),
> + false, errp)) {
> +return;
> +}
> +
> +if (!blkconf_geometry(conf, NULL, 65535, 255, 255, errp)) {
> +return;
> +}
> +
> +dinfo = blk_legacy_dinfo(conf->blk);
> +is_cdrom = (dinfo && dinfo->media_cd);
It's called legacy for a reason. Don't use this in new devices.
The proper way is to have two different devices for hard disks and CDs
(like scsi-hd and scsi-cd).
Kevin
Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
On Wed, 28 Nov 2018, Paul Durrant wrote: > > -Original Message- > > From: Kevin Wolf [mailto:kw...@redhat.com] > > Sent: 28 November 2018 16:19 > > To: Paul Durrant > > Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen- > > de...@lists.xenproject.org; Stefano Stabellini ; > > Eduardo Habkost ; Michael S. Tsirkin > > ; Marcel Apfelbaum ; Anthony > > Perard ; Paolo Bonzini ; > > Richard Henderson > > Subject: Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and > > 'XenDevice' object hierarchy > > > > Am 21.11.2018 um 16:11 hat Paul Durrant geschrieben: > > > 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 > > > > > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c > > > new file mode 100644 > > > index 00..dede2d914a > > > --- /dev/null > > > +++ b/hw/xen/xen-bus.c > > > @@ -0,0 +1,125 @@ > > > +/* > > > + * Copyright (c) Citrix Systems Inc. > > > + * All rights reserved. > > > + */ > > > > This doesn't look very compatible with the GPL. In fact it might even > > make it illegal for the QEMU project to distribute this code. :-) > > > > Other files you add throughout the series seem to have the same problem. > > > > I was working on the assumption that a lack of explicit license meant that > the overall project license as described in item 2 in LICENSE. Did I > misinterpret that text? It's "All rights reserved." the problem
Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
> -Original Message- > From: Paul Durrant > Sent: 28 November 2018 16:27 > To: 'Kevin Wolf' > Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen- > de...@lists.xenproject.org; Stefano Stabellini ; > Eduardo Habkost ; Michael S. Tsirkin > ; Marcel Apfelbaum ; Anthony > Perard ; Paolo Bonzini ; > Richard Henderson > Subject: RE: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and > 'XenDevice' object hierarchy > > > -Original Message- > > From: Kevin Wolf [mailto:kw...@redhat.com] > > Sent: 28 November 2018 16:19 > > To: Paul Durrant > > Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen- > > de...@lists.xenproject.org; Stefano Stabellini ; > > Eduardo Habkost ; Michael S. Tsirkin > > ; Marcel Apfelbaum ; Anthony > > Perard ; Paolo Bonzini ; > > Richard Henderson > > Subject: Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and > > 'XenDevice' object hierarchy > > > > Am 21.11.2018 um 16:11 hat Paul Durrant geschrieben: > > > 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 > > > > > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c > > > new file mode 100644 > > > index 00..dede2d914a > > > --- /dev/null > > > +++ b/hw/xen/xen-bus.c > > > @@ -0,0 +1,125 @@ > > > +/* > > > + * Copyright (c) Citrix Systems Inc. > > > + * All rights reserved. > > > + */ > > > > This doesn't look very compatible with the GPL. In fact it might even > > make it illegal for the QEMU project to distribute this code. :-) > > > > Other files you add throughout the series seem to have the same problem. > > > > I was working on the assumption that a lack of explicit license meant that > the overall project license as described in item 2 in LICENSE. Did I ^ applied > misinterpret that text? > > Paul > > > Kevin
Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
Am 28.11.2018 um 17:29 hat Paul Durrant geschrieben: > > -Original Message- > > From: Stefano Stabellini [mailto:sstabell...@kernel.org] > > Sent: 28 November 2018 16:28 > > To: Paul Durrant > > Cc: 'Kevin Wolf' ; qemu-block@nongnu.org; qemu- > > de...@nongnu.org; xen-de...@lists.xenproject.org; Stefano Stabellini > > ; Eduardo Habkost ; Michael > > S. Tsirkin ; Marcel Apfelbaum > > ; Anthony Perard ; > > Paolo Bonzini ; Richard Henderson > > Subject: RE: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and > > 'XenDevice' object hierarchy > > > > On Wed, 28 Nov 2018, Paul Durrant wrote: > > > > -Original Message- > > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > > Sent: 28 November 2018 16:19 > > > > To: Paul Durrant > > > > Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen- > > > > de...@lists.xenproject.org; Stefano Stabellini > > ; > > > > Eduardo Habkost ; Michael S. Tsirkin > > > > ; Marcel Apfelbaum ; > > Anthony > > > > Perard ; Paolo Bonzini > > ; > > > > Richard Henderson > > > > Subject: Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' > > and > > > > 'XenDevice' object hierarchy > > > > > > > > Am 21.11.2018 um 16:11 hat Paul Durrant geschrieben: > > > > > 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 > > > > > > > > > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c > > > > > new file mode 100644 > > > > > index 00..dede2d914a > > > > > --- /dev/null > > > > > +++ b/hw/xen/xen-bus.c > > > > > @@ -0,0 +1,125 @@ > > > > > +/* > > > > > + * Copyright (c) Citrix Systems Inc. > > > > > + * All rights reserved. > > > > > + */ > > > > > > > > This doesn't look very compatible with the GPL. In fact it might even > > > > make it illegal for the QEMU project to distribute this code. :-) > > > > > > > > Other files you add throughout the series seem to have the same > > problem. > > > > > > > > > > I was working on the assumption that a lack of explicit license meant > > that the overall project license as described in item 2 in LICENSE. Did I > > misinterpret that text? > > > > It's "All rights reserved." the problem > > Oh, I see. I'm happy to remove that. That would be better at least. I'm not sure about files that have a copyright header, but no license statement. Do such files exist yet in the source tree? To be on the safe side, I'd just stick with the established practice, which is having a license header in every file. By the way, in a later patch you remove the existing license header, which is different from the default license (because parts of the source file are GPL 2 only). If you can't prove that all such parts (and parts derived from them) have been removed, this is also a problem. Kevin
Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
> -Original Message- > From: Kevin Wolf [mailto:kw...@redhat.com] > Sent: 28 November 2018 16:39 > To: Paul Durrant > Cc: 'Stefano Stabellini' ; qemu-block@nongnu.org; > qemu-de...@nongnu.org; xen-de...@lists.xenproject.org; Eduardo Habkost > ; Michael S. Tsirkin ; Marcel > Apfelbaum ; Anthony Perard > ; Paolo Bonzini ; Richard > Henderson > Subject: Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and > 'XenDevice' object hierarchy > > Am 28.11.2018 um 17:29 hat Paul Durrant geschrieben: > > > -Original Message- > > > From: Stefano Stabellini [mailto:sstabell...@kernel.org] > > > Sent: 28 November 2018 16:28 > > > To: Paul Durrant > > > Cc: 'Kevin Wolf' ; qemu-block@nongnu.org; qemu- > > > de...@nongnu.org; xen-de...@lists.xenproject.org; Stefano Stabellini > > > ; Eduardo Habkost ; > Michael > > > S. Tsirkin ; Marcel Apfelbaum > > > ; Anthony Perard > ; > > > Paolo Bonzini ; Richard Henderson > > > > Subject: RE: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' > and > > > 'XenDevice' object hierarchy > > > > > > On Wed, 28 Nov 2018, Paul Durrant wrote: > > > > > -Original Message- > > > > > From: Kevin Wolf [mailto:kw...@redhat.com] > > > > > Sent: 28 November 2018 16:19 > > > > > To: Paul Durrant > > > > > Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen- > > > > > de...@lists.xenproject.org; Stefano Stabellini > > > ; > > > > > Eduardo Habkost ; Michael S. Tsirkin > > > > > ; Marcel Apfelbaum ; > > > Anthony > > > > > Perard ; Paolo Bonzini > > > ; > > > > > Richard Henderson > > > > > Subject: Re: [Qemu-block] [PATCH 02/18] xen: introduce new > 'XenBus' > > > and > > > > > 'XenDevice' object hierarchy > > > > > > > > > > Am 21.11.2018 um 16:11 hat Paul Durrant geschrieben: > > > > > > 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 > > > > > > > > > > > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c > > > > > > new file mode 100644 > > > > > > index 00..dede2d914a > > > > > > --- /dev/null > > > > > > +++ b/hw/xen/xen-bus.c > > > > > > @@ -0,0 +1,125 @@ > > > > > > +/* > > > > > > + * Copyright (c) Citrix Systems Inc. > > > > > > + * All rights reserved. > > > > > > + */ > > > > > > > > > > This doesn't look very compatible with the GPL. In fact it might > even > > > > > make it illegal for the QEMU project to distribute this code. :-) > > > > > > > > > > Other files you add throughout the series seem to have the same > > > problem. > > > > > > > > > > > > > I was working on the assumption that a lack of explicit license > meant > > > that the overall project license as described in item 2 in LICENSE. > Did I > > > misinterpret that text? > > > > > > It's "All rights reserved." the problem > > > > Oh, I see. I'm happy to remove that. > > That would be better at least. I'm not sure about files that have a > copyright header, but no license statement. Do such files exist yet in > the source tree? Yes, there's quite a few... e.g. (first ones I tripped over) hw/rdma/rdma_backend.c, hw/virtio/vhost-backend.c, ... > To be on the safe side, I'd just stick with the > established practice, which is having a license header in every file. Ok... if that is established practice. It really wasn't clear. > > By the way, in a later patch you remove the existing license header, > which is different from the default license (because parts of the source > file are GPL 2 only). If you can't prove that all such parts (and parts > derived from them) have been removed, this is also a problem. > What should I do? I am duplicating xen_disk, and then heavily modifying it. Should I just leave the old boilerplate in place? Paul > Kevin
Re: [Qemu-block] [PATCH 14/18] xen: add implementations of xen-qdisk connect and disconnect functions...
> -Original Message-
> From: Kevin Wolf [mailto:kw...@redhat.com]
> Sent: 28 November 2018 16:35
> To: Paul Durrant
> Cc: qemu-block@nongnu.org; qemu-de...@nongnu.org; xen-
> de...@lists.xenproject.org; Stefano Stabellini ;
> Anthony Perard ; Max Reitz
> Subject: Re: [PATCH 14/18] xen: add implementations of xen-qdisk connect
> and disconnect functions...
>
> Am 21.11.2018 um 16:12 hat Paul Durrant geschrieben:
> > ...and wire in the dataplane.
> >
> > This patch adds the remaining code to make the xen-qdisk 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
> > ---
> > Cc: Stefano Stabellini
> > Cc: Anthony Perard
> > Cc: Kevin Wolf
> > Cc: Max Reitz
> > ---
> > hw/block/xen-qdisk.c | 140
> +
> > hw/xen/xen-bus.c | 12 ++--
> > include/hw/xen/xen-bus.h | 8 +++
> > include/hw/xen/xen-qdisk.h | 12
> > 4 files changed, 166 insertions(+), 6 deletions(-)
> >
> > 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
> > @@ -9,6 +9,10 @@
> > #include "qapi/visitor.h"
> > #include "hw/hw.h"
> > #include "hw/xen/xen-qdisk.h"
> > +#include "sysemu/blockdev.h"
> > +#include "sysemu/block-backend.h"
> > +#include "sysemu/iothread.h"
> > +#include "dataplane/xen-qdisk.h"
> > #include "trace.h"
> >
> > static char *xen_qdisk_get_name(XenDevice *xendev, Error **errp)
> > @@ -23,6 +27,11 @@ static void xen_qdisk_realize(XenDevice *xendev,
> Error **errp)
> > {
> > XenQdiskDevice *qdiskdev = XEN_QDISK_DEVICE(xendev);
> > XenQdiskVdev *vdev = >vdev;
> > +BlockConf *conf = >conf;
> > +DriveInfo *dinfo;
> > +bool is_cdrom;
> > +unsigned int info;
> > +int64_t size;
> >
> > if (!vdev->valid) {
> > error_setg(errp, "vdev property not set");
> > @@ -30,13 +39,134 @@ static void xen_qdisk_realize(XenDevice *xendev,
> Error **errp)
> > }
> >
> > trace_xen_qdisk_realize(vdev->disk, vdev->partition);
> > +
> > +if (!conf->blk) {
> > +error_setg(errp, "drive property not set");
> > +return;
> > +}
> > +
> > +if (!blk_is_inserted(conf->blk)) {
> > +error_setg(errp, "device needs media, but drive is empty");
> > +return;
> > +}
>
> Hm, the code below suggests that you support CD-ROMs. Don't you want to
> support media change as well then? Which would mean that you need to
> support empty drives.
Yes, that's a good point. I should get rid of that check.
>
> > +if (!blkconf_apply_backend_options(conf, blk_is_read_only(conf-
> >blk),
> > + false, errp)) {
> > +return;
> > +}
> > +
> > +if (!blkconf_geometry(conf, NULL, 65535, 255, 255, errp)) {
> > +return;
> > +}
> > +
> > +dinfo = blk_legacy_dinfo(conf->blk);
> > +is_cdrom = (dinfo && dinfo->media_cd);
>
> It's called legacy for a reason. Don't use this in new devices.
>
> The proper way is to have two different devices for hard disks and CDs
> (like scsi-hd and scsi-cd).
...or presumably I could have a property? The legacy init code could then set
it based on the drive info.
Paul
>
> Kevin
Re: [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
Am 21.11.2018 um 16:11 hat Paul Durrant geschrieben: > 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 > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c > new file mode 100644 > index 00..dede2d914a > --- /dev/null > +++ b/hw/xen/xen-bus.c > @@ -0,0 +1,125 @@ > +/* > + * Copyright (c) Citrix Systems Inc. > + * All rights reserved. > + */ This doesn't look very compatible with the GPL. In fact it might even make it illegal for the QEMU project to distribute this code. :-) Other files you add throughout the series seem to have the same problem. Kevin
Re: [Qemu-block] [PATCH 01/18] xen: re-name XenDevice to XenLegacyDevice...
On Wed, Nov 21, 2018 at 03:11:54PM +, Paul Durrant wrote: > ...and xen_backend.h to xen-legacy-backend.h > > Rather than attempting to convert the existing backend infrastructure to > be QOM compliant (which would be hard to do in an incremental fashion), > subsequent patches will introduce a completely new framework for Xen PV > backends. Hence it is necessary to re-name parts of existing code to avoid > name clashes. The re-named 'legacy' infrastructure will be removed once all > backends have been ported to the new framework. > > This patch is purely cosmetic. No functional change. Acked-by: Anthony PERARD -- Anthony PERARD
