Re: [PATCH 00/13] Subject: [PATCH 00/15] qapi: Improve command response documentation

[Konstantin Kostiuk]

Hi Markus,

I will merge qga-related patches in my PULL.

Best Regards,
Konstantin Kostiuk.


On Tue, Feb 27, 2024 at 1:39 PM Markus Armbruster  wrote:

> We use doc comment "Returns" sections both for success and error
> response.  This series moves the latter to new "Errors" sections.
> Enables some cleanup, visible in the diffstat.q
>
> Markus Armbruster (13):
>   qapi: Memorize since & returns sections
>   qapi: Slightly clearer error message for invalid "Returns" section
>   qapi: New documentation section tag "Errors"
>   qapi: Move error documentation to new "Errors" sections
>   qapi: Delete useless "Returns" sections
>   qapi: Clean up "Returns" sections
>   qapi/yank: Tweak @yank's error description for consistency
>   qga/qapi-schema: Move error documentation to new "Errors" sections
>   qga/qapi-schema: Delete useless "Returns" sections
>   qga/qapi-schema: Clean up "Returns" sections
>   qga/qapi-schema: Tweak documentation of fsfreeze commands
>   qga/qapi-schema: Fix guest-set-memory-blocks documentation
>   qapi: Reject "Returns" section when command doesn't return anything
>
>  docs/devel/qapi-code-gen.rst   |  6 +-
>  qapi/block-core.json   | 74 --
>  qapi/block-export.json | 23 ---
>  qapi/block.json| 10 ++-
>  qapi/char.json |  6 --
>  qapi/dump.json |  2 -
>  qapi/machine-target.json   | 37 ++-
>  qapi/machine.json  | 19 ++
>  qapi/migration.json| 26 
>  qapi/misc-target.json  |  3 -
>  qapi/misc.json | 25 +++-
>  qapi/net.json  | 17 +++--
>  qapi/qdev.json |  3 +-
>  qapi/qom.json  |  6 +-
>  qapi/run-state.json|  5 +-
>  qapi/tpm.json  |  2 -
>  qapi/transaction.json  |  5 +-
>  qapi/ui.json   | 17 +
>  qapi/yank.json |  5 +-
>  qga/qapi-schema.json   | 72 +
>  scripts/qapi/parser.py | 50 ++-
>  tests/qapi-schema/doc-good.json|  2 +
>  tests/qapi-schema/doc-good.out |  2 +
>  tests/qapi-schema/doc-good.txt |  6 ++
>  tests/qapi-schema/doc-invalid-return.err   |  2 +-
>  tests/qapi-schema/doc-invalid-return2.err  |  1 +
>  tests/qapi-schema/doc-invalid-return2.json |  7 ++
>  tests/qapi-schema/doc-invalid-return2.out  |  0
>  tests/qapi-schema/meson.build  |  1 +
>  29 files changed, 189 insertions(+), 245 deletions(-)
>  create mode 100644 tests/qapi-schema/doc-invalid-return2.err
>  create mode 100644 tests/qapi-schema/doc-invalid-return2.json
>  create mode 100644 tests/qapi-schema/doc-invalid-return2.out
>
> --
> 2.43.0
>
>

[no subject]

[Yu Zhang]

Hello Het and all,

while I was testing qemu-8.2, I saw a lot of our migration test cases failed.
After debugging the commits of the 8.2 branch, I saw the issue and mad a diff:

diff --git a/migration/rdma.c b/migration/rdma.c
index 6a29e53daf..f10d56f556 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3353,9 +3353,9 @@ static int qemu_rdma_accept(RDMAContext *rdma)
 goto err_rdma_dest_wait;
 }

-isock->host = rdma->host;
+isock->host = g_strdup_printf("%s", rdma->host);
 isock->port = g_strdup_printf("%d", rdma->port);

which was introduced by the commit below:

commit 3fa9642ff7d51f7fc3ba68e6ccd13a939d5bd609 (HEAD)
Author: Het Gala 
Date:   Mon Oct 23 15:20:45 2023 -0300

migration: convert rdma backend to accept MigrateAddress

RDMA based transport backend for 'migrate'/'migrate-incoming' QAPIs
accept new wire protocol of MigrateAddress struct.

It is achived by parsing 'uri' string and storing migration parameters
required for RDMA connection into well defined InetSocketAddress struct.
...

A debug line
 isock->host = rdma->host;
 isock->port = g_strdup_printf("%d", rdma->port);
+fprintf(stdout, "QEMU: %s, host %s, port %s\n", __func__,
isock->host, isock->port);

produced this error:
QEMU: qemu_rdma_accept, host ::, port 8089
corrupted size vs. prev_size in fastbins

on the target host, which may indicate a crash related to the memory
allocation or a memory
corruption of the data. With the patch, it doesn't happen any more,
and the migration is fine.
Could you be kind to test this and confirm the issue?

Furthermore, I'm confused by the two struct:

struct InetSocketAddressBase {
char *host;
char *port;
};

struct InetSocketAddress {
/* Members inherited from InetSocketAddressBase: */
char *host;
char *port;

To my understanding, they are used to consolidate the separated data
to a well-defined
struct "MigrateAddress", while the struct whose member receive their
data has a different type:

typedef struct RDMAContext {
char *host;
int port;
...
}

Is there any reason to keep "port" like this (char* instead of int) or
can we improve it?
Thank you so much for any of your comments!

Best regards,

Yu Zhang @ IONOS Compute Platform
05.03.2024

[PATCH 00/13] Subject: [PATCH 00/15] qapi: Improve command response documentation

[Markus Armbruster]

We use doc comment "Returns" sections both for success and error
response.  This series moves the latter to new "Errors" sections.
Enables some cleanup, visible in the diffstat.q

Markus Armbruster (13):
  qapi: Memorize since & returns sections
  qapi: Slightly clearer error message for invalid "Returns" section
  qapi: New documentation section tag "Errors"
  qapi: Move error documentation to new "Errors" sections
  qapi: Delete useless "Returns" sections
  qapi: Clean up "Returns" sections
  qapi/yank: Tweak @yank's error description for consistency
  qga/qapi-schema: Move error documentation to new "Errors" sections
  qga/qapi-schema: Delete useless "Returns" sections
  qga/qapi-schema: Clean up "Returns" sections
  qga/qapi-schema: Tweak documentation of fsfreeze commands
  qga/qapi-schema: Fix guest-set-memory-blocks documentation
  qapi: Reject "Returns" section when command doesn't return anything

 docs/devel/qapi-code-gen.rst   |  6 +-
 qapi/block-core.json   | 74 --
 qapi/block-export.json | 23 ---
 qapi/block.json| 10 ++-
 qapi/char.json |  6 --
 qapi/dump.json |  2 -
 qapi/machine-target.json   | 37 ++-
 qapi/machine.json  | 19 ++
 qapi/migration.json| 26 
 qapi/misc-target.json  |  3 -
 qapi/misc.json | 25 +++-
 qapi/net.json  | 17 +++--
 qapi/qdev.json |  3 +-
 qapi/qom.json  |  6 +-
 qapi/run-state.json|  5 +-
 qapi/tpm.json  |  2 -
 qapi/transaction.json  |  5 +-
 qapi/ui.json   | 17 +
 qapi/yank.json |  5 +-
 qga/qapi-schema.json   | 72 +
 scripts/qapi/parser.py | 50 ++-
 tests/qapi-schema/doc-good.json|  2 +
 tests/qapi-schema/doc-good.out |  2 +
 tests/qapi-schema/doc-good.txt |  6 ++
 tests/qapi-schema/doc-invalid-return.err   |  2 +-
 tests/qapi-schema/doc-invalid-return2.err  |  1 +
 tests/qapi-schema/doc-invalid-return2.json |  7 ++
 tests/qapi-schema/doc-invalid-return2.out  |  0
 tests/qapi-schema/meson.build  |  1 +
 29 files changed, 189 insertions(+), 245 deletions(-)
 create mode 100644 tests/qapi-schema/doc-invalid-return2.err
 create mode 100644 tests/qapi-schema/doc-invalid-return2.json
 create mode 100644 tests/qapi-schema/doc-invalid-return2.out

-- 
2.43.0

[no subject]

[Leo Hou]

Just to test the email address, no reply required.

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 17.10.23 09:49, Viresh Kumar wrote:

On 13-10-23, 20:02, Hanna Czenczek wrote:

On 10.10.23 16:35, Alex Bennée wrote:

I was going to say there is also the rust-vmm vhost-user-master crates
which we've imported:

https://github.com/vireshk/vhost

for the Xen Vhost Frontend:

https://github.com/vireshk/xen-vhost-frontend

but I can't actually see any handling for GET/SET_STATUS at all which
makes me wonder how we actually work. Viresh?

As far as I know the only back-end implementation of F_STATUS is in DPDK.
As I said, if anyone else implemented it right now, that would be dangerous,
because qemu doesn’t adhere to the virtio protocol when it comes to the
status byte.

Yeah, none of the Rust based Virtio backends enable `STATUS` in
`VhostUserProtocolFeatures` and so these messages are never exchanged.

The generic Rust code for the backends, doesn't even implement them.
Not sure if they should or not.


It absolutely should not, for evidence see this whole thread.  qemu 
sends a SET_STATUS 0, which amounts to a reset, when the VM is merely 
paused[1], and when it sets status bytes, it does not set them according 
to virtio specification.  Implementing it right now means relying on and 
working around qemu’s implementation-defined spec-breaking behavior.  
Also, note that qemu ignores feature negotiation response through 
FEATURES_OK, and DEVICE_NEEDS_RESET, so unless it’s worth working around 
the problems just to get some form of DRIVER_OK information (note this 
information does not come from the driver, but qemu makes it up), I 
absolutely would not implement it.


[1] Notably, it does restore the virtio state to the best of its 
abilities when the VM is resumed, but this is all still wrong (there is 
no point in doing so much on a pause/resume, it needlessly costs time) 
and any implementation that does a reset then will rely on the 
implementation-defined behavior that qemu is actually able to restore 
all the state that the back-end would lose during a reset. Notably, 
reset is not even well-defined in the vhost-user specification.  It was 
argued, in this thread, that DPDK works just fine with this, precisely 
because it ignores SET_STATUS 0.  Finally, if virtiofsd in particular, 
as a user of the Rust crates, is reset, it would lose its internal 
state, which qemu cannot restore short of using the upcoming migration 
facilities.

Re: [Virtio-fs] (no subject)

[Viresh Kumar]

On 13-10-23, 20:02, Hanna Czenczek wrote:
> On 10.10.23 16:35, Alex Bennée wrote:
> > I was going to say there is also the rust-vmm vhost-user-master crates
> > which we've imported:
> > 
> >https://github.com/vireshk/vhost
> > 
> > for the Xen Vhost Frontend:
> > 
> >https://github.com/vireshk/xen-vhost-frontend
> > 
> > but I can't actually see any handling for GET/SET_STATUS at all which
> > makes me wonder how we actually work. Viresh?
> 
> As far as I know the only back-end implementation of F_STATUS is in DPDK. 
> As I said, if anyone else implemented it right now, that would be dangerous,
> because qemu doesn’t adhere to the virtio protocol when it comes to the
> status byte.

Yeah, none of the Rust based Virtio backends enable `STATUS` in
`VhostUserProtocolFeatures` and so these messages are never exchanged.

The generic Rust code for the backends, doesn't even implement them.
Not sure if they should or not.

-- 
viresh

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 10.10.23 16:35, Alex Bennée wrote:

Hanna Czenczek  writes:

(adding Viresh to CC for Xen Vhost questions)


On 10.10.23 12:36, Alex Bennée wrote:

Hanna Czenczek  writes:


On 10.10.23 06:00, Yajun Wu wrote:

On 10/9/2023 5:13 PM, Hanna Czenczek wrote:

External email: Use caution opening links or attachments


On 09.10.23 11:07, Hanna Czenczek wrote:

On 09.10.23 10:21, Hanna Czenczek wrote:

On 07.10.23 04:22, Yajun Wu wrote:

[...]




So as far as I understand, the feature is supposed to rely on
implementation-specific behavior between specifically qemu as a
front-end and dpdk as a back-end, nothing else.  Honestly, that to me
is a very good reason to deprecate it.  That would make it clear that
any implementation that implements it does so because it relies on
implementation-specific behavior from other implementations.

Option 2 is to fix it.  It is not right to use this broadly defined
feature with its clear protocol as given in the virtio specification
just to set and clear a single bit (DRIVER_OK).  The vhost-user
specification points to that virtio protocol.  We must adhere to the
protocol.  And note that we must not reset devices just because the VM
is paused/resumed.  (That is why I wanted to deprecate SET_STATUS, so
that Stefan’s series would introduce RESET_DEVICE where we need it,
and we can (for now) ignore the SET_STATUS 0 in vhost_dev_stop().)

Option 3 would be to just be honest in the specification, and limit
the scope of F_STATUS to say the only bit that matters is DRIVER_OK.
I would say this is not really different from deprecating, though it
wouldn’t affect your case.  However, I understand Alex relies on a
full status byte.  I’m still interested to know why that is.

For an F_TRANSPORT backend (or whatever the final name ends up being) we
need the backend to have full control of the status byte because all the
handling of VirtIO is deferred to it. Therefor it has to handle all the
feature negotiation and indicate when the device needs resetting.

(side note: feature negotiation is another slippery area when QEMU gets
involved in gating which feature bits may or may not be exposed to the
backend. The only one it should ever mask is F_UNUSED which is used
(sic) to trigger the vhost protocol negotiation)

That’s the thing, feature negotiation is done with GET_FEATURES and
SET_FEATURES.  Configuring F_REPLY_ACK lets SET_FEATURES return
errors.

OK but then what - QEMU fakes up FEATURES_OK in the Device Status field
on the behalf of the backend?


It does that right now.  When using qemu, vhost-user status byte is not 
exposed to the guest at all.  qemu makes it up completely, and 
effectively ignores the response from GET_STATUS completely.


(The only use of GET_STATUS is (right now): There is a function to set a 
flag in the status byte, and it calls GET_STATUS, ORs the flag in, and 
calls SET_STATUS with the result.)



I should point out QEMU doesn't exist in some of these use case. When
using the rust-vmm backends with Xen for example there is no VMM to talk
to so we have a Xen Vhost Frontend which is entirely concerned with
setup and then once connected up leaves the backend to do its thing. I'd
rather leave the frontend as dumb as possible rather than splitting
logic between the two.


Indicating that the device needs reset is a good point, there is no
other feature to do that.  (And something qemu currently ignores, just
like any value the device returns through GET_STATUS, but that’s
besides the point.)


Option 4 is of course not to do anything, and leave everything as-is,
waiting for the next person to stir the hornet’s nest.


Cc-ing Alex on this mail, because to me, this seems like an important
detail when he plans on using the byte in the future. If we need a
virtio status byte, I can’t see how we could use the existing F_STATUS
for it.

What would we use instead of F_STATUS to query the Device Status field?

We would emulate it in the front-end, just like we need to do for
back-ends without F_STATUS.  We can’t emulate the DEVICE_NEEDS_RESET
bit, though, that’s correct.

Given that qemu currently ignores DEVICE_NEEDS_RESET, I’m not 100 %
convinced that your use case has a hard dependency on F_STATUS.
However, this still does make a fair point in general that it would be
useful to keep it.

OK/


That still leaves us with the situation that currently, the only
implementations with F_STATUS support are qemu and dpdk, which both
handle it incorrectly.

I was going to say there is also the rust-vmm vhost-user-master crates
which we've imported:

   https://github.com/vireshk/vhost

for the Xen Vhost Frontend:

   https://github.com/vireshk/xen-vhost-frontend

but I can't actually see any handling for GET/SET_STATUS at all which
makes me wonder how we actually work. Viresh?


As far as I know the only back-end implementation of F_STATUS is in 
DPDK.  As I said, if anyone else implemented it right now, that would be 
dangerous, because qemu doesn’t adhere to the virtio 

Re: [Virtio-fs] (no subject)

[Alex Bennée]

Hanna Czenczek  writes:

(adding Viresh to CC for Xen Vhost questions)

> On 10.10.23 12:36, Alex Bennée wrote:
>> Hanna Czenczek  writes:
>>
>>> On 10.10.23 06:00, Yajun Wu wrote:
 On 10/9/2023 5:13 PM, Hanna Czenczek wrote:
> External email: Use caution opening links or attachments
>
>
> On 09.10.23 11:07, Hanna Czenczek wrote:
>> On 09.10.23 10:21, Hanna Czenczek wrote:
>>> On 07.10.23 04:22, Yajun Wu wrote:
>> [...]
>>
>> 
>>> So as far as I understand, the feature is supposed to rely on
>>> implementation-specific behavior between specifically qemu as a
>>> front-end and dpdk as a back-end, nothing else.  Honestly, that to me
>>> is a very good reason to deprecate it.  That would make it clear that
>>> any implementation that implements it does so because it relies on
>>> implementation-specific behavior from other implementations.
>>>
>>> Option 2 is to fix it.  It is not right to use this broadly defined
>>> feature with its clear protocol as given in the virtio specification
>>> just to set and clear a single bit (DRIVER_OK).  The vhost-user
>>> specification points to that virtio protocol.  We must adhere to the
>>> protocol.  And note that we must not reset devices just because the VM
>>> is paused/resumed.  (That is why I wanted to deprecate SET_STATUS, so
>>> that Stefan’s series would introduce RESET_DEVICE where we need it,
>>> and we can (for now) ignore the SET_STATUS 0 in vhost_dev_stop().)
>>>
>>> Option 3 would be to just be honest in the specification, and limit
>>> the scope of F_STATUS to say the only bit that matters is DRIVER_OK.
>>> I would say this is not really different from deprecating, though it
>>> wouldn’t affect your case.  However, I understand Alex relies on a
>>> full status byte.  I’m still interested to know why that is.
>> For an F_TRANSPORT backend (or whatever the final name ends up being) we
>> need the backend to have full control of the status byte because all the
>> handling of VirtIO is deferred to it. Therefor it has to handle all the
>> feature negotiation and indicate when the device needs resetting.
>>
>> (side note: feature negotiation is another slippery area when QEMU gets
>> involved in gating which feature bits may or may not be exposed to the
>> backend. The only one it should ever mask is F_UNUSED which is used
>> (sic) to trigger the vhost protocol negotiation)
>
> That’s the thing, feature negotiation is done with GET_FEATURES and
> SET_FEATURES.  Configuring F_REPLY_ACK lets SET_FEATURES return
> errors.

OK but then what - QEMU fakes up FEATURES_OK in the Device Status field
on the behalf of the backend?

I should point out QEMU doesn't exist in some of these use case. When
using the rust-vmm backends with Xen for example there is no VMM to talk
to so we have a Xen Vhost Frontend which is entirely concerned with
setup and then once connected up leaves the backend to do its thing. I'd
rather leave the frontend as dumb as possible rather than splitting
logic between the two.

> Indicating that the device needs reset is a good point, there is no
> other feature to do that.  (And something qemu currently ignores, just
> like any value the device returns through GET_STATUS, but that’s
> besides the point.)
>
>>> Option 4 is of course not to do anything, and leave everything as-is,
>>> waiting for the next person to stir the hornet’s nest.
>>>
>> Cc-ing Alex on this mail, because to me, this seems like an important
>> detail when he plans on using the byte in the future. If we need a
>> virtio status byte, I can’t see how we could use the existing F_STATUS
>> for it.
>> What would we use instead of F_STATUS to query the Device Status field?
>
> We would emulate it in the front-end, just like we need to do for
> back-ends without F_STATUS.  We can’t emulate the DEVICE_NEEDS_RESET
> bit, though, that’s correct.
>
> Given that qemu currently ignores DEVICE_NEEDS_RESET, I’m not 100 %
> convinced that your use case has a hard dependency on F_STATUS.
> However, this still does make a fair point in general that it would be
> useful to keep it.

OK/

> That still leaves us with the situation that currently, the only
> implementations with F_STATUS support are qemu and dpdk, which both
> handle it incorrectly. 

I was going to say there is also the rust-vmm vhost-user-master crates
which we've imported:

  https://github.com/vireshk/vhost

for the Xen Vhost Frontend:

  https://github.com/vireshk/xen-vhost-frontend

but I can't actually see any handling for GET/SET_STATUS at all which
makes me wonder how we actually work. Viresh?

> Furthermore, the specification leaves much to
> be desired, specifically in how F_STATUS interacts with other
> vhost-user commands (which is something I cited as a reason for my
> original patch), i.e. whether RESET_DEVICE and SET_STATUS 0 are
> equivalent, and whether failures in feature negotiation must result in
> both SET_FEATURES returning an error (with F_REPLY_ACK), and

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 10.10.23 12:36, Alex Bennée wrote:

Hanna Czenczek  writes:


On 10.10.23 06:00, Yajun Wu wrote:

On 10/9/2023 5:13 PM, Hanna Czenczek wrote:

External email: Use caution opening links or attachments


On 09.10.23 11:07, Hanna Czenczek wrote:

On 09.10.23 10:21, Hanna Czenczek wrote:

On 07.10.23 04:22, Yajun Wu wrote:

[...]




So as far as I understand, the feature is supposed to rely on
implementation-specific behavior between specifically qemu as a
front-end and dpdk as a back-end, nothing else.  Honestly, that to me
is a very good reason to deprecate it.  That would make it clear that
any implementation that implements it does so because it relies on
implementation-specific behavior from other implementations.

Option 2 is to fix it.  It is not right to use this broadly defined
feature with its clear protocol as given in the virtio specification
just to set and clear a single bit (DRIVER_OK).  The vhost-user
specification points to that virtio protocol.  We must adhere to the
protocol.  And note that we must not reset devices just because the VM
is paused/resumed.  (That is why I wanted to deprecate SET_STATUS, so
that Stefan’s series would introduce RESET_DEVICE where we need it,
and we can (for now) ignore the SET_STATUS 0 in vhost_dev_stop().)

Option 3 would be to just be honest in the specification, and limit
the scope of F_STATUS to say the only bit that matters is DRIVER_OK.
I would say this is not really different from deprecating, though it
wouldn’t affect your case.  However, I understand Alex relies on a
full status byte.  I’m still interested to know why that is.

For an F_TRANSPORT backend (or whatever the final name ends up being) we
need the backend to have full control of the status byte because all the
handling of VirtIO is deferred to it. Therefor it has to handle all the
feature negotiation and indicate when the device needs resetting.

(side note: feature negotiation is another slippery area when QEMU gets
involved in gating which feature bits may or may not be exposed to the
backend. The only one it should ever mask is F_UNUSED which is used
(sic) to trigger the vhost protocol negotiation)


That’s the thing, feature negotiation is done with GET_FEATURES and 
SET_FEATURES.  Configuring F_REPLY_ACK lets SET_FEATURES return errors.


Indicating that the device needs reset is a good point, there is no 
other feature to do that.  (And something qemu currently ignores, just 
like any value the device returns through GET_STATUS, but that’s besides 
the point.)



Option 4 is of course not to do anything, and leave everything as-is,
waiting for the next person to stir the hornet’s nest.


Cc-ing Alex on this mail, because to me, this seems like an important
detail when he plans on using the byte in the future. If we need a
virtio status byte, I can’t see how we could use the existing F_STATUS
for it.

What would we use instead of F_STATUS to query the Device Status field?


We would emulate it in the front-end, just like we need to do for 
back-ends without F_STATUS.  We can’t emulate the DEVICE_NEEDS_RESET 
bit, though, that’s correct.


Given that qemu currently ignores DEVICE_NEEDS_RESET, I’m not 100 % 
convinced that your use case has a hard dependency on F_STATUS. However, 
this still does make a fair point in general that it would be useful to 
keep it.


That still leaves us with the situation that currently, the only 
implementations with F_STATUS support are qemu and dpdk, which both 
handle it incorrectly.  Furthermore, the specification leaves much to be 
desired, specifically in how F_STATUS interacts with other vhost-user 
commands (which is something I cited as a reason for my original patch), 
i.e. whether RESET_DEVICE and SET_STATUS 0 are equivalent, and whether 
failures in feature negotiation must result in both SET_FEATURES 
returning an error (with F_REPLY_ACK), and FEATURES_OK being reset in 
the status byte, or whether either is sufficient.  What happens when 
DEVICE_NEEDS_RESET is set, i.e. do we just need RESET_DEVICE / 
SET_STATUS 0, or do we also need to reset some protocol state?  (This is 
also connected to the fact that what happens on RESET_DEVICE is largely 
undefined, which I said on Stefan’s series.)


In general, because we have our own transport, we should make a note how 
it interacts with the status negotiation phases, i.e. that GET_FEATURES 
must not be called before S_ACKNOWLEDGE | S_DRIVER are set, that 
FEATURES_OK must be set after the SET_FEATURES call, and that DRIVER_OK 
must not be set without FEATURES_OK set / SET_FEATURES having returned 
success.  Here we would also answer the question about the interaction 
of F_REPLY_ACK+SET_FEATURES with F_STATUS, specifically whether an 
implementation with F_REPLY_ACK even needs to read back the status byte 
after setting FEATURES_OK because it could have got the feature 
negotiation result already as a result of the SET_FEATURES call.


After migration, can you just set all flags immediately or 

Re: [Virtio-fs] (no subject)

[Alex Bennée]

Hanna Czenczek  writes:

> On 10.10.23 06:00, Yajun Wu wrote:
>>
>> On 10/9/2023 5:13 PM, Hanna Czenczek wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 09.10.23 11:07, Hanna Czenczek wrote:
 On 09.10.23 10:21, Hanna Czenczek wrote:
> On 07.10.23 04:22, Yajun Wu wrote:
 [...]


> So as far as I understand, the feature is supposed to rely on
> implementation-specific behavior between specifically qemu as a
> front-end and dpdk as a back-end, nothing else.  Honestly, that to me
> is a very good reason to deprecate it.  That would make it clear that
> any implementation that implements it does so because it relies on
> implementation-specific behavior from other implementations.
>
> Option 2 is to fix it.  It is not right to use this broadly defined
> feature with its clear protocol as given in the virtio specification
> just to set and clear a single bit (DRIVER_OK).  The vhost-user
> specification points to that virtio protocol.  We must adhere to the
> protocol.  And note that we must not reset devices just because the VM
> is paused/resumed.  (That is why I wanted to deprecate SET_STATUS, so
> that Stefan’s series would introduce RESET_DEVICE where we need it,
> and we can (for now) ignore the SET_STATUS 0 in vhost_dev_stop().)
>
> Option 3 would be to just be honest in the specification, and limit
> the scope of F_STATUS to say the only bit that matters is DRIVER_OK. 
> I would say this is not really different from deprecating, though it
> wouldn’t affect your case.  However, I understand Alex relies on a
> full status byte.  I’m still interested to know why that is.

For an F_TRANSPORT backend (or whatever the final name ends up being) we
need the backend to have full control of the status byte because all the
handling of VirtIO is deferred to it. Therefor it has to handle all the
feature negotiation and indicate when the device needs resetting.

(side note: feature negotiation is another slippery area when QEMU gets
involved in gating which feature bits may or may not be exposed to the
backend. The only one it should ever mask is F_UNUSED which is used
(sic) to trigger the vhost protocol negotiation)

> Option 4 is of course not to do anything, and leave everything as-is,
> waiting for the next person to stir the hornet’s nest.
>
 Cc-ing Alex on this mail, because to me, this seems like an important
 detail when he plans on using the byte in the future. If we need a
 virtio status byte, I can’t see how we could use the existing F_STATUS
 for it.

What would we use instead of F_STATUS to query the Device Status field?


