| Message ID | 20170411131733.27542-3-lvivier@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 11, 2017 at 03:17:33PM +0200, Laurent Vivier wrote: > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c > index 9639f4e..d270d56 100644 > --- a/hw/virtio/virtio-rng.c > +++ b/hw/virtio/virtio-rng.c > @@ -53,6 +53,15 @@ static void chr_read(void *opaque, const void *buf, size_t size) > return; > } > > + /* we can't modify the virtqueue until > + * our state is fully synced > + */ > + > + if (!runstate_check(RUN_STATE_RUNNING)) { > + trace_virtio_rng_cpu_is_stopped(vrng); > + return; > + } > + I'm concerned about what happens when the guest is stopped and resumed (e.g. 'stop' and 'cont' monitor commands). Since we throw away the chr_read() callback the device will hang unless the guest kicks it again? It's not clear to me that the rate limit timer will help us... Stefan
On 11/04/2017 19:02, Stefan Hajnoczi wrote: > On Tue, Apr 11, 2017 at 03:17:33PM +0200, Laurent Vivier wrote: >> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c >> index 9639f4e..d270d56 100644 >> --- a/hw/virtio/virtio-rng.c >> +++ b/hw/virtio/virtio-rng.c >> @@ -53,6 +53,15 @@ static void chr_read(void *opaque, const void *buf, size_t size) >> return; >> } >> >> + /* we can't modify the virtqueue until >> + * our state is fully synced >> + */ >> + >> + if (!runstate_check(RUN_STATE_RUNNING)) { >> + trace_virtio_rng_cpu_is_stopped(vrng); >> + return; >> + } >> + > > I'm concerned about what happens when the guest is stopped and resumed > (e.g. 'stop' and 'cont' monitor commands). Since we throw away the > chr_read() callback the device will hang unless the guest kicks it > again? > > It's not clear to me that the rate limit timer will help us... I think you're right (even if it seems hard to generate this case) What is the best solution: - re-arming the timer/the backend request by calling virtio_rng_process() before the "return;" or - adding a vmstate change handler to call virtio_rng_process()? Thanks, Laurent
On Tue, Apr 11, 2017 at 07:42:02PM +0200, Laurent Vivier wrote: > On 11/04/2017 19:02, Stefan Hajnoczi wrote: > > On Tue, Apr 11, 2017 at 03:17:33PM +0200, Laurent Vivier wrote: > >> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c > >> index 9639f4e..d270d56 100644 > >> --- a/hw/virtio/virtio-rng.c > >> +++ b/hw/virtio/virtio-rng.c > >> @@ -53,6 +53,15 @@ static void chr_read(void *opaque, const void *buf, size_t size) > >> return; > >> } > >> > >> + /* we can't modify the virtqueue until > >> + * our state is fully synced > >> + */ > >> + > >> + if (!runstate_check(RUN_STATE_RUNNING)) { > >> + trace_virtio_rng_cpu_is_stopped(vrng); > >> + return; > >> + } > >> + > > > > I'm concerned about what happens when the guest is stopped and resumed > > (e.g. 'stop' and 'cont' monitor commands). Since we throw away the > > chr_read() callback the device will hang unless the guest kicks it > > again? > > > > It's not clear to me that the rate limit timer will help us... > > I think you're right (even if it seems hard to generate this case) > > What is the best solution: > > - re-arming the timer/the backend request by calling > virtio_rng_process() before the "return;" This would waste CPU and throw away entropy bits. > or > > - adding a vmstate change handler to call virtio_rng_process()? Yes, I think vm change handlers solve the problem.
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events index 6926eed..564a4b8 100644 --- a/hw/virtio/trace-events +++ b/hw/virtio/trace-events @@ -11,6 +11,8 @@ virtio_set_status(void *vdev, uint8_t val) "vdev %p val %u" # hw/virtio/virtio-rng.c virtio_rng_guest_not_ready(void *rng) "rng %p: guest not ready" +virtio_rng_cpu_is_stopped(void *rng) "rng %p: cpu is stopped" +virtio_rng_popped(void *rng) "rng %p: elem popped" virtio_rng_pushed(void *rng, size_t len) "rng %p: %zd bytes pushed" virtio_rng_request(void *rng, size_t size, unsigned quota) "rng %p: %zd bytes requested, %u bytes quota left" diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c index 9639f4e..d270d56 100644 --- a/hw/virtio/virtio-rng.c +++ b/hw/virtio/virtio-rng.c @@ -53,6 +53,15 @@ static void chr_read(void *opaque, const void *buf, size_t size) return; } + /* we can't modify the virtqueue until + * our state is fully synced + */ + + if (!runstate_check(RUN_STATE_RUNNING)) { + trace_virtio_rng_cpu_is_stopped(vrng); + return; + } + vrng->quota_remaining -= size; offset = 0; @@ -61,6 +70,7 @@ static void chr_read(void *opaque, const void *buf, size_t size) if (!elem) { break; } + trace_virtio_rng_popped(vrng); len = iov_from_buf(elem->in_sg, elem->in_num, 0, buf + offset, size - offset); offset += len;
If we modify the virtio-rng virqueue while the vmstate is already migrated we can have some inconsistencies between the virtqueue state and the memory content. To avoid this, stop the virtqueue while the CPU is stopped. Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- hw/virtio/trace-events | 2 ++ hw/virtio/virtio-rng.c | 10 ++++++++++ 2 files changed, 12 insertions(+)