Re: [Libguestfs] [libnbd PATCH v3 05/22] states: Prepare to receive 64-bit replies
On Thu, Jun 01, 2023 at 08:00:55AM -0500, Eric Blake wrote:
> > > @@ -83,13 +96,18 @@ REPLY.STRUCTURED_REPLY.CHECK:
> > > * not worth keeping the connection alive.
> > > */
> > >if (length > MAX_REQUEST_SIZE + sizeof
> > > h->sbuf.reply.payload.offset_data) {
> > > -set_error (0, "invalid server reply length %" PRIu32, length);
> > > +set_error (0, "invalid server reply length %" PRIu64, length);
> > > SET_NEXT_STATE (%.DEAD);
> > > return 0;
> > >}
> > > + /* For convenience, we now normalize extended replies into compact,
> > > + * doable since we validated that payload length fits in 32 bits.
> > > + */
> > > + h->sbuf.reply.hdr.structured.length = length;
> >
> > (8) I'm very confused by this. For an extended reply, this will
> > overwrite the "offset" field.
>
> At one point, I considered normalizing in the opposite direction (take
> structured replies and widen them into the extended header form); it
> required touching more lines of code, so I didn't pursue it at the
> time. But I can see how compressing things down (intentionally
> throwing away information we will no longer reference) can be
> confusing without good comments, so at a minimum, I will be adding
> comments, if not outright going with the widening rather than
> narrowing approach.
In fact, doing it in place is bad. Later code in this function can
trigger a transition to state RESYNC, which assumes
sbuf.reply.hdr.structured.length still holds the wire value, not the
normalized value. I'm fixing it up by creating h->payload_left,
initialized in START to the endian-corrected wire value, and then
decremented as various states peel off parts of the payload, so that
FINISH can then assert all payload has been accounted for.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH 5/5] cmd646: move device-specific BMDMA registers to separate memory region
On 12/6/23 21:28, Bernhard Beschow wrote:
Am 9. Juni 2023 18:51:19 UTC schrieb Mark Cave-Ayland
:
The aim here is to eliminate any device-specific registers from the main BMDMA
bar memory region so it can potentially be moved into the shared PCI IDE device.
For each BMDMA bus create a new cmd646-bmdma-specific memory region representing
the device-specific BMDMA registers and then map them using aliases onto the
existing BMDMAState memory region.
Signed-off-by: Mark Cave-Ayland
---
hw/ide/cmd646.c | 111 +++-
include/hw/ide/cmd646.h | 4 ++
2 files changed, 90 insertions(+), 25 deletions(-)
struct CMD646IDEState {
PCIIDEState parent_obj;
+
+MemoryRegion bmdma_mem[2];
+MemoryRegion bmdma_mem_alias[2][2];
The added complexity of a two-dimensional alias array seems like a tough call
for me. I'm not totally against it but I'm reluctant.
Alternative:
struct {
MemoryRegion mem;
MemoryRegion mem_alias[2];
} bmdma[2];
};
#endif
Re: [RFC 4/6] migration: Deprecate -incoming
On Mon, Jun 12, 2023 at 10:51:08PM +0200, Juan Quintela wrote:
> Peter Xu wrote:
> > On Mon, Jun 12, 2023 at 09:33:42PM +0200, Juan Quintela wrote:
> >> Only "defer" is recommended. After setting all migation parameters,
> >> start incoming migration with "migrate-incoming uri" command.
> >>
> >> Signed-off-by: Juan Quintela
> >> ---
> >> docs/about/deprecated.rst | 7 +++
> >> softmmu/vl.c | 2 ++
> >> 2 files changed, 9 insertions(+)
> >>
> >> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> >> index 47e98dc95e..518672722d 100644
> >> --- a/docs/about/deprecated.rst
> >> +++ b/docs/about/deprecated.rst
> >> @@ -447,3 +447,10 @@ The new way to modify migration is using migration
> >> parameters.
> >> ``blk`` functionality can be acchieved using
> >> ``migrate_set_parameter block-incremental true``.
> >>
> >> +``-incoming uri`` (since 8.1)
> >> +'
> >> +
> >> +Everything except ``-incoming defer`` are deprecated. This allows to
> >> +setup parameters before launching the proper migration with
> >> +``migrate-incoming uri``.
> >> +
> >> diff --git a/softmmu/vl.c b/softmmu/vl.c
> >> index b0b96f67fa..7fe865ab59 100644
> >> --- a/softmmu/vl.c
> >> +++ b/softmmu/vl.c
> >> @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp)
> >> if (incoming) {
> >> Error *local_err = NULL;
> >> if (strcmp(incoming, "defer") != 0) {
> >> +warn_report("-incoming %s is deprecated, use -incoming defer
> >> and "
> >> +" set the uri with migrate-incoming.", incoming);
> >
> > I still use uri for all my scripts, alongside with "-global migration.xxx"
> > and it works.
>
> You know what you are doing (TM).
> And remember that we don't support -gobal migration.x-foo.
> Yes, I know, we should drop the "x-" prefixes.
I hope they'll always be there. :) They're pretty handy for tests, when we
want to boot a VM without the need to script the sequences of qmp cmds.
Yes, we probably should just always drop the x-. We can always declare
debugging purpose for all -global migration.* fields.
>
> > Shall we just leave it there? Or is deprecating it helps us in any form?
>
> See the patches two weeks ago when people complained that lisen(.., num)
> was too low. And there are other parameters that work the same way
> (that I convenientely had forgotten). So the easiest way to get things
> right is to use "defer" always. Using -incoming "uri" should only be
> for people that "know what they are doing", so we had to ways to do it:
> - review all migration options and see which ones work without defer
> and document it
> - deprecate everything that is not defer.
>
> Anything else is not going to be very user unfriendly.
> What do you think.
IIRC Wei Wang had a series just for that, so after that patchset applied we
should have fixed all issues cleanly? Is there one more thing that's not
working right there?
>
> Later, Juan.
>
> PD. This series are RFC for multiple reasons O:-)
Happy to know the rest (besides which I know will break my script :).
--
Peter Xu
Re: [RFC 4/6] migration: Deprecate -incoming
Peter Xu wrote:
> On Mon, Jun 12, 2023 at 09:33:42PM +0200, Juan Quintela wrote:
>> Only "defer" is recommended. After setting all migation parameters,
>> start incoming migration with "migrate-incoming uri" command.
>>
>> Signed-off-by: Juan Quintela
>> ---
>> docs/about/deprecated.rst | 7 +++
>> softmmu/vl.c | 2 ++
>> 2 files changed, 9 insertions(+)
>>
>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>> index 47e98dc95e..518672722d 100644
>> --- a/docs/about/deprecated.rst
>> +++ b/docs/about/deprecated.rst
>> @@ -447,3 +447,10 @@ The new way to modify migration is using migration
>> parameters.
>> ``blk`` functionality can be acchieved using
>> ``migrate_set_parameter block-incremental true``.
>>
>> +``-incoming uri`` (since 8.1)
>> +'
>> +
>> +Everything except ``-incoming defer`` are deprecated. This allows to
>> +setup parameters before launching the proper migration with
>> +``migrate-incoming uri``.
>> +
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index b0b96f67fa..7fe865ab59 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp)
>> if (incoming) {
>> Error *local_err = NULL;
>> if (strcmp(incoming, "defer") != 0) {
>> +warn_report("-incoming %s is deprecated, use -incoming defer
>> and "
>> +" set the uri with migrate-incoming.", incoming);
>
> I still use uri for all my scripts, alongside with "-global migration.xxx"
> and it works.
You know what you are doing (TM).
And remember that we don't support -gobal migration.x-foo.
Yes, I know, we should drop the "x-" prefixes.
> Shall we just leave it there? Or is deprecating it helps us in any form?
See the patches two weeks ago when people complained that lisen(.., num)
was too low. And there are other parameters that work the same way
(that I convenientely had forgotten). So the easiest way to get things
right is to use "defer" always. Using -incoming "uri" should only be
for people that "know what they are doing", so we had to ways to do it:
- review all migration options and see which ones work without defer
and document it
- deprecate everything that is not defer.
Anything else is not going to be very user unfriendly.
What do you think.
Later, Juan.
PD. This series are RFC for multiple reasons O:-)
Re: [RFC 4/6] migration: Deprecate -incoming
On Mon, Jun 12, 2023 at 09:33:42PM +0200, Juan Quintela wrote:
> Only "defer" is recommended. After setting all migation parameters,
> start incoming migration with "migrate-incoming uri" command.
>
> Signed-off-by: Juan Quintela
> ---
> docs/about/deprecated.rst | 7 +++
> softmmu/vl.c | 2 ++
> 2 files changed, 9 insertions(+)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 47e98dc95e..518672722d 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -447,3 +447,10 @@ The new way to modify migration is using migration
> parameters.
> ``blk`` functionality can be acchieved using
> ``migrate_set_parameter block-incremental true``.
>
> +``-incoming uri`` (since 8.1)
> +'
> +
> +Everything except ``-incoming defer`` are deprecated. This allows to
> +setup parameters before launching the proper migration with
> +``migrate-incoming uri``.
> +
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index b0b96f67fa..7fe865ab59 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp)
> if (incoming) {
> Error *local_err = NULL;
> if (strcmp(incoming, "defer") != 0) {
> +warn_report("-incoming %s is deprecated, use -incoming defer and
> "
> +" set the uri with migrate-incoming.", incoming);
I still use uri for all my scripts, alongside with "-global migration.xxx"
and it works.
Shall we just leave it there? Or is deprecating it helps us in any form?
--
Peter Xu
[RFC 6/6] migration: Deprecated old compression method
Signed-off-by: Juan Quintela
---
docs/about/deprecated.rst | 8
qapi/migration.json | 92 ---
migration/options.c | 13 ++
3 files changed, 79 insertions(+), 34 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 173c5ba5cb..fe7f2bbde8 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -460,3 +460,11 @@ block migration (since 8.1)
Block migration is too inflexible. It needs to migrate all block
devices or none. Use driver_mirror+NBD instead.
+old compression method (since 8.1)
+''
+
+Compression method fails too much. Too many races. We are going to
+remove it if nobody fixes it. For starters, migration-test
+compression tests are disabled becase they hand randomly. If you need
+compression, use multifd compression methods.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index a8497de48d..40a8b5d124 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -244,6 +244,7 @@
#
# @compression: migration compression statistics, only returned if
# compression feature is on and status is 'active' or 'completed'
+# It is obsolete and deprecated. Use multifd compression methods.
# (Since 3.1)
#
# @socket-address: Only used for tcp, to know what the real port is
@@ -261,7 +262,8 @@
# Features:
#
# @deprecated: @disk migration is deprecated. Use driver_mirror+NBD
-# instead.
+# instead. @compression is obsolete use multifd compression
+# methods instead.
#
# Since: 0.14
##
@@ -279,7 +281,7 @@
'*blocked-reasons': ['str'],
'*postcopy-blocktime': 'uint32',
'*postcopy-vcpu-blocktime': ['uint32'],
- '*compression': 'CompressionStats',
+ '*compression': { 'type': 'CompressionStats', 'features':
['deprecated'] },
'*socket-address': ['SocketAddress'] } }
##
@@ -432,7 +434,8 @@
# compress and xbzrle are both on, compress only takes effect in
# the ram bulk stage, after that, it will be disabled and only
# xbzrle takes effect, this can help to minimize migration
-# traffic. The feature is disabled by default. (since 2.4 )
+# traffic. The feature is disabled by default. Obsolete. Use
+# multifd compression methods if needed. (since 2.4 )
#
# @events: generate events for each migration state change (since 2.4
# )
@@ -503,6 +506,7 @@
# Features:
#
# @deprecated: @block migration is deprecated. Use driver_mirror+NBD
+# instead. @compress is obsolete use multifd compression methods
# instead.
#
# @unstable: Members @x-colo and @x-ignore-shared are experimental.
@@ -511,7 +515,8 @@
##
{ 'enum': 'MigrationCapability',
'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
- 'compress', 'events', 'postcopy-ram',
+ { 'name': 'compress', 'features': [ 'deprecated' ] },
+ 'events', 'postcopy-ram',
{ 'name': 'x-colo', 'features': [ 'unstable' ] },
'release-ram',
{ 'name': 'block', 'features': [ 'deprecated' ] },
@@ -671,22 +676,24 @@
# migration, the compression level is an integer between 0 and 9,
# where 0 means no compression, 1 means the best compression
# speed, and 9 means best compression ratio which will consume
-# more CPU.
+# more CPU. Obsolete, see multifd compression if needed.
#
# @compress-threads: Set compression thread count to be used in live
# migration, the compression thread count is an integer between 1
-# and 255.
+# and 255. Obsolete, see multifd compression if needed.
#
# @compress-wait-thread: Controls behavior when all compression
# threads are currently busy. If true (default), wait for a free
# compression thread to become available; otherwise, send the page
-# uncompressed. (Since 3.1)
+# uncompressed. Obsolete, see multifd compression if
+# needed. (Since 3.1)
#
# @decompress-threads: Set decompression thread count to be used in
# live migration, the decompression thread count is an integer
# between 1 and 255. Usually, decompression is at least 4 times as
# fast as compression, so set the decompress-threads to the number
-# about 1/4 of compress-threads is adequate.
+# about 1/4 of compress-threads is adequate. Obsolete, see multifd
+# compression if needed.
#
# @throttle-trigger-threshold: The ratio of bytes_dirty_period and
# bytes_xfer_period to trigger throttling. It is expressed as
@@ -799,7 +806,9 @@
# Features:
#
# @deprecated: Member @block-incremental is obsolete. Use
-# driver_mirror+NBD instead.
+# driver_mirror+NBD instead. Compression is obsolete, so members
+# @compress-level, @compress-threads, @decompress-threads and
+# @compress-wait-thread should not be used.
#
# @unstable: Member @x-checkpoint-delay is experimental.
#
@@ -808,8 +817,11 @@
{ 'enum': 'Migra
[RFC 0/6] Migration deprecated parts
Hi this series describe the migration parts that have to be deprecated. - It is an rfc because I doubt that I did the deprecation process right. Hello Markus O:-) - skipped field: It is older than me, I have never know what it stands for. As far as I know it has always been zero. - inc/blk migrate command options. They are only used by block migration (that I deprecate on the following patch). And they are really bad. grep must_remove_block_options. - block migration. block jobs, whatever they are called this week are way more flexible. Current code works, but we broke it here and there, and really nobody has stand up to maintain it. It is quite contained and can be left there. Is anyone really using it? - old compression method. It don't work. See last try from Lukas to make a test that works reliabely. I failed with the same task years ago. It is really slow, and if compression is good for you, multifd + zlib is going to perform/compress way more. I don't know what to do with this code, really. * Remove it for this release? It don't work, and haven't work reliabely in quite a few time. * Deprecate it and remove in another couple of releases, i.e. normal deprecation. * Ideas? - -incoming if you need to set parameters (multifd cames to mind, and preempt has the same problem), you really needs to use defer. So what should we do here? This part is not urget, because management apps have a working option that are already using "defer", and the code simplifacation if we remove it is not so big. So we can leave it until 9.0 or whatever we think fit. What do you think? Later, Juan. Juan Quintela (6): migration: skipped field is really obsolete. migration: migrate 'inc' command option is deprecated. migration: migrate 'blk' command option is deprecated. migration: Deprecate -incoming migration: Deprecate block migration migration: Deprecated old compression method docs/about/deprecated.rst | 45 qapi/migration.json | 141 +++--- migration/block.c | 2 + migration/migration.c | 10 +++ migration/options.c | 20 ++ softmmu/vl.c | 2 + 6 files changed, 181 insertions(+), 39 deletions(-) base-commit: 5f9dd6a8ce3961db4ce47411ed2097ad88bdf5fc prerequisite-patch-id: 99c8bffa9428838925e330eb2881bab476122579 prerequisite-patch-id: 77ba427fd916aeb395e95aa0e7190f84e98e96ab prerequisite-patch-id: 9983d46fa438d7075a37be883529e37ae41e4228 prerequisite-patch-id: 207f7529924b12dcb57f6557d6db6f79ceb2d682 prerequisite-patch-id: 5ad1799a13845dbf893a28a202b51a6b50d95d90 prerequisite-patch-id: c51959aacd6d65ee84fcd4f1b2aed3dd6f6af879 prerequisite-patch-id: da9dbb6799b2da002c0896574334920097e4c50a prerequisite-patch-id: c1110ffafbaf5465fb277a20db809372291f7846 prerequisite-patch-id: 8307c92bedd07446214b35b40206eb6793a7384d prerequisite-patch-id: 0a6106cd4a508d5e700a7ff6c25edfdd03c8ca3d prerequisite-patch-id: 83205051de22382e75bf4acdf69e59315801fa0d prerequisite-patch-id: 8c9b3cba89d555c071a410041e6da41806106a7e prerequisite-patch-id: 0ff62a33b9a242226ccc1f5424a516de803c9fe5 prerequisite-patch-id: 25b8ae1ebe09ace14457c454cfcb23077c37346c prerequisite-patch-id: 466ea91d5be41fe345dacd4d17bbbe5ce13118c2 prerequisite-patch-id: d1045858f9729ac62eccf2e83ebf95cfebae2cb5 prerequisite-patch-id: 0276ec02073bda5426de39e2f2e81eef080b4f54 prerequisite-patch-id: 7afb4450a163cc1a63ea23831c50214966969131 prerequisite-patch-id: 06c053ce4f41db9675bd1778ae8f6a483641fcef prerequisite-patch-id: 13ea05d54d741ed08b3bfefa1fc8bedb9c81c782 prerequisite-patch-id: 99c4e2b7101bc8c4b9515129a1bbe6f068053dbf prerequisite-patch-id: 1e393a196dc7a1ee75f3cc3cebbb591c5422102f prerequisite-patch-id: 2cf497b41f5024ede0a224b1f5b172226067a534 prerequisite-patch-id: 2a70276ed61d33fc4f3b52560753c05d1cd413be prerequisite-patch-id: 17ec40f4388b62ba8bf3ac1546c6913f5d1f6079 prerequisite-patch-id: dba969ce9d6cf69c1319661a7d81b1c1c719804d prerequisite-patch-id: 8d800cda87167314f07320bdb3df936c323e4a40 prerequisite-patch-id: 25d4aaf54ea66f30e426fa38bdd4e0f47303c513 prerequisite-patch-id: 082c9d8584c1daff1e827e44ee3047178e7004a7 prerequisite-patch-id: 0ef73900899425ae2f00751347afdce3739aa954 prerequisite-patch-id: e7db4730b791b71aaf417ee0f65fb6304566aaf8 prerequisite-patch-id: 62d7f28f8196039507ffe362f97723395d7bb704 prerequisite-patch-id: ea8de47bcb54e33bcc67e59e9ed752a4d1fad703 prerequisite-patch-id: 497893ef92e1ea56bd8605e6990a05cb4c7f9293 prerequisite-patch-id: 3dc869c80ee568449bbfa2a9bc427524d0e8970b prerequisite-patch-id: 52c14b6fb14ed4ccd685385a9fbc6297b762c0ef prerequisite-patch-id: 23de8371e9e3277c374a47f9bd10de209a22fdd5 prerequisite-patch-id: d21f036dd106af3375fb920bf0a557fe2b86d98e prerequisite-patch-id: ca717076e9de83d6ce370f7374c4729a9f586fae prerequisite-patch-id: a235b6ab3237155f2b39e8e10d47ddd250f6b6cc prerequisite-patch-id: 6db2aa3d8a5804c85dd171864f5140fadf5f72da prerequisite-patch-i
[RFC 4/6] migration: Deprecate -incoming
Only "defer" is recommended. After setting all migation parameters,
start incoming migration with "migrate-incoming uri" command.
Signed-off-by: Juan Quintela
---
docs/about/deprecated.rst | 7 +++
softmmu/vl.c | 2 ++
2 files changed, 9 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 47e98dc95e..518672722d 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -447,3 +447,10 @@ The new way to modify migration is using migration
parameters.
``blk`` functionality can be acchieved using
``migrate_set_parameter block-incremental true``.
+``-incoming uri`` (since 8.1)
+'
+
+Everything except ``-incoming defer`` are deprecated. This allows to
+setup parameters before launching the proper migration with
+``migrate-incoming uri``.
+
diff --git a/softmmu/vl.c b/softmmu/vl.c
index b0b96f67fa..7fe865ab59 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2651,6 +2651,8 @@ void qmp_x_exit_preconfig(Error **errp)
if (incoming) {
Error *local_err = NULL;
if (strcmp(incoming, "defer") != 0) {
+warn_report("-incoming %s is deprecated, use -incoming defer and "
+" set the uri with migrate-incoming.", incoming);
qmp_migrate_incoming(incoming, &local_err);
if (local_err) {
error_reportf_err(local_err, "-incoming %s: ", incoming);
--
2.40.1
[RFC 2/6] migration: migrate 'inc' command option is deprecated.
Use 'migrate_set_parameter block_incremental true' instead.
Signed-off-by: Juan Quintela
---
docs/about/deprecated.rst | 7 +++
qapi/migration.json | 11 +--
migration/migration.c | 5 +
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index e1aa0eafc8..c75a3a8f5a 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -433,3 +433,10 @@ Migration
``skipped`` field in Migration stats has been deprecated. It hasn't
been used for more than 10 years.
+``inc`` migrate command option (since 8.1)
+''
+
+The new way to modify migration is using migration parameters.
+``inc`` functionality can be acchieved using
+``migrate_set_parameter block-incremental true``.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index bcae193733..4ee28df6da 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1424,13 +1424,19 @@
#
# @blk: do block migration (full disk copy)
#
-# @inc: incremental disk copy migration
+# @inc: incremental disk copy migration. This option is deprecated.
+#Use 'migrate_set_parameter block-incremetantal true' instead.
#
# @detach: this argument exists only for compatibility reasons and is
# ignored by QEMU
#
# @resume: resume one paused migration, default "off". (since 3.0)
#
+# Features:
+#
+# @deprecated: option @inc is better set with
+# 'migrate_set_parameter block-incremental true'.
+#
# Returns: nothing on success
#
# Since: 0.14
@@ -1452,7 +1458,8 @@
# <- { "return": {} }
##
{ 'command': 'migrate',
- 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
+ 'data': {'uri': 'str', '*blk': 'bool',
+ '*inc': { 'type': 'bool', 'features': ['deprecated'] },
'*detach': 'bool', '*resume': 'bool' } }
##
diff --git a/migration/migration.c b/migration/migration.c
index dc05c6f6ea..7ebce7c7bf 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1544,6 +1544,11 @@ static bool migrate_prepare(MigrationState *s, bool blk,
bool blk_inc,
{
Error *local_err = NULL;
+if (blk_inc) {
+warn_report("-inc migrate option is deprecated, use"
+"'migrate_set_parameter block-incremental true' instead.");
+}
+
if (resume) {
if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
error_setg(errp, "Cannot resume if there is no "
--
2.40.1
[RFC 3/6] migration: migrate 'blk' command option is deprecated.
Use 'migrate_set_capability block true' instead.
Signed-off-by: Juan Quintela
---
docs/about/deprecated.rst | 7 +++
qapi/migration.json | 11 +++
migration/migration.c | 5 +
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index c75a3a8f5a..47e98dc95e 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -440,3 +440,10 @@ The new way to modify migration is using migration
parameters.
``inc`` functionality can be acchieved using
``migrate_set_parameter block-incremental true``.
+``blk`` migrate command option (since 8.1)
+''
+
+The new way to modify migration is using migration parameters.
+``blk`` functionality can be acchieved using
+``migrate_set_parameter block-incremental true``.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index 4ee28df6da..b71e00737e 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1422,7 +1422,8 @@
#
# @uri: the Uniform Resource Identifier of the destination VM
#
-# @blk: do block migration (full disk copy)
+# @blk: do block migration (full disk copy). This option is deprecated.
+#Use 'migrate_set_capability block true' instead.
#
# @inc: incremental disk copy migration. This option is deprecated.
#Use 'migrate_set_parameter block-incremetantal true' instead.
@@ -1434,8 +1435,9 @@
#
# Features:
#
-# @deprecated: option @inc is better set with
-# 'migrate_set_parameter block-incremental true'.
+# @deprecated: options @inc and @blk are better set with
+# 'migrate_set_parameter block-incremental true' and
+# 'migrate_set_capability block true' respectively.
#
# Returns: nothing on success
#
@@ -1458,7 +1460,8 @@
# <- { "return": {} }
##
{ 'command': 'migrate',
- 'data': {'uri': 'str', '*blk': 'bool',
+ 'data': {'uri': 'str',
+ '*blk': { 'type': 'bool', 'features': ['deprecated'] },
'*inc': { 'type': 'bool', 'features': ['deprecated'] },
'*detach': 'bool', '*resume': 'bool' } }
diff --git a/migration/migration.c b/migration/migration.c
index 7ebce7c7bf..b7d5f6b96c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1549,6 +1549,11 @@ static bool migrate_prepare(MigrationState *s, bool blk,
bool blk_inc,
"'migrate_set_parameter block-incremental true' instead.");
}
+if (blk) {
+warn_report("-blk migrate option is deprecated, use"
+"'migrate_set_capability block true' instead.");
+}
+
if (resume) {
if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
error_setg(errp, "Cannot resume if there is no "
--
2.40.1
[RFC 5/6] migration: Deprecate block migration
It is obsolete. It is better to use driver_mirror+NBD instead.
CC: Kevin Wolf
CC: Eric Blake
CC: Stefan Hajnoczi
CC: Hanna Czenczek
Signed-off-by: Juan Quintela
---
Can any of you give one example of how to use driver_mirror+NBD for
deprecated.rst?
Thanks, Juan.
---
docs/about/deprecated.rst | 6 ++
qapi/migration.json | 29 +
migration/block.c | 2 ++
migration/options.c | 7 +++
4 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 518672722d..173c5ba5cb 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -454,3 +454,9 @@ Everything except ``-incoming defer`` are deprecated. This
allows to
setup parameters before launching the proper migration with
``migrate-incoming uri``.
+block migration (since 8.1)
+'''
+
+Block migration is too inflexible. It needs to migrate all block
+devices or none. Use driver_mirror+NBD instead.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index b71e00737e..a8497de48d 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -258,11 +258,16 @@
# blocked. Present and non-empty when migration is blocked.
# (since 6.0)
#
+# Features:
+#
+# @deprecated: @disk migration is deprecated. Use driver_mirror+NBD
+# instead.
+#
# Since: 0.14
##
{ 'struct': 'MigrationInfo',
'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
- '*disk': 'MigrationStats',
+ '*disk': { 'type': 'MigrationStats', 'features': ['deprecated'] },
'*vfio': 'VfioStats',
'*xbzrle-cache': 'XBZRLECacheStats',
'*total-time': 'int',
@@ -497,6 +502,9 @@
#
# Features:
#
+# @deprecated: @block migration is deprecated. Use driver_mirror+NBD
+# instead.
+#
# @unstable: Members @x-colo and @x-ignore-shared are experimental.
#
# Since: 1.2
@@ -506,7 +514,8 @@
'compress', 'events', 'postcopy-ram',
{ 'name': 'x-colo', 'features': [ 'unstable' ] },
'release-ram',
- 'block', 'return-path', 'pause-before-switchover', 'multifd',
+ { 'name': 'block', 'features': [ 'deprecated' ] },
+ 'return-path', 'pause-before-switchover', 'multifd',
'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
{ 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
'validate-uuid', 'background-snapshot',
@@ -789,6 +798,9 @@
#
# Features:
#
+# @deprecated: Member @block-incremental is obsolete. Use
+# driver_mirror+NBD instead.
+#
# @unstable: Member @x-checkpoint-delay is experimental.
#
# Since: 2.4
@@ -803,7 +815,7 @@
'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
'downtime-limit',
{ 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] },
- 'block-incremental',
+ { 'name': 'block-incremental', 'features': [ 'deprecated' ] },
'multifd-channels',
'xbzrle-cache-size', 'max-postcopy-bandwidth',
'max-cpu-throttle', 'multifd-compression',
@@ -945,6 +957,9 @@
#
# Features:
#
+# @deprecated: Member @block-incremental is obsolete. Use
+# driver_mirror+NBD instead.
+#
# @unstable: Member @x-checkpoint-delay is experimental.
#
# TODO: either fuse back into MigrationParameters, or make
@@ -972,7 +987,8 @@
'*downtime-limit': 'uint64',
'*x-checkpoint-delay': { 'type': 'uint32',
'features': [ 'unstable' ] },
-'*block-incremental': 'bool',
+'*block-incremental': { 'type': 'bool',
+'features': [ 'deprecated' ] },
'*multifd-channels': 'uint8',
'*xbzrle-cache-size': 'size',
'*max-postcopy-bandwidth': 'size',
@@ -1137,6 +1153,9 @@
#
# Features:
#
+# @deprecated: Member @block-incremental is obsolete. Use
+# driver_mirror+NBD instead.
+#
# @unstable: Member @x-checkpoint-delay is experimental.
#
# Since: 2.4
@@ -1161,6 +1180,8 @@
'*downtime-limit': 'uint64',
'*x-checkpoint-delay': { 'type': 'uint32',
'features': [ 'unstable' ] },
+'*block-incremental': { 'type': 'bool',
+'features': [ 'deprecated' ] },
'*block-incremental': 'bool',
'*multifd-channels': 'uint8',
'*xbzrle-cache-size': 'size',
diff --git a/migration/block.c b/migration/block.c
index b9580a6c7e..2521a22269 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -722,6 +722,8 @@ static int block_save_setup(QEMUFile *f, void *opaque)
trace_migration_block_save("setup", block_mig_state.submitted,
block_mig_state.transferred);
+warn_report("block migration is deprecated. Use mirror jobs instead.");
+
qemu_
[RFC 1/6] migration: skipped field is really obsolete.
Has return zero for more than 10 years. Just mark it deprecated.
Signed-off-by: Juan Quintela
---
docs/about/deprecated.rst | 10 ++
qapi/migration.json | 12 ++--
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 0743459862..e1aa0eafc8 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -423,3 +423,13 @@ both, older and future versions of QEMU.
The ``blacklist`` config file option has been renamed to ``block-rpcs``
(to be in sync with the renaming of the corresponding command line
option).
+
+Migration
+-
+
+``skipped`` MigrationStats field (since 8.1)
+
+
+``skipped`` field in Migration stats has been deprecated. It hasn't
+been used for more than 10 years.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index cb7cd3e578..bcae193733 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -23,7 +23,8 @@
#
# @duplicate: number of duplicate (zero) pages (since 1.2)
#
-# @skipped: number of skipped zero pages (since 1.5)
+# @skipped: number of skipped zero pages. Don't use, only provided for
+# compatibility (since 1.5)
#
# @normal: number of normal pages (since 1.2)
#
@@ -62,11 +63,18 @@
# between 0 and @dirty-sync-count * @multifd-channels. (since
# 7.1)
#
+# Features:
+#
+# @deprecated: Member @skipped has not been used for a long time.
+#
# Since: 0.14
+#
##
{ 'struct': 'MigrationStats',
'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
- 'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
+ 'duplicate': 'int',
+ 'skipped': { 'type': 'int', 'features': ['deprecated'] },
+ 'normal': 'int',
'normal-bytes': 'int', 'dirty-pages-rate': 'int',
'mbps': 'number', 'dirty-sync-count': 'int',
'postcopy-requests': 'int', 'page-size': 'int',
--
2.40.1
Re: [PATCH 5/5] cmd646: move device-specific BMDMA registers to separate memory region
Am 9. Juni 2023 18:51:19 UTC schrieb Mark Cave-Ayland
:
>The aim here is to eliminate any device-specific registers from the main BMDMA
>bar memory region so it can potentially be moved into the shared PCI IDE
>device.
>
>For each BMDMA bus create a new cmd646-bmdma-specific memory region
>representing
>the device-specific BMDMA registers and then map them using aliases onto the
>existing BMDMAState memory region.
>
>Signed-off-by: Mark Cave-Ayland
>---
> hw/ide/cmd646.c | 111 +++-
> include/hw/ide/cmd646.h | 4 ++
> 2 files changed, 90 insertions(+), 25 deletions(-)
>
>diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>index 9103da581f..dd495f2e1b 100644
>--- a/hw/ide/cmd646.c
>+++ b/hw/ide/cmd646.c
>@@ -90,7 +90,6 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
>unsigned size)
> {
> BMDMAState *bm = opaque;
>-PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
> uint32_t val;
>
> if (size != 1) {
>@@ -101,19 +100,9 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
> case 0:
> val = bm->cmd;
> break;
>-case 1:
>-val = pci_dev->config[MRDMODE];
>-break;
> case 2:
> val = bm->status;
> break;
>-case 3:
>-if (bm == &bm->pci_dev->bmdma[0]) {
>-val = pci_dev->config[UDIDETCR0];
>-} else {
>-val = pci_dev->config[UDIDETCR1];
>-}
>-break;
> default:
> val = 0xff;
> break;
>@@ -127,7 +116,6 @@ static void bmdma_write(void *opaque, hwaddr addr,
> uint64_t val, unsigned size)
> {
> BMDMAState *bm = opaque;
>-PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
>
> if (size != 1) {
> return;
>@@ -138,23 +126,10 @@ static void bmdma_write(void *opaque, hwaddr addr,
> case 0:
> bmdma_cmd_writeb(bm, val);
> break;
>-case 1:
>-pci_dev->config[MRDMODE] =
>-(pci_dev->config[MRDMODE] & ~0x30) | (val & 0x30);
>-cmd646_update_dma_interrupts(pci_dev);
>-cmd646_update_irq(pci_dev);
>-break;
> case 2:
> bm->status = (val & 0x60) | (bm->status & 1) |
> (bm->status & ~val & 0x06);
> break;
>-case 3:
>-if (bm == &bm->pci_dev->bmdma[0]) {
>-pci_dev->config[UDIDETCR0] = val;
>-} else {
>-pci_dev->config[UDIDETCR1] = val;
>-}
>-break;
> }
> }
>
>@@ -181,6 +156,91 @@ static void bmdma_setup_bar(PCIIDEState *d)
> }
> }
>
>+static uint64_t cmd646_bmdma_read(void *opaque, hwaddr addr, unsigned size)
>+{
>+BMDMAState *bm = opaque;
>+PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
>+uint32_t val;
>+
>+if (size != 1) {
>+return ((uint64_t)1 << (size * 8)) - 1;
>+}
>+
>+switch (addr & 3) {
>+case 1:
>+val = pci_dev->config[MRDMODE];
>+break;
>+case 3:
>+if (bm == &bm->pci_dev->bmdma[0]) {
>+val = pci_dev->config[UDIDETCR0];
>+} else {
>+val = pci_dev->config[UDIDETCR1];
>+}
>+break;
>+default:
>+val = 0xff;
>+break;
>+}
>+
>+trace_bmdma_read_cmd646(addr, val);
>+return val;
>+}
>+
>+static void cmd646_bmdma_write(void *opaque, hwaddr addr, uint64_t val,
>+ unsigned size)
>+{
>+BMDMAState *bm = opaque;
>+PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
>+
>+if (size != 1) {
>+return;
>+}
>+
>+trace_bmdma_write_cmd646(addr, val);
>+switch (addr & 3) {
>+case 1:
>+pci_dev->config[MRDMODE] =
>+(pci_dev->config[MRDMODE] & ~0x30) | (val & 0x30);
>+cmd646_update_dma_interrupts(pci_dev);
>+cmd646_update_irq(pci_dev);
>+break;
>+case 3:
>+if (bm == &bm->pci_dev->bmdma[0]) {
>+pci_dev->config[UDIDETCR0] = val;
>+} else {
>+pci_dev->config[UDIDETCR1] = val;
>+}
>+break;
>+}
>+}
>+
>+static const MemoryRegionOps cmd646_bmdma_ops = {
>+.read = cmd646_bmdma_read,
>+.write = cmd646_bmdma_write,
>+};
>+
>+static void cmd646_bmdma_setup(PCIIDEState *d)
>+{
>+CMD646IDEState *s = CMD646_IDE(d);
>+BMDMAState *bm;
>+int i;
>+
>+/* Setup aliases for device-specific BMDMA registers */
>+for (i = 0; i < 2; i++) {
I'd use `ARRAY_SIZE()` instead of the hardcoded constant here.
>+bm = &d->bmdma[i];
>+memory_region_init_io(&s->bmdma_mem[i], OBJECT(d), &cmd646_bmdma_ops,
>+ bm, "cmd646-bmdma", 4);
>+memory_region_init_alias(&s->bmdma_mem_alias[i][0], OBJECT(d),
>+ "cmd646-bmdma[1]", &s->bmdma_mem[i], 0x1, 1);
>+memory_region_add_subregion(&bm->extra_io, 0x1,
>+&s->bmdma_mem_alias[i][0]);
>+memory_region_init_alias(&s->bmdma_mem_alias[i][1], OBJECT
Re: [Libguestfs] [PATCH v4 09/24] nbd: Replace bool structured_reply with mode enum
On Mon, Jun 12, 2023 at 06:07:59PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 08.06.23 16:56, Eric Blake wrote:
> > The upcoming patches for 64-bit extensions requires various points in
> > the protocol to make decisions based on what was negotiated. While we
> > could easily add a 'bool extended_headers' alongside the existing
> > 'bool structured_reply', this does not scale well if more modes are
> > added in the future. Better is to expose the mode enum added in the
> > previous patch out to a wider use in the code base.
> >
> > Where the code previously checked for structured_reply being set or
> > clear, it now prefers checking for an inequality; this works because
> > the nodes are in a continuum of increasing abilities, and allows us to
> > touch fewer places if we ever insert other modes in the middle of the
> > enum. There should be no semantic change in this patch.
> >
> > Signed-off-by: Eric Blake
> > ---
> >
> > v4: new patch, expanding enum idea from v3 4/14
> > ---
>
> [..]
>
> > diff --git a/nbd/server.c b/nbd/server.c
> > index 8486b64b15d..bade4f7990c 100644
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -1261,13 +1262,13 @@ static int nbd_negotiate_options(NBDClient *client,
> > Error **errp)
> > case NBD_OPT_STRUCTURED_REPLY:
> > if (length) {
> > ret = nbd_reject_length(client, false, errp);
> > -} else if (client->structured_reply) {
> > +} else if (client->mode >= NBD_MODE_STRUCTURED) {
> > ret = nbd_negotiate_send_rep_err(
> > client, NBD_REP_ERR_INVALID, errp,
> > "structured reply already negotiated");
> > } else {
> > ret = nbd_negotiate_send_rep(client, NBD_REP_ACK,
> > errp);
> > -client->structured_reply = true;
> > +client->mode = NBD_MODE_STRUCTURED;
>
> Hmm. in all other cases in server code client.mode remains zero = OLDSTYLE,
> which is not quite correct.
Good catch. Consider this squashed in (note that as a server we NEVER
talk NBD_MODE_OLDSTYLE - we ripped that out back in commit 7f7dfe2a;
but whether we end up on EXPORT_NAME or SIMPLE depends on the client's
response to our initial flag advertisement. The only reason I didn't
spot it sooner is that in the server, all subsequent checks of
client->mode grouped OLDSTYLE, EXPORT_NAME, and SIMPLE into the same
handling.
diff --git a/nbd/server.c b/nbd/server.c
index bade4f7990c..bc6858cafe6 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1123,10 +1123,12 @@ static int nbd_negotiate_options(NBDClient *client,
Error **errp)
if (nbd_read32(client->ioc, &flags, "flags", errp) < 0) {
return -EIO;
}
+client->mode = NBD_MODE_EXPORT_NAME;
trace_nbd_negotiate_options_flags(flags);
if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) {
fixedNewstyle = true;
flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
+client->mode = NBD_MODE_SIMPLE;
}
if (flags & NBD_FLAG_C_NO_ZEROES) {
no_zeroes = true;
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v4 06/24] nbd/client: Simplify cookie vs. index computation
On Mon, Jun 12, 2023 at 05:27:19PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 08.06.23 16:56, Eric Blake wrote: > > Our code relies on a sentinel cookie value of zero for deciding when a > > packet has been handled, as well as relying on array indices between 0 > > and MAX_NBD_REQUESTS-1 for dereferencing purposes. As long as we can > > symmetrically convert between two forms, there is no reason to go with > > the odd choice of using XOR with a random pointer, when we can instead > > simplify the mappings with a mere offset of 1. > > Should we go further and use (uint64)-1 as a sentinel cookie value, and just > use index as a cookie? Or, using zero cookie in a wire looks too asymmetric? I thought about that too, but in the end I decided it would require auditing more lines of code to make sure I was catching all places where we currently expected a zero sentinel (where some of those uses are not obvious, because of things like hiding behind g_new0). And there is indeed the argument that if data corruption is going to happen, it's harder to tell if an all-zero field on the wire was intentional than a non-zero field. > > > > > Signed-off-by: Eric Blake > > Reviewed-by: Vladimir Sementsov-Ogievskiy Thanks; for now, I'll just leave this one as-is. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/5] cmd646: create separate header and QOM type for CMD646_IDE
On 12/06/2023 10:21, Bernhard Beschow wrote:
Am 9. Juni 2023 18:51:16 UTC schrieb Mark Cave-Ayland
:
This will enable CMD646-specific fields to be added to CMD6464IDEState in
future.
Signed-off-by: Mark Cave-Ayland
---
hw/ide/cmd646.c | 4 +++-
include/hw/ide/cmd646.h | 38 ++
2 files changed, 41 insertions(+), 1 deletion(-)
create mode 100644 include/hw/ide/cmd646.h
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 20f1e41d57..a3e227fba7 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -33,6 +33,7 @@
#include "sysemu/reset.h"
#include "hw/ide/pci.h"
+#include "hw/ide/cmd646.h"
#include "trace.h"
/* CMD646 specific */
@@ -339,9 +340,10 @@ static void cmd646_ide_class_init(ObjectClass *klass, void
*data)
}
static const TypeInfo cmd646_ide_info = {
-.name = "cmd646-ide",
+.name = TYPE_CMD646_IDE,
.parent= TYPE_PCI_IDE,
.class_init= cmd646_ide_class_init,
+.instance_size = sizeof(CMD646IDEState),
};
static void cmd646_ide_register_types(void)
diff --git a/include/hw/ide/cmd646.h b/include/hw/ide/cmd646.h
new file mode 100644
index 00..4780b1132c
--- /dev/null
+++ b/include/hw/ide/cmd646.h
@@ -0,0 +1,38 @@
+/*
+ * QEMU IDE Emulation: PCI cmd646 support.
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * Copyright (c) 2006 Openedhand Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef HW_IDE_CMD646_H
+#define HW_IDE_CMD646_H
+
+#define TYPE_CMD646_IDE "cmd646-ide"
+OBJECT_DECLARE_SIMPLE_TYPE(CMD646IDEState, CMD646_IDE)
+
+#include "hw/ide/pci.h"
+
+struct CMD646IDEState {
+PCIIDEState parent_obj;
No public / private sections here? From the naming this attribute is often
considered private and the rest public. I guess what this scheme communicates
is that `parent_obj` should be accessed via `IDE_PCI()` only.
Whilst the public/private comments were included in the original QOM documentation,
it's not something that tends to be used now. These days people are looking to use
interfaces such as SysBus to indicated which MemoryRegions and IRQs are "public", but
certainly the detail is still up for active discussion.
The QOM cast macros will allow the object referenced to be converted to any of the
parent types directly, so indeed there should be no direct references to parent_obj.
ATB,
Mark.
Re: [PATCH v4 11/24] nbd: Add types for extended headers
On 08.06.23 16:56, Eric Blake wrote: Add the constants and structs necessary for later patches to start implementing the NBD_OPT_EXTENDED_HEADERS extension in both the client and server, matching recent upstream nbd.git (through commit e6f3b94a934). This patch does not change any existing behavior, but merely sets the stage for upcoming patches. This patch does not change the status quo that neither the client nor server use a packed-struct representation for the request header. While most of the patch adds new types, there is also some churn for renaming the existing NBDExtent to NBDExtent32 to contrast it with NBDExtent64, which I thought was a nicer name than NBDExtentExt. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH v4 10/24] nbd/client: Pass mode through to nbd_send_request
On 08.06.23 16:56, Eric Blake wrote: Once the 64-bit headers extension is enabled, the data layout we send over the wire for a client request depends on the mode negotiated with the server. Rather than adding a parameter to nbd_send_request, we can add a member to struct NBDRequest, since it already does not reflect on-wire format. Some callers initialize it directly; many others rely on a common initialization point during nbd_co_send_request(). At this point, there is no semantic change. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH v4 09/24] nbd: Replace bool structured_reply with mode enum
On 08.06.23 16:56, Eric Blake wrote:
The upcoming patches for 64-bit extensions requires various points in
the protocol to make decisions based on what was negotiated. While we
could easily add a 'bool extended_headers' alongside the existing
'bool structured_reply', this does not scale well if more modes are
added in the future. Better is to expose the mode enum added in the
previous patch out to a wider use in the code base.
Where the code previously checked for structured_reply being set or
clear, it now prefers checking for an inequality; this works because
the nodes are in a continuum of increasing abilities, and allows us to
touch fewer places if we ever insert other modes in the middle of the
enum. There should be no semantic change in this patch.
Signed-off-by: Eric Blake
---
v4: new patch, expanding enum idea from v3 4/14
---
[..]
diff --git a/nbd/server.c b/nbd/server.c
index 8486b64b15d..bade4f7990c 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -143,7 +143,7 @@ struct NBDClient {
uint32_t check_align; /* If non-zero, check for aligned client requests */
-bool structured_reply;
+NBDMode mode;
NBDExportMetaContexts export_meta;
uint32_t opt; /* Current option being negotiated */
@@ -502,7 +502,7 @@ static int nbd_negotiate_handle_export_name(NBDClient
*client, bool no_zeroes,
}
myflags = client->exp->nbdflags;
-if (client->structured_reply) {
+if (client->mode >= NBD_MODE_STRUCTURED) {
myflags |= NBD_FLAG_SEND_DF;
}
trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags);
@@ -687,7 +687,7 @@ static int nbd_negotiate_handle_info(NBDClient *client,
Error **errp)
/* Send NBD_INFO_EXPORT always */
myflags = exp->nbdflags;
-if (client->structured_reply) {
+if (client->mode >= NBD_MODE_STRUCTURED) {
myflags |= NBD_FLAG_SEND_DF;
}
trace_nbd_negotiate_new_style_size_flags(exp->size, myflags);
@@ -985,7 +985,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
size_t i;
size_t count = 0;
-if (client->opt == NBD_OPT_SET_META_CONTEXT && !client->structured_reply) {
+if (client->opt == NBD_OPT_SET_META_CONTEXT &&
+client->mode < NBD_MODE_STRUCTURED) {
return nbd_opt_invalid(client, errp,
"request option '%s' when structured reply "
"is not negotiated",
@@ -1261,13 +1262,13 @@ static int nbd_negotiate_options(NBDClient *client,
Error **errp)
case NBD_OPT_STRUCTURED_REPLY:
if (length) {
ret = nbd_reject_length(client, false, errp);
-} else if (client->structured_reply) {
+} else if (client->mode >= NBD_MODE_STRUCTURED) {
ret = nbd_negotiate_send_rep_err(
client, NBD_REP_ERR_INVALID, errp,
"structured reply already negotiated");
} else {
ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
-client->structured_reply = true;
+client->mode = NBD_MODE_STRUCTURED;
Hmm. in all other cases in server code client.mode remains zero = OLDSTYLE,
which is not quite correct.
}
break;
@@ -1907,7 +1908,9 @@ static int coroutine_fn
nbd_co_send_simple_reply(NBDClient *client,
};
assert(!len || !nbd_err);
-assert(!client->structured_reply || request->type != NBD_CMD_READ);
+assert(client->mode < NBD_MODE_STRUCTURED ||
+ (client->mode == NBD_MODE_STRUCTURED &&
+request->type != NBD_CMD_READ));
trace_nbd_co_send_simple_reply(request->cookie, nbd_err,
nbd_err_lookup(nbd_err), len);
set_be_simple_reply(&reply, nbd_err, request->cookie);
@@ -2409,7 +2412,7 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req, NBDRequest *
client->check_align);
}
valid_flags = NBD_CMD_FLAG_FUA;
[..]
--
Best regards,
Vladimir
Re: [PATCH v4 08/24] nbd: Use enum for various negotiation modes
On 08.06.23 16:56, Eric Blake wrote: Deciphering the hard-coded list of integer return values from nbd_start_negotiate() will only get more confusing when adding support for 64-bit extended headers. Better is to name things in an enum. Although the function in question is private to client.c, putting the enum in a public header and including an enum-to-string conversion will allow its use in more places in upcoming patches. The enum is intentionally laid out so that operators like <= can be used to group multiple modes with similar characteristics, and where the least powerful mode has value 0, even though this patch does not exploit that. No semantic change intended. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH v4 06/24] nbd/client: Simplify cookie vs. index computation
On 08.06.23 16:56, Eric Blake wrote: Our code relies on a sentinel cookie value of zero for deciding when a packet has been handled, as well as relying on array indices between 0 and MAX_NBD_REQUESTS-1 for dereferencing purposes. As long as we can symmetrically convert between two forms, there is no reason to go with the odd choice of using XOR with a random pointer, when we can instead simplify the mappings with a mere offset of 1. Should we go further and use (uint64)-1 as a sentinel cookie value, and just use index as a cookie? Or, using zero cookie in a wire looks too asymmetric? Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- v4: new patch --- block/nbd.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index be3c46c6fee..5322e66166c 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -50,8 +50,8 @@ #define EN_OPTSTR ":exportname=" #define MAX_NBD_REQUESTS16 -#define COOKIE_TO_INDEX(bs, cookie) ((cookie) ^ (uint64_t)(intptr_t)(bs)) -#define INDEX_TO_COOKIE(bs, index) ((index) ^ (uint64_t)(intptr_t)(bs)) That looked like some security trick to hide real indices. But I don't think that index of request in a list is a secret information. +#define COOKIE_TO_INDEX(cookie) ((cookie) - 1) +#define INDEX_TO_COOKIE(index) ((index) + 1) [..] -- Best regards, Vladimir
Re: [PATCH v4 05/24] nbd: s/handle/cookie/ to match NBD spec
On 08.06.23 16:56, Eric Blake wrote: Externally, libnbd exposed the 64-bit opaque marker for each client NBD packet as the "cookie", because it was less confusing when contrasted with 'struct nbd_handle *' holding all libnbd state. It also avoids confusion between the nown 'handle' as a way to identify a packet and the verb 'handle' for reacting to things like signals. Upstream NBD changed their spec to favor the name "cookie" based on libnbd's recommendations[1], so we can do likewise. [1]https://github.com/NetworkBlockDevice/nbd/commit/ca4392eb2b Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH v4 03/24] nbd/server: Prepare for alternate-size headers
On 08.06.23 16:56, Eric Blake wrote: Upstream NBD now documents[1] an extension that supports 64-bit effect lengths in requests. As part of that extension, the size of the reply headers will change in order to permit a 64-bit length in the reply for symmetry[2]. Additionally, where the reply header is currently 16 bytes for simple reply, and 20 bytes for structured reply; with the extension enabled, there will only be one extended reply header, of 32 bytes, with both structured and extended modes sending identical payloads for chunked replies. Since we are already wired up to use iovecs, it is easiest to allow for this change in header size by splitting each structured reply across multiple iovecs, one for the header (which will become wider in a future patch according to client negotiation), and the other(s) for the chunk payload, and removing the header from the payload struct definitions. Rename the affected functions with s/structured/chunk/ to make it obvious that the code will be reused in extended mode. Interestingly, the client side code never utilized the packed types, so only the server code needs to be updated. [1]https://github.com/NetworkBlockDevice/nbd/blob/extension-ext-header/doc/proto.md as of NBD commit e6f3b94a934 [2] Note that on the surface, this is because some future server might permit a 4G+ NBD_CMD_READ and need to reply with that much data in one transaction. But even though the extended reply length is widened to 64 bits, for now the NBD spec is clear that servers will not reply with more than a maximum payload bounded by the 32-bit NBD_INFO_BLOCK_SIZE field; allowing a client and server to mutually agree to transactions larger than 4G would require yet another extension. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH 1/5] cmd646: checkpatch fixes
On 9/6/23 20:51, Mark Cave-Ayland wrote: Signed-off-by: Mark Cave-Ayland --- hw/ide/cmd646.c | 13 +++-- 1 file changed, 7 insertions(+), 6 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 4/5] cmd646: rename cmd646_bmdma_ops to bmdma_ops
On 9/6/23 20:51, Mark Cave-Ayland wrote: This is to allow us to use the cmd646_bmdma_ops name for the CMD646 device-specific registers in the next commit. Signed-off-by: Mark Cave-Ayland --- hw/ide/cmd646.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 2/5] cmd646: create separate header and QOM type for CMD646_IDE
Am 9. Juni 2023 18:51:16 UTC schrieb Mark Cave-Ayland
:
>This will enable CMD646-specific fields to be added to CMD6464IDEState in
>future.
>
>Signed-off-by: Mark Cave-Ayland
>---
> hw/ide/cmd646.c | 4 +++-
> include/hw/ide/cmd646.h | 38 ++
> 2 files changed, 41 insertions(+), 1 deletion(-)
> create mode 100644 include/hw/ide/cmd646.h
>
>diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>index 20f1e41d57..a3e227fba7 100644
>--- a/hw/ide/cmd646.c
>+++ b/hw/ide/cmd646.c
>@@ -33,6 +33,7 @@
> #include "sysemu/reset.h"
>
> #include "hw/ide/pci.h"
>+#include "hw/ide/cmd646.h"
> #include "trace.h"
>
> /* CMD646 specific */
>@@ -339,9 +340,10 @@ static void cmd646_ide_class_init(ObjectClass *klass,
>void *data)
> }
>
> static const TypeInfo cmd646_ide_info = {
>-.name = "cmd646-ide",
>+.name = TYPE_CMD646_IDE,
> .parent= TYPE_PCI_IDE,
> .class_init= cmd646_ide_class_init,
>+.instance_size = sizeof(CMD646IDEState),
> };
>
> static void cmd646_ide_register_types(void)
>diff --git a/include/hw/ide/cmd646.h b/include/hw/ide/cmd646.h
>new file mode 100644
>index 00..4780b1132c
>--- /dev/null
>+++ b/include/hw/ide/cmd646.h
>@@ -0,0 +1,38 @@
>+/*
>+ * QEMU IDE Emulation: PCI cmd646 support.
>+ *
>+ * Copyright (c) 2003 Fabrice Bellard
>+ * Copyright (c) 2006 Openedhand Ltd.
>+ *
>+ * Permission is hereby granted, free of charge, to any person obtaining a
>copy
>+ * of this software and associated documentation files (the "Software"), to
>deal
>+ * in the Software without restriction, including without limitation the
>rights
>+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>+ * copies of the Software, and to permit persons to whom the Software is
>+ * furnished to do so, subject to the following conditions:
>+ *
>+ * The above copyright notice and this permission notice shall be included in
>+ * all copies or substantial portions of the Software.
>+ *
>+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>FROM,
>+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>+ * THE SOFTWARE.
>+ */
>+
>+#ifndef HW_IDE_CMD646_H
>+#define HW_IDE_CMD646_H
>+
>+#define TYPE_CMD646_IDE "cmd646-ide"
>+OBJECT_DECLARE_SIMPLE_TYPE(CMD646IDEState, CMD646_IDE)
>+
>+#include "hw/ide/pci.h"
>+
>+struct CMD646IDEState {
>+PCIIDEState parent_obj;
No public / private sections here? From the naming this attribute is often
considered private and the rest public. I guess what this scheme communicates
is that `parent_obj` should be accessed via `IDE_PCI()` only.
>+};
>+
>+#endif
Re: [Libguestfs] [PATCH v4 02/24] nbd: Consistent typedef usage in header
On 08.06.23 17:17, Eric Blake wrote: On Thu, Jun 08, 2023 at 08:56:31AM -0500, Eric Blake wrote: We had a mix of struct declarataions followed by typedefs, and direct declarations struct definitions as part of a typedef. Pick a single style. Also float a couple of opaque typedefs earlier in the file, as a later patch wants to refer NBDExport* in NBDRequest. No semantic impact. The curse of writing a commit message and then rebasing to a different idea; in patch 22, I had originally intended to make NBDMetaContexts a concrete type in nbd.h (which depends on NBDExport*, and would be directly used in NBDRequest, which in turn is declared before the pre-patch mention of NBDExport), but then changed my mind to instead have NBDMetaContexts itself also be an opaque type with NBDRequest only using NBDMetaContexts*. And I missed floating the typedef for NBDClientConnection to the same point, because we somewhat separated opaque types along the lines of which .c files provide various functions and opaque types. @@ -26,24 +26,25 @@ #include "qapi/error.h" #include "qemu/bswap.h" +typedef struct NBDExport NBDExport; +typedef struct NBDClient NBDClient; + Preferences on how I should tweak that aspect of this patch? Options: - Don't float NBDExport or NBDClient, and drop that part of the commit message. However, the later patch that adds the typedef for NBDMetaContexts still has to do it earlier than the definition of NBDRequest, rather than alongside the other opaque types relevant to server.c - Also float NBDClientConnection up here, and reword the commit message along the lines of: Also float forward declarations of opaque types to the top of the file, rather than interspersed with function declarations, which will help a future patch that wants to expose yet another opaque type that will be referenced in NBDRequest. Sounds good to me. Anyway: Reviewed-by: Vladimir Sementsov-Ogievskiy - something else? -- Best regards, Vladimir