 Hanna
>>


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [Virtio-fs] (no subject)

[German Maglione]

On Tue, Oct 10, 2023 at 4:57 AM Yajun Wu  wrote:
>
>
> On 10/9/2023 6:28 PM, German Maglione wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Sat, Oct 7, 2023 at 4:23 AM Yajun Wu  wrote:
> >>
> >> On 10/6/2023 6:34 PM, Michael S. Tsirkin wrote:
> >>> External email: Use caution opening links or attachments
> >>>
> >>>
> >>> On Fri, Oct 06, 2023 at 11:47:55AM +0200, Hanna Czenczek wrote:
>  On 06.10.23 11:26, Michael S. Tsirkin wrote:
> > On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:
> >> On 06.10.23 10:45, Michael S. Tsirkin wrote:
> >>> On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:
>  On 05.10.23 19:15, Michael S. Tsirkin wrote:
> > On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:
> >> On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:
> >>> There is no clearly defined purpose for the virtio status byte in
> >>> vhost-user: For resetting, we already have RESET_DEVICE; and for 
> >>> virtio
> >>> feature negotiation, we have [GS]ET_FEATURES.  With the REPLY_ACK
> >>> protocol extension, it is possible for SET_FEATURES to return 
> >>> errors
> >>> (SET_PROTOCOL_FEATURES may be called before SET_FEATURES).
> >>>
> >>> As for implementations, SET_STATUS is not widely implemented.  
> >>> dpdk does
> >>> implement it, but only uses it to signal feature negotiation 
> >>> failure.
> >>> While it does log reset requests (SET_STATUS 0) as such, it 
> >>> effectively
> >>> ignores them, in contrast to RESET_OWNER (which is deprecated, 
> >>> and today
> >>> means the same thing as RESET_DEVICE).
> >>>
> >>> While qemu superficially has support for [GS]ET_STATUS, it does 
> >>> not
> >>> forward the guest-set status byte, but instead just makes it up
> >>> internally, and actually completely ignores what the back-end 
> >>> returns,
> >>> only using it as the template for a subsequent SET_STATUS to add 
> >>> single
> >>> bits to it.  Notably, after setting FEATURES_OK, it never reads 
> >>> it back
> >>> to see whether the flag is still set, which is the only way in 
> >>> which
> >>> dpdk uses the status byte.
> >>>
> >>> As-is, no front-end or back-end can rely on the other side 
> >>> handling this
> >>> field in a useful manner, and it also provides no practical use 
> >>> over
> >>> other mechanisms the vhost-user protocol has, which are more 
> >>> clearly
> >>> defined.  Deprecate it.
> >>>
> >>> Suggested-by: Stefan Hajnoczi 
> >>> Signed-off-by: Hanna Czenczek 
> >>> ---
> >>>   docs/interop/vhost-user.rst | 28 
> >>> +---
> >>>   1 file changed, 21 insertions(+), 7 deletions(-)
> >> Reviewed-by: Stefan Hajnoczi 
> > SET_STATUS is the only way to signal failure to acknowledge 
> > FEATURES_OK.
> > The fact current backends never check errors does not mean they 
> > never
> > will. So no, not applying this.
>  Can this not be done with REPLY_ACK?  I.e., with the following 
>  message
>  order:
> 
>  1. GET_FEATURES to find out whether VHOST_USER_F_PROTOCOL_FEATURES is
>  present
>  2. GET_PROTOCOL_FEATURES to hopefully get 
>  VHOST_USER_PROTOCOL_F_REPLY_ACK
>  3. SET_PROTOCOL_FEATURES to set VHOST_USER_PROTOCOL_F_REPLY_ACK
>  4. SET_FEATURES with need_reply
> 
>  If not, the problem is that qemu has sent SET_STATUS 0 for a while 
>  when the
>  vCPUs are stopped, which generally seems to request a device reset.  
>  If we
>  don’t state at least that SET_STATUS 0 is to be ignored, back-ends 
>  that will
>  implement SET_STATUS later may break with at least these qemu 
>  versions.  But
>  documenting that a particular use of the status byte is to be 
>  ignored would
>  be really strange.
> 
>  Hanna
> >>> Hmm I guess. Though just following virtio spec seems cleaner to me...
> >>> vhost-user reconfigures the state fully on start.
> >> Not the internal device state, though.  virtiofsd has internal state, 
> >> and
> >> other devices like vhost-gpu back-ends would probably, too.
> >>
> >> Stefan has recently sent a series
> >> (https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg00709.html)
> >>  to
> >> put the reset (RESET_DEVICE) into virtio_reset() (when we really need a
> >> reset).
> >>
> >> I really don’t like our current approach with the status byte. 
> >> Following the
> 

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 10.10.23 06:00, Yajun Wu wrote:


On 10/9/2023 5:13 PM, Hanna Czenczek wrote:

External email: Use caution opening links or attachments


On 09.10.23 11:07, Hanna Czenczek wrote:

On 09.10.23 10:21, Hanna Czenczek wrote:

On 07.10.23 04:22, Yajun Wu wrote:

[...]


The main motivation of adding VHOST_USER_SET_STATUS is to let
backend DPDK know
when DRIVER_OK bit is valid. It's an indication of all VQ
configuration has sent,
otherwise DPDK has to rely on first queue pair is ready, then
receiving/applying
VQ configuration one by one.

During live migration, configuring VQ one by one is very time
consuming.

One question I have here is why it wasn’t then introduced in the live
migration code, but in the general VM stop/cont code instead. It does
seem time-consuming to do this every time the VM is paused and 
resumed.


Yes, VM stop/cont will call vhost_net_stop/vhost_net_start. Maybe 
because there's no device level stop/cont vhost message?


No, it is because qemu will reset the status in stop/cont*, which it 
should not do.  Aside from guest-initiated resets, the only thing where 
a reset comes into play is when the back-end is changed, e.g. during 
migration.  In that case, the source back-end will see a disconnect on 
the vhost-user socket and can then do whatever uninitialization it needs 
to do, and the destination front-end will need to be reconfigured by 
qemu anyway, because it’s just a case of the destination qemu initiating 
a fresh connection to a new back-end (except that it will need to 
restore the state from the source).


*Yes, technically, dpdk will ignore that reset, but it still stops the 
device on a different message (when it should just pause processing 
vrings), so the outcome is the same.





For VIRTIO
net vDPA, HW needs to know how many VQs are enabled to set
RSS(Receive-Side Scaling).

If you don’t want SET_STATUS message, backend can remove protocol
feature bit
VHOST_USER_PROTOCOL_F_STATUS.

The problem isn’t back-ends that don’t want the message, the problem
is that qemu uses the message wrongly, which prevents well-behaving
back-ends from implementing the message.


DPDK is ignoring SET_STATUS 0, but using GET_VRING_BASE to do device
close/reset.

So the right thing to do for back-ends is to announce STATUS support
and then not implement it correctly?

GET_VRING_BASE should not reset the close or reset the device, by the
way.  It should stop that one vring, not more.  We have a
RESET_DEVICE command for resetting.
I believe dpdk uses GET_VRING_BASE long before qemu has RESET_DEVICE? 


I don’t think it matters who came first.  What matters is the 
specification, and that dpdk decided to rely on implementation-specific 
behavior without having all involved parties agree by matters of putting 
that in the specification.  And now dpdk clearly deviates from the 
specification as a result of that action, which can result in problems 
if the front-end doesn’t do what qemu always used to do.  (E.g. the 
front-end might just send GET_VRING_BASE for all vrings when suspending 
the guest, and then only send kicks on resume to re-start the vrings.  
dpdk would most likely be left in a state where the whole device is 
stopped, expecting DRIVER_OK.  Same thing in general for front-ends that 
don’t support F_STATUS.)


It's a compatible issue. For new backend implements, we can have 
better solution, right?


The fact that dpdk and qemu deviate from the specification is a problem 
as-is.



I'm not involved in discussion about adding SET_STATUS in Vhost
protocol. This feature
is essential for vDPA(same as vhost-vdpa implements
VHOST_VDPA_SET_STATUS).

So from what I gather from your response is that there is only a
single use for SET_STATUS, which is the DRIVER_OK bit.  If so,
documenting that all other bits are to be ignored by both back-end
and front-end would be fine by me.

I’m not fully serious about that suggestion, but I hear the strong
implication that nothing but DRIVER_OK was of any concern, and this
is really important to note when we talk about the status of the
STATUS feature in vhost today.  It seems to me now that it was not
intended to be the virtio-level status byte, but just a DRIVER_OK
signalling path from front-end to back-end.  That makes it a
vhost-level protocol feature to me.

On second thought, it just is a pure vhost-level protocol feature, and
has nothing to do with the virtio status byte as-is.  The only stated
purpose is for the front-end to send DRIVER_OK after migration, but
migration is transparent to the guest, so the guest would never change
the status byte during migration.  Therefore, if this feature is
essential, we will never be able to have a status byte that is
transparently shared between guest and back-end device, i.e. the
virtio status byte.

On third thought, scratch that.  The guest wouldn’t set it, but
naturally, after migration, the front-end will need to restore the
status byte from the source, so the front-end will always need to set
it, 

Re: [Virtio-fs] (no subject)

[Yajun Wu]

On 10/9/2023 5:13 PM, Hanna Czenczek wrote:

External email: Use caution opening links or attachments


On 09.10.23 11:07, Hanna Czenczek wrote:

On 09.10.23 10:21, Hanna Czenczek wrote:

On 07.10.23 04:22, Yajun Wu wrote:

[...]


The main motivation of adding VHOST_USER_SET_STATUS is to let
backend DPDK know
when DRIVER_OK bit is valid. It's an indication of all VQ
configuration has sent,
otherwise DPDK has to rely on first queue pair is ready, then
receiving/applying
VQ configuration one by one.

During live migration, configuring VQ one by one is very time
consuming.

One question I have here is why it wasn’t then introduced in the live
migration code, but in the general VM stop/cont code instead. It does
seem time-consuming to do this every time the VM is paused and resumed.


Yes, VM stop/cont will call vhost_net_stop/vhost_net_start. Maybe 
because there's no device level stop/cont vhost message?





For VIRTIO
net vDPA, HW needs to know how many VQs are enabled to set
RSS(Receive-Side Scaling).

If you don’t want SET_STATUS message, backend can remove protocol
feature bit
VHOST_USER_PROTOCOL_F_STATUS.

The problem isn’t back-ends that don’t want the message, the problem
is that qemu uses the message wrongly, which prevents well-behaving
back-ends from implementing the message.


DPDK is ignoring SET_STATUS 0, but using GET_VRING_BASE to do device
close/reset.

So the right thing to do for back-ends is to announce STATUS support
and then not implement it correctly?

GET_VRING_BASE should not reset the close or reset the device, by the
way.  It should stop that one vring, not more.  We have a
RESET_DEVICE command for resetting.
I believe dpdk uses GET_VRING_BASE long before qemu has RESET_DEVICE? 
It's a compatible issue. For new backend implements, we can have better 
solution, right?

I'm not involved in discussion about adding SET_STATUS in Vhost
protocol. This feature
is essential for vDPA(same as vhost-vdpa implements
VHOST_VDPA_SET_STATUS).

So from what I gather from your response is that there is only a
single use for SET_STATUS, which is the DRIVER_OK bit.  If so,
documenting that all other bits are to be ignored by both back-end
and front-end would be fine by me.

I’m not fully serious about that suggestion, but I hear the strong
implication that nothing but DRIVER_OK was of any concern, and this
is really important to note when we talk about the status of the
STATUS feature in vhost today.  It seems to me now that it was not
intended to be the virtio-level status byte, but just a DRIVER_OK
signalling path from front-end to back-end.  That makes it a
vhost-level protocol feature to me.

On second thought, it just is a pure vhost-level protocol feature, and
has nothing to do with the virtio status byte as-is.  The only stated
purpose is for the front-end to send DRIVER_OK after migration, but
migration is transparent to the guest, so the guest would never change
the status byte during migration.  Therefore, if this feature is
essential, we will never be able to have a status byte that is
transparently shared between guest and back-end device, i.e. the
virtio status byte.

On third thought, scratch that.  The guest wouldn’t set it, but
naturally, after migration, the front-end will need to restore the
status byte from the source, so the front-end will always need to set
it, even if it were otherwise used controlled only by the guest and the
back-end device.  So technically, this doesn’t prevent such a use case.
(In practice, it isn’t controlled by the guest right now, but that could
be fixed.)
I only tested the feature with DPDK(the only backend use it today?). Max 
defined the protocol and added the corresponding code in DPDK before I 
added QEMU support. If other backend or different device type want to 
use this, we can have further discussion?

Cc-ing Alex on this mail, because to me, this seems like an important
detail when he plans on using the byte in the future. If we need a
virtio status byte, I can’t see how we could use the existing F_STATUS
for it.

Hanna

Re: [Virtio-fs] (no subject)

[Yajun Wu]

On 10/9/2023 6:28 PM, German Maglione wrote:

External email: Use caution opening links or attachments


On Sat, Oct 7, 2023 at 4:23 AM Yajun Wu  wrote:


On 10/6/2023 6:34 PM, Michael S. Tsirkin wrote:

External email: Use caution opening links or attachments


On Fri, Oct 06, 2023 at 11:47:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 11:26, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 10:45, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:

On 05.10.23 19:15, Michael S. Tsirkin wrote:

On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:

On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:

There is no clearly defined purpose for the virtio status byte in
vhost-user: For resetting, we already have RESET_DEVICE; and for virtio
feature negotiation, we have [GS]ET_FEATURES.  With the REPLY_ACK
protocol extension, it is possible for SET_FEATURES to return errors
(SET_PROTOCOL_FEATURES may be called before SET_FEATURES).

As for implementations, SET_STATUS is not widely implemented.  dpdk does
implement it, but only uses it to signal feature negotiation failure.
While it does log reset requests (SET_STATUS 0) as such, it effectively
ignores them, in contrast to RESET_OWNER (which is deprecated, and today
means the same thing as RESET_DEVICE).

While qemu superficially has support for [GS]ET_STATUS, it does not
forward the guest-set status byte, but instead just makes it up
internally, and actually completely ignores what the back-end returns,
only using it as the template for a subsequent SET_STATUS to add single
bits to it.  Notably, after setting FEATURES_OK, it never reads it back
to see whether the flag is still set, which is the only way in which
dpdk uses the status byte.

As-is, no front-end or back-end can rely on the other side handling this
field in a useful manner, and it also provides no practical use over
other mechanisms the vhost-user protocol has, which are more clearly
defined.  Deprecate it.

Suggested-by: Stefan Hajnoczi 
Signed-off-by: Hanna Czenczek 
---
  docs/interop/vhost-user.rst | 28 +---
  1 file changed, 21 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi 

SET_STATUS is the only way to signal failure to acknowledge FEATURES_OK.
The fact current backends never check errors does not mean they never
will. So no, not applying this.

Can this not be done with REPLY_ACK?  I.e., with the following message
order:

1. GET_FEATURES to find out whether VHOST_USER_F_PROTOCOL_FEATURES is
present
2. GET_PROTOCOL_FEATURES to hopefully get VHOST_USER_PROTOCOL_F_REPLY_ACK
3. SET_PROTOCOL_FEATURES to set VHOST_USER_PROTOCOL_F_REPLY_ACK
4. SET_FEATURES with need_reply

If not, the problem is that qemu has sent SET_STATUS 0 for a while when the
vCPUs are stopped, which generally seems to request a device reset.  If we
don’t state at least that SET_STATUS 0 is to be ignored, back-ends that will
implement SET_STATUS later may break with at least these qemu versions.  But
documenting that a particular use of the status byte is to be ignored would
be really strange.

Hanna

Hmm I guess. Though just following virtio spec seems cleaner to me...
vhost-user reconfigures the state fully on start.

Not the internal device state, though.  virtiofsd has internal state, and
other devices like vhost-gpu back-ends would probably, too.

Stefan has recently sent a series
(https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg00709.html) to
put the reset (RESET_DEVICE) into virtio_reset() (when we really need a
reset).

I really don’t like our current approach with the status byte. Following the
virtio specification to me would mean that the guest directly controls this
byte, which it does not.  qemu makes up values as it deems appropriate, and
this includes sending a SET_STATUS 0 when the guest is just paused, i.e.
when the guest really doesn’t want a device reset.

That means that qemu does not treat this as a virtio device field (because
that would mean exposing it to the guest driver), but instead treats it as
part of the vhost(-user) protocol.  It doesn’t feel right to me that we use
a virtio-defined feature for communication on the vhost level, i.e. between
front-end and back-end, and not between guest driver and device.  I think
all vhost-level protocol features should be fully defined in the vhost-user
specification, which REPLY_ACK is.

Hmm that makes sense. Maybe we should have done what stefan's patch
is doing.

Do look at the original commit that introduced it to understand why
it was added.

I don’t understand why this was added to the stop/cont code, though.  If it
is time consuming to make these changes, why are they done every time the VM
is paused
and resumed?  It makes sense that this would be done for the initial
configuration (where a reset also wouldn’t hurt), but here it seems wrong.

(To be clear, a reset in the 

Re: [Virtio-fs] (no subject)

[German Maglione]

On Sat, Oct 7, 2023 at 4:23 AM Yajun Wu  wrote:
>
>
> On 10/6/2023 6:34 PM, Michael S. Tsirkin wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Fri, Oct 06, 2023 at 11:47:55AM +0200, Hanna Czenczek wrote:
> >> On 06.10.23 11:26, Michael S. Tsirkin wrote:
> >>> On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:
>  On 06.10.23 10:45, Michael S. Tsirkin wrote:
> > On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:
> >> On 05.10.23 19:15, Michael S. Tsirkin wrote:
> >>> On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:
>  On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:
> > There is no clearly defined purpose for the virtio status byte in
> > vhost-user: For resetting, we already have RESET_DEVICE; and for 
> > virtio
> > feature negotiation, we have [GS]ET_FEATURES.  With the REPLY_ACK
> > protocol extension, it is possible for SET_FEATURES to return errors
> > (SET_PROTOCOL_FEATURES may be called before SET_FEATURES).
> >
> > As for implementations, SET_STATUS is not widely implemented.  dpdk 
> > does
> > implement it, but only uses it to signal feature negotiation 
> > failure.
> > While it does log reset requests (SET_STATUS 0) as such, it 
> > effectively
> > ignores them, in contrast to RESET_OWNER (which is deprecated, and 
> > today
> > means the same thing as RESET_DEVICE).
> >
> > While qemu superficially has support for [GS]ET_STATUS, it does not
> > forward the guest-set status byte, but instead just makes it up
> > internally, and actually completely ignores what the back-end 
> > returns,
> > only using it as the template for a subsequent SET_STATUS to add 
> > single
> > bits to it.  Notably, after setting FEATURES_OK, it never reads it 
> > back
> > to see whether the flag is still set, which is the only way in which
> > dpdk uses the status byte.
> >
> > As-is, no front-end or back-end can rely on the other side handling 
> > this
> > field in a useful manner, and it also provides no practical use over
> > other mechanisms the vhost-user protocol has, which are more clearly
> > defined.  Deprecate it.
> >
> > Suggested-by: Stefan Hajnoczi 
> > Signed-off-by: Hanna Czenczek 
> > ---
> >  docs/interop/vhost-user.rst | 28 +---
> >  1 file changed, 21 insertions(+), 7 deletions(-)
>  Reviewed-by: Stefan Hajnoczi 
> >>> SET_STATUS is the only way to signal failure to acknowledge 
> >>> FEATURES_OK.
> >>> The fact current backends never check errors does not mean they never
> >>> will. So no, not applying this.
> >> Can this not be done with REPLY_ACK?  I.e., with the following message
> >> order:
> >>
> >> 1. GET_FEATURES to find out whether VHOST_USER_F_PROTOCOL_FEATURES is
> >> present
> >> 2. GET_PROTOCOL_FEATURES to hopefully get 
> >> VHOST_USER_PROTOCOL_F_REPLY_ACK
> >> 3. SET_PROTOCOL_FEATURES to set VHOST_USER_PROTOCOL_F_REPLY_ACK
> >> 4. SET_FEATURES with need_reply
> >>
> >> If not, the problem is that qemu has sent SET_STATUS 0 for a while 
> >> when the
> >> vCPUs are stopped, which generally seems to request a device reset.  
> >> If we
> >> don’t state at least that SET_STATUS 0 is to be ignored, back-ends 
> >> that will
> >> implement SET_STATUS later may break with at least these qemu 
> >> versions.  But
> >> documenting that a particular use of the status byte is to be ignored 
> >> would
> >> be really strange.
> >>
> >> Hanna
> > Hmm I guess. Though just following virtio spec seems cleaner to me...
> > vhost-user reconfigures the state fully on start.
>  Not the internal device state, though.  virtiofsd has internal state, and
>  other devices like vhost-gpu back-ends would probably, too.
> 
>  Stefan has recently sent a series
>  (https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg00709.html) 
>  to
>  put the reset (RESET_DEVICE) into virtio_reset() (when we really need a
>  reset).
> 
>  I really don’t like our current approach with the status byte. Following 
>  the
>  virtio specification to me would mean that the guest directly controls 
>  this
>  byte, which it does not.  qemu makes up values as it deems appropriate, 
>  and
>  this includes sending a SET_STATUS 0 when the guest is just paused, i.e.
>  when the guest really doesn’t want a device reset.
> 
>  That means that qemu does not treat this as a virtio device field 
>  (because
>  that would mean exposing it to the guest driver), but instead treats 

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 09.10.23 11:07, Hanna Czenczek wrote:

On 09.10.23 10:21, Hanna Czenczek wrote:

On 07.10.23 04:22, Yajun Wu wrote:


[...]

The main motivation of adding VHOST_USER_SET_STATUS is to let 
backend DPDK know
when DRIVER_OK bit is valid. It's an indication of all VQ 
configuration has sent,
otherwise DPDK has to rely on first queue pair is ready, then 
receiving/applying

VQ configuration one by one.

During live migration, configuring VQ one by one is very time 
consuming.


One question I have here is why it wasn’t then introduced in the live 
migration code, but in the general VM stop/cont code instead. It does 
seem time-consuming to do this every time the VM is paused and resumed.



For VIRTIO
net vDPA, HW needs to know how many VQs are enabled to set 
RSS(Receive-Side Scaling).


If you don’t want SET_STATUS message, backend can remove protocol 
feature bit

VHOST_USER_PROTOCOL_F_STATUS.


The problem isn’t back-ends that don’t want the message, the problem 
is that qemu uses the message wrongly, which prevents well-behaving 
back-ends from implementing the message.


DPDK is ignoring SET_STATUS 0, but using GET_VRING_BASE to do device 
close/reset.


So the right thing to do for back-ends is to announce STATUS support 
and then not implement it correctly?


GET_VRING_BASE should not reset the close or reset the device, by the 
way.  It should stop that one vring, not more.  We have a 
RESET_DEVICE command for resetting.


I'm not involved in discussion about adding SET_STATUS in Vhost 
protocol. This feature
is essential for vDPA(same as vhost-vdpa implements 
VHOST_VDPA_SET_STATUS).


So from what I gather from your response is that there is only a 
single use for SET_STATUS, which is the DRIVER_OK bit.  If so, 
documenting that all other bits are to be ignored by both back-end 
and front-end would be fine by me.


I’m not fully serious about that suggestion, but I hear the strong 
implication that nothing but DRIVER_OK was of any concern, and this 
is really important to note when we talk about the status of the 
STATUS feature in vhost today.  It seems to me now that it was not 
intended to be the virtio-level status byte, but just a DRIVER_OK 
signalling path from front-end to back-end.  That makes it a 
vhost-level protocol feature to me.


On second thought, it just is a pure vhost-level protocol feature, and 
has nothing to do with the virtio status byte as-is.  The only stated 
purpose is for the front-end to send DRIVER_OK after migration, but 
migration is transparent to the guest, so the guest would never change 
the status byte during migration.  Therefore, if this feature is 
essential, we will never be able to have a status byte that is 
transparently shared between guest and back-end device, i.e. the 
virtio status byte.


On third thought, scratch that.  The guest wouldn’t set it, but 
naturally, after migration, the front-end will need to restore the 
status byte from the source, so the front-end will always need to set 
it, even if it were otherwise used controlled only by the guest and the 
back-end device.  So technically, this doesn’t prevent such a use case.  
(In practice, it isn’t controlled by the guest right now, but that could 
be fixed.)


Cc-ing Alex on this mail, because to me, this seems like an important 
detail when he plans on using the byte in the future. If we need a 
virtio status byte, I can’t see how we could use the existing F_STATUS 
for it.


Hanna

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 09.10.23 10:21, Hanna Czenczek wrote:

On 07.10.23 04:22, Yajun Wu wrote:


[...]

The main motivation of adding VHOST_USER_SET_STATUS is to let backend 
DPDK know
when DRIVER_OK bit is valid. It's an indication of all VQ 
configuration has sent,
otherwise DPDK has to rely on first queue pair is ready, then 
receiving/applying

VQ configuration one by one.

During live migration, configuring VQ one by one is very time consuming.


One question I have here is why it wasn’t then introduced in the live 
migration code, but in the general VM stop/cont code instead. It does 
seem time-consuming to do this every time the VM is paused and resumed.



For VIRTIO
net vDPA, HW needs to know how many VQs are enabled to set 
RSS(Receive-Side Scaling).


If you don’t want SET_STATUS message, backend can remove protocol 
feature bit

VHOST_USER_PROTOCOL_F_STATUS.


The problem isn’t back-ends that don’t want the message, the problem 
is that qemu uses the message wrongly, which prevents well-behaving 
back-ends from implementing the message.


DPDK is ignoring SET_STATUS 0, but using GET_VRING_BASE to do device 
close/reset.


So the right thing to do for back-ends is to announce STATUS support 
and then not implement it correctly?


GET_VRING_BASE should not reset the close or reset the device, by the 
way.  It should stop that one vring, not more.  We have a RESET_DEVICE 
command for resetting.


I'm not involved in discussion about adding SET_STATUS in Vhost 
protocol. This feature
is essential for vDPA(same as vhost-vdpa implements 
VHOST_VDPA_SET_STATUS).


So from what I gather from your response is that there is only a 
single use for SET_STATUS, which is the DRIVER_OK bit.  If so, 
documenting that all other bits are to be ignored by both back-end and 
front-end would be fine by me.


I’m not fully serious about that suggestion, but I hear the strong 
implication that nothing but DRIVER_OK was of any concern, and this is 
really important to note when we talk about the status of the STATUS 
feature in vhost today.  It seems to me now that it was not intended 
to be the virtio-level status byte, but just a DRIVER_OK signalling 
path from front-end to back-end.  That makes it a vhost-level protocol 
feature to me.


On second thought, it just is a pure vhost-level protocol feature, and 
has nothing to do with the virtio status byte as-is.  The only stated 
purpose is for the front-end to send DRIVER_OK after migration, but 
migration is transparent to the guest, so the guest would never change 
the status byte during migration.  Therefore, if this feature is 
essential, we will never be able to have a status byte that is 
transparently shared between guest and back-end device, i.e. the virtio 
status byte.


Cc-ing Alex on this mail, because to me, this seems like an important 
detail when he plans on using the byte in the future.  If we need a 
virtio status byte, I can’t see how we could use the existing F_STATUS 
for it.


Hanna

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 07.10.23 04:22, Yajun Wu wrote:


On 10/6/2023 6:34 PM, Michael S. Tsirkin wrote:

External email: Use caution opening links or attachments


On Fri, Oct 06, 2023 at 11:47:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 11:26, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 10:45, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:

On 05.10.23 19:15, Michael S. Tsirkin wrote:

On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:

On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:
There is no clearly defined purpose for the virtio status 
byte in
vhost-user: For resetting, we already have RESET_DEVICE; and 
for virtio

feature negotiation, we have [GS]ET_FEATURES. With the REPLY_ACK
protocol extension, it is possible for SET_FEATURES to return 
errors

(SET_PROTOCOL_FEATURES may be called before SET_FEATURES).

As for implementations, SET_STATUS is not widely 
implemented.  dpdk does
implement it, but only uses it to signal feature negotiation 
failure.
While it does log reset requests (SET_STATUS 0) as such, it 
effectively
ignores them, in contrast to RESET_OWNER (which is 
deprecated, and today

means the same thing as RESET_DEVICE).

While qemu superficially has support for [GS]ET_STATUS, it 
does not

forward the guest-set status byte, but instead just makes it up
internally, and actually completely ignores what the back-end 
returns,
only using it as the template for a subsequent SET_STATUS to 
add single
bits to it.  Notably, after setting FEATURES_OK, it never 
reads it back
to see whether the flag is still set, which is the only way 
in which

dpdk uses the status byte.

As-is, no front-end or back-end can rely on the other side 
handling this
field in a useful manner, and it also provides no practical 
use over
other mechanisms the vhost-user protocol has, which are more 
clearly

defined.  Deprecate it.

Suggested-by: Stefan Hajnoczi 
Signed-off-by: Hanna Czenczek 
---
 docs/interop/vhost-user.rst | 28 
+---

 1 file changed, 21 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi 
SET_STATUS is the only way to signal failure to acknowledge 
FEATURES_OK.
The fact current backends never check errors does not mean they 
never

will. So no, not applying this.
Can this not be done with REPLY_ACK?  I.e., with the following 
message

order:

1. GET_FEATURES to find out whether 
VHOST_USER_F_PROTOCOL_FEATURES is

present
2. GET_PROTOCOL_FEATURES to hopefully get 
VHOST_USER_PROTOCOL_F_REPLY_ACK

3. SET_PROTOCOL_FEATURES to set VHOST_USER_PROTOCOL_F_REPLY_ACK
4. SET_FEATURES with need_reply

If not, the problem is that qemu has sent SET_STATUS 0 for a 
while when the
vCPUs are stopped, which generally seems to request a device 
reset.  If we
don’t state at least that SET_STATUS 0 is to be ignored, 
back-ends that will
implement SET_STATUS later may break with at least these qemu 
versions.  But
documenting that a particular use of the status byte is to be 
ignored would

be really strange.

Hanna
Hmm I guess. Though just following virtio spec seems cleaner to 
me...

vhost-user reconfigures the state fully on start.
Not the internal device state, though.  virtiofsd has internal 
state, and

other devices like vhost-gpu back-ends would probably, too.

Stefan has recently sent a series
(https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg00709.html) 
to
put the reset (RESET_DEVICE) into virtio_reset() (when we really 
need a

reset).

I really don’t like our current approach with the status byte. 
Following the
virtio specification to me would mean that the guest directly 
controls this
byte, which it does not.  qemu makes up values as it deems 
appropriate, and
this includes sending a SET_STATUS 0 when the guest is just 
paused, i.e.

when the guest really doesn’t want a device reset.

That means that qemu does not treat this as a virtio device field 
(because
that would mean exposing it to the guest driver), but instead 
treats it as
part of the vhost(-user) protocol.  It doesn’t feel right to me 
that we use
a virtio-defined feature for communication on the vhost level, 
i.e. between
front-end and back-end, and not between guest driver and device.  
I think
all vhost-level protocol features should be fully defined in the 
vhost-user

specification, which REPLY_ACK is.

Hmm that makes sense. Maybe we should have done what stefan's patch
is doing.

Do look at the original commit that introduced it to understand why
it was added.
I don’t understand why this was added to the stop/cont code, 
though.  If it
is time consuming to make these changes, why are they done every 
time the VM

is paused
and resumed?  It makes sense that this would be done for the initial
configuration (where a reset also wouldn’t hurt), but here it seems 
wrong.


(To be clear, a reset in the stop/cont code is wrong, because it breaks
stateful devices.)

Also, note 

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 06.10.23 22:49, Alex Bennée wrote:

Hanna Czenczek  writes:


On 06.10.23 17:17, Alex Bennée wrote:

Hanna Czenczek  writes:


On 06.10.23 12:34, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 11:47:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 11:26, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 10:45, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:

On 05.10.23 19:15, Michael S. Tsirkin wrote:

On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:

On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:



What I’m saying is, 923b8921d21 introduced SET_STATUS calls that broke all
devices that would implement them as per virtio spec, and even today it’s
broken for stateful devices.  The mentioned performance issue is likely
real, but we can’t address it by making up SET_STATUS calls that are wrong.

I concede that I didn’t think about DRIVER_OK.  Personally, I would do all
final configuration that would happen upon a DRIVER_OK once the first vring
is started (i.e. receives a kick).  That has the added benefit of being
asynchronous because it doesn’t block any vhost-user messages (which are
synchronous, and thus block downtime).

Hanna

For better or worse kick is per ring. It's out of spec to start rings
that were not kicked but I guess you could do configuration ...
Seems somewhat asymmetrical though.

I meant to take the first ring being started as the signal to do the
global configuration, i.e. not do this once per vring, but once
globally.


Let's wait until next week, hopefully Yajun Wu will answer.

I mean, personally I don’t really care about the whole SET_STATUS
thing.  It’s clear that it’s broken for stateful devices.  The fact
that it took until 6f8be29ec17d to fix it for just any device that
would implement it according to spec to me is a strong indication that
nobody does implement it according to spec, and is currently only used
to signal to some specific back-end that all rings have been set up
and should be configured in a single block.

I'm certainly using [GS]ET_STATUS for the proposed F_TRANSPORT
extensions where everything is off-loaded to the vhost-user backend.

How do these back-ends work with the fact that qemu uses SET_STATUS
incorrectly when not offloading?  Do you plan on fixing that?

Mainly having a common base implementation which does it right and
having very lightweight derivations for legacy stubs using it. The
aim is to eliminate the need for QEMU stubs entirely by fully specifying
the device from the vhost-user API.


If the current SET_STATUS use is overhauled, too, that would be good.  I 
wonder why you need the status byte, though.



(I.e. that we send SET_STATUS 0 when the VM is paused, potentially
resetting state that is not recoverable, and that we set DRIVER and
DRIVER_OK simultaneously.)

This is QEMU simulating a SET_STATUS rather than the guest triggering
it?


Yes, and the fact that we simulate it when the guest will not have 
triggered it, i.e. we reset the device (SET_STATUS 0) when the VM is 
paused.  Effectively, qemu injects virtio commands that the guest has 
never requested, which generally feels like a bad idea, because qemu 
will need to get the device back to its previous state before the guest 
is resumed, which may or may not work.  Specifically, it won’t work for 
devices that have internal state.


Furthermore, we use SET_STATUS to set ACKNOWLEDGE | DRIVER | DRIVER_OK 
simultaneously, which is wrong.  ACKNOWLEDGE | DRIVER may perhaps be set 
simultaneously, but then comes feature negotiation (setting and checking 
FEATURES_OK), and then DRIVER_OK.


Finally, how the status byte is to be used is not noted in the 
vhost-user specification, which instead points to the virtio 
specification.  I think if we keep SET_STATUS, it must be documented how 
it interacts with other vhost-user commands.  For example, how the 
FEATURES_OK protocol described in the virtio specification interacts 
with GET_FEATURES/SET_FEATURES, or whether SET_STATUS 0 and RESET_DEVICE 
are equivalent.  Currently, the only implementation of SET_STATUS I know 
(DPDK) ignores SET_STATUS 0, i.e. doesn’t do a reset.  To me that 
indicates that the spec must be clear on what these status values mean 
with regards to the vhost-user protocol as a whole.


So every software implementation with STATUS support that I know 
implements SET_STATUS wrongly right now, and that’s a problem, because 
it prevents implementations like virtiofsd from doing so correctly.


Hanna

Re: [Virtio-fs] (no subject)

[Yajun Wu]

On 10/6/2023 6:34 PM, Michael S. Tsirkin wrote:

External email: Use caution opening links or attachments


On Fri, Oct 06, 2023 at 11:47:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 11:26, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 10:45, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:

On 05.10.23 19:15, Michael S. Tsirkin wrote:

On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:

On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:

There is no clearly defined purpose for the virtio status byte in
vhost-user: For resetting, we already have RESET_DEVICE; and for virtio
feature negotiation, we have [GS]ET_FEATURES.  With the REPLY_ACK
protocol extension, it is possible for SET_FEATURES to return errors
(SET_PROTOCOL_FEATURES may be called before SET_FEATURES).

As for implementations, SET_STATUS is not widely implemented.  dpdk does
implement it, but only uses it to signal feature negotiation failure.
While it does log reset requests (SET_STATUS 0) as such, it effectively
ignores them, in contrast to RESET_OWNER (which is deprecated, and today
means the same thing as RESET_DEVICE).

While qemu superficially has support for [GS]ET_STATUS, it does not
forward the guest-set status byte, but instead just makes it up
internally, and actually completely ignores what the back-end returns,
only using it as the template for a subsequent SET_STATUS to add single
bits to it.  Notably, after setting FEATURES_OK, it never reads it back
to see whether the flag is still set, which is the only way in which
dpdk uses the status byte.

As-is, no front-end or back-end can rely on the other side handling this
field in a useful manner, and it also provides no practical use over
other mechanisms the vhost-user protocol has, which are more clearly
defined.  Deprecate it.

Suggested-by: Stefan Hajnoczi 
Signed-off-by: Hanna Czenczek 
---
 docs/interop/vhost-user.rst | 28 +---
 1 file changed, 21 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi 

SET_STATUS is the only way to signal failure to acknowledge FEATURES_OK.
The fact current backends never check errors does not mean they never
will. So no, not applying this.

Can this not be done with REPLY_ACK?  I.e., with the following message
order:

1. GET_FEATURES to find out whether VHOST_USER_F_PROTOCOL_FEATURES is
present
2. GET_PROTOCOL_FEATURES to hopefully get VHOST_USER_PROTOCOL_F_REPLY_ACK
3. SET_PROTOCOL_FEATURES to set VHOST_USER_PROTOCOL_F_REPLY_ACK
4. SET_FEATURES with need_reply

If not, the problem is that qemu has sent SET_STATUS 0 for a while when the
vCPUs are stopped, which generally seems to request a device reset.  If we
don’t state at least that SET_STATUS 0 is to be ignored, back-ends that will
implement SET_STATUS later may break with at least these qemu versions.  But
documenting that a particular use of the status byte is to be ignored would
be really strange.

Hanna

Hmm I guess. Though just following virtio spec seems cleaner to me...
vhost-user reconfigures the state fully on start.

Not the internal device state, though.  virtiofsd has internal state, and
other devices like vhost-gpu back-ends would probably, too.

Stefan has recently sent a series
(https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg00709.html) to
put the reset (RESET_DEVICE) into virtio_reset() (when we really need a
reset).

I really don’t like our current approach with the status byte. Following the
virtio specification to me would mean that the guest directly controls this
byte, which it does not.  qemu makes up values as it deems appropriate, and
this includes sending a SET_STATUS 0 when the guest is just paused, i.e.
when the guest really doesn’t want a device reset.

That means that qemu does not treat this as a virtio device field (because
that would mean exposing it to the guest driver), but instead treats it as
part of the vhost(-user) protocol.  It doesn’t feel right to me that we use
a virtio-defined feature for communication on the vhost level, i.e. between
front-end and back-end, and not between guest driver and device.  I think
all vhost-level protocol features should be fully defined in the vhost-user
specification, which REPLY_ACK is.

Hmm that makes sense. Maybe we should have done what stefan's patch
is doing.

Do look at the original commit that introduced it to understand why
it was added.

I don’t understand why this was added to the stop/cont code, though.  If it
is time consuming to make these changes, why are they done every time the VM
is paused
and resumed?  It makes sense that this would be done for the initial
configuration (where a reset also wouldn’t hurt), but here it seems wrong.

(To be clear, a reset in the stop/cont code is wrong, because it breaks
stateful devices.)

Also, note the newer commits 6f8be29ec17 and c3716f260bf.  The reset as
originally introduced was 

Re: [Virtio-fs] (no subject)

[Alex Bennée]

Hanna Czenczek  writes:

> On 06.10.23 17:17, Alex Bennée wrote:
>> Hanna Czenczek  writes:
>>
>>> On 06.10.23 12:34, Michael S. Tsirkin wrote:
 On Fri, Oct 06, 2023 at 11:47:55AM +0200, Hanna Czenczek wrote:
> On 06.10.23 11:26, Michael S. Tsirkin wrote:
>> On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:
>>> On 06.10.23 10:45, Michael S. Tsirkin wrote:
 On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:
> On 05.10.23 19:15, Michael S. Tsirkin wrote:
>> On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:
>>> On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:
>> 
> What I’m saying is, 923b8921d21 introduced SET_STATUS calls that broke all
> devices that would implement them as per virtio spec, and even today it’s
> broken for stateful devices.  The mentioned performance issue is likely
> real, but we can’t address it by making up SET_STATUS calls that are 
> wrong.
>
> I concede that I didn’t think about DRIVER_OK.  Personally, I would do all
> final configuration that would happen upon a DRIVER_OK once the first 
> vring
> is started (i.e. receives a kick).  That has the added benefit of being
> asynchronous because it doesn’t block any vhost-user messages (which are
> synchronous, and thus block downtime).
>
> Hanna
 For better or worse kick is per ring. It's out of spec to start rings
 that were not kicked but I guess you could do configuration ...
 Seems somewhat asymmetrical though.
>>> I meant to take the first ring being started as the signal to do the
>>> global configuration, i.e. not do this once per vring, but once
>>> globally.
>>>
 Let's wait until next week, hopefully Yajun Wu will answer.
>>> I mean, personally I don’t really care about the whole SET_STATUS
>>> thing.  It’s clear that it’s broken for stateful devices.  The fact
>>> that it took until 6f8be29ec17d to fix it for just any device that
>>> would implement it according to spec to me is a strong indication that
>>> nobody does implement it according to spec, and is currently only used
>>> to signal to some specific back-end that all rings have been set up
>>> and should be configured in a single block.
>> I'm certainly using [GS]ET_STATUS for the proposed F_TRANSPORT
>> extensions where everything is off-loaded to the vhost-user backend.
>
> How do these back-ends work with the fact that qemu uses SET_STATUS
> incorrectly when not offloading?  Do you plan on fixing that?

Mainly having a common base implementation which does it right and
having very lightweight derivations for legacy stubs using it. The
aim is to eliminate the need for QEMU stubs entirely by fully specifying
the device from the vhost-user API. 

> (I.e. that we send SET_STATUS 0 when the VM is paused, potentially
> resetting state that is not recoverable, and that we set DRIVER and
> DRIVER_OK simultaneously.)

This is QEMU simulating a SET_STATUS rather than the guest triggering
it?

>
> Hanna


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 06.10.23 17:17, Alex Bennée wrote:

Hanna Czenczek  writes:


On 06.10.23 12:34, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 11:47:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 11:26, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 10:45, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:

On 05.10.23 19:15, Michael S. Tsirkin wrote:

On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:

On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:



What I’m saying is, 923b8921d21 introduced SET_STATUS calls that broke all
devices that would implement them as per virtio spec, and even today it’s
broken for stateful devices.  The mentioned performance issue is likely
real, but we can’t address it by making up SET_STATUS calls that are wrong.

I concede that I didn’t think about DRIVER_OK.  Personally, I would do all
final configuration that would happen upon a DRIVER_OK once the first vring
is started (i.e. receives a kick).  That has the added benefit of being
asynchronous because it doesn’t block any vhost-user messages (which are
synchronous, and thus block downtime).

Hanna

For better or worse kick is per ring. It's out of spec to start rings
that were not kicked but I guess you could do configuration ...
Seems somewhat asymmetrical though.

I meant to take the first ring being started as the signal to do the
global configuration, i.e. not do this once per vring, but once
globally.


Let's wait until next week, hopefully Yajun Wu will answer.

I mean, personally I don’t really care about the whole SET_STATUS
thing.  It’s clear that it’s broken for stateful devices.  The fact
that it took until 6f8be29ec17d to fix it for just any device that
would implement it according to spec to me is a strong indication that
nobody does implement it according to spec, and is currently only used
to signal to some specific back-end that all rings have been set up
and should be configured in a single block.

I'm certainly using [GS]ET_STATUS for the proposed F_TRANSPORT
extensions where everything is off-loaded to the vhost-user backend.


How do these back-ends work with the fact that qemu uses SET_STATUS 
incorrectly when not offloading?  Do you plan on fixing that?


(I.e. that we send SET_STATUS 0 when the VM is paused, potentially 
resetting state that is not recoverable, and that we set DRIVER and 
DRIVER_OK simultaneously.)


Hanna

Re: [Virtio-fs] (no subject)

[Alex Bennée]

Hanna Czenczek  writes:

> On 06.10.23 12:34, Michael S. Tsirkin wrote:
>> On Fri, Oct 06, 2023 at 11:47:55AM +0200, Hanna Czenczek wrote:
>>> On 06.10.23 11:26, Michael S. Tsirkin wrote:
 On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:
> On 06.10.23 10:45, Michael S. Tsirkin wrote:
>> On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:
>>> On 05.10.23 19:15, Michael S. Tsirkin wrote:
 On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:
> On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:

>>> What I’m saying is, 923b8921d21 introduced SET_STATUS calls that broke all
>>> devices that would implement them as per virtio spec, and even today it’s
>>> broken for stateful devices.  The mentioned performance issue is likely
>>> real, but we can’t address it by making up SET_STATUS calls that are wrong.
>>>
>>> I concede that I didn’t think about DRIVER_OK.  Personally, I would do all
>>> final configuration that would happen upon a DRIVER_OK once the first vring
>>> is started (i.e. receives a kick).  That has the added benefit of being
>>> asynchronous because it doesn’t block any vhost-user messages (which are
>>> synchronous, and thus block downtime).
>>>
>>> Hanna
>>
>> For better or worse kick is per ring. It's out of spec to start rings
>> that were not kicked but I guess you could do configuration ...
>> Seems somewhat asymmetrical though.
>
> I meant to take the first ring being started as the signal to do the
> global configuration, i.e. not do this once per vring, but once
> globally.
>
>> Let's wait until next week, hopefully Yajun Wu will answer.
>
> I mean, personally I don’t really care about the whole SET_STATUS
> thing.  It’s clear that it’s broken for stateful devices.  The fact
> that it took until 6f8be29ec17d to fix it for just any device that
> would implement it according to spec to me is a strong indication that
> nobody does implement it according to spec, and is currently only used
> to signal to some specific back-end that all rings have been set up
> and should be configured in a single block.

I'm certainly using [GS]ET_STATUS for the proposed F_TRANSPORT
extensions where everything is off-loaded to the vhost-user backend.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 06.10.23 12:34, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 11:47:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 11:26, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 10:45, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:

On 05.10.23 19:15, Michael S. Tsirkin wrote:

On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:

On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:

There is no clearly defined purpose for the virtio status byte in
vhost-user: For resetting, we already have RESET_DEVICE; and for virtio
feature negotiation, we have [GS]ET_FEATURES.  With the REPLY_ACK
protocol extension, it is possible for SET_FEATURES to return errors
(SET_PROTOCOL_FEATURES may be called before SET_FEATURES).

As for implementations, SET_STATUS is not widely implemented.  dpdk does
implement it, but only uses it to signal feature negotiation failure.
While it does log reset requests (SET_STATUS 0) as such, it effectively
ignores them, in contrast to RESET_OWNER (which is deprecated, and today
means the same thing as RESET_DEVICE).

While qemu superficially has support for [GS]ET_STATUS, it does not
forward the guest-set status byte, but instead just makes it up
internally, and actually completely ignores what the back-end returns,
only using it as the template for a subsequent SET_STATUS to add single
bits to it.  Notably, after setting FEATURES_OK, it never reads it back
to see whether the flag is still set, which is the only way in which
dpdk uses the status byte.

As-is, no front-end or back-end can rely on the other side handling this
field in a useful manner, and it also provides no practical use over
other mechanisms the vhost-user protocol has, which are more clearly
defined.  Deprecate it.

Suggested-by: Stefan Hajnoczi 
Signed-off-by: Hanna Czenczek 
---
 docs/interop/vhost-user.rst | 28 +---
 1 file changed, 21 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi 

SET_STATUS is the only way to signal failure to acknowledge FEATURES_OK.
The fact current backends never check errors does not mean they never
will. So no, not applying this.

Can this not be done with REPLY_ACK?  I.e., with the following message
order:

1. GET_FEATURES to find out whether VHOST_USER_F_PROTOCOL_FEATURES is
present
2. GET_PROTOCOL_FEATURES to hopefully get VHOST_USER_PROTOCOL_F_REPLY_ACK
3. SET_PROTOCOL_FEATURES to set VHOST_USER_PROTOCOL_F_REPLY_ACK
4. SET_FEATURES with need_reply

If not, the problem is that qemu has sent SET_STATUS 0 for a while when the
vCPUs are stopped, which generally seems to request a device reset.  If we
don’t state at least that SET_STATUS 0 is to be ignored, back-ends that will
implement SET_STATUS later may break with at least these qemu versions.  But
documenting that a particular use of the status byte is to be ignored would
be really strange.

Hanna

Hmm I guess. Though just following virtio spec seems cleaner to me...
vhost-user reconfigures the state fully on start.

Not the internal device state, though.  virtiofsd has internal state, and
other devices like vhost-gpu back-ends would probably, too.

Stefan has recently sent a series
(https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg00709.html) to
put the reset (RESET_DEVICE) into virtio_reset() (when we really need a
reset).

I really don’t like our current approach with the status byte. Following the
virtio specification to me would mean that the guest directly controls this
byte, which it does not.  qemu makes up values as it deems appropriate, and
this includes sending a SET_STATUS 0 when the guest is just paused, i.e.
when the guest really doesn’t want a device reset.

That means that qemu does not treat this as a virtio device field (because
that would mean exposing it to the guest driver), but instead treats it as
part of the vhost(-user) protocol.  It doesn’t feel right to me that we use
a virtio-defined feature for communication on the vhost level, i.e. between
front-end and back-end, and not between guest driver and device.  I think
all vhost-level protocol features should be fully defined in the vhost-user
specification, which REPLY_ACK is.

Hmm that makes sense. Maybe we should have done what stefan's patch
is doing.

Do look at the original commit that introduced it to understand why
it was added.

I don’t understand why this was added to the stop/cont code, though.  If it
is time consuming to make these changes, why are they done every time the VM
is paused
and resumed?  It makes sense that this would be done for the initial
configuration (where a reset also wouldn’t hurt), but here it seems wrong.

(To be clear, a reset in the stop/cont code is wrong, because it breaks
stateful devices.)

Also, note the newer commits 6f8be29ec17 and c3716f260bf.  The reset as
originally introduced was wrong even for non-stateful devices, because it
occurred before we 

Re: [Virtio-fs] (no subject)

[Michael S. Tsirkin]

On Fri, Oct 06, 2023 at 11:47:55AM +0200, Hanna Czenczek wrote:
> On 06.10.23 11:26, Michael S. Tsirkin wrote:
> > On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:
> > > On 06.10.23 10:45, Michael S. Tsirkin wrote:
> > > > On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:
> > > > > On 05.10.23 19:15, Michael S. Tsirkin wrote:
> > > > > > On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:
> > > > > > > On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:
> > > > > > > > There is no clearly defined purpose for the virtio status byte 
> > > > > > > > in
> > > > > > > > vhost-user: For resetting, we already have RESET_DEVICE; and 
> > > > > > > > for virtio
> > > > > > > > feature negotiation, we have [GS]ET_FEATURES.  With the 
> > > > > > > > REPLY_ACK
> > > > > > > > protocol extension, it is possible for SET_FEATURES to return 
> > > > > > > > errors
> > > > > > > > (SET_PROTOCOL_FEATURES may be called before SET_FEATURES).
> > > > > > > > 
> > > > > > > > As for implementations, SET_STATUS is not widely implemented.  
> > > > > > > > dpdk does
> > > > > > > > implement it, but only uses it to signal feature negotiation 
> > > > > > > > failure.
> > > > > > > > While it does log reset requests (SET_STATUS 0) as such, it 
> > > > > > > > effectively
> > > > > > > > ignores them, in contrast to RESET_OWNER (which is deprecated, 
> > > > > > > > and today
> > > > > > > > means the same thing as RESET_DEVICE).
> > > > > > > > 
> > > > > > > > While qemu superficially has support for [GS]ET_STATUS, it does 
> > > > > > > > not
> > > > > > > > forward the guest-set status byte, but instead just makes it up
> > > > > > > > internally, and actually completely ignores what the back-end 
> > > > > > > > returns,
> > > > > > > > only using it as the template for a subsequent SET_STATUS to 
> > > > > > > > add single
> > > > > > > > bits to it.  Notably, after setting FEATURES_OK, it never reads 
> > > > > > > > it back
> > > > > > > > to see whether the flag is still set, which is the only way in 
> > > > > > > > which
> > > > > > > > dpdk uses the status byte.
> > > > > > > > 
> > > > > > > > As-is, no front-end or back-end can rely on the other side 
> > > > > > > > handling this
> > > > > > > > field in a useful manner, and it also provides no practical use 
> > > > > > > > over
> > > > > > > > other mechanisms the vhost-user protocol has, which are more 
> > > > > > > > clearly
> > > > > > > > defined.  Deprecate it.
> > > > > > > > 
> > > > > > > > Suggested-by: Stefan Hajnoczi 
> > > > > > > > Signed-off-by: Hanna Czenczek 
> > > > > > > > ---
> > > > > > > > docs/interop/vhost-user.rst | 28 
> > > > > > > > +---
> > > > > > > > 1 file changed, 21 insertions(+), 7 deletions(-)
> > > > > > > Reviewed-by: Stefan Hajnoczi 
> > > > > > SET_STATUS is the only way to signal failure to acknowledge 
> > > > > > FEATURES_OK.
> > > > > > The fact current backends never check errors does not mean they 
> > > > > > never
> > > > > > will. So no, not applying this.
> > > > > Can this not be done with REPLY_ACK?  I.e., with the following message
> > > > > order:
> > > > > 
> > > > > 1. GET_FEATURES to find out whether VHOST_USER_F_PROTOCOL_FEATURES is
> > > > > present
> > > > > 2. GET_PROTOCOL_FEATURES to hopefully get 
> > > > > VHOST_USER_PROTOCOL_F_REPLY_ACK
> > > > > 3. SET_PROTOCOL_FEATURES to set VHOST_USER_PROTOCOL_F_REPLY_ACK
> > > > > 4. SET_FEATURES with need_reply
> > > > > 
> > > > > If not, the problem is that qemu has sent SET_STATUS 0 for a while 
> > > > > when the
> > > > > vCPUs are stopped, which generally seems to request a device reset.  
> > > > > If we
> > > > > don’t state at least that SET_STATUS 0 is to be ignored, back-ends 
> > > > > that will
> > > > > implement SET_STATUS later may break with at least these qemu 
> > > > > versions.  But
> > > > > documenting that a particular use of the status byte is to be ignored 
> > > > > would
> > > > > be really strange.
> > > > > 
> > > > > Hanna
> > > > Hmm I guess. Though just following virtio spec seems cleaner to me...
> > > > vhost-user reconfigures the state fully on start.
> > > Not the internal device state, though.  virtiofsd has internal state, and
> > > other devices like vhost-gpu back-ends would probably, too.
> > > 
> > > Stefan has recently sent a series
> > > (https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg00709.html) 
> > > to
> > > put the reset (RESET_DEVICE) into virtio_reset() (when we really need a
> > > reset).
> > > 
> > > I really don’t like our current approach with the status byte. Following 
> > > the
> > > virtio specification to me would mean that the guest directly controls 
> > > this
> > > byte, which it does not.  qemu makes up values as it deems appropriate, 
> > > and
> > > this includes sending a SET_STATUS 0 when the guest is just paused, i.e.
> > > when the guest really doesn’t want a device reset.
> > > 
> > > That 

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 06.10.23 11:26, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:

On 06.10.23 10:45, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:

On 05.10.23 19:15, Michael S. Tsirkin wrote:

On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:

On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:

There is no clearly defined purpose for the virtio status byte in
vhost-user: For resetting, we already have RESET_DEVICE; and for virtio
feature negotiation, we have [GS]ET_FEATURES.  With the REPLY_ACK
protocol extension, it is possible for SET_FEATURES to return errors
(SET_PROTOCOL_FEATURES may be called before SET_FEATURES).

As for implementations, SET_STATUS is not widely implemented.  dpdk does
implement it, but only uses it to signal feature negotiation failure.
While it does log reset requests (SET_STATUS 0) as such, it effectively
ignores them, in contrast to RESET_OWNER (which is deprecated, and today
means the same thing as RESET_DEVICE).

While qemu superficially has support for [GS]ET_STATUS, it does not
forward the guest-set status byte, but instead just makes it up
internally, and actually completely ignores what the back-end returns,
only using it as the template for a subsequent SET_STATUS to add single
bits to it.  Notably, after setting FEATURES_OK, it never reads it back
to see whether the flag is still set, which is the only way in which
dpdk uses the status byte.

As-is, no front-end or back-end can rely on the other side handling this
field in a useful manner, and it also provides no practical use over
other mechanisms the vhost-user protocol has, which are more clearly
defined.  Deprecate it.

Suggested-by: Stefan Hajnoczi 
Signed-off-by: Hanna Czenczek 
---
docs/interop/vhost-user.rst | 28 +---
1 file changed, 21 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi 

SET_STATUS is the only way to signal failure to acknowledge FEATURES_OK.
The fact current backends never check errors does not mean they never
will. So no, not applying this.

Can this not be done with REPLY_ACK?  I.e., with the following message
order:

1. GET_FEATURES to find out whether VHOST_USER_F_PROTOCOL_FEATURES is
present
2. GET_PROTOCOL_FEATURES to hopefully get VHOST_USER_PROTOCOL_F_REPLY_ACK
3. SET_PROTOCOL_FEATURES to set VHOST_USER_PROTOCOL_F_REPLY_ACK
4. SET_FEATURES with need_reply

If not, the problem is that qemu has sent SET_STATUS 0 for a while when the
vCPUs are stopped, which generally seems to request a device reset.  If we
don’t state at least that SET_STATUS 0 is to be ignored, back-ends that will
implement SET_STATUS later may break with at least these qemu versions.  But
documenting that a particular use of the status byte is to be ignored would
be really strange.

Hanna

Hmm I guess. Though just following virtio spec seems cleaner to me...
vhost-user reconfigures the state fully on start.

Not the internal device state, though.  virtiofsd has internal state, and
other devices like vhost-gpu back-ends would probably, too.

Stefan has recently sent a series
(https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg00709.html) to
put the reset (RESET_DEVICE) into virtio_reset() (when we really need a
reset).

I really don’t like our current approach with the status byte. Following the
virtio specification to me would mean that the guest directly controls this
byte, which it does not.  qemu makes up values as it deems appropriate, and
this includes sending a SET_STATUS 0 when the guest is just paused, i.e.
when the guest really doesn’t want a device reset.

That means that qemu does not treat this as a virtio device field (because
that would mean exposing it to the guest driver), but instead treats it as
part of the vhost(-user) protocol.  It doesn’t feel right to me that we use
a virtio-defined feature for communication on the vhost level, i.e. between
front-end and back-end, and not between guest driver and device.  I think
all vhost-level protocol features should be fully defined in the vhost-user
specification, which REPLY_ACK is.

Hmm that makes sense. Maybe we should have done what stefan's patch
is doing.

Do look at the original commit that introduced it to understand why
it was added.


I don’t understand why this was added to the stop/cont code, though.  If 
it is time consuming to make these changes, why are they done every time 
the VM is paused
and resumed?  It makes sense that this would be done for the initial 
configuration (where a reset also wouldn’t hurt), but here it seems wrong.


(To be clear, a reset in the stop/cont code is wrong, because it breaks 
stateful devices.)


Also, note the newer commits 6f8be29ec17 and c3716f260bf.  The reset as 
originally introduced was wrong even for non-stateful devices, because 
it occurred before we fetched the state (vring indices) so we could 
restore it later.  I don’t know how 923b8921d21 was 

Re: [Virtio-fs] (no subject)

[Michael S. Tsirkin]

On Fri, Oct 06, 2023 at 11:15:55AM +0200, Hanna Czenczek wrote:
> On 06.10.23 10:45, Michael S. Tsirkin wrote:
> > On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:
> > > On 05.10.23 19:15, Michael S. Tsirkin wrote:
> > > > On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:
> > > > > On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:
> > > > > > There is no clearly defined purpose for the virtio status byte in
> > > > > > vhost-user: For resetting, we already have RESET_DEVICE; and for 
> > > > > > virtio
> > > > > > feature negotiation, we have [GS]ET_FEATURES.  With the REPLY_ACK
> > > > > > protocol extension, it is possible for SET_FEATURES to return errors
> > > > > > (SET_PROTOCOL_FEATURES may be called before SET_FEATURES).
> > > > > > 
> > > > > > As for implementations, SET_STATUS is not widely implemented.  dpdk 
> > > > > > does
> > > > > > implement it, but only uses it to signal feature negotiation 
> > > > > > failure.
> > > > > > While it does log reset requests (SET_STATUS 0) as such, it 
> > > > > > effectively
> > > > > > ignores them, in contrast to RESET_OWNER (which is deprecated, and 
> > > > > > today
> > > > > > means the same thing as RESET_DEVICE).
> > > > > > 
> > > > > > While qemu superficially has support for [GS]ET_STATUS, it does not
> > > > > > forward the guest-set status byte, but instead just makes it up
> > > > > > internally, and actually completely ignores what the back-end 
> > > > > > returns,
> > > > > > only using it as the template for a subsequent SET_STATUS to add 
> > > > > > single
> > > > > > bits to it.  Notably, after setting FEATURES_OK, it never reads it 
> > > > > > back
> > > > > > to see whether the flag is still set, which is the only way in which
> > > > > > dpdk uses the status byte.
> > > > > > 
> > > > > > As-is, no front-end or back-end can rely on the other side handling 
> > > > > > this
> > > > > > field in a useful manner, and it also provides no practical use over
> > > > > > other mechanisms the vhost-user protocol has, which are more clearly
> > > > > > defined.  Deprecate it.
> > > > > > 
> > > > > > Suggested-by: Stefan Hajnoczi 
> > > > > > Signed-off-by: Hanna Czenczek 
> > > > > > ---
> > > > > >docs/interop/vhost-user.rst | 28 +---
> > > > > >1 file changed, 21 insertions(+), 7 deletions(-)
> > > > > Reviewed-by: Stefan Hajnoczi 
> > > > SET_STATUS is the only way to signal failure to acknowledge FEATURES_OK.
> > > > The fact current backends never check errors does not mean they never
> > > > will. So no, not applying this.
> > > Can this not be done with REPLY_ACK?  I.e., with the following message
> > > order:
> > > 
> > > 1. GET_FEATURES to find out whether VHOST_USER_F_PROTOCOL_FEATURES is
> > > present
> > > 2. GET_PROTOCOL_FEATURES to hopefully get VHOST_USER_PROTOCOL_F_REPLY_ACK
> > > 3. SET_PROTOCOL_FEATURES to set VHOST_USER_PROTOCOL_F_REPLY_ACK
> > > 4. SET_FEATURES with need_reply
> > > 
> > > If not, the problem is that qemu has sent SET_STATUS 0 for a while when 
> > > the
> > > vCPUs are stopped, which generally seems to request a device reset.  If we
> > > don’t state at least that SET_STATUS 0 is to be ignored, back-ends that 
> > > will
> > > implement SET_STATUS later may break with at least these qemu versions.  
> > > But
> > > documenting that a particular use of the status byte is to be ignored 
> > > would
> > > be really strange.
> > > 
> > > Hanna
> > Hmm I guess. Though just following virtio spec seems cleaner to me...
> > vhost-user reconfigures the state fully on start.
> 
> Not the internal device state, though.  virtiofsd has internal state, and
> other devices like vhost-gpu back-ends would probably, too.
> 
> Stefan has recently sent a series
> (https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg00709.html) to
> put the reset (RESET_DEVICE) into virtio_reset() (when we really need a
> reset).
> 
> I really don’t like our current approach with the status byte. Following the
> virtio specification to me would mean that the guest directly controls this
> byte, which it does not.  qemu makes up values as it deems appropriate, and
> this includes sending a SET_STATUS 0 when the guest is just paused, i.e.
> when the guest really doesn’t want a device reset.
> 
> That means that qemu does not treat this as a virtio device field (because
> that would mean exposing it to the guest driver), but instead treats it as
> part of the vhost(-user) protocol.  It doesn’t feel right to me that we use
> a virtio-defined feature for communication on the vhost level, i.e. between
> front-end and back-end, and not between guest driver and device.  I think
> all vhost-level protocol features should be fully defined in the vhost-user
> specification, which REPLY_ACK is.

Hmm that makes sense. Maybe we should have done what stefan's patch
is doing.

Do look at the original commit that introduced it to understand why
it was added.

> Now, we 

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 06.10.23 10:45, Michael S. Tsirkin wrote:

On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:

On 05.10.23 19:15, Michael S. Tsirkin wrote:

On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:

On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:

There is no clearly defined purpose for the virtio status byte in
vhost-user: For resetting, we already have RESET_DEVICE; and for virtio
feature negotiation, we have [GS]ET_FEATURES.  With the REPLY_ACK
protocol extension, it is possible for SET_FEATURES to return errors
(SET_PROTOCOL_FEATURES may be called before SET_FEATURES).

As for implementations, SET_STATUS is not widely implemented.  dpdk does
implement it, but only uses it to signal feature negotiation failure.
While it does log reset requests (SET_STATUS 0) as such, it effectively
ignores them, in contrast to RESET_OWNER (which is deprecated, and today
means the same thing as RESET_DEVICE).

While qemu superficially has support for [GS]ET_STATUS, it does not
forward the guest-set status byte, but instead just makes it up
internally, and actually completely ignores what the back-end returns,
only using it as the template for a subsequent SET_STATUS to add single
bits to it.  Notably, after setting FEATURES_OK, it never reads it back
to see whether the flag is still set, which is the only way in which
dpdk uses the status byte.

As-is, no front-end or back-end can rely on the other side handling this
field in a useful manner, and it also provides no practical use over
other mechanisms the vhost-user protocol has, which are more clearly
defined.  Deprecate it.

Suggested-by: Stefan Hajnoczi 
Signed-off-by: Hanna Czenczek 
---
   docs/interop/vhost-user.rst | 28 +---
   1 file changed, 21 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi 

SET_STATUS is the only way to signal failure to acknowledge FEATURES_OK.
The fact current backends never check errors does not mean they never
will. So no, not applying this.

Can this not be done with REPLY_ACK?  I.e., with the following message
order:

1. GET_FEATURES to find out whether VHOST_USER_F_PROTOCOL_FEATURES is
present
2. GET_PROTOCOL_FEATURES to hopefully get VHOST_USER_PROTOCOL_F_REPLY_ACK
3. SET_PROTOCOL_FEATURES to set VHOST_USER_PROTOCOL_F_REPLY_ACK
4. SET_FEATURES with need_reply

If not, the problem is that qemu has sent SET_STATUS 0 for a while when the
vCPUs are stopped, which generally seems to request a device reset.  If we
don’t state at least that SET_STATUS 0 is to be ignored, back-ends that will
implement SET_STATUS later may break with at least these qemu versions.  But
documenting that a particular use of the status byte is to be ignored would
be really strange.

Hanna

Hmm I guess. Though just following virtio spec seems cleaner to me...
vhost-user reconfigures the state fully on start.


Not the internal device state, though.  virtiofsd has internal state, 
and other devices like vhost-gpu back-ends would probably, too.


Stefan has recently sent a series 
(https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg00709.html) 
to put the reset (RESET_DEVICE) into virtio_reset() (when we really need 
a reset).


I really don’t like our current approach with the status byte. Following 
the virtio specification to me would mean that the guest directly 
controls this byte, which it does not.  qemu makes up values as it deems 
appropriate, and this includes sending a SET_STATUS 0 when the guest is 
just paused, i.e. when the guest really doesn’t want a device reset.


That means that qemu does not treat this as a virtio device field 
(because that would mean exposing it to the guest driver), but instead 
treats it as part of the vhost(-user) protocol.  It doesn’t feel right 
to me that we use a virtio-defined feature for communication on the 
vhost level, i.e. between front-end and back-end, and not between guest 
driver and device.  I think all vhost-level protocol features should be 
fully defined in the vhost-user specification, which REPLY_ACK is.


Now, we could hand full control of the status byte to the guest, and 
that would make me content.  But I feel like that doesn’t really work, 
because qemu needs to intercept the status byte anyway (it needs to know 
when there is a reset, probably wants to know when the device is 
configured, etc.), so I don’t think having the status byte in vhost-user 
really gains us much when qemu could translate status byte changes 
to/from other vhost-user commands.


Hanna


I guess symmetry was the
point. So I don't see why SET_STATUS 0 has to be ignored.


SET_STATUS was introduced by:

commit 923b8921d210763359e96246a58658ac0db6c645
Author: Yajun Wu 
Date:   Mon Oct 17 14:44:52 2022 +0800

 vhost-user: Support vhost_dev_start

CC the author.

Re: [Virtio-fs] (no subject)

[Michael S. Tsirkin]

On Fri, Oct 06, 2023 at 09:48:14AM +0200, Hanna Czenczek wrote:
> On 05.10.23 19:15, Michael S. Tsirkin wrote:
> > On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:
> > > On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:
> > > > There is no clearly defined purpose for the virtio status byte in
> > > > vhost-user: For resetting, we already have RESET_DEVICE; and for virtio
> > > > feature negotiation, we have [GS]ET_FEATURES.  With the REPLY_ACK
> > > > protocol extension, it is possible for SET_FEATURES to return errors
> > > > (SET_PROTOCOL_FEATURES may be called before SET_FEATURES).
> > > > 
> > > > As for implementations, SET_STATUS is not widely implemented.  dpdk does
> > > > implement it, but only uses it to signal feature negotiation failure.
> > > > While it does log reset requests (SET_STATUS 0) as such, it effectively
> > > > ignores them, in contrast to RESET_OWNER (which is deprecated, and today
> > > > means the same thing as RESET_DEVICE).
> > > > 
> > > > While qemu superficially has support for [GS]ET_STATUS, it does not
> > > > forward the guest-set status byte, but instead just makes it up
> > > > internally, and actually completely ignores what the back-end returns,
> > > > only using it as the template for a subsequent SET_STATUS to add single
> > > > bits to it.  Notably, after setting FEATURES_OK, it never reads it back
> > > > to see whether the flag is still set, which is the only way in which
> > > > dpdk uses the status byte.
> > > > 
> > > > As-is, no front-end or back-end can rely on the other side handling this
> > > > field in a useful manner, and it also provides no practical use over
> > > > other mechanisms the vhost-user protocol has, which are more clearly
> > > > defined.  Deprecate it.
> > > > 
> > > > Suggested-by: Stefan Hajnoczi 
> > > > Signed-off-by: Hanna Czenczek 
> > > > ---
> > > >   docs/interop/vhost-user.rst | 28 +---
> > > >   1 file changed, 21 insertions(+), 7 deletions(-)
> > > Reviewed-by: Stefan Hajnoczi 
> > 
> > SET_STATUS is the only way to signal failure to acknowledge FEATURES_OK.
> > The fact current backends never check errors does not mean they never
> > will. So no, not applying this.
> 
> Can this not be done with REPLY_ACK?  I.e., with the following message
> order:
> 
> 1. GET_FEATURES to find out whether VHOST_USER_F_PROTOCOL_FEATURES is
> present
> 2. GET_PROTOCOL_FEATURES to hopefully get VHOST_USER_PROTOCOL_F_REPLY_ACK
> 3. SET_PROTOCOL_FEATURES to set VHOST_USER_PROTOCOL_F_REPLY_ACK
> 4. SET_FEATURES with need_reply
> 
> If not, the problem is that qemu has sent SET_STATUS 0 for a while when the
> vCPUs are stopped, which generally seems to request a device reset.  If we
> don’t state at least that SET_STATUS 0 is to be ignored, back-ends that will
> implement SET_STATUS later may break with at least these qemu versions.  But
> documenting that a particular use of the status byte is to be ignored would
> be really strange.
> 
> Hanna

Hmm I guess. Though just following virtio spec seems cleaner to me...
vhost-user reconfigures the state fully on start. I guess symmetry was the
point. So I don't see why SET_STATUS 0 has to be ignored.


SET_STATUS was introduced by:

commit 923b8921d210763359e96246a58658ac0db6c645
Author: Yajun Wu 
Date:   Mon Oct 17 14:44:52 2022 +0800

vhost-user: Support vhost_dev_start

CC the author.

-- 
MST

Re: [Virtio-fs] (no subject)

[Hanna Czenczek]

On 05.10.23 19:15, Michael S. Tsirkin wrote:

On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:

On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:

There is no clearly defined purpose for the virtio status byte in
vhost-user: For resetting, we already have RESET_DEVICE; and for virtio
feature negotiation, we have [GS]ET_FEATURES.  With the REPLY_ACK
protocol extension, it is possible for SET_FEATURES to return errors
(SET_PROTOCOL_FEATURES may be called before SET_FEATURES).

As for implementations, SET_STATUS is not widely implemented.  dpdk does
implement it, but only uses it to signal feature negotiation failure.
While it does log reset requests (SET_STATUS 0) as such, it effectively
ignores them, in contrast to RESET_OWNER (which is deprecated, and today
means the same thing as RESET_DEVICE).

While qemu superficially has support for [GS]ET_STATUS, it does not
forward the guest-set status byte, but instead just makes it up
internally, and actually completely ignores what the back-end returns,
only using it as the template for a subsequent SET_STATUS to add single
bits to it.  Notably, after setting FEATURES_OK, it never reads it back
to see whether the flag is still set, which is the only way in which
dpdk uses the status byte.

As-is, no front-end or back-end can rely on the other side handling this
field in a useful manner, and it also provides no practical use over
other mechanisms the vhost-user protocol has, which are more clearly
defined.  Deprecate it.

Suggested-by: Stefan Hajnoczi 
Signed-off-by: Hanna Czenczek 
---
  docs/interop/vhost-user.rst | 28 +---
  1 file changed, 21 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi 


SET_STATUS is the only way to signal failure to acknowledge FEATURES_OK.
The fact current backends never check errors does not mean they never
will. So no, not applying this.


Can this not be done with REPLY_ACK?  I.e., with the following message 
order:


1. GET_FEATURES to find out whether VHOST_USER_F_PROTOCOL_FEATURES is 
present

2. GET_PROTOCOL_FEATURES to hopefully get VHOST_USER_PROTOCOL_F_REPLY_ACK
3. SET_PROTOCOL_FEATURES to set VHOST_USER_PROTOCOL_F_REPLY_ACK
4. SET_FEATURES with need_reply

If not, the problem is that qemu has sent SET_STATUS 0 for a while when 
the vCPUs are stopped, which generally seems to request a device reset.  
If we don’t state at least that SET_STATUS 0 is to be ignored, back-ends 
that will implement SET_STATUS later may break with at least these qemu 
versions.  But documenting that a particular use of the status byte is 
to be ignored would be really strange.


Hanna

[no subject]

[Michael S. Tsirkin]

On Thu, Oct 05, 2023 at 01:08:52PM -0400, Stefan Hajnoczi wrote:
> On Wed, Oct 04, 2023 at 02:58:57PM +0200, Hanna Czenczek wrote:
> > There is no clearly defined purpose for the virtio status byte in
> > vhost-user: For resetting, we already have RESET_DEVICE; and for virtio
> > feature negotiation, we have [GS]ET_FEATURES.  With the REPLY_ACK
> > protocol extension, it is possible for SET_FEATURES to return errors
> > (SET_PROTOCOL_FEATURES may be called before SET_FEATURES).
> > 
> > As for implementations, SET_STATUS is not widely implemented.  dpdk does
> > implement it, but only uses it to signal feature negotiation failure.
> > While it does log reset requests (SET_STATUS 0) as such, it effectively
> > ignores them, in contrast to RESET_OWNER (which is deprecated, and today
> > means the same thing as RESET_DEVICE).
> > 
> > While qemu superficially has support for [GS]ET_STATUS, it does not
> > forward the guest-set status byte, but instead just makes it up
> > internally, and actually completely ignores what the back-end returns,
> > only using it as the template for a subsequent SET_STATUS to add single
> > bits to it.  Notably, after setting FEATURES_OK, it never reads it back
> > to see whether the flag is still set, which is the only way in which
> > dpdk uses the status byte.
> > 
> > As-is, no front-end or back-end can rely on the other side handling this
> > field in a useful manner, and it also provides no practical use over
> > other mechanisms the vhost-user protocol has, which are more clearly
> > defined.  Deprecate it.
> > 
> > Suggested-by: Stefan Hajnoczi 
> > Signed-off-by: Hanna Czenczek 
> > ---
> >  docs/interop/vhost-user.rst | 28 +---
> >  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi 


SET_STATUS is the only way to signal failure to acknowledge FEATURES_OK.
The fact current backends never check errors does not mean they never
will. So no, not applying this.

-- 
MST

[no subject]

[Bilal Elmoussaoui]

Expose the recently added multi touch support on the UI DBus backend

Bilal Elmoussaoui (2):
  ui/touch: Move event handling to a common helper
  ui/dbus: Expose a touch device interface

 include/ui/console.h | 15 ++
 ui/console.c | 65 
 ui/dbus-console.c| 59 +++-
 ui/dbus-display1.xml | 45 --
 ui/gtk.c | 61 -
 ui/trace-events  |  1 +
 6 files changed, 187 insertions(+), 59 deletions(-)

From: Bilal Elmoussaoui 
Reply-To: 
Subject: 
In-Reply-To: 

[no subject]

[Bilal Elmoussaoui]

Expose the recently added multi touch support on the UI DBus backend

Bilal Elmoussaoui (2):
  ui/touch: Move event handling to a common helper
  ui/dbus: Expose a touch device interface

 include/ui/console.h | 15 ++
 ui/console.c | 65 
 ui/dbus-console.c| 59 +++-
 ui/dbus-display1.xml | 45 --
 ui/gtk.c | 61 -
 ui/trace-events  |  1 +
 6 files changed, 187 insertions(+), 59 deletions(-)

Re: [PATCH] Subject:[PATCH] cxl-cdat:Fix open file not closed in ct3_load_cdat

[Hao Zeng]

On Tue, 2023-04-11 at 16:39 +0100, Peter Maydell wrote:
Dear Peter
Thank you for taking the time to reply to my email. I appreciate your
the valuable information you have provided.

> On Mon, 3 Apr 2023 at 13:51, zenghao  wrote:
> > 
> > opened file processor not closed,May cause file processor leaks
> > 
> > Fixes:aba578bdace5303a441f8a37aad781b5cb06f38c
> > 
> > Signed-off-by: Zeng Hao 
> > Suggested-by: Xie Ming 
> > ---
> >  hw/cxl/cxl-cdat.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> > index 137abd0992..ba7ed1aafd 100644
> > --- a/hw/cxl/cxl-cdat.c
> > +++ b/hw/cxl/cxl-cdat.c
> > @@ -128,6 +128,7 @@ static void ct3_load_cdat(CDATObject *cdat,
> > Error **errp)
> > 
> >  if (fread(cdat->buf, file_size, 1, fp) == 0) {
> >  error_setg(errp, "CDAT: File read failed");
> > +    fclose(fp);
> >  return;
> >  }
> 
> Coverity also spotted this, as CID 1508069.
> 
> The other bug in this code (CID 1507822) is that the
> check on the return value of fread() is wrong. fread()
> returns the number of items read or written, so
> checking for == 0 only catches "no data read at all",
> not "only read half the data". This check should be
>  if (fread(cdat->buf, file_size, 1, fp) != file_size) {
>     ...
>  }
> I think.
>  
> thanks
> -- PMM
I couldn't agree more with your thoughts.
I will fix the bug in a separate commit.(CID 1507822)

Best regards
Hao

Re: [PATCH] Subject:[PATCH] cxl-cdat:Fix open file not closed in ct3_load_cdat

[Peter Maydell]

On Mon, 3 Apr 2023 at 13:51, zenghao  wrote:
>
> opened file processor not closed,May cause file processor leaks
>
> Fixes:aba578bdace5303a441f8a37aad781b5cb06f38c
>
> Signed-off-by: Zeng Hao 
> Suggested-by: Xie Ming 
> ---
>  hw/cxl/cxl-cdat.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 137abd0992..ba7ed1aafd 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -128,6 +128,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
>
>  if (fread(cdat->buf, file_size, 1, fp) == 0) {
>  error_setg(errp, "CDAT: File read failed");
> +fclose(fp);
>  return;
>  }

Coverity also spotted this, as CID 1508069.

The other bug in this code (CID 1507822) is that the
check on the return value of fread() is wrong. fread()
returns the number of items read or written, so
checking for == 0 only catches "no data read at all",
not "only read half the data". This check should be
 if (fread(cdat->buf, file_size, 1, fp) != file_size) {
...
 }
I think.

thanks
-- PMM

Re: [PATCH] Subject:[PATCH] cxl-cdat:Fix open file not closed in ct3_load_cdat

[Fan Ni]

On Mon, Apr 03, 2023 at 04:42:45PM +0800, zenghao wrote:
> opened file processor not closed,May cause file processor leaks
> 
> Fixes:aba578bdace5303a441f8a37aad781b5cb06f38c
> 
> Signed-off-by: Zeng Hao 
> Suggested-by: Xie Ming 
> ---
>  hw/cxl/cxl-cdat.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 137abd0992..ba7ed1aafd 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -128,6 +128,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
>  
>  if (fread(cdat->buf, file_size, 1, fp) == 0) {
>  error_setg(errp, "CDAT: File read failed");
> +fclose(fp);
>  return;
>  }
>  
Good catch.

Reviewed-by: Fan Ni 

> -- 
> 2.37.2
> 
> 
> No virus found
>   Checked by Hillstone Network AntiVirus

[PATCH] Subject:[PATCH] cxl-cdat:Fix open file not closed in ct3_load_cdat

[zenghao]

opened file processor not closed,May cause file processor leaks

Fixes:aba578bdace5303a441f8a37aad781b5cb06f38c

Signed-off-by: Zeng Hao 
Suggested-by: Xie Ming 
---
 hw/cxl/cxl-cdat.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 137abd0992..ba7ed1aafd 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -128,6 +128,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
 
 if (fread(cdat->buf, file_size, 1, fp) == 0) {
 error_setg(errp, "CDAT: File read failed");
+fclose(fp);
 return;
 }
 
-- 
2.37.2


No virus found
Checked by Hillstone Network AntiVirus

[no subject]

[grace funmilola]

I would like to unsubscribe to the list pls

Subject: [PATCH] hw/intc/arm_gicv3: Fix GICD_TYPER ITLinesNumber advertisement

[Luke Starrett]

The ARM GICv3 TRM describes that the ITLinesNumber field of GICD_TYPER
register:

"indicates the maximum SPI INTID that the GIC implementation supports"

As SPI #0 is absolute IRQ #32, the max SPI INTID should have accounted
for the internal 16x SGI's and 16x PPI's.  However, the original GICv3
model subtracted off the SGI/PPI.  Cosmetically this can be seen at OS
boot (Linux) showing 32 shy of what should be there, i.e.:

    [    0.00] GICv3: 224 SPIs implemented

Though in hw/arm/virt.c, the machine is configured for 256 SPI's. ARM
virt machine likely doesn't have a problem with this because the upper
32 IRQ's don't actually have anything meaningful wired. But, this does
become a functional issue on a custom use case which wants to make use
of these IRQ's.  Additionally, boot code (i.e. TF-A) will only init up
to the number (blocks of 32) that it believes to actually be there.

Signed-off-by: Luke Starrett 
---
 hw/intc/arm_gicv3_dist.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
index eea0368118..d599fefcbc 100644
--- a/hw/intc/arm_gicv3_dist.c
+++ b/hw/intc/arm_gicv3_dist.c
@@ -390,9 +390,9 @@ static bool gicd_readl(GICv3State *s, hwaddr offset,
  * MBIS == 0 (message-based SPIs not supported)
  * SecurityExtn == 1 if security extns supported
  * CPUNumber == 0 since for us ARE is always 1
- * ITLinesNumber == (num external irqs / 32) - 1
+ * ITLinesNumber == (((max SPI IntID + 1) / 32) - 1)
  */
-    int itlinesnumber = ((s->num_irq - GIC_INTERNAL) / 32) - 1;
+    int itlinesnumber = (s->num_irq / 32) - 1;
 /*
  * SecurityExtn must be RAZ if GICD_CTLR.DS == 1, and
  * "security extensions not supported" always implies DS == 1,
--
2.27.0

Re: Subject: [PATCH] hw/intc/arm_gicv3: Fix GICD_TYPER ITLinesNumber advertisement

[Luke Starrett]

Please disregard this one.  Sent in error.

On 11/22/2022 2:18 PM, Luke Starrett wrote:

The ARM GICv3 TRM describes that the ITLinesNumber field of GICD_TYPER
register:

"indicates the maximum SPI INTID that the GIC implementation supports"

As SPI #0 is absolute IRQ #32, the max SPI INTID should have accounted
for the internal 16x SGI's and 16x PPI's.  However, the original GICv3
model subtracted off the SGI/PPI.  Cosmetically this can be seen at OS
boot (Linux) showing 32 shy of what should be there, i.e.:

    [    0.00] GICv3: 224 SPIs implemented

Though in hw/arm/virt.c, the machine is configured for 256 SPI's. ARM
virt machine likely doesn't have a problem with this because the upper
32 IRQ's don't actually have anything meaningful wired. But, this does
become a functional issue on a custom use case which wants to make use
of these IRQ's.  Additionally, boot code (i.e. TF-A) will only init up
to the number (blocks of 32) that it believes to actually be there.

Signed-off-by: Luke Starrett 
---
 hw/intc/arm_gicv3_dist.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
index eea0368118..d599fefcbc 100644
--- a/hw/intc/arm_gicv3_dist.c
+++ b/hw/intc/arm_gicv3_dist.c
@@ -390,9 +390,9 @@ static bool gicd_readl(GICv3State *s, hwaddr offset,
  * MBIS == 0 (message-based SPIs not supported)
  * SecurityExtn == 1 if security extns supported
  * CPUNumber == 0 since for us ARE is always 1
- * ITLinesNumber == (num external irqs / 32) - 1
+ * ITLinesNumber == (((max SPI IntID + 1) / 32) - 1)
  */
-    int itlinesnumber = ((s->num_irq - GIC_INTERNAL) / 32) - 1;
+    int itlinesnumber = (s->num_irq / 32) - 1;
 /*
  * SecurityExtn must be RAZ if GICD_CTLR.DS == 1, and
  * "security extensions not supported" always implies DS == 1,

[no subject]

[张 泽宇]

unsubscribe

[no subject]

[张 泽宇]

unsubscribe

[no subject]

[Ankur Agrawal]

[no subject]

[Eugenio Pérez]

Please review this new minimal version. It is way shorter, but this
comes with a cost:
* Iteration does not stop at the end of range (but an out of range
  allocation never happens)
* Iteration must start from iova == 0 instead of first valid entry in
  the hole.

These should not be a big deal though.

Another possible optimization that comes to my mind is to allocate
always at the end of range. In the case of having to allocate and
deallocate frequently, this should avoid to iterate over long lived
entries. Better justify this with numbers, so I left that out.

Thanks!

[no subject]

[Damien Hedde]

Subject: [PATCH for 6.2?] gicv3: fix ICH_MISR's LRENP computation

According to the "Arm Generic Interrupt Controller Architecture
Specification GIC architecture version 3 and 4" (version G: page 345
for aarch64 or 509 for aarch32):
LRENP bit of ICH_MISR is set when ICH_HCR.LRENPIE==1 and
ICH_HCR.EOIcount is non-zero.

When only LRENPIE was set (and EOI count was zero), the LRENP bit was
wrongly set and MISR value was wrong.

As an additional consequence, if an hypervisor set ICH_HCR.LRENPIE,
the maintenance interrupt was constantly fired. It happens since patch
9cee1efe92 ("hw/intc: Set GIC maintenance interrupt level to only 0 or 1")
which fixed another bug about maintenance interrupt (most significant
bits of misr, including this one, were ignored in the interrupt trigger).

Fixes: 83f036fe3d ("hw/intc/arm_gicv3: Add accessors for ICH_ system registers")
Signed-off-by: Damien Hedde 
---
The gic doc is available here:
https://developer.arm.com/documentation/ihi0069/g
---
 hw/intc/arm_gicv3_cpuif.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 7fba931450..85fc369e55 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -351,7 +351,8 @@ static uint32_t maintenance_interrupt_state(GICv3CPUState 
*cs)
 /* Scan list registers and fill in the U, NP and EOI bits */
 eoi_maintenance_interrupt_state(cs, );
 
-if (cs->ich_hcr_el2 & (ICH_HCR_EL2_LRENPIE | ICH_HCR_EL2_EOICOUNT_MASK)) {
+if ((cs->ich_hcr_el2 & ICH_HCR_EL2_LRENPIE) &&
+(cs->ich_hcr_el2 & ICH_HCR_EL2_EOICOUNT_MASK)) {
 value |= ICH_MISR_EL2_LRENP;
 }
 
-- 
2.34.0

[no subject]

[David Dai]

Add a virtual pci to QEMU, this pci device is used to dynamically attach memroy 
to VM,
so driver in guest can apply host memory in fly without virtualization 
management
software's help, such as libvirt/manager. The attached memory is isolated from 
System RAM,
 it can be used in heterogeneous memory management for virtualization.

Re: [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 16.09.2021 15:57, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
>> 
>>> Great! Thanks for working on this!
>>>
>>> 15.09.2021 22:24, Markus Armbruster wrote:
 PATCH 1+2 add feature flags to enum members.  Awkward due to an
 introspection design mistake; see PATCH 1 for details.  Feedback
 welcome, in particular from management application guys.
 PATCH 3+4 implement policy deprecated-input={reject,crash} for enum
 values.

 Policy deprecated-output=hide is not implemented, because we can't
 hide a value without hiding the entire member, which is almost
 certainly more than the requester of this policy bargained for.
 Perhaps we want a new policy deprecated-output=crash to help us catch
 unwanted use of deprecated enum values.  Thoughts?
>>>
>>> I think crash policy for output is doubtful. If we have query-*
>>> command that returns a lot of things and some of them are deprecated
>>> the whole command will always crash..
>> 
>> Policy "crash" is not quite as crazy as it may look :)
>> 
>> The non-default policies for handling deprecated interfaces are intended
>> for testing users of these interfaces.
>> 
>> Input policy reject and output policy hide enable "testing the future".
>> 
>> Input policy crash is a robust way to double-check "management
>> application does not send deprecated input".  This is not quite the same
>> as "testing the future".  A management application may choose to send
>> deprecated input, detect the failure and recover.  Testing that should
>> pass with input policy reject, but not with input policy crash.
>> 
>> Output policy crash could be a way to double-check "the management
>> application does not make QEMU send deprecated output".
>> 
>> Example: Say we deprecate BlockdevDriver member 'vvfat'.  We know that
>> output of query-named-block-nodes can contain 'vvfat' only if something
>> creates such nodes.  Input policy reject will reject attempts to use
>> this driver with blockdev-add.  But what if the management application
>> uses some other way to create such nodes, not (yet) covered by input
>> policy?  Output policy crash could be used to double-check it doesn't.
>> 
>> Except it doesn't actually work, because as you said, testing will
>> likely produce *other* deprecated output that should *not* crash the
>> test.
>> 
>> Perhaps a policy hide-or-else-crash could work.
>> 
>>> Deprecated is "use" of the
>>> deprecated field, but we can't control does user use a specific field
>>> or not.
>>>
>>> If some deprecated output is a consequence of deprecated input, we'd
>>> better always show it.. Or in other words, we shouldn't deprecate such
>>> output, deprecating of the corresponding input is enough.
>> 
>> Deprecating only input may require duplicating definitions used both for
>> input and output, unless we replace today's feature flags by something
>> more sophisticated.
>> 
>> Example: BlockdevDriver is used both as input of blockdev-add and as
>> output of query-named-block-nodes.  Deprecate member 'vvfat' affects
>> both input and output.
>
>
> If we deprecate a vvfat, but still have some not deprecated ways to
> create vvfat node, that's a kind of bug[1] in deprecation system: if
> vvfat is deprecated, all ways to create vvfat should go through input
> compat policy.

We need to distinguish between three separate things:

(1) Deprecating certain interface usage

(2) QAPI feature flag 'deprecated'

(3) Policy for handling deprecated interface usage

Note that (2) cannot cover all of (1) for two reasons, only one of them
fixable:

* Some interfaces still aren't QAPI-based.  QAPIfying them all is hard.

* Feature flags only apply to *syntactic* elements, such as a command
  argument.

  Example for a deprecation where they don't apply: we deprecated use of
  chardev-add argument "wait" together with "server": false in v4.0
  (it's an error since v6.0).  Use without "server": false remains okay.

Note further that (3) may cover both less and more than (2).

Before this series, it covers exactly (2).  Afterwards, there is a hole:

# Limitation: deprecated-output policy @hide is not implemented for
# enumeration values.  They behave the same as with policy @accept.

But it could also go beyond (2) in the future:

# Limitation: covers only syntactic aspects of QMP, i.e. stuff tagged
# with feature 'deprecated'.  We may want to extend it to cover
# semantic aspects, CLI, and experimental features.

> So, making qemu crash on trying to output "vvfat" for me looks like a
> workaround for that bug[1]. And it's strange to create and interface
> to workaround the internal bug..
>
> May be, we can just enable hide-or-else-crash behavior automatically,
> when user choose input=crash and output=hide?

Hmm, interesting idea.  Needs to be documented, obviously.

[...]

Re: [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members

[Vladimir Sementsov-Ogievskiy]

16.09.2021 15:57, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


Great! Thanks for working on this!

15.09.2021 22:24, Markus Armbruster wrote:

PATCH 1+2 add feature flags to enum members.  Awkward due to an
introspection design mistake; see PATCH 1 for details.  Feedback
welcome, in particular from management application guys.
PATCH 3+4 implement policy deprecated-input={reject,crash} for enum
values.

Policy deprecated-output=hide is not implemented, because we can't
hide a value without hiding the entire member, which is almost
certainly more than the requester of this policy bargained for.
Perhaps we want a new policy deprecated-output=crash to help us catch
unwanted use of deprecated enum values.  Thoughts?


I think crash policy for output is doubtful. If we have query-*
command that returns a lot of things and some of them are deprecated
the whole command will always crash..


Policy "crash" is not quite as crazy as it may look :)

The non-default policies for handling deprecated interfaces are intended
for testing users of these interfaces.

Input policy reject and output policy hide enable "testing the future".

Input policy crash is a robust way to double-check "management
application does not send deprecated input".  This is not quite the same
as "testing the future".  A management application may choose to send
deprecated input, detect the failure and recover.  Testing that should
pass with input policy reject, but not with input policy crash.

Output policy crash could be a way to double-check "the management
application does not make QEMU send deprecated output".

Example: Say we deprecate BlockdevDriver member 'vvfat'.  We know that
output of query-named-block-nodes can contain 'vvfat' only if something
creates such nodes.  Input policy reject will reject attempts to use
this driver with blockdev-add.  But what if the management application
uses some other way to create such nodes, not (yet) covered by input
policy?  Output policy crash could be used to double-check it doesn't.

Except it doesn't actually work, because as you said, testing will
likely produce *other* deprecated output that should *not* crash the
test.

Perhaps a policy hide-or-else-crash could work.


Deprecated is "use" of the
deprecated field, but we can't control does user use a specific field
or not.

If some deprecated output is a consequence of deprecated input, we'd
better always show it.. Or in other words, we shouldn't deprecate such
output, deprecating of the corresponding input is enough.


Deprecating only input may require duplicating definitions used both for
input and output, unless we replace today's feature flags by something
more sophisticated.

Example: BlockdevDriver is used both as input of blockdev-add and as
output of query-named-block-nodes.  Deprecate member 'vvfat' affects
both input and output.



If we deprecate a vvfat, but still have some not deprecated ways to create 
vvfat node, that's a kind of bug[1] in deprecation system: if vvfat is 
deprecated, all ways to create vvfat should go through input compat policy.

So, making qemu crash on trying to output "vvfat" for me looks like a 
workaround for that bug[1]. And it's strange to create and interface to workaround the 
internal bug..

May be, we can just enable hide-or-else-crash behavior automatically, when user 
choose input=crash and output=hide?




So, we are saying about showing some internal state to the user. And
part of this state becomes deprecated because internal implementation
changed. I think the only correct thing to do is handle
deprecated=hide by hand, in the place where we set this deprecated
field. Only at this place we can decide, should we simulate old
deprecated output value or not. For this we'll need a possibility to
get current policy at any place, but that doesn't seem to be a
problem, I see, it's just a global variable compat_policy.


I'm not sure I fully got this.

Compat policies are not about optionally providing older versions of an
interface ("simulate old deprecated output value").  They try to help
with testing users of interfaces.  One aspect of that is optionally
approximating expected future interfaces.



--
Best regards,
Vladimir

Re: [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> Great! Thanks for working on this!
>
> 15.09.2021 22:24, Markus Armbruster wrote:
>> PATCH 1+2 add feature flags to enum members.  Awkward due to an
>> introspection design mistake; see PATCH 1 for details.  Feedback
>> welcome, in particular from management application guys.
>> PATCH 3+4 implement policy deprecated-input={reject,crash} for enum
>> values.
>>
>> Policy deprecated-output=hide is not implemented, because we can't
>> hide a value without hiding the entire member, which is almost
>> certainly more than the requester of this policy bargained for.
>> Perhaps we want a new policy deprecated-output=crash to help us catch
>> unwanted use of deprecated enum values.  Thoughts?
>
> I think crash policy for output is doubtful. If we have query-*
> command that returns a lot of things and some of them are deprecated
> the whole command will always crash..

Policy "crash" is not quite as crazy as it may look :)

The non-default policies for handling deprecated interfaces are intended
for testing users of these interfaces.

Input policy reject and output policy hide enable "testing the future".

Input policy crash is a robust way to double-check "management
application does not send deprecated input".  This is not quite the same
as "testing the future".  A management application may choose to send
deprecated input, detect the failure and recover.  Testing that should
pass with input policy reject, but not with input policy crash.

Output policy crash could be a way to double-check "the management
application does not make QEMU send deprecated output".

Example: Say we deprecate BlockdevDriver member 'vvfat'.  We know that
output of query-named-block-nodes can contain 'vvfat' only if something
creates such nodes.  Input policy reject will reject attempts to use
this driver with blockdev-add.  But what if the management application
uses some other way to create such nodes, not (yet) covered by input
policy?  Output policy crash could be used to double-check it doesn't.

Except it doesn't actually work, because as you said, testing will
likely produce *other* deprecated output that should *not* crash the
test.

Perhaps a policy hide-or-else-crash could work.

>Deprecated is "use" of the
> deprecated field, but we can't control does user use a specific field
> or not.
>
> If some deprecated output is a consequence of deprecated input, we'd
> better always show it.. Or in other words, we shouldn't deprecate such
> output, deprecating of the corresponding input is enough.

Deprecating only input may require duplicating definitions used both for
input and output, unless we replace today's feature flags by something
more sophisticated.

Example: BlockdevDriver is used both as input of blockdev-add and as
output of query-named-block-nodes.  Deprecate member 'vvfat' affects
both input and output.

> So, we are saying about showing some internal state to the user. And
> part of this state becomes deprecated because internal implementation
> changed. I think the only correct thing to do is handle
> deprecated=hide by hand, in the place where we set this deprecated
> field. Only at this place we can decide, should we simulate old
> deprecated output value or not. For this we'll need a possibility to
> get current policy at any place, but that doesn't seem to be a
> problem, I see, it's just a global variable compat_policy.

I'm not sure I fully got this.

Compat policies are not about optionally providing older versions of an
interface ("simulate old deprecated output value").  They try to help
with testing users of interfaces.  One aspect of that is optionally
approximating expected future interfaces.

Re: [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members

[Vladimir Sementsov-Ogievskiy]

Great! Thanks for working on this!

15.09.2021 22:24, Markus Armbruster wrote:

PATCH 1+2 add feature flags to enum members.  Awkward due to an
introspection design mistake; see PATCH 1 for details.  Feedback
welcome, in particular from management application guys.

PATCH 3+4 implement policy deprecated-input={reject,crash} for enum
values.

Policy deprecated-output=hide is not implemented, because we can't
hide a value without hiding the entire member, which is almost
certainly more than the requester of this policy bargained for.
Perhaps we want a new policy deprecated-output=crash to help us catch
unwanted use of deprecated enum values.  Thoughts?


I think crash policy for output is doubtful. If we have query-* command that returns a 
lot of things and some of them are deprecated the whole command will always crash..  
Deprecated is "use" of the deprecated field, but we can't control does user use 
a specific field or not.

If some deprecated output is a consequence of deprecated input, we'd better 
always show it.. Or in other words, we shouldn't deprecate such output, 
deprecating of the corresponding input is enough.

So, we are saying about showing some internal state to the user. And part of 
this state becomes deprecated because internal implementation changed. I think 
the only correct thing to do is handle deprecated=hide by hand, in the place 
where we set this deprecated field. Only at this place we can decide, should we 
simulate old deprecated output value or not. For this we'll need a possibility 
to get current policy at any place, but that doesn't seem to be a problem, I 
see, it's just a global variable compat_policy.



PATCH 5 puts the new feature flags to use.  It makes sense only on top
of Vladimir's deprecation of drive-backup.  See its commit message for
a reference.

Based on my "[PATCH 00/22] qapi: Remove simple unions from the schema
language".

Based-on: Message-Id: <20210913123932.3306639-1-arm...@redhat.com>

Markus Armbruster (5):
   qapi: Enable enum member introspection to show more than name
   qapi: Add feature flags to enum members
   qapi: Move compat policy from QObject to generic visitor
   qapi: Implement deprecated-input={reject,crash} for enum values
   block: Deprecate transaction type drive-backup

  docs/devel/qapi-code-gen.rst  |  4 ++-
  qapi/compat.json  |  3 +++
  qapi/introspect.json  | 23 ++--
  qapi/transaction.json |  5 +++-
  include/qapi/qobject-input-visitor.h  |  4 ---
  include/qapi/qobject-output-visitor.h |  4 ---
  include/qapi/util.h   |  6 -
  include/qapi/visitor-impl.h   |  3 +++
  include/qapi/visitor.h|  9 +++
  qapi/qapi-visit-core.c| 27 ---
  qapi/qmp-dispatch.c   |  4 +--
  qapi/qobject-input-visitor.c  | 14 +-
  qapi/qobject-output-visitor.c | 14 +-
  scripts/qapi/expr.py  |  3 ++-
  scripts/qapi/introspect.py| 19 ++---
  scripts/qapi/schema.py| 22 +--
  scripts/qapi/types.py | 17 +++-
  tests/qapi-schema/doc-good.json   |  5 +++-
  tests/qapi-schema/doc-good.out|  3 +++
  tests/qapi-schema/doc-good.txt|  3 +++
  .../qapi-schema/enum-dict-member-unknown.err  |  2 +-
  tests/qapi-schema/qapi-schema-test.json   |  3 ++-
  tests/qapi-schema/qapi-schema-test.out|  1 +
  tests/qapi-schema/test-qapi.py|  1 +
  24 files changed, 144 insertions(+), 55 deletions(-)




--
Best regards,
Vladimir

[PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members

[Markus Armbruster]

PATCH 1+2 add feature flags to enum members.  Awkward due to an
introspection design mistake; see PATCH 1 for details.  Feedback
welcome, in particular from management application guys.

PATCH 3+4 implement policy deprecated-input={reject,crash} for enum
values.

Policy deprecated-output=hide is not implemented, because we can't
hide a value without hiding the entire member, which is almost
certainly more than the requester of this policy bargained for.
Perhaps we want a new policy deprecated-output=crash to help us catch
unwanted use of deprecated enum values.  Thoughts?

PATCH 5 puts the new feature flags to use.  It makes sense only on top
of Vladimir's deprecation of drive-backup.  See its commit message for
a reference.

Based on my "[PATCH 00/22] qapi: Remove simple unions from the schema
language".

Based-on: Message-Id: <20210913123932.3306639-1-arm...@redhat.com>

Markus Armbruster (5):
  qapi: Enable enum member introspection to show more than name
  qapi: Add feature flags to enum members
  qapi: Move compat policy from QObject to generic visitor
  qapi: Implement deprecated-input={reject,crash} for enum values
  block: Deprecate transaction type drive-backup

 docs/devel/qapi-code-gen.rst  |  4 ++-
 qapi/compat.json  |  3 +++
 qapi/introspect.json  | 23 ++--
 qapi/transaction.json |  5 +++-
 include/qapi/qobject-input-visitor.h  |  4 ---
 include/qapi/qobject-output-visitor.h |  4 ---
 include/qapi/util.h   |  6 -
 include/qapi/visitor-impl.h   |  3 +++
 include/qapi/visitor.h|  9 +++
 qapi/qapi-visit-core.c| 27 ---
 qapi/qmp-dispatch.c   |  4 +--
 qapi/qobject-input-visitor.c  | 14 +-
 qapi/qobject-output-visitor.c | 14 +-
 scripts/qapi/expr.py  |  3 ++-
 scripts/qapi/introspect.py| 19 ++---
 scripts/qapi/schema.py| 22 +--
 scripts/qapi/types.py | 17 +++-
 tests/qapi-schema/doc-good.json   |  5 +++-
 tests/qapi-schema/doc-good.out|  3 +++
 tests/qapi-schema/doc-good.txt|  3 +++
 .../qapi-schema/enum-dict-member-unknown.err  |  2 +-
 tests/qapi-schema/qapi-schema-test.json   |  3 ++-
 tests/qapi-schema/qapi-schema-test.out|  1 +
 tests/qapi-schema/test-qapi.py|  1 +
 24 files changed, 144 insertions(+), 55 deletions(-)

-- 
2.31.1

[PULL 2/6] docs/about: Unify the subject format

[Thomas Huth]

From: Yanan Wang 

There is a mixture of "since/removed in X.Y" vs "since/removed in X.Y.Z"
in the subjects in deprecated.rst/removed-features.rst. It will be better
to use an unified format. It seems unlikely that we will ever deprecate
something in a stable release, and even more unlikely that we'll remove
something in one, so the short versions look like the thing we want to
standardize on.

So here we unify the subject format in deprecated.rst to "since X.Y", and
unify the subject format in removed-features.rst to "removed in X.Y".

Signed-off-by: Yanan Wang 
Reviewed-by: Cornelia Huck 
Reviewed-by: Andrew Jones 
Reviewed-by: Thomas Huth 
Message-Id: <20210823030005.165668-3-wangyana...@huawei.com>
Signed-off-by: Thomas Huth 
---
 docs/about/deprecated.rst   | 56 -
 docs/about/removed-features.rst | 28 -
 2 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 6d438f1c8d..8d4fd384a5 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -107,8 +107,8 @@ the process listing. This is replaced by the new 
``password-secret``
 option which lets the password be securely provided on the command
 line using a ``secret`` object instance.
 
-``opened`` property of ``rng-*`` objects (since 6.0.0)
-''
+``opened`` property of ``rng-*`` objects (since 6.0)
+
 
 The only effect of specifying ``opened=on`` in the command line or QMP
 ``object-add`` is that the device is opened immediately, possibly before all
@@ -116,8 +116,8 @@ other options have been processed.  This will either have 
no effect (if
 ``opened`` was the last option) or cause errors.  The property is therefore
 useless and should not be specified.
 
-``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0.0)
-''
+``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0)
+
 
 The only effect of specifying ``loaded=on`` in the command line or QMP
 ``object-add`` is that the secret is loaded immediately, possibly before all
@@ -142,33 +142,33 @@ should be used instead.
 QEMU Machine Protocol (QMP) commands
 
 
-``blockdev-open-tray``, ``blockdev-close-tray`` argument ``device`` (since 
2.8.0)
-'
+``blockdev-open-tray``, ``blockdev-close-tray`` argument ``device`` (since 2.8)
+'''
 
 Use argument ``id`` instead.
 
-``eject`` argument ``device`` (since 2.8.0)
-'''
+``eject`` argument ``device`` (since 2.8)
+'
 
 Use argument ``id`` instead.
 
-``blockdev-change-medium`` argument ``device`` (since 2.8.0)
-
+``blockdev-change-medium`` argument ``device`` (since 2.8)
+''
 
 Use argument ``id`` instead.
 
-``block_set_io_throttle`` argument ``device`` (since 2.8.0)
-'''
+``block_set_io_throttle`` argument ``device`` (since 2.8)
+'
 
 Use argument ``id`` instead.
 
-``blockdev-add`` empty string argument ``backing`` (since 2.10.0)
-'
+``blockdev-add`` empty string argument ``backing`` (since 2.10)
+'''
 
 Use argument value ``null`` instead.
 
-``block-commit`` arguments ``base`` and ``top`` (since 3.1.0)
-'
+``block-commit`` arguments ``base`` and ``top`` (since 3.1)
+'''
 
 Use arguments ``base-node`` and ``top-node`` instead.
 
@@ -191,8 +191,8 @@ from Linux upstream kernel, declare it deprecated.
 System emulator CPUS
 
 
-``Icelake-Client`` CPU Model (since 5.2.0)
-''
+``Icelake-Client`` CPU Model (since 5.2)
+
 
 ``Icelake-Client`` CPU Models are deprecated. Use ``Icelake-Server`` CPU
 Models instead.
@@ -245,8 +245,8 @@ Device options
 Emulated device options
 '''
 
-``-device virtio-blk,scsi=on|off`` (since 5.0.0)
-
+``-device virtio-blk,scsi=on|off`` (since 5.0)
+^^
 
 The virtio-blk SCSI passthrough feature is a legacy VIRTIO feature.  V

[PULL 3/6] docs/about: Add the missing release record in the subject

[Thomas Huth]

From: Yanan Wang 

Commit 29e0447551
(docs/about/removed-features: Document removed CLI options from QEMU v3.1)
has recorded some CLI options as replaced/removed from QEMU v3.1, but one
of the subjects has missed the release record. Let's fix it.

Reported-by: Cornelia Huck 
Signed-off-by: Yanan Wang 
Reviewed-by: Andrew Jones 
Reviewed-by: Thomas Huth 
Message-Id: <20210823030005.165668-4-wangyana...@huawei.com>
Signed-off-by: Thomas Huth 
---
 docs/about/removed-features.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 1c926a8bc1..8feeead449 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -140,8 +140,8 @@ Use ``-rtc driftfix=slew`` instead.
 
 Replaced by ``-rtc base=date``.
 
-``-vnc ...,tls=...``, ``-vnc ...,x509=...`` & ``-vnc ...,x509verify=...``
-'
+``-vnc ...,tls=...``, ``-vnc ...,x509=...`` & ``-vnc ...,x509verify=...`` 
(removed in 3.1)
+''
 
 The "tls-creds" option should be used instead to point to a "tls-creds-x509"
 object created using "-object".
-- 
2.27.0

Re: [PATCH v2 3/3] docs/about: Add the missing release record in the subject

[Andrew Jones]

On Mon, Aug 23, 2021 at 11:00:05AM +0800, Yanan Wang wrote:
> Commit 29e0447551
> (docs/about/removed-features: Document removed CLI options from QEMU v3.1)
> has recorded some CLI options as replaced/removed from QEMU v3.1, but one
> of the subjects has missed the release record. Let's fix it.
> 
> Reported-by: Cornelia Huck 
> Signed-off-by: Yanan Wang 
> ---
>  docs/about/removed-features.rst | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
> index 1c926a8bc1..8feeead449 100644
> --- a/docs/about/removed-features.rst
> +++ b/docs/about/removed-features.rst
> @@ -140,8 +140,8 @@ Use ``-rtc driftfix=slew`` instead.
>  
>  Replaced by ``-rtc base=date``.
>  
> -``-vnc ...,tls=...``, ``-vnc ...,x509=...`` & ``-vnc ...,x509verify=...``
> -'
> +``-vnc ...,tls=...``, ``-vnc ...,x509=...`` & ``-vnc ...,x509verify=...`` 
> (removed in 3.1)
> +''
>  
>  The "tls-creds" option should be used instead to point to a "tls-creds-x509"
>  object created using "-object".
> -- 
> 2.19.1
>

Reviewed-by: Andrew Jones 

Re: [PATCH v2 2/3] docs/about: Unify the subject format

[Andrew Jones]

On Mon, Aug 23, 2021 at 11:00:04AM +0800, Yanan Wang wrote:
> There is a mixture of "since/removed in X.Y" vs "since/removed in X.Y.Z"
> in the subjects in deprecated.rst/removed-features.rst. It will be better
> to use an unified format. It seems unlikely that we will ever deprecate
> something in a stable release, and even more unlikely that we'll remove
> something in one, so the short versions look like the thing we want to
> standardize on.
> 
> So here we unify the subject format in deprecated.rst to "since X.Y", and
> unify the subject format in removed-features.rst to "removed in X.Y".
> 
> Signed-off-by: Yanan Wang 
> Reviewed-by: Cornelia Huck 
> ---
>  docs/about/deprecated.rst   | 56 -
>  docs/about/removed-features.rst | 28 -
>  2 files changed, 42 insertions(+), 42 deletions(-)
>

Reviewed-by: Andrew Jones 

Re: [PATCH v2 3/3] docs/about: Add the missing release record in the subject

[Thomas Huth]

On 23/08/2021 05.00, Yanan Wang wrote:

Commit 29e0447551
(docs/about/removed-features: Document removed CLI options from QEMU v3.1)
has recorded some CLI options as replaced/removed from QEMU v3.1, but one
of the subjects has missed the release record. Let's fix it.

Reported-by: Cornelia Huck 
Signed-off-by: Yanan Wang 
---
  docs/about/removed-features.rst | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 1c926a8bc1..8feeead449 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -140,8 +140,8 @@ Use ``-rtc driftfix=slew`` instead.
  
  Replaced by ``-rtc base=date``.
  
-``-vnc ...,tls=...``, ``-vnc ...,x509=...`` & ``-vnc ...,x509verify=...``

-'
+``-vnc ...,tls=...``, ``-vnc ...,x509=...`` & ``-vnc ...,x509verify=...`` 
(removed in 3.1)
+''


Reviewed-by: Thomas Huth 

Re: [PATCH v2 2/3] docs/about: Unify the subject format

[Thomas Huth]

On 23/08/2021 05.00, Yanan Wang wrote:

There is a mixture of "since/removed in X.Y" vs "since/removed in X.Y.Z"
in the subjects in deprecated.rst/removed-features.rst. It will be better
to use an unified format. It seems unlikely that we will ever deprecate
something in a stable release, and even more unlikely that we'll remove
something in one, so the short versions look like the thing we want to
standardize on.

So here we unify the subject format in deprecated.rst to "since X.Y", and
unify the subject format in removed-features.rst to "removed in X.Y".

Signed-off-by: Yanan Wang 
Reviewed-by: Cornelia Huck 
---
  docs/about/deprecated.rst   | 56 -
  docs/about/removed-features.rst | 28 -
  2 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 6d438f1c8d..8d4fd384a5 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -107,8 +107,8 @@ the process listing. This is replaced by the new 
``password-secret``
  option which lets the password be securely provided on the command
  line using a ``secret`` object instance.
  
-``opened`` property of ``rng-*`` objects (since 6.0.0)

-''
+``opened`` property of ``rng-*`` objects (since 6.0)
+
  
  The only effect of specifying ``opened=on`` in the command line or QMP

  ``object-add`` is that the device is opened immediately, possibly before all
@@ -116,8 +116,8 @@ other options have been processed.  This will either have 
no effect (if
  ``opened`` was the last option) or cause errors.  The property is therefore
  useless and should not be specified.
  
-``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0.0)

-''
+``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0)
+
  
  The only effect of specifying ``loaded=on`` in the command line or QMP

  ``object-add`` is that the secret is loaded immediately, possibly before all
@@ -142,33 +142,33 @@ should be used instead.
  QEMU Machine Protocol (QMP) commands
  
  
-``blockdev-open-tray``, ``blockdev-close-tray`` argument ``device`` (since 2.8.0)

-'
+``blockdev-open-tray``, ``blockdev-close-tray`` argument ``device`` (since 2.8)
+'''
  
  Use argument ``id`` instead.
  
-``eject`` argument ``device`` (since 2.8.0)

-'''
+``eject`` argument ``device`` (since 2.8)
+'
  
  Use argument ``id`` instead.
  
-``blockdev-change-medium`` argument ``device`` (since 2.8.0)

-
+``blockdev-change-medium`` argument ``device`` (since 2.8)
+''
  
  Use argument ``id`` instead.
  
-``block_set_io_throttle`` argument ``device`` (since 2.8.0)

-'''
+``block_set_io_throttle`` argument ``device`` (since 2.8)
+'
  
  Use argument ``id`` instead.
  
-``blockdev-add`` empty string argument ``backing`` (since 2.10.0)

-'
+``blockdev-add`` empty string argument ``backing`` (since 2.10)
+'''
  
  Use argument value ``null`` instead.
  
-``block-commit`` arguments ``base`` and ``top`` (since 3.1.0)

-'
+``block-commit`` arguments ``base`` and ``top`` (since 3.1)
+'''
  
  Use arguments ``base-node`` and ``top-node`` instead.
  
@@ -191,8 +191,8 @@ from Linux upstream kernel, declare it deprecated.

  System emulator CPUS
  
  
-``Icelake-Client`` CPU Model (since 5.2.0)

-''
+``Icelake-Client`` CPU Model (since 5.2)
+
  
  ``Icelake-Client`` CPU Models are deprecated. Use ``Icelake-Server`` CPU

  Models instead.
@@ -245,8 +245,8 @@ Device options
  Emulated device options
  '''
  
-``-device virtio-blk,scsi=on|off`` (since 5.0.0)

-
+``-device virtio-blk,scsi=on|off`` (since 5.0)
+^^
  
  The virtio-blk SCSI passthrough feature is a legacy VIRTIO feature.  VIRTIO 1.0

  and later do not support it because the virtio-scsi devi

[PATCH v2 2/3] docs/about: Unify the subject format

[Yanan Wang]

There is a mixture of "since/removed in X.Y" vs "since/removed in X.Y.Z"
in the subjects in deprecated.rst/removed-features.rst. It will be better
to use an unified format. It seems unlikely that we will ever deprecate
something in a stable release, and even more unlikely that we'll remove
something in one, so the short versions look like the thing we want to
standardize on.

So here we unify the subject format in deprecated.rst to "since X.Y", and
unify the subject format in removed-features.rst to "removed in X.Y".

Signed-off-by: Yanan Wang 
Reviewed-by: Cornelia Huck 
---
 docs/about/deprecated.rst   | 56 -
 docs/about/removed-features.rst | 28 -
 2 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 6d438f1c8d..8d4fd384a5 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -107,8 +107,8 @@ the process listing. This is replaced by the new 
``password-secret``
 option which lets the password be securely provided on the command
 line using a ``secret`` object instance.
 
-``opened`` property of ``rng-*`` objects (since 6.0.0)
-''
+``opened`` property of ``rng-*`` objects (since 6.0)
+
 
 The only effect of specifying ``opened=on`` in the command line or QMP
 ``object-add`` is that the device is opened immediately, possibly before all
@@ -116,8 +116,8 @@ other options have been processed.  This will either have 
no effect (if
 ``opened`` was the last option) or cause errors.  The property is therefore
 useless and should not be specified.
 
-``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0.0)
-''
+``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0)
+
 
 The only effect of specifying ``loaded=on`` in the command line or QMP
 ``object-add`` is that the secret is loaded immediately, possibly before all
@@ -142,33 +142,33 @@ should be used instead.
 QEMU Machine Protocol (QMP) commands
 
 
-``blockdev-open-tray``, ``blockdev-close-tray`` argument ``device`` (since 
2.8.0)
-'
+``blockdev-open-tray``, ``blockdev-close-tray`` argument ``device`` (since 2.8)
+'''
 
 Use argument ``id`` instead.
 
-``eject`` argument ``device`` (since 2.8.0)
-'''
+``eject`` argument ``device`` (since 2.8)
+'
 
 Use argument ``id`` instead.
 
-``blockdev-change-medium`` argument ``device`` (since 2.8.0)
-
+``blockdev-change-medium`` argument ``device`` (since 2.8)
+''
 
 Use argument ``id`` instead.
 
-``block_set_io_throttle`` argument ``device`` (since 2.8.0)
-'''
+``block_set_io_throttle`` argument ``device`` (since 2.8)
+'
 
 Use argument ``id`` instead.
 
-``blockdev-add`` empty string argument ``backing`` (since 2.10.0)
-'
+``blockdev-add`` empty string argument ``backing`` (since 2.10)
+'''
 
 Use argument value ``null`` instead.
 
-``block-commit`` arguments ``base`` and ``top`` (since 3.1.0)
-'
+``block-commit`` arguments ``base`` and ``top`` (since 3.1)
+'''
 
 Use arguments ``base-node`` and ``top-node`` instead.
 
@@ -191,8 +191,8 @@ from Linux upstream kernel, declare it deprecated.
 System emulator CPUS
 
 
-``Icelake-Client`` CPU Model (since 5.2.0)
-''
+``Icelake-Client`` CPU Model (since 5.2)
+
 
 ``Icelake-Client`` CPU Models are deprecated. Use ``Icelake-Server`` CPU
 Models instead.
@@ -245,8 +245,8 @@ Device options
 Emulated device options
 '''
 
-``-device virtio-blk,scsi=on|off`` (since 5.0.0)
-
+``-device virtio-blk,scsi=on|off`` (since 5.0)
+^^
 
 The virtio-blk SCSI passthrough feature is a legacy VIRTIO feature.  VIRTIO 1.0
 and later do not support it because the virtio-scsi device was introduced for
@@ -258,14 +258,14 @@ alias.
 Block device options
 '''

[PATCH v2 3/3] docs/about: Add the missing release record in the subject

[Yanan Wang]

Commit 29e0447551
(docs/about/removed-features: Document removed CLI options from QEMU v3.1)
has recorded some CLI options as replaced/removed from QEMU v3.1, but one
of the subjects has missed the release record. Let's fix it.

Reported-by: Cornelia Huck 
Signed-off-by: Yanan Wang 
---
 docs/about/removed-features.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 1c926a8bc1..8feeead449 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -140,8 +140,8 @@ Use ``-rtc driftfix=slew`` instead.
 
 Replaced by ``-rtc base=date``.
 
-``-vnc ...,tls=...``, ``-vnc ...,x509=...`` & ``-vnc ...,x509verify=...``
-'
+``-vnc ...,tls=...``, ``-vnc ...,x509=...`` & ``-vnc ...,x509verify=...`` 
(removed in 3.1)
+''
 
 The "tls-creds" option should be used instead to point to a "tls-creds-x509"
 object created using "-object".
-- 
2.19.1

Re: [PATCH 2/2] docs/about: Unify the subject format

[wangyanan (Y)]

On 2021/8/20 18:18, Cornelia Huck wrote:

On Fri, Aug 20 2021, Yanan Wang  wrote:


Unify the subject format in deprecated.rst to "since X.Y".
Unify the subject format in removed-features.rst to "removed in X.Y".

It seems unlikely that we will ever deprecate something in a stable
release, and even more unlikely that we'll remove something in one, so
the short versions look like the thing we want to standardize on.


Signed-off-by: Yanan Wang 
---
  docs/about/deprecated.rst   | 56 -
  docs/about/removed-features.rst | 28 -
  2 files changed, 42 insertions(+), 42 deletions(-)

Unrelated to your patch, line 143 in removed-features.rst reads

``-vnc ...,tls=...``, ``-vnc ...,x509=...`` & ``-vnc ...,x509verify=...``

and is missing the release it was removed in (presumably 3.1?)

Yes, this part of section was introduced by commit 29e0447551
(docs/about/removed-features: Document removed CLI options from QEMU v3.1),
so I think a record of "removed in 3.1" is needed. I rechecked both 
deprecated.rst
and removed-features.rst, and this is the only place where we are 
missing a release.

I can fix this in a third patch.

Anyway,

Reviewed-by: Cornelia Huck 

.

Thanks,
Yanan

Re: [PATCH 2/2] docs/about: Unify the subject format

[Cornelia Huck]

On Fri, Aug 20 2021, Yanan Wang  wrote:

> Unify the subject format in deprecated.rst to "since X.Y".
> Unify the subject format in removed-features.rst to "removed in X.Y".

It seems unlikely that we will ever deprecate something in a stable
release, and even more unlikely that we'll remove something in one, so
the short versions look like the thing we want to standardize on.

>
> Signed-off-by: Yanan Wang 
> ---
>  docs/about/deprecated.rst   | 56 -
>  docs/about/removed-features.rst | 28 -
>  2 files changed, 42 insertions(+), 42 deletions(-)

Unrelated to your patch, line 143 in removed-features.rst reads

``-vnc ...,tls=...``, ``-vnc ...,x509=...`` & ``-vnc ...,x509verify=...``

and is missing the release it was removed in (presumably 3.1?)

Anyway,

Reviewed-by: Cornelia Huck 

[PATCH 2/2] docs/about: Unify the subject format

[Yanan Wang]

Unify the subject format in deprecated.rst to "since X.Y".
Unify the subject format in removed-features.rst to "removed in X.Y".

Signed-off-by: Yanan Wang 
---
 docs/about/deprecated.rst   | 56 -
 docs/about/removed-features.rst | 28 -
 2 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 6d438f1c8d..8d4fd384a5 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -107,8 +107,8 @@ the process listing. This is replaced by the new 
``password-secret``
 option which lets the password be securely provided on the command
 line using a ``secret`` object instance.
 
-``opened`` property of ``rng-*`` objects (since 6.0.0)
-''
+``opened`` property of ``rng-*`` objects (since 6.0)
+
 
 The only effect of specifying ``opened=on`` in the command line or QMP
 ``object-add`` is that the device is opened immediately, possibly before all
@@ -116,8 +116,8 @@ other options have been processed.  This will either have 
no effect (if
 ``opened`` was the last option) or cause errors.  The property is therefore
 useless and should not be specified.
 
-``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0.0)
-''
+``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0)
+
 
 The only effect of specifying ``loaded=on`` in the command line or QMP
 ``object-add`` is that the secret is loaded immediately, possibly before all
@@ -142,33 +142,33 @@ should be used instead.
 QEMU Machine Protocol (QMP) commands
 
 
-``blockdev-open-tray``, ``blockdev-close-tray`` argument ``device`` (since 
2.8.0)
-'
+``blockdev-open-tray``, ``blockdev-close-tray`` argument ``device`` (since 2.8)
+'''
 
 Use argument ``id`` instead.
 
-``eject`` argument ``device`` (since 2.8.0)
-'''
+``eject`` argument ``device`` (since 2.8)
+'
 
 Use argument ``id`` instead.
 
-``blockdev-change-medium`` argument ``device`` (since 2.8.0)
-
+``blockdev-change-medium`` argument ``device`` (since 2.8)
+''
 
 Use argument ``id`` instead.
 
-``block_set_io_throttle`` argument ``device`` (since 2.8.0)
-'''
+``block_set_io_throttle`` argument ``device`` (since 2.8)
+'
 
 Use argument ``id`` instead.
 
-``blockdev-add`` empty string argument ``backing`` (since 2.10.0)
-'
+``blockdev-add`` empty string argument ``backing`` (since 2.10)
+'''
 
 Use argument value ``null`` instead.
 
-``block-commit`` arguments ``base`` and ``top`` (since 3.1.0)
-'
+``block-commit`` arguments ``base`` and ``top`` (since 3.1)
+'''
 
 Use arguments ``base-node`` and ``top-node`` instead.
 
@@ -191,8 +191,8 @@ from Linux upstream kernel, declare it deprecated.
 System emulator CPUS
 
 
-``Icelake-Client`` CPU Model (since 5.2.0)
-''
+``Icelake-Client`` CPU Model (since 5.2)
+
 
 ``Icelake-Client`` CPU Models are deprecated. Use ``Icelake-Server`` CPU
 Models instead.
@@ -245,8 +245,8 @@ Device options
 Emulated device options
 '''
 
-``-device virtio-blk,scsi=on|off`` (since 5.0.0)
-
+``-device virtio-blk,scsi=on|off`` (since 5.0)
+^^
 
 The virtio-blk SCSI passthrough feature is a legacy VIRTIO feature.  VIRTIO 1.0
 and later do not support it because the virtio-scsi device was introduced for
@@ -258,14 +258,14 @@ alias.
 Block device options
 
 
-``"backing": ""`` (since 2.12.0)
-
+``"backing": ""`` (since 2.12)
+^^
 
 In order to prevent QEMU from automatically opening an image's backing
 chain, use ``"backing": null`` instead.
 
-``rbd`` keyvalue pair encoded filenames: ``""`` (since 3.1.0)
-^

[PATCH,updated 2/2] docs/about: Unify the subject format

[Yanan Wang]

Unify the subject format in deprecated.rst to "since X.Y".
Unify the subject format in removed-features.rst to "removed in X.Y".

Signed-off-by: Yanan Wang 
---
 docs/about/deprecated.rst   | 56 -
 docs/about/removed-features.rst | 28 -
 2 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 6d438f1c8d..8d4fd384a5 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -107,8 +107,8 @@ the process listing. This is replaced by the new 
``password-secret``
 option which lets the password be securely provided on the command
 line using a ``secret`` object instance.
 
-``opened`` property of ``rng-*`` objects (since 6.0.0)
-''
+``opened`` property of ``rng-*`` objects (since 6.0)
+
 
 The only effect of specifying ``opened=on`` in the command line or QMP
 ``object-add`` is that the device is opened immediately, possibly before all
@@ -116,8 +116,8 @@ other options have been processed.  This will either have 
no effect (if
 ``opened`` was the last option) or cause errors.  The property is therefore
 useless and should not be specified.
 
-``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0.0)
-''
+``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0)
+
 
 The only effect of specifying ``loaded=on`` in the command line or QMP
 ``object-add`` is that the secret is loaded immediately, possibly before all
@@ -142,33 +142,33 @@ should be used instead.
 QEMU Machine Protocol (QMP) commands
 
 
-``blockdev-open-tray``, ``blockdev-close-tray`` argument ``device`` (since 
2.8.0)
-'
+``blockdev-open-tray``, ``blockdev-close-tray`` argument ``device`` (since 2.8)
+'''
 
 Use argument ``id`` instead.
 
-``eject`` argument ``device`` (since 2.8.0)
-'''
+``eject`` argument ``device`` (since 2.8)
+'
 
 Use argument ``id`` instead.
 
-``blockdev-change-medium`` argument ``device`` (since 2.8.0)
-
+``blockdev-change-medium`` argument ``device`` (since 2.8)
+''
 
 Use argument ``id`` instead.
 
-``block_set_io_throttle`` argument ``device`` (since 2.8.0)
-'''
+``block_set_io_throttle`` argument ``device`` (since 2.8)
+'
 
 Use argument ``id`` instead.
 
-``blockdev-add`` empty string argument ``backing`` (since 2.10.0)
-'
+``blockdev-add`` empty string argument ``backing`` (since 2.10)
+'''
 
 Use argument value ``null`` instead.
 
-``block-commit`` arguments ``base`` and ``top`` (since 3.1.0)
-'
+``block-commit`` arguments ``base`` and ``top`` (since 3.1)
+'''
 
 Use arguments ``base-node`` and ``top-node`` instead.
 
@@ -191,8 +191,8 @@ from Linux upstream kernel, declare it deprecated.
 System emulator CPUS
 
 
-``Icelake-Client`` CPU Model (since 5.2.0)
-''
+``Icelake-Client`` CPU Model (since 5.2)
+
 
 ``Icelake-Client`` CPU Models are deprecated. Use ``Icelake-Server`` CPU
 Models instead.
@@ -245,8 +245,8 @@ Device options
 Emulated device options
 '''
 
-``-device virtio-blk,scsi=on|off`` (since 5.0.0)
-
+``-device virtio-blk,scsi=on|off`` (since 5.0)
+^^
 
 The virtio-blk SCSI passthrough feature is a legacy VIRTIO feature.  VIRTIO 1.0
 and later do not support it because the virtio-scsi device was introduced for
@@ -258,14 +258,14 @@ alias.
 Block device options
 
 
-``"backing": ""`` (since 2.12.0)
-
+``"backing": ""`` (since 2.12)
+^^
 
 In order to prevent QEMU from automatically opening an image's backing
 chain, use ``"backing": null`` instead.
 
-``rbd`` keyvalue pair encoded filenames: ``""`` (since 3.1.0)
-^

[no subject]

[Kevin Townsend]

Updates the proposed LSM303DLHC magnetometer device following review by
Philippe Mathieu-Daudé.

This has been tested with Zephyr 2.6.0, as follows:

$ west build -p auto -b mps2_an521 \
  zephyr/samples/sensor/sensor_shell/ \
  -- -DCONFIG_I2C_SHELL=y

$ qemu-system-arm -M mps2-an521 -device loader,file=build/zephyr/zephyr.elf \
  -serial stdio \
  -monitor tcp:localhost:,server,nowait \
  -device lsm303dlhc_mag,id=lsm303,address=0x1E

uart:~$ i2c scan I2C_SHIELD1 
 0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00: -- -- -- -- -- -- -- -- -- -- -- -- 
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- 1e -- 
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
70: -- -- -- -- -- -- -- -- 
1 devices found on I2C_SHIELD1

Zephyr's LSM303DLHC driver can be enabled in a sample by adding the following
DTS overlay:

/*
 * Copyright (c) 2021 Linaro Limited
 *
 * SPDX-License-Identifier: Apache-2.0
 */

_shield1 {
lsm303dlhc-magn@1e {
compatible = "st,lsm303dlhc-magn";
reg = <0x1e>;
label = "LSM303DLHC-MAGN";
};
};

And the following KConfig settings:

CONFIG_I2C=y
CONFIG_I2C_SHELL=y
CONFIG_SENSOR=y
CONFIG_LSM303DLHC_MAGN=y

When a sample with the above settings is run, the magnetometer can be read
via the shell sensor command:

uart:~$ sensor get LSM303DLHC-MAGN magn_xyz
channel idx=11 magn_xyz x =   0.00 y =   0.00 z =   0.00

Set the y-axis (via human monitor or qmp) to 1100, which equals 1 Gauss
with the default gain settings:

(qemu) qom-set lsm303 mag_y 1100
qom-set lsm303 mag_y 1100

And test again in Zephyr:

uart:~$ sensor get LSM303DLHC-MAGN magn_xyz
channel idx=11 magn_xyz x =   0.00 y =   1.00 z =   0.00

[no subject]

[Daniel Page]

Is it possible to increase and decrease ram, cpu and also limit
bandwidth speed without shut down the guest?

Re: [RFC PATCH] Subject: [RFC PATCH] plugins: Passed the parsed arguments directly to plugins

[Alex Bennée]

Mahmoud Mandour  writes:

The subject got a bit mangled. I usually send single one off patches
directly from the command line like this:

  git send-email --subject-prefix "RFC PATCH" HEAD^.. --to qemu HEAD^..

> Arguments were passed to plugins in the following form:
> -plugin path/to/plugin,arg="positional_arg=value",arg="second_arg"
>
> This patch removes the need for "arg" so that the argument name itself
> is now expected and passed directly to the plugin.
>
> Now options can be passed in the following manner:
> -plugin path/to/plugin,positional_arg=value,second_arg
>
> Since short boolean arguments are deprecated, passing an argument that
> takes no value will trigger a warning saying that the user should use a
> full "arg_name=on" instead of just "arg_name". In either case, the
> argument is passed to the plugin as only the name, omitting the "=on"
> part.
>
> Signed-off-by: Mahmoud Mandour 
> ---
>  plugins/loader.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/plugins/loader.c b/plugins/loader.c
> index 05df40398d..7f32b8c8bd 100644
> --- a/plugins/loader.c
> +++ b/plugins/loader.c
> @@ -94,6 +94,7 @@ static int plugin_add(void *opaque, const char *name, const 
> char *value,
>  {
>  struct qemu_plugin_parse_arg *arg = opaque;
>  struct qemu_plugin_desc *p;
> +char *full_arg;
>  
>  if (strcmp(name, "file") == 0) {
>  if (strcmp(value, "") == 0) {
> @@ -107,7 +108,7 @@ static int plugin_add(void *opaque, const char *name, 
> const char *value,
>  QTAILQ_INSERT_TAIL(arg->head, p, entry);
>  }
>  arg->curr = p;
> -} else if (strcmp(name, "arg") == 0) {
> +} else {

Unfortunately I think we also want to support the arg= form for now and
also update the deprecated.rst and plugin.rst documentation to point to
the new format.

>  if (arg->curr == NULL) {
>  error_setg(errp, "missing earlier '-plugin file=' option");
>  return 1;
> @@ -115,9 +116,12 @@ static int plugin_add(void *opaque, const char *name, 
> const char *value,
>  p = arg->curr;
>  p->argc++;
>  p->argv = g_realloc_n(p->argv, p->argc, sizeof(char *));
> -p->argv[p->argc - 1] = g_strdup(value);
> -} else {
> -error_setg(errp, "-plugin: unexpected parameter '%s'; ignored", 
> name);
> +if (strcmp(value, "on")) {
> +full_arg = g_strdup_printf("%s=%s", name, value);

We should probably prefer g_strcmp0 to strcmp and an explicit value test
(!= 0) because strcmp is weird with truth/falsity. However as we are
handling the syntactic sugar of adding "on" in the command line I wonder
if we can query QemuOpt directly. Maybe something like:

/* pass true bools as single arg */
if (qapi_bool_parse(name, value, _true, errp) && is_true) {
full_arg = g_strdup(name);
} else {
full_arg = g_strdup_printf("%s=%s", name, value);
}


> +} else {
> +full_arg = g_strdup(name);
> +}
> +p->argv[p->argc - 1] = full_arg;
>  }
>  return 0;
>  }


-- 
Alex Bennée

[RFC PATCH] Subject: [RFC PATCH] plugins: Passed the parsed arguments directly to plugins

[Mahmoud Mandour]

Arguments were passed to plugins in the following form:
-plugin path/to/plugin,arg="positional_arg=value",arg="second_arg"

This patch removes the need for "arg" so that the argument name itself
is now expected and passed directly to the plugin.

Now options can be passed in the following manner:
-plugin path/to/plugin,positional_arg=value,second_arg

Since short boolean arguments are deprecated, passing an argument that
takes no value will trigger a warning saying that the user should use a
full "arg_name=on" instead of just "arg_name". In either case, the
argument is passed to the plugin as only the name, omitting the "=on"
part.

Signed-off-by: Mahmoud Mandour 
---
 plugins/loader.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/plugins/loader.c b/plugins/loader.c
index 05df40398d..7f32b8c8bd 100644
--- a/plugins/loader.c
+++ b/plugins/loader.c
@@ -94,6 +94,7 @@ static int plugin_add(void *opaque, const char *name, const 
char *value,
 {
 struct qemu_plugin_parse_arg *arg = opaque;
 struct qemu_plugin_desc *p;
+char *full_arg;
 
 if (strcmp(name, "file") == 0) {
 if (strcmp(value, "") == 0) {
@@ -107,7 +108,7 @@ static int plugin_add(void *opaque, const char *name, const 
char *value,
 QTAILQ_INSERT_TAIL(arg->head, p, entry);
 }
 arg->curr = p;
-} else if (strcmp(name, "arg") == 0) {
+} else {
 if (arg->curr == NULL) {
 error_setg(errp, "missing earlier '-plugin file=' option");
 return 1;
@@ -115,9 +116,12 @@ static int plugin_add(void *opaque, const char *name, 
const char *value,
 p = arg->curr;
 p->argc++;
 p->argv = g_realloc_n(p->argv, p->argc, sizeof(char *));
-p->argv[p->argc - 1] = g_strdup(value);
-} else {
-error_setg(errp, "-plugin: unexpected parameter '%s'; ignored", name);
+if (strcmp(value, "on")) {
+full_arg = g_strdup_printf("%s=%s", name, value);
+} else {
+full_arg = g_strdup(name);
+}
+p->argv[p->argc - 1] = full_arg;
 }
 return 0;
 }
-- 
2.25.1

Subject: [PATCH] hw/vmxnet3: fix vmxnet3 g_assert_not_reached bug

[Liu Cyrus]

From: cyruscyliu 

A g_assert_not_reached of vmxnet3 can be triggered by a guest with the root
privilege.
Remove the VMXNET3_REG_ICR branch thus get rid of this crash.

Fixes: 786fd2b0f87b ("VMXNET3 device implementation")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/309
Buglink: https://bugs.launchpad.net/qemu/+bug/1913923
Signed-off-by: cyruscyliu 
---
 hw/net/vmxnet3.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index eff299f..a388918 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1786,13 +1786,6 @@ vmxnet3_io_bar1_write(void *opaque,
 vmxnet3_set_variable_mac(s, val, s->temp_mac);
 break;

-/* Interrupt Cause Register */
-case VMXNET3_REG_ICR:
-VMW_CBPRN("Write BAR1 [VMXNET3_REG_ICR] = %" PRIx64 ", size %d",
-  val, size);
-g_assert_not_reached();
-break;
-
 /* Event Cause Register */
 case VMXNET3_REG_ECR:
 VMW_CBPRN("Write BAR1 [VMXNET3_REG_ECR] = %" PRIx64 ", size %d",
--
2.7.4

Hi folks, this is a suggestion for fixing this bug.
I'm willing to discuss this with you because I'm new to contribute to QEMU.

Best,
Qiang

(No Subject)

[Nathan Ringo]

Updates for QEMU 6.0.0.

[no subject]

[Dev Audsin]

virtio-fs with DAX is currently not compatible with NIC Pass through.
When a SR-IOV VF attaches to a qemu process, vfio will try to pin the entire 
DAX Window but it is empty when the guest boots and will fail.
A method to make VFIO and DAX to work together is to make vfio skip DAX cache.
Currently DAX cache need to be set to 0, for the SR-IOV VF to be attached to 
Kata containers.
Enabling both SR-IOV VF and DAX work together will potentially improve 
performance for workloads which are I/O and network intensive

[no subject]

[Yonggang Luo]

qapi docs tests building failed on win32

looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] doc-good

build succeeded.

The text files are in tests/qapi-schema.
"C:/CI-Tools/msys64/mingw64/bin/python3.exe"
"C:/work/xemu/qemu/meson/meson.py" "--internal" "exe" "--capture"
"tests/qapi-schema/doc-good.txt.nocr" "--" "perl" "-pe" "$x = chr 13;
s/$x$//" "tests/qapi-schema/doc-good.txt" && if test -e
tests/qapi-schema/doc-good.txt.nocr; then printf '%s\n'
tests/qapi-schema/doc-good.txt.nocr >
tests/qapi-schema/doc-good.txt.nocr.stamp; fi
syntax error at -e line 1, near "="
Execution of -e aborted due to compilation errors.
make: *** [Makefile.ninja:2555：tests/qapi-schema/doc-good.txt.nocr.stamp]
错误 255
"C:/CI-Tools/msys64/mingw64/bin/python3.exe"
"C:/work/xemu/qemu/meson/meson.py" "--internal" "exe" "--capture"
"qemu-version.h" "--" "C:/CI-Tools/msys64/mingw64/bin/python3.exe"
"C:/work/xemu/qemu/scripts/qemu-version.py" "C:/work/xemu/qemu" "" "5.1.50"
&& if test -e qemu-version.h; then printf '%s\n' qemu-version.h >
qemu-version.h.stamp; fi
"C:/CI-Tools/msys64/mingw64/bin/python3.exe"
"C:/work/xemu/qemu/meson/meson.py" "--internal" "exe" "--capture"
"tests/qapi-schema/doc-good.txt.nocr" "--" "perl" "-pe" "$x = chr 13;
s/$x$//" "tests/qapi-schema/doc-good.txt" && if test -e
tests/qapi-schema/doc-good.txt.nocr; then printf '%s\n'
tests/qapi-schema/doc-good.txt.nocr >
tests/qapi-schema/doc-good.txt.nocr.stamp; fi
syntax error at -e line 1, near "="
Execution of -e aborted due to compilation errors.
make: *** [Makefile.ninja:2555：tests/qapi-schema/doc-good.txt.nocr.stamp]
错误 255
--
 此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo

[no subject]

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> error_vprepend() is only used by util/error.c where it is
> defined. Make it static to reduce its scope.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/qapi/error.h | 6 --
>  util/error.c | 6 +-
>  2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 7932594dce..fa2308dedd 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -384,12 +384,6 @@ void error_propagate(Error **dst_errp, Error *local_err);
>  void error_propagate_prepend(Error **dst_errp, Error *local_err,
>   const char *fmt, ...);
>  
> -/*
> - * Prepend some text to @errp's human-readable error message.
> - * The text is made by formatting @fmt, @ap like vprintf().
> - */
> -void error_vprepend(Error *const *errp, const char *fmt, va_list ap);
> -
>  /*
>   * Prepend some text to @errp's human-readable error message.
>   * The text is made by formatting @fmt, ... like printf().
> diff --git a/util/error.c b/util/error.c
> index b6c89d1412..3990b741ff 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -121,7 +121,11 @@ void error_setg_file_open_internal(Error **errp,
>"Could not open '%s'", filename);
>  }
>  
> -void error_vprepend(Error *const *errp, const char *fmt, va_list ap)
> +/*
> + * Prepend some text to @errp's human-readable error message.
> + * The text is made by formatting @fmt, @ap like vprintf().
> + */
> +static void error_vprepend(Error *const *errp, const char *fmt, va_list ap)
>  {
>  GString *newmsg;

I prefer to keep error_vprepend() in error.h even though it's only used
in error.c at the moment.

In an external library interface, every ... function needs a va_list
buddy.

This is an internal interface, where providing only a ... function is
just fine.  I happily do that when I have no use fo the va_list
function.  But when the va_list function exists, hiding it buys us
nothing.

[no subject]

[Antonio Raffaele]

Hi I'm trying to create a qemu virtual machine that runs windows 10. I
would like to try to make it almost indistinguishable from a real computer
(I know it's impossible, but at least I get close). I have already changed
any suspicious identifiers (smbios, hard disk, card network and so on,
host-passthroug cpu etc.) But now checking the various components that the
guest computer recognizes, I realized (through the hwinfo64 program) that
in the bus section, then pcibus there are devices called "Red Hat , Device
ID "and with the same devicename, as device class have:" PCI-to-PCI Bridge
". Is there a way to change the devicename of these virtual compontents
(maybe even changing the qemu source)?

[PATCH 0/6] *** SUBJECT HERE ***

[Geoffrey McRae]

This patch set addresses several issues that cause inconsistent
behaviour in the guest when the sound device is stopped and started or
the JACK server stops responding on the host.

Geoffrey McRae (6):
  audio/jack: fix invalid minimum buffer size check
  audio/jack: remove unused stopped state
  audio/jack: remove invalid set of input support bool
  audio/jack: do not remove ports when finishing
  audio/jack: honour the enable state of the audio device
  audio/jack: simplify the re-init code path

 audio/jackaudio.c | 73 ---
 1 file changed, 38 insertions(+), 35 deletions(-)

-- 
2.20.1

[PATCH 0/4] Subject: [PATCH 0/4] smbus: SPD fixes

[Markus Armbruster]

PATCH 1 fixes a regression, but it's a rather old one: regressed in
v4.0.0.  I doubt it needs to go into 5.0 at this stage.  But it's up
to the maintainer(s).

Markus Armbruster (4):
  sam460ex: Revert change to SPD memory type for <= 128 MiB
  smbus: Fix spd_data_generate() error API violation
  bamboo, sam460ex: Tidy up error message for unsupported RAM size
  smbus: Fix spd_data_generate() for number of banks > 2

 include/hw/i2c/smbus_eeprom.h |  2 +-
 hw/i2c/smbus_eeprom.c | 32 +---
 hw/mips/mips_fulong2e.c   | 10 ++
 hw/ppc/ppc4xx_devs.c  |  4 ++--
 hw/ppc/sam460ex.c | 13 -
 5 files changed, 14 insertions(+), 47 deletions(-)

-- 
2.21.1

[no subject]

[Adema Yergara]

I understand that I subscribed to your mailing lit now but what about the
point
- To create an account in the QEMU wiki, you must ask on the mailing list
for someone else to do it on your behalf (self-creation is prohibited to
cut down on spam accounts)
Who can create this for me?

[no subject]

[Wayne Li]

Dear QEMU list members,

This will kind of be a repost but I'd like to post my question again
because I've gained some more knowledge that makes me feel that my question
would be easier to answer.  So we developed a custom-made QEMU VM that
emulates a custom machine that has an e5500 processor.  I'm running this VM
on a T4240-RDB board which has an e6500 processor and I'm trying to get the
VM running with KVM enabled.  The problem I'm having is the program counter
refuses to increment at all.  It just stays at the address 0xFFFC.  On
a run without KVM enabled, the VM will also start executing at this same
address but the program counter beings to increment immediately.  I know
this is a custom QEMU VM and maybe some of the startup stuff we do could be
causing problems, but what could possibly stop the program counter from
incrementing altogether?

Also, I do have another side question.  When running with KVM enabled, I
see the kernel-level ioctl call KVM_RUN running and then returning over and
over again (by the way before the VM kinda grinds to a halt I only see QEMU
make the KVM_RUN call twice, but the kernel-level ioctl function is being
called over and over again for some reason).  And each time the KVM_RUN
call returns, the return-from-interrupt takes the VM to the address
0xFFFC.  What is the KVM_RUN ioctl call used for?  Why is it being
called over and over again?  Maybe if I understood this better I'd be able
to figure out what's stopping my program counter from incrementing.

-Thanks, Wayne Li

[no subject]

[Sergio Lopez]

Hi,

Commit 137b5cb6ab565cb3781d5337591e155932b4230e (hmp: change
hmp_info_cpus to use query-cpus-fast) updated the "info cpus" commit to
make it more lightweight, but also removed the ability to get the
architecture specific status of each vCPU.

This information was really useful to diagnose certain Guest issues,
without the need of using GDB, which is more intrusive and requires
enabling it in advance.

Is there an alternative way of getting something equivalent to what
"info cpus" provided previously (in 2.10)?

Thanks,
Sergio.


signature.asc
Description: PGP signature

Re: [Qemu-devel] Subject: Re: [PATCH] hw/block/nvme

[Stefan Hajnoczi]

On Tue, Sep 10, 2019 at 11:23:50PM +0300, Toe Dev wrote:
> Hey,
> While reviewing I noticed maybe we need to update the spec revision.
> In: nvme_class_init(...)
> 
> current code  pc->revision=2
> change to: pc->revision=3
> However not really important I think.. Just for consistency.
> When I done reviewing, should it be patched too, How?

The NVMe specification declares the PCI Revision ID field implementation
specific.  It doesn't seem to be a reflection of the NVMe specification
supported by the device.

If there is a PCI Vendor 0x8086 Device 0x5845 in the real world with
revision = 3 that we now emulate correctly, then it could be updated.

However, for live migration compatibility QEMU must keep old
guest-visible behavior too.  It's not as simple as changing the revision
value to 3, because then existing VMs migrating from an old QEMU to a
new QEMU would suddenly see the hardware change beneath them.

QEMU has the "machine types" mechanism to deal with this.  QEMU 4.2 and
later machine types (e.g. "pc-q35-4.2") would use revision = 3 while
older machine types would use revision = 2.  This maintains live
migration compatibility.

In summary, there is probably no strong reason to change this (although
I'm not an NVMe expert so maybe I've missed something).


signature.asc
Description: PGP signature

[Qemu-devel] Subject: Re: [PATCH] hw/block/nvme

[Toe Dev]

Hey,
While reviewing I noticed maybe we need to update the spec revision.
In: nvme_class_init(...)

current code  pc->revision=2
change to: pc->revision=3
However not really important I think.. Just for consistency.
When I done reviewing, should it be patched too, How?

[Qemu-devel] ANNOUNCE: emails from this mailing list will soon drop the [qemu-*] subject tag

[Peter Maydell]

Hi; this is an announcement to let you know that in future
emails to all QEMU project mailing lists (including this one)
will no longer have the [qemu-*] tag in their Subject line.

We need to make this config change because having the mailing
list server edit subject lines like this conflicts with the
increasingly common DKIM/DMARC anti-email-forgery system. We
used to work around this by having the list server rewrite
email From addresses instead, but this has proven to have
bad consequences (notably that patches applied from emails
to QEMU can end up with incorrect authorship by accident).

If you were using the Subject line tag to filter QEMU emails,
you'll need to change your mail client's config to instead
look at the "List-Id:" message header to identify list traffic
(you can do this now without waiting for us to change the
list config to drop the subject tags).

Apologies for any inconvenience that this upcoming config
change might cause you.

thanks
-- PMM

[Qemu-devel] (no subject)

[Yang Weijiang]

Subject: [Qemu-devel][PATCH v3 0/5] This patch-set is to enable Guest
CET support.

Control-flow Enforcement Technology (CET) provides protection against
return/jump-oriented programming (ROP) attacks. To make kvm Guest OS own
the capability, this patch-set is required. It enables CET related CPUID
report, xsaves/xrstors and live-migration etc. in Qemu.

Changelog:

 v3:
 - Add CET MSR save/restore support for live-migration.

 v2:
 - In CPUID.(EAX=d, ECX=1), set return ECX[n] = 0 if bit n corresponds
   to a bit in MSR_IA32_XSS.
 - In CPUID.(EAX=d, ECX=n), set return ECX = 1 if bit n corresponds
   to a bit in MSR_IA32_XSS.
 - Skip Supervisor mode xsave component when calculate User mode
   xave component size in xsave_area_size() and x86_cpu_reset().

Yang Weijiang (5):
  Add CET xsaves/xrstors related macros and structures.
  Add CET SHSTK and IBT CPUID feature-word definitions.
  Add hepler functions for CPUID xsave area size calculation.
  Report CPUID xsave area support for CET.
  Add CET MSR save/restore support for migration

 target/i386/cpu.c |  73 --
 target/i386/cpu.h |  48 +++-
 target/i386/kvm.c |  27 
 target/i386/machine.c | 100 ++
 4 files changed, 244 insertions(+), 4 deletions(-)

-- 
2.17.1

[Qemu-devel] (no subject)

[Yaowei Bai]

baiyao...@cmss.chinamobile.com
Bcc: 
Subject: Re: [Qemu-devel] [PATCH] tcmu: Introduce qemu-tcmu utility
Reply-To: baiyao...@cmss.chinamobile.com
In-Reply-To: <20190102015321.GA26514@byw>

Add Xiubo.

On Wed, Jan 02, 2019 at 09:53:21AM +0800, Yaowei Bai wrote:
> Ping.
> 
> BTW, it should be update docker image to install glib to fix this.
> 
> On Wed, Dec 26, 2018 at 12:19:48AM -0800, no-re...@patchew.org wrote:
> > Patchew URL: 
> > https://patchew.org/QEMU/1545387387-9613-1-git-send-email-baiyao...@cmss.chinamobile.com/
> > 
> > 
> > 
> > Hi,
> > 
> > This series seems to have some coding style problems. See output below for
> > more information:
> > 
> > Message-id: 1545387387-9613-1-git-send-email-baiyao...@cmss.chinamobile.com
> > Type: series
> > Subject: [Qemu-devel] [PATCH] tcmu: Introduce qemu-tcmu utility
> > 
> > === TEST SCRIPT BEGIN ===
> > #!/bin/bash
> > 
> > BASE=base
> > n=1
> > total=$(git log --oneline $BASE.. | wc -l)
> > failed=0
> > 
> > git config --local diff.renamelimit 0
> > git config --local diff.renames True
> > git config --local diff.algorithm histogram
> > 
> > commits="$(git log --format=%H --reverse $BASE..)"
> > for c in $commits; do
> > echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
> > if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; 
> > then
> > failed=1
> > echo
> > fi
> > n=$((n+1))
> > done
> > 
> > exit $failed
> > === TEST SCRIPT END ===
> > 
> > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> > Switched to a new branch 'test'
> > 52869e1 tcmu: Introduce qemu-tcmu utility
> > 
> > === OUTPUT BEGIN ===
> > Checking PATCH 1/1: tcmu: Introduce qemu-tcmu utility...
> > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> > #157: 
> > new file mode 100644
> > 
> > ERROR: trailing whitespace
> > #329: FILE: qemu-tcmu.c:51:
> > +"Usage:\n" $
> > 
> > WARNING: Block comments use a leading /* on a separate line
> > #466: FILE: qemu-tcmu.c:188:
> > +/* now when the initialization is (almost) complete, chdir("/")
> > 
> > WARNING: Block comments use a trailing */ on a separate line
> > #467: FILE: qemu-tcmu.c:189:
> > + * to free any busy filesystems */
> > 
> > ERROR: code indent should never use tabs
> > #525: FILE: tcmu/helper.c:16:
> > +^Iuint8_t *cdb,$
> > 
> > ERROR: code indent should never use tabs
> > #526: FILE: tcmu/helper.c:17:
> > +^Istruct iovec *iovec,$
> > 
> > ERROR: code indent should never use tabs
> > #527: FILE: tcmu/helper.c:18:
> > +^Isize_t iov_cnt)$
> > 
> > ERROR: code indent should never use tabs
> > #529: FILE: tcmu/helper.c:20:
> > +^Iuint8_t buf[36];$
> > 
> > ERROR: code indent should never use tabs
> > #531: FILE: tcmu/helper.c:22:
> > +^Imemset(buf, 0, sizeof(buf));$
> > 
> > ERROR: code indent should never use tabs
> > #533: FILE: tcmu/helper.c:24:
> > +^Ibuf[2] = 0x05; /* SPC-3 */$
> > 
> > ERROR: code indent should never use tabs
> > #534: FILE: tcmu/helper.c:25:
> > +^Ibuf[3] = 0x02; /* response data format */$
> > 
> > ERROR: code indent should never use tabs
> > #536: FILE: tcmu/helper.c:27:
> > +^I/*$
> > 
> > ERROR: code indent should never use tabs
> > #537: FILE: tcmu/helper.c:28:
> > +^I * A Third-Party Copy (3PC)$
> > 
> > ERROR: code indent should never use tabs
> > #538: FILE: tcmu/helper.c:29:
> > +^I *$
> > 
> > ERROR: code indent should never use tabs
> > #539: FILE: tcmu/helper.c:30:
> > +^I * Enable the XCOPY$
> > 
> > ERROR: code indent should never use tabs
> > #540: FILE: tcmu/helper.c:31:
> > +^I */$
> > 
> > ERROR: code indent should never use tabs
> > #541: FILE: tcmu/helper.c:32:
> > +^Ibuf[5] = 0x08;$
> > 
> > ERROR: code indent should never use tabs
> > #543: FILE: tcmu/helper.c:34:
> > +^Ibuf[7] = 0x02; /* CmdQue */$
> > 
> > ERROR: code indent should never use tabs
> > #545: FILE: tcmu/helper.c:36:
> > +^Imemcpy([8], "LIO-ORG ", 8);$
> > 
> > ERROR: code indent should never use tabs
> > #546: FILE: tcmu/helper.c:37:
> > +^Imemset([16], 0x20, 16);$
> > 
> > ERROR: code indent should never use tabs
> > #547: FILE: tcmu/helper.c:38:
> > +^Imemcpy([16], "TCMU device"

[Qemu-devel] (no subject)

[Илья Резников]

Please add android support

Re: [Qemu-devel] (no subject)

[Peter Maydell]

On Thu, 29 Nov 2018 at 02:11, berkus infinitus  wrote:
>
> I suspect the main problem is the blocking call to qemu_main
> from the UI thread in the app delegate didFinishLoadingWithOptions
> if i’m not mistaken and everything else grows from there.

Yes; if there's no way that Mojave will allow us to run
qemu_main directly on the main thread, then we have to
create a 2nd thread to run qemu_main on (which then becomes
what QEMU thinks of as the "main loop thread"), and then
we run into the need to make all the UI calls be forwarded
from the main loop thread to the main thread, and to get
QEMU locks when making calls into qemu from the main thread.

I think the code we have in git currently will already do
all the UI calls on the main thread -- it just does it by
doing a blocking call into qemu_main which later does
event processing itself. (It's a shame OSX doesn't document
what you need to do to write code that way, it's a fairly
common paradigm for other GUIs.)

For High Sierra there is apparently a "main thread checker"
utility: 
https://developer.apple.com/documentation/code_diagnostics/main_thread_checker
so running
DYLD_INSERT_LIBRARIES=/Applications/Xcode.app/Contents/Developer/usr/lib/libMainThreadChecker.dylib
qemu-system-x86_64 args...

will let us check for violations of the "do things on main
thread" principle even without Mohave. For me with current
QEMU it doesn't report any issues.

thanks
-- PMM

Re: [Qemu-devel] (no subject)

[berkus infinitus]

I suspect the main problem is the blocking call to qemu_main from the UI
thread in the app delegate didFinishLoadingWithOptions if i’m not mistaken
and everything else grows from there. Going to build and run it now, since
I woke up in the middle of the night anyway for reasons unexplainable)
On Thu, 29 Nov 2018 at 02:21, Programmingkid 
wrote:

>
> > On Nov 28, 2018, at 2:39 PM, Peter Maydell 
> wrote:
> >
> > On Wed, 28 Nov 2018 at 01:12, John Arbuckle 
> wrote:
> >>
> >> From af4497f2b161bb4165acb8eee5cae3f2a7ea2227 Mon Sep 17 00:00:00 2001
> >> From: John Arbuckle 
> >> Date: Tue, 27 Nov 2018 20:01:20 -0500
> >> Subject: [PATCH] ui/cocoa.m: fix crash due to cocoa_refresh() on Mac OS
> 10.14
> >
> > Something seems to have got the formatting of this patch email
> > wrong -- it's got all this in the body and the actual Subject
> > line of the email is blank.
>
> I don't know what happened.
>
> >
> >> Mac OS 10.14 only wants UI code to be called from the main thread. The
> >> cocoa_refresh() function is called on another thread and this causes a
> >> crash to take place. To fix this problem the cocoa_refresh() code is
> >> called from the main thread only.
> >>
> >> Signed-off-by: John Arbuckle 
> >> ---
> >> ui/cocoa.m | 59
> ++-
> >> 1 file changed, 34 insertions(+), 25 deletions(-)
> >
> > I get a compile warning with this patch:
> > /Users/pm215/src/qemu/ui/cocoa.m:1615:23: warning: instance method
> > '-cocoa_refresh' not found (return type defaults to 'id')
> > [-Wobjc-method-access]
> >[[NSApp delegate] cocoa_refresh];
> >  ^
>
> This will fix the problem:
>
> static void cocoa_refresh(DisplayChangeListener *dcl)
> {
> QemuCocoaAppController *controller = (QemuCocoaAppController *)[NSApp
> delegate];
> [controller cocoa_refresh];
> }
>
> > To be honest, I'm still confused about what is causing the
> > problems on Mojave. The refresh method should be being called
> > on the main thread even with the code on master -- it's just
> > that that is the iothread and it's running the event pumping
> > code in the refresh callback rather than a standard OSX
> > event loop. I'm hoping that the backtrace with symbols of
> > the Mojave assertion failure will help there, since I
> > can't currently see where refresh gets called from some
> > non-main thread.
>
> This might be a Mojave issue and not a QEMU issue.
> I'm on Mac OS 10.12 so I can't confirm anything.
>
> > I also think that this patch doesn't address the problems
> > of locking that I mention on the discussion thread for
> > Berkus' patch. It also doesn't handle any of the other
> > callbacks from QEMU to the cocoa UI -- surely we need to
> > handle all of them if there is a problem here?
>
> To answer this question I would have to know how my patch
> does on Mac OS 10.14. Does it stop the crashing issue? If
> this patch does fix that problem then I think sticking to
> a simple solution may be the answer. The use of locks may
> not be needed.
>
> > (I have some prototype patches which I've been working
> > on which address the locking problem and also make all
> > the QEMU callbacks run their work on the main thread.
> > I may be able get those into shape to post those next week.)
>
> Please CC me when do release it. I will test it on Mac OS 10.12
> and Mac OS 10.6.

Re: [Qemu-devel] (no subject)

[Programmingkid]

> On Nov 28, 2018, at 2:39 PM, Peter Maydell  wrote:
> 
> On Wed, 28 Nov 2018 at 01:12, John Arbuckle  wrote:
>> 
>> From af4497f2b161bb4165acb8eee5cae3f2a7ea2227 Mon Sep 17 00:00:00 2001
>> From: John Arbuckle 
>> Date: Tue, 27 Nov 2018 20:01:20 -0500
>> Subject: [PATCH] ui/cocoa.m: fix crash due to cocoa_refresh() on Mac OS 10.14
> 
> Something seems to have got the formatting of this patch email
> wrong -- it's got all this in the body and the actual Subject
> line of the email is blank.

I don't know what happened.

> 
>> Mac OS 10.14 only wants UI code to be called from the main thread. The
>> cocoa_refresh() function is called on another thread and this causes a
>> crash to take place. To fix this problem the cocoa_refresh() code is
>> called from the main thread only.
>> 
>> Signed-off-by: John Arbuckle 
>> ---
>> ui/cocoa.m | 59 ++-
>> 1 file changed, 34 insertions(+), 25 deletions(-)
> 
> I get a compile warning with this patch:
> /Users/pm215/src/qemu/ui/cocoa.m:1615:23: warning: instance method
> '-cocoa_refresh' not found (return type defaults to 'id')
> [-Wobjc-method-access]
>[[NSApp delegate] cocoa_refresh];
>  ^

This will fix the problem:

static void cocoa_refresh(DisplayChangeListener *dcl)
{
QemuCocoaAppController *controller = (QemuCocoaAppController *)[NSApp 
delegate];
[controller cocoa_refresh];
}

> To be honest, I'm still confused about what is causing the
> problems on Mojave. The refresh method should be being called
> on the main thread even with the code on master -- it's just
> that that is the iothread and it's running the event pumping
> code in the refresh callback rather than a standard OSX
> event loop. I'm hoping that the backtrace with symbols of
> the Mojave assertion failure will help there, since I
> can't currently see where refresh gets called from some
> non-main thread.

This might be a Mojave issue and not a QEMU issue. 
I'm on Mac OS 10.12 so I can't confirm anything.

> I also think that this patch doesn't address the problems
> of locking that I mention on the discussion thread for
> Berkus' patch. It also doesn't handle any of the other
> callbacks from QEMU to the cocoa UI -- surely we need to
> handle all of them if there is a problem here?

To answer this question I would have to know how my patch
does on Mac OS 10.14. Does it stop the crashing issue? If
this patch does fix that problem then I think sticking to
a simple solution may be the answer. The use of locks may
not be needed.

> (I have some prototype patches which I've been working
> on which address the locking problem and also make all
> the QEMU callbacks run their work on the main thread.
> I may be able get those into shape to post those next week.)

Please CC me when do release it. I will test it on Mac OS 10.12
and Mac OS 10.6. 

Re: [Qemu-devel] (no subject)

[Peter Maydell]

On Wed, 28 Nov 2018 at 01:12, John Arbuckle  wrote:
>
> From af4497f2b161bb4165acb8eee5cae3f2a7ea2227 Mon Sep 17 00:00:00 2001
> From: John Arbuckle 
> Date: Tue, 27 Nov 2018 20:01:20 -0500
> Subject: [PATCH] ui/cocoa.m: fix crash due to cocoa_refresh() on Mac OS 10.14

Something seems to have got the formatting of this patch email
wrong -- it's got all this in the body and the actual Subject
line of the email is blank.

> Mac OS 10.14 only wants UI code to be called from the main thread. The
> cocoa_refresh() function is called on another thread and this causes a
> crash to take place. To fix this problem the cocoa_refresh() code is
> called from the main thread only.
>
> Signed-off-by: John Arbuckle 
> ---
>  ui/cocoa.m | 59 ++-
>  1 file changed, 34 insertions(+), 25 deletions(-)

I get a compile warning with this patch:
/Users/pm215/src/qemu/ui/cocoa.m:1615:23: warning: instance method
'-cocoa_refresh' not found (return type defaults to 'id')
[-Wobjc-method-access]
[[NSApp delegate] cocoa_refresh];
  ^

To be honest, I'm still confused about what is causing the
problems on Mojave. The refresh method should be being called
on the main thread even with the code on master -- it's just
that that is the iothread and it's running the event pumping
code in the refresh callback rather than a standard OSX
event loop. I'm hoping that the backtrace with symbols of
the Mojave assertion failure will help there, since I
can't currently see where refresh gets called from some
non-main thread.

I also think that this patch doesn't address the problems
of locking that I mention on the discussion thread for
Berkus' patch. It also doesn't handle any of the other
callbacks from QEMU to the cocoa UI -- surely we need to
handle all of them if there is a problem here?

(I have some prototype patches which I've been working
on which address the locking problem and also make all
the QEMU callbacks run their work on the main thread.
I may be able get those into shape to post those next week.)

thanks
-- PMM

[Qemu-devel] (no subject)

[John Arbuckle]

>From af4497f2b161bb4165acb8eee5cae3f2a7ea2227 Mon Sep 17 00:00:00 2001
From: John Arbuckle 
Date: Tue, 27 Nov 2018 20:01:20 -0500
Subject: [PATCH] ui/cocoa.m: fix crash due to cocoa_refresh() on Mac OS 10.14

Mac OS 10.14 only wants UI code to be called from the main thread. The
cocoa_refresh() function is called on another thread and this causes a
crash to take place. To fix this problem the cocoa_refresh() code is
called from the main thread only. 

Signed-off-by: John Arbuckle 
---
 ui/cocoa.m | 59 ++-
 1 file changed, 34 insertions(+), 25 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index ecf12bfc2e..17c168d08f 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -972,6 +972,8 @@ - (void)openDocumentation:(NSString *)filename;
 - (IBAction) do_about_menu_item: (id) sender;
 - (void)make_about_window;
 - (void)adjustSpeed:(id)sender;
+- (void) cocoa_refresh;
+- (void) cocoa_refresh_internal: (id) dummy;
 @end
 
 @implementation QemuCocoaAppController
@@ -1406,6 +1408,37 @@ - (void)adjustSpeed:(id)sender
 COCOA_DEBUG("cpu throttling at %d%c\n", cpu_throttle_get_percentage(), 
'%');
 }
 
+- (void) cocoa_refresh
+{
+[self performSelectorOnMainThread: @selector(cocoa_refresh_internal:) 
withObject: nil waitUntilDone: YES];
+}
+
+- (void) cocoa_refresh_internal: (id) dummy
+{
+COCOA_DEBUG("qemu_cocoa: cocoa_refresh\n");
+graphic_hw_update(NULL);
+
+if (qemu_input_is_absolute()) {
+if (![cocoaView isAbsoluteEnabled]) {
+if ([cocoaView isMouseGrabbed]) {
+[cocoaView ungrabMouse];
+}
+}
+[cocoaView setAbsoluteEnabled:YES];
+}
+
+NSDate *distantPast;
+NSEvent *event;
+distantPast = [NSDate distantPast];
+do {
+event = [NSApp nextEventMatchingMask:NSEventMaskAny 
untilDate:distantPast
+  inMode: NSDefaultRunLoopMode 
dequeue:YES];
+if (event != nil) {
+[cocoaView handleEvent:event];
+}
+} while(event != nil);
+}
+
 @end
 
 
@@ -1579,31 +1612,7 @@ static void cocoa_switch(DisplayChangeListener *dcl,
 
 static void cocoa_refresh(DisplayChangeListener *dcl)
 {
-NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
-
-COCOA_DEBUG("qemu_cocoa: cocoa_refresh\n");
-graphic_hw_update(NULL);
-
-if (qemu_input_is_absolute()) {
-if (![cocoaView isAbsoluteEnabled]) {
-if ([cocoaView isMouseGrabbed]) {
-[cocoaView ungrabMouse];
-}
-}
-[cocoaView setAbsoluteEnabled:YES];
-}
-
-NSDate *distantPast;
-NSEvent *event;
-distantPast = [NSDate distantPast];
-do {
-event = [NSApp nextEventMatchingMask:NSEventMaskAny 
untilDate:distantPast
-inMode: NSDefaultRunLoopMode dequeue:YES];
-if (event != nil) {
-[cocoaView handleEvent:event];
-}
-} while(event != nil);
-[pool release];
+[[NSApp delegate] cocoa_refresh];
 }
 
 static void cocoa_cleanup(void)
-- 
2.14.3 (Apple Git-98)

Re: [Qemu-devel] Subject: [RFC PATCH v2] migration: calculate remaining pages accurately during the bulk stage

[Eric Blake]

On 09/05/2018 09:17 AM, Quan Xu wrote:

 From 7de4cc7c944bfccde0ef10992a7ec882fdcf0508 Mon Sep 17 00:00:00 2001
From: Quan Xu 
Date: Wed, 5 Sep 2018 22:06:58 +0800
Subject: [RFC PATCH v2] migration: calculate remaining pages accurately 
during the bulk stage


Since the bulk stage assumes in (migration_bitmap_find_dirty) that every
page is dirty, initialize the number of dirty pages at the beggining of


s/beggining/beginning/


the iteration and then decrease it for each processed page.

Signed-off-by: Quan Xu 
---
  migration/ram.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-devel] Subject: [RFC PATCH v2] migration: calculate remaining pages accurately during the bulk stage

[Quan Xu]

From 7de4cc7c944bfccde0ef10992a7ec882fdcf0508 Mon Sep 17 00:00:00 2001
From: Quan Xu 
Date: Wed, 5 Sep 2018 22:06:58 +0800
Subject: [RFC PATCH v2] migration: calculate remaining pages accurately 
during the bulk stage


Since the bulk stage assumes in (migration_bitmap_find_dirty) that every
page is dirty, initialize the number of dirty pages at the beggining of
the iteration and then decrease it for each processed page.

Signed-off-by: Quan Xu 
---
 migration/ram.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 79c8942..1a11436 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -290,6 +290,8 @@ struct RAMState {
 uint32_t last_version;
 /* We are in the first round */
 bool ram_bulk_stage;
+/* Remaining bytes in the first round */
+uint64_t ram_bulk_bytes;
 /* How many times we have dirty too many pages */
 int dirty_rate_high_cnt;
 /* these variables are used for bitmap sync */
@@ -1540,6 +1542,7 @@ unsigned long migration_bitmap_find_dirty(RAMState 
*rs, RAMBlock *rb,


 if (rs->ram_bulk_stage && start > 0) {
 next = start + 1;
+rs->ram_bulk_bytes -= TARGET_PAGE_SIZE;
 } else {
 next = find_next_bit(bitmap, size, start);
 }
@@ -2001,6 +2004,7 @@ static bool find_dirty_block(RAMState *rs, 
PageSearchStatus *pss, bool *again)

 /* Flag that we've looped */
 pss->complete_round = true;
 rs->ram_bulk_stage = false;
+rs->ram_bulk_bytes = 0;
 if (migrate_use_xbzrle()) {
 /* If xbzrle is on, stop using the data compression at 
this
  * point. In theory, xbzrle can do better than 
compression.

@@ -2513,6 +2517,7 @@ static void ram_state_reset(RAMState *rs)
 rs->last_page = 0;
 rs->last_version = ram_list.version;
 rs->ram_bulk_stage = true;
+rs->ram_bulk_bytes = ram_bytes_total();
 }

 #define MAX_WAIT 50 /* ms, half buffered_file limit */
@@ -3308,7 +3313,7 @@ static void ram_save_pending(QEMUFile *f, void 
*opaque, uint64_t max_size,

 /* We can do postcopy, and all the data is postcopiable */
 *res_compatible += remaining_size;
 } else {
-*res_precopy_only += remaining_size;
+*res_precopy_only += remaining_size + rs->ram_bulk_bytes;
 }
 }

--
1.8.3.1

[Qemu-devel] (no subject)

[Liujinsong (Paul)]

[Qemu-devel] (no subject)

[Markus Armbruster]

I fooled around a bit, and I think there are a few lose ends.

Lets update the examples in docs/interop/qmp-spec.txt to show the
current greeting (section 3.1) and how to accept a capability (section
3.2).  The capability negotiation documentation could use some polish.
I'll post a patch.

Talking to a QMP monitor that supports OOB:

$ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, 
"package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
{"error": {"class": "GenericError", "desc": "Parameter 'oob' is 
unexpected"}}
QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
{"return": {}}
QMP> { "execute": "query-qmp-schema" }
{"error": {"class": "GenericError", "desc": "Out-Of-Band capability 
requires that every command contains an 'id' field"}}

Why does every command require 'id'?

Talking to a QMP monitor that doesn't support OOB:

{"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, 
"package": "v2.12.0-1703-gb909799463"}, "capabilities": []}}
QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
{"error": {"class": "GenericError", "desc": "This monitor does not support 
Out-Of-Band (OOB)"}}
QMP> { "execute": "qmp_capabilities" }
{"return": {}}
QMP> { "execute": "query-kvm" }
{"return": {"enabled": true, "present": true}}
QMP> { "execute": "query-kvm", "control": { "run-oob": true } }
{"error": {"class": "GenericError", "desc": "Please enable Out-Of-Band 
first for the session during capabilities negotiation"}}

Telling people to enable OOB when that cannot be done is suboptimal.
More so when it cannot be used here anyway.  I'll post a patch.

[Qemu-devel] (no subject)

[Anatol Pomozov]

It is V3 of multiboot improvements to Qemu

Changes made sinse V2:
 - rebase on top of qemu master changes
 - make multiboot/sections test more reliable
 Add generate_sections_out.py script that generates ELF sections information
 - rename 'struct section_data' to 'struct SectionData' to match naming
 convention in include/hw/loader.h

[Qemu-devel] (no subject)

[Eduardo Otubo]

zhangchen.f...@cn.fujitsu.com, wang.guan...@zte.com.cn,
wang.yong...@zte.com.cn 
Bcc: 
Subject: colo-compare: segfault and assert on colo_compare_finalize
Reply-To: 

Hi all,

I have found a problem on colo-compare that leads to segmentation fault
when calling qemu like this:

 $ qemu-system-x86_64 -S -machine pc -object colo-compare,id=test-object

First I got an assert failed:

 (qemu-system-x86_64:7887): GLib-CRITICAL **: g_main_loop_quit: assertion 'loop 
!= NULL' failed

>From this looks like s->compare_loop is NULL on the function
colo_compare_finalize(), then I just added a check there and the assert went
away. But then there's the segfault:

 Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
 0x7333f79e in pthread_join () from /lib64/libpthread.so.0
 (gdb) bt
 #0  0x7333f79e in pthread_join () at /lib64/libpthread.so.0
 #1  0x55c379d2 in qemu_thread_join (thread=0x77ff5160) at 
util/qemu-thread-posix.c:547
 #2  0x55adfc1a in colo_compare_finalize (obj=0x77fd3010) at 
net/colo-compare.c:867
 #3  0x55b2cd87 in object_deinit (obj=0x77fd3010, 
type=0x567432e0) at qom/object.c:453
 #4  0x55b2cdf9 in object_finalize (data=0x77fd3010) at 
qom/object.c:467
 #5  0x55b2dd80 in object_unref (obj=0x77fd3010) at qom/object.c:902
 #6  0x55b319a5 in user_creatable_add_type (type=0x567499a0 
"colo-compare", id=0x56749960 "test-object", qdict=0x56835750, 
v=0x5681a3f0, errp=0x7fffde58) at qom/object_interfaces.c:105
 #7  0x55b31b02 in user_creatable_add_opts (opts=0x56749910, 
errp=0x7fffde58) at qom/object_interfaces.c:135
 #8  0x55b31bfd in user_creatable_add_opts_foreach 
(opaque=0x558e9c39 , opts=0x56749910, errp=0x0) 
at qom/object_interfaces.c:159
 #9  0x55c4aecf in qemu_opts_foreach (list=0x56157ac0 
, func=0x55b31b6f , 
opaque=0x558e9c39 , errp=0x0) at 
util/qemu-option.c:1104
 #10 0x558edb75 in main (argc=6, argv=0x7fffe2d8, 
envp=0x7fffe310) at vl.c:4520

At this point '>thread' is '0'. Is this segfault and the above mentioned
assert trigged because I'm creating a colo-compare object without any other
parameter? In a positive case, a simple workaround and error check should do
it. Otherwise I'll debug a little more.

Best regards,

-- 
Eduardo Otubo
Senior Software Engineer @ RedHat

[Qemu-devel] (no subject)

[vaibhav shukla]

Hello,

I am Vaibhav Shukla, sophomore student of Indian Institute of Information 
Technology, Kalyani, India.

I would like to contribute in some projects in your company, please guide me 
that how can I do so.

I shall be highly grateful to you.

Yours Sincerely

Re: [Qemu-devel] (no subject)

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] (no subject)
Type: series
Message-id: 536fb79a-5753-4143-a5a6-7a189ef5137e@ONE.local

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
32d5d78 (no subject)

=== OUTPUT BEGIN ===
Checking PATCH 1/1: (no subject)...
ERROR: space prohibited before that '++' (ctx:WxB)
#34: FILE: hw/gpio/bcm2835_gpio.c:58:
+for (i = 0; i < 10; i ++) {
   ^

ERROR: space prohibited between function name and open parenthesis '('
#37: FILE: hw/gpio/bcm2835_gpio.c:60:
+if (index < sizeof (s->fsel)) {

ERROR: space prohibited before that '++' (ctx:WxB)
#46: FILE: hw/gpio/bcm2835_gpio.c:70:
+for (i = 0; i < 10; i ++) {
   ^

ERROR: space prohibited between function name and open parenthesis '('
#49: FILE: hw/gpio/bcm2835_gpio.c:72:
+if (index < sizeof (s->fsel)) {

ERROR: space prohibited after that '~' (ctx:WxW)
#100: FILE: hw/gpio/bcm2835_gpio.c:115:
+uint32_t changes = val & ~ *lev;
  ^

ERROR: space prohibited before that '++' (ctx:WxB)
#105: FILE: hw/gpio/bcm2835_gpio.c:119:
+for (i = 0; i < count; i ++) {
  ^

ERROR: space prohibited before that '++' (ctx:WxB)
#121: FILE: hw/gpio/bcm2835_gpio.c:136:
+for (i = 0; i < count; i ++) {
  ^

ERROR: space prohibited after that '~' (ctx:WxW)
#129: FILE: hw/gpio/bcm2835_gpio.c:143:
+*lev &= ~ val;
 ^

ERROR: switch and case should be at the same indent
#141: FILE: hw/gpio/bcm2835_gpio.c:152:
 switch (offset) {
+case GPFSEL0:
+case GPFSEL1:
+case GPFSEL2:
+case GPFSEL3:
+case GPFSEL4:
+case GPFSEL5:
[...]
+case GPSET0:
+case GPSET1:
[...]
+case GPCLR0:
+case GPCLR1:
[...]
+case GPLEV0:
[...]
+case GPLEV1:
[...]
+case GPEDS0:
+case GPEDS1:
+case GPREN0:
+case GPREN1:
+case GPFEN0:
+case GPFEN1:
+case GPHEN0:
+case GPHEN1:
+case GPLEN0:
+case GPLEN1:
+case GPAREN0:
+case GPAREN1:
+case GPAFEN0:
+case GPAFEN1:
+case GPPUD:
+case GPPUDCLK0:
+case GPPUDCLK1:
[...]
+default:

ERROR: space prohibited after that '-' (ctx:WxW)
#200: FILE: hw/gpio/bcm2835_gpio.c:169:
+if (s->panel.socket != - 1) {
^

ERROR: space prohibited after that '-' (ctx:WxW)
#208: FILE: hw/gpio/bcm2835_gpio.c:177:
+if (s->panel.socket != - 1) {
^

ERROR: switch and case should be at the same indent
#252: FILE: hw/gpio/bcm2835_gpio.c:219:
 switch (offset) {
+case GPFSEL0:
+case GPFSEL1:
+case GPFSEL2:
+case GPFSEL3:
+case GPFSEL4:
+case GPFSEL5:
[...]
+case GPSET0:
[...]
+case GPSET1:
[...]
+case GPCLR0:
[...]
+case GPCLR1:
[...]
+case GPLEV0:
+case GPLEV1:
[...]
+case GPEDS0:
+case GPEDS1:
+case GPREN0:
+case GPREN1:
+case GPFEN0:
+case GPFEN1:
+case GPHEN0:
+case GPHEN1:
+case GPLEN0:
+case GPLEN1:
+case GPAREN0:
+case GPAREN1:
+case GPAFEN0:
+case GPAFEN1:
+case GPPUD:
+case GPPUDCLK0:
+case GPPUDCLK1:
[...]
+default:

ERROR: space prohibited after that '-' (ctx:WxW)
#308: FILE: hw/gpio/bcm2835_gpio.c:230:
+if (s->panel.socket != - 1) {
^

WARNING: line over 80 characters
#310: FILE: hw/gpio/bcm2835_gpio.c:232:
+senddatatopanel(>panel, Data, true); //John Bradley dummy 
GPIO Panel

ERROR: do not use C99 // comments
#310: FILE: hw/gpio/bcm2835_gpio.c:232:
+senddatatopanel(>panel, Data, true); //John Bradley dummy 
GPIO Panel

ERROR: space prohibited after that '-' (ctx:WxW)
#315: FILE: hw/gpio/bcm2835_gpio.c:237:
+if (s->panel.socket != - 1) {
^

WARNING: line over 80 characters
#318: FILE: hw/gpio/bcm2835_gpio.c:240:
+senddatatopanel(>panel, Data, true); //John Bradley dummy 
GPIO Panel

Re: [Qemu-devel] (no subject)

[no-reply]

Hi,

This series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Subject: [Qemu-devel] (no subject)
Type: series
Message-id: 536fb79a-5753-4143-a5a6-7a189ef5137e@ONE.local

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-quick@centos6
time make docker-test-mingw@fedora
time make docker-test-build@min-glib
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/536fb79a-5753-4143-a5a6-7a189ef5137e@ONE.local -> 
patchew/536fb79a-5753-4143-a5a6-7a189ef5137e@ONE.local
Switched to a new branch 'test'
32d5d78 (no subject)

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-1zdsj2xe/src/dtc'...
Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d'
  BUILD   centos6
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-1zdsj2xe/src'
  ARCHIVE qemu.tgz
  ARCHIVE dtc.tgz
  COPYRUNNER
RUN test-quick in qemu:centos6 
Packages installed:
SDL-devel-1.2.14-7.el6_7.1.x86_64
ccache-3.1.6-2.el6.x86_64
epel-release-6-8.noarch
gcc-4.4.7-17.el6.x86_64
git-1.7.1-4.el6_7.1.x86_64
glib2-devel-2.28.8-5.el6.x86_64
libfdt-devel-1.4.0-1.el6.x86_64
make-3.81-23.el6.x86_64
package g++ is not installed
pixman-devel-0.32.8-1.el6.x86_64
tar-1.23-15.el6_8.x86_64
zlib-devel-1.2.3-29.el6.x86_64

Environment variables:
PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel 
glib2-devel SDL-devel pixman-devel epel-release
HOSTNAME=d9b60ec0a426
TERM=xterm
MAKEFLAGS= -j8
HISTSIZE=1000
J=8
USER=root
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1
MAIL=/var/spool/mail/root
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
LANG=en_US.UTF-8
TARGET_LIST=
HISTCONTROL=ignoredups
SHLVL=1
HOME=/root
TEST_DIR=/tmp/qemu-test
LOGNAME=root
LESSOPEN=||/usr/bin/lesspipe.sh %s
FEATURES= dtc
DEBUG=
G_BROKEN_FILENAMES=1
CCACHE_HASHDIR=
_=/usr/bin/env

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu 
--prefix=/var/tmp/qemu-build/install
No C++ compiler available; disabling C++ specific optional code
Install prefix/var/tmp/qemu-build/install
BIOS directory/var/tmp/qemu-build/install/share/qemu
binary directory  /var/tmp/qemu-build/install/bin
library directory /var/tmp/qemu-build/install/lib
module directory  /var/tmp/qemu-build/install/lib/qemu
libexec directory /var/tmp/qemu-build/install/libexec
include directory /var/tmp/qemu-build/install/include
config directory  /var/tmp/qemu-build/install/etc
local state directory   /var/tmp/qemu-build/install/var
Manual directory  /var/tmp/qemu-build/install/share/man
ELF interp prefix /usr/gnemul/qemu-%M
Source path   /tmp/qemu-test/src
C compilercc
Host C compiler   cc
C++ compiler  
Objective-C compiler cc
ARFLAGS   rv
CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g 
QEMU_CFLAGS   -I/usr/include/pixman-1   -I$(SRC_PATH)/dtc/libfdt -pthread 
-I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include   -fPIE -DPIE -m64 -mcx16 
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels 
-Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security 
-Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration 
-Wold-style-definition -Wtype-limits -fstack-protector-all
LDFLAGS   -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g 
make  make
install   install
pythonpython -B
smbd  /usr/sbin/smbd
module supportno
host CPU  x86_64
host big endian   no
target list   x86_64-softmmu aarch64-softmmu
tcg debug enabled no
gprof enabled no
sparse enabledno
strip binariesyes
profiler  no
static build  no
pixmansystem
SDL support   yes (1.2.14)
GTK support   no 
GTK GL supportno
VTE support   no 
TLS priority  NORMAL
GNUTLS supportno
GNUTLS rndno
libgcrypt no
libgcrypt kdf no
nettleno 
nettle kdfno
libtasn1  no
curses supportno
virgl support no
curl support  no
mingw32 support   no
Audio drivers oss
Block whitelist (rw) 
Block whitelist (ro) 
VirtFS supportno
VNC support   yes
VNC SASL support  no
VNC JPEG support  no
VNC PNG support   no
xen support   no
brlapi supportno
bluez  supportno
Documentation no
PIE   yes
vde support   no
netmap supportno
Linux AIO support no
ATTR/XATTR support yes
Install blobs yes
KVM support   

[Qemu-devel] (no subject)

[John Bradley]

>From 836daaff38940535548043f2e8f2e3df7a62d473 Mon Sep 17 00:00:00 2001
From: John Bradley <fly...@rocketmail.com>
Date: Wed, 17 May 2017 18:57:21 +0100
Subject: [PATCH] [PATCH] Add code to connect with
 https://github.com/flypie/GDummyPanel The code uses GNU Sockets & Windows
 sockets as on MINGW GNU no available. This is inteded as a Demo for RFC.

Signed-off-by: John Bradley <fly...@rocketmail.com>
---
 hw/gpio/bcm2835_gpio.c | 330 +++--
 include/hw/gpio/bcm2835_gpio.h |   5 +
 include/qemu/PanelEmu.h|  54 +++
 util/Makefile.objs |   1 +
 util/PanelEmu.c| 329 
 5 files changed, 577 insertions(+), 142 deletions(-)
 create mode 100644 include/qemu/PanelEmu.h
 create mode 100644 util/PanelEmu.c

diff --git a/hw/gpio/bcm2835_gpio.c b/hw/gpio/bcm2835_gpio.c
index acc2e3cf9e..14bd059861 100644
--- a/hw/gpio/bcm2835_gpio.c
+++ b/hw/gpio/bcm2835_gpio.c
@@ -19,6 +19,8 @@
 #include "hw/sd/sd.h"
 #include "hw/gpio/bcm2835_gpio.h"
 
+
+
 #define GPFSEL0   0x00
 #define GPFSEL1   0x04
 #define GPFSEL2   0x08
@@ -53,9 +55,9 @@ static uint32_t gpfsel_get(BCM2835GpioState *s, uint8_t reg)
 {
 int i;
 uint32_t value = 0;
-for (i = 0; i < 10; i++) {
+for (i = 0; i < 10; i ++) {
 uint32_t index = 10 * reg + i;
-if (index < sizeof(s->fsel)) {
+if (index < sizeof (s->fsel)) {
 value |= (s->fsel[index] & 0x7) << (3 * i);
 }
 }
@@ -65,9 +67,9 @@ static uint32_t gpfsel_get(BCM2835GpioState *s, uint8_t reg)
 static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
 {
 int i;
-for (i = 0; i < 10; i++) {
+for (i = 0; i < 10; i ++) {
 uint32_t index = 10 * reg + i;
-if (index < sizeof(s->fsel)) {
+if (index < sizeof (s->fsel)) {
 int fsel = (value >> (3 * i)) & 0x7;
 s->fsel[index] = fsel;
 }
@@ -75,24 +77,24 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, 
uint32_t value)
 
 /* SD controller selection (48-53) */
 if (s->sd_fsel != 0
-&& (s->fsel[48] == 0) /* SD_CLK_R */
-&& (s->fsel[49] == 0) /* SD_CMD_R */
-&& (s->fsel[50] == 0) /* SD_DATA0_R */
-&& (s->fsel[51] == 0) /* SD_DATA1_R */
-&& (s->fsel[52] == 0) /* SD_DATA2_R */
-&& (s->fsel[53] == 0) /* SD_DATA3_R */
-) {
+&& (s->fsel[48] == 0) /* SD_CLK_R */
+&& (s->fsel[49] == 0) /* SD_CMD_R */
+&& (s->fsel[50] == 0) /* SD_DATA0_R */
+&& (s->fsel[51] == 0) /* SD_DATA1_R */
+&& (s->fsel[52] == 0) /* SD_DATA2_R */
+&& (s->fsel[53] == 0) /* SD_DATA3_R */
+) {
 /* SDHCI controller selected */
 sdbus_reparent_card(s->sdbus_sdhost, s->sdbus_sdhci);
 s->sd_fsel = 0;
 } else if (s->sd_fsel != 4
-&& (s->fsel[48] == 4) /* SD_CLK_R */
-&& (s->fsel[49] == 4) /* SD_CMD_R */
-&& (s->fsel[50] == 4) /* SD_DATA0_R */
-&& (s->fsel[51] == 4) /* SD_DATA1_R */
-&& (s->fsel[52] == 4) /* SD_DATA2_R */
-&& (s->fsel[53] == 4) /* SD_DATA3_R */
-) {
+   && (s->fsel[48] == 4) /* SD_CLK_R */
+   && (s->fsel[49] == 4) /* SD_CMD_R */
+   && (s->fsel[50] == 4) /* SD_DATA0_R */
+   && (s->fsel[51] == 4) /* SD_DATA1_R */
+   && (s->fsel[52] == 4) /* SD_DATA2_R */
+   && (s->fsel[53] == 4) /* SD_DATA3_R */
+   ) {
 /* SDHost controller selected */
 sdbus_reparent_card(s->sdbus_sdhci, s->sdbus_sdhost);
 s->sd_fsel = 4;
@@ -108,13 +110,13 @@ static int gpfsel_is_out(BCM2835GpioState *s, int index)
 }
 
 static void gpset(BCM2835GpioState *s,
-uint32_t val, uint8_t start, uint8_t count, uint32_t *lev)
+  uint32_t val, uint8_t start, uint8_t count, uint32_t *lev)
 {
-uint32_t changes = val & ~*lev;
+uint32_t changes = val & ~ *lev;
 uint32_t cur = 1;
 
 int i;
-for (i = 0; i < count; i++) {
+for (i = 0; i < count; i ++) {
 if ((changes & cur) && (gpfsel_is_out(s, start + i))) {
 qemu_set_irq(s->out[start + i], 1);
 }
@@ -125,132 +127,165 @@ static void gpset(BCM2835GpioState *s,
 }
 
 static void gpclr(BCM2835GpioState *s,
-uint32_t val, uint8_t start, uint8_t count, uint32_t *lev)
+  uint32_t val, uint8_t start, uint8_t count, uint32_t *lev)
 {
 uint32_t chang

Re: [Qemu-devel] (no subject)

[gengdongjiu]

Dear James,
   Thanks a lot for your review and comments. I am very sorry for the
late response.


2017-05-04 23:42 GMT+08:00 gengdongjiu :
>  Hi Dongjiu Geng,
>
> On 30/04/17 06:37, Dongjiu Geng wrote:
>> when happen SEA, deliver signal bus and handle the ioctl that
>> inject SEA abort to guest, so that guest can handle the SEA error.
>
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 105b6ab..a96594f 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -20,8 +20,10 @@
>> @@ -1238,6 +1240,36 @@ static void coherent_cache_guest_page(struct kvm_vcpu 
>> *vcpu, kvm_pfn_t pfn,
>>   __coherent_cache_guest_page(vcpu, pfn, size);
>>  }
>>
>> +static void kvm_send_signal(unsigned long address, bool hugetlb, bool 
>> hwpoison)
>> +{
>> + siginfo_t info;
>> +
>> + info.si_signo   = SIGBUS;
>> + info.si_errno   = 0;
>> + if (hwpoison)
>> + info.si_code= BUS_MCEERR_AR;
>> + else
>> + info.si_code= 0;
>> +
>> + info.si_addr= (void __user *)address;
>> + if (hugetlb)
>> + info.si_addr_lsb = PMD_SHIFT;
>> + else
>> + info.si_addr_lsb = PAGE_SHIFT;
>> +
>> + send_sig_info(SIGBUS, , current);
>> +}
>> +
> «  [hide part of quote]
>
> Punit reviewed the other version of this patch, this PMD_SHIFT is not the 
> right
> thing to do, it needs a more accurate set of calls and shifts as there may be
> hugetlbfs pages other than PMD_SIZE.
>
> https://www.spinics.net/lists/arm-kernel/msg568919.html
>
> I haven't posted a new version of that patch because I was still hunting a bug
> in the hugepage/hwpoison code, even with Punit's fixes series I see -EFAULT
> returned to userspace instead of this hwpoison code being invoked.

  Ok, got it, thanks for your information.
>
> Please avoid duplicating functionality between patches, it wastes reviewers
> time, especially when we know there are problems with this approach.
>
>
>> +static void kvm_handle_bad_page(unsigned long address,
>> + bool hugetlb, bool hwpoison)
>> +{
>> + /* handle both hwpoison and other synchronous external Abort */
>> + if (hwpoison)
>> + kvm_send_signal(address, hugetlb, true);
>> + else
>> + kvm_send_signal(address, hugetlb, false);
>> +}
>
> Why the extra level of indirection? We only want to signal userspace like this
> from KVM for hwpoison. Signals for RAS related reasons should come from the 
> bits
> of the kernel that decoded the error.

For the SEA, the are maily two types:
0b01 Synchronous External Abort on memory access.
0b0101xx Synchronous External Abort on page table walk. DFSC[1:0]
encode the level.

hwpoison should belong to the  "Synchronous External Abort on memory access"
if the SEA type is not hwpoison, such as page table walk, do you mean
KVM do not deliver the SIGBUS?
If so, how the KVM handle the SEA type other than hwpoison?

>
> (hwpoison for KVM is a corner case as Qemu's memory effectively has two users,
> Qemu and KVM. This isn't the example of how user-space gets signalled.)
>
>
>> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
>> index b37446a..780e3c4 100644
>> --- a/arch/arm64/kvm/guest.c
>> +++ b/arch/arm64/kvm/guest.c
>> @@ -277,6 +277,13 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
>>   return -EINVAL;
>>  }
>>
>> +int kvm_vcpu_ioctl_sea(struct kvm_vcpu *vcpu)
>> +{
>> + kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
>> +
>> + return 0;
>> +}
>
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index bb02909..1d2e2e7 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1306,6 +1306,7 @@ struct kvm_s390_ucas_mapping {
>>  #define KVM_S390_GET_IRQ_STATE  _IOW(KVMIO, 0xb6, struct kvm_s390_irq_state)
>>  /* Available with KVM_CAP_X86_SMM */
>>  #define KVM_SMI   _IO(KVMIO,   0xb7)
>> +#define KVM_ARM_SEA   _IO(KVMIO,   0xb8)
>>
>>  #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)
>>  #define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1)
>>
>
> Why do we need a userspace API for SEA? It can also be done by using
> KVM_{G,S}ET_ONE_REG to change the vcpu registers. The advantage of doing it 
> this
> way is you can choose which ESR value to use.
>
> Adding a new API call to do something you could do with an old one doesn't 
> look
> right.

James, I considered your suggestion before that use the
KVM_{G,S}ET_ONE_REG to change the vcpu registers. but I found it does
not have difference to use the alread existed KVM API.  so may be
changing the vcpu registers in qemu will duplicate with the KVM APIs.

injection a SEA is no more than setting some registers: elr_el1, PC,
PSTATE, SPSR_el1, far_el1, esr_el1
I seen this KVM API do the same thing as Qemu.  do you found call this
API will have issue and necessary to choose another ESR value?

I pasted the alread existed KVM API code:

static void inject_abt64(struct kvm_vcpu *vcpu, bool is_iabt, unsigned
long addr)
{
 unsigned long cpsr = *vcpu_cpsr(vcpu);
 bool is_aarch32 = vcpu_mode_is_32bit(vcpu);
 u32 esr = 0;
 *vcpu_elr_el1(vcpu)