From: "Alex Bennée" <alex.bennee@linaro.org>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: qemu-devel@nongnu.org, Peter Maydell <peter.maydell@linaro.org>,
"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
Stefan Hajnoczi <stefanha@redhat.com>,
Mathieu Poirier <mathieu.poirier@linaro.org>,
virtio-fs@redhat.com, Viresh Kumar <viresh.kumar@linaro.org>
Subject: Re: [Virtio-fs] [PULL 07/55] hw/virtio: move vm_running check to virtio_device_started
Date: Mon, 07 Nov 2022 09:21:49 +0000 [thread overview]
Message-ID: <87y1snglt2.fsf@linaro.org> (raw)
In-Reply-To: <20221105093645-mutt-send-email-mst@kernel.org>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Mon, Oct 10, 2022 at 01:29:10PM -0400, Michael S. Tsirkin wrote:
>> From: Alex Bennée <alex.bennee@linaro.org>
>>
>> All the boilerplate virtio code does the same thing (or should at
>> least) of checking to see if the VM is running before attempting to
>> start VirtIO. Push the logic up to the common function to avoid
>> getting a copy and paste wrong.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20220802095010.3330793-11-alex.bennee@linaro.org>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> So, looking at the resulting code, I missed the fact that this function
> is also used in virtio core. So this patch does not do what it's saying
> it does (just refactor code).
> Instead it completely changes the meaning for virtio core.
> I thunk we should revert upstream, however, gpio has grown a
> dependency on this since then.
> Alex, could you take a look please?
So I guess we have three choices:
new function for use by backends
new function for use by core
parameterise virtio_device_started to ignore vm state
I'll add some usage doc comments whichever way.
Do you have a preference?
>
>> ---
>> include/hw/virtio/virtio.h | 5 +++++
>> hw/virtio/vhost-user-fs.c | 6 +-----
>> hw/virtio/vhost-user-i2c.c | 6 +-----
>> hw/virtio/vhost-user-rng.c | 6 +-----
>> hw/virtio/vhost-user-vsock.c | 6 +-----
>> hw/virtio/vhost-vsock.c | 6 +-----
>> 6 files changed, 10 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 9bb2485415..74e7ad5a92 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -100,6 +100,7 @@ struct VirtIODevice
>> VirtQueue *vq;
>> MemoryListener listener;
>> uint16_t device_id;
>> + /* @vm_running: current VM running state via virtio_vmstate_change() */
>> bool vm_running;
>> bool broken; /* device in invalid state, needs reset */
>> bool use_disabled_flag; /* allow use of 'disable' flag when needed */
>> @@ -376,6 +377,10 @@ static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
>> return vdev->started;
>> }
>>
>> + if (!vdev->vm_running) {
>> + return false;
>> + }
>> +
>> return status & VIRTIO_CONFIG_S_DRIVER_OK;
>> }
>>
>> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
>> index e513e4fdda..d2bebba785 100644
>> --- a/hw/virtio/vhost-user-fs.c
>> +++ b/hw/virtio/vhost-user-fs.c
>> @@ -122,11 +122,7 @@ static void vuf_stop(VirtIODevice *vdev)
>> static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>> {
>> VHostUserFS *fs = VHOST_USER_FS(vdev);
>> - bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> - if (!vdev->vm_running) {
>> - should_start = false;
>> - }
>> + bool should_start = virtio_device_started(vdev, status);
>>
>> if (fs->vhost_dev.started == should_start) {
>> return;
>> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
>> index 6020eee093..b930cf6d5e 100644
>> --- a/hw/virtio/vhost-user-i2c.c
>> +++ b/hw/virtio/vhost-user-i2c.c
>> @@ -93,11 +93,7 @@ static void vu_i2c_stop(VirtIODevice *vdev)
>> static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
>> {
>> VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
>> - bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> - if (!vdev->vm_running) {
>> - should_start = false;
>> - }
>> + bool should_start = virtio_device_started(vdev, status);
>>
>> if (i2c->vhost_dev.started == should_start) {
>> return;
>> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
>> index 3a7bf8e32d..a9c1c4bc79 100644
>> --- a/hw/virtio/vhost-user-rng.c
>> +++ b/hw/virtio/vhost-user-rng.c
>> @@ -90,11 +90,7 @@ static void vu_rng_stop(VirtIODevice *vdev)
>> static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
>> {
>> VHostUserRNG *rng = VHOST_USER_RNG(vdev);
>> - bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> - if (!vdev->vm_running) {
>> - should_start = false;
>> - }
>> + bool should_start = virtio_device_started(vdev, status);
>>
>> if (rng->vhost_dev.started == should_start) {
>> return;
>> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
>> index 0f8ff99f85..22c1616ebd 100644
>> --- a/hw/virtio/vhost-user-vsock.c
>> +++ b/hw/virtio/vhost-user-vsock.c
>> @@ -55,11 +55,7 @@ const VhostDevConfigOps vsock_ops = {
>> static void vuv_set_status(VirtIODevice *vdev, uint8_t status)
>> {
>> VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>> - bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> -
>> - if (!vdev->vm_running) {
>> - should_start = false;
>> - }
>> + bool should_start = virtio_device_started(vdev, status);
>>
>> if (vvc->vhost_dev.started == should_start) {
>> return;
>> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
>> index 0338de892f..8031c164a5 100644
>> --- a/hw/virtio/vhost-vsock.c
>> +++ b/hw/virtio/vhost-vsock.c
>> @@ -70,13 +70,9 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)
>> static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>> {
>> VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>> - bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
>> + bool should_start = virtio_device_started(vdev, status);
>> int ret;
>>
>> - if (!vdev->vm_running) {
>> - should_start = false;
>> - }
>> -
>> if (vvc->vhost_dev.started == should_start) {
>> return;
>> }
>> --
>> MST
>>
--
Alex Bennée
next prev parent reply other threads:[~2022-11-07 9:21 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20221010172813.204597-1-mst@redhat.com>
2022-10-10 17:29 ` [Virtio-fs] [PULL 07/55] hw/virtio: move vm_running check to virtio_device_started Michael S. Tsirkin
2022-10-14 7:30 ` [Virtio-fs] Regression save/restore of vsock: (was [PULL 07/55] hw/virtio: move vm_running check to virtio_device_started) Christian Borntraeger
2022-10-14 8:31 ` Christian Borntraeger
2022-10-14 11:07 ` Alex Bennée
2022-10-14 11:58 ` Christian Borntraeger
2022-10-14 8:37 ` Alex Bennée
2022-10-14 8:44 ` Christian Borntraeger
2022-11-05 16:45 ` [Virtio-fs] [PULL 07/55] hw/virtio: move vm_running check to virtio_device_started Michael S. Tsirkin
2022-11-07 9:21 ` Alex Bennée [this message]
2022-10-10 17:29 ` [Virtio-fs] [PULL 08/55] hw/virtio: move vhd->started check into helper and add FIXME Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y1snglt2.fsf@linaro.org \
--to=alex.bennee@linaro.org \
--cc=dgilbert@redhat.com \
--cc=mathieu.poirier@linaro.org \
--cc=mst@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
--cc=viresh.kumar@linaro.org \
--cc=virtio-fs@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).