Re: [PATCH 00/13] Subject: [PATCH 00/15] qapi: Improve command response documentation
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]
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
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]
Just to test the email address, no reply required.
Re: [Virtio-fs] (no subject)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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]
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]
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]
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
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
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
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
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]
I would like to unsubscribe to the list pls
Subject: [PATCH] hw/intc/arm_gicv3: Fix GICD_TYPER ITLinesNumber advertisement
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
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]
[no subject]
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]
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]
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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]
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]
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
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
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
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)
Updates for QEMU 6.0.0.
[no subject]
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]
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]
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]
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 ***
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
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]
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]
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]
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
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
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
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)
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)
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)
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)
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)
> 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)
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)
>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
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
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)
[Qemu-devel] (no subject)
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)
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)
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)
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)
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)
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)
>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)
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)