virtio-rng: fixes
v2 cover virtio-rng: fixes
Hello,
This series fixes a few things, especially the migration support, making it endian-safe.
New in this submission: delete the timer on device remove, else hot-unplug of an rng device could result in a bad access.
The backend should be decoupled on hot-unplug of the virtio-rng device as well, but that's for later.
v2:
* fix typo in patch 4
* travel back to present, fix cover letter subject line
* add Anthony's reviewed-by tags to patches.
Please apply.
Amit Shah (4):
virtio-rng: use virtqueue_get_avail_bytes, fix migration
virtio-rng: remove extra request for entropy
virtio-rng: disable timer on device removal
virtio-rng: fix typos, comments
hw/virtio-rng.c | 92 ++++++++++-------------------------------------------- include/qemu/rng.h | 6 ++-- 2 files changed, 20 insertions(+), 78 deletions(-)
Signature · 1 line
1.8.0
PATCH v2 1/4 virtio-rng: use virtqueue_get_avail_bytes, fix migration
Popping an elem from the vq just to find out its length causes problems with save/load later on. Use the new virtqueue_get_avail_bytes() function instead, saves us the complexity in the migration code, as well as makes the migration endian-safe.
--- hw/virtio-rng.c | 76 +++++++++------------------------------------------------ 1 file changed, 12 insertions(+), 64 deletions(-)
hw/virtio-rng.c +12 -64
@@ -22,14 +22,10 @@ typedef struct VirtIORNG {/* Only one vq - guest puts buffer(s) on it when it needs entropy */ VirtQueue *vq; -VirtQueueElement elem;/* Config data for the device -- currently only chardev */ VirtIORNGConf *conf; -/* Whether we've popped a vq element into 'elem' above */ -bool popped; -RngBackend *rng;/* We purposefully don't migrate this state. The quota will reset on the
@@ -48,17 +44,12 @@ static bool is_guest_ready(VirtIORNG *vrng)return false; } -static size_t pop_an_elem (VirtIORNG *vrng) +static size_t get_request_size (VirtQueue *vq) { -size_t size; +unsigned int in, out; -if (!vrng->popped && !virtqueue_pop (vrng->vq, &vrng->elem)) { -return 0 ; - } - vrng->popped = true; - - size =iov_size (vrng->elem.in_sg, vrng->elem.in_num); -return size; +virtqueue_get_avail_bytes (vq, &in, &out); +return in; }static void virtio_rng_process (VirtIORNG *vrng);
@@ -67,6 +58,7 @@ static void virtio_rng_process(VirtIORNG *vrng);static void chr_read (void *opaque,const void *buf,size_t size) {VirtIORNG *vrng = opaque; +VirtQueueElement elem;size_t len;int offset;
@@ -78,15 +70,14 @@ static void chr_read(void *opaque, const void *buf, size_t size) offset =0 ;while (offset < size) { -if (!pop_an_elem (vrng)) { +if (!virtqueue_pop (vrng->vq, &elem)) {break ; } - len =iov_from_buf (vrng->elem.in_sg, vrng->elem.in_num, + len =iov_from_buf (elem.in_sg, elem.in_num,0 , buf + offset, size - offset); offset += len; -virtqueue_push (vrng->vq, &vrng->elem, len); - vrng->popped = false; +virtqueue_push (vrng->vq, &elem, len); }virtio_notify (&vrng->vdev, vrng->vq);
@@ -100,21 +91,19 @@ static void chr_read(void *opaque, const void *buf, size_t size)static void virtio_rng_process (VirtIORNG *vrng) { -ssize_t size; +size_t size;if (!is_guest_ready (vrng)) {return ; } - size =pop_an_elem (vrng); + size =get_request_size (vrng->vq); size =MIN (vrng->quota_remaining, size); - -if (size >0 ) { +if (size) {rng_backend_request_entropy (vrng->rng, size, chr_read, vrng); } } -static void handle_input (VirtIODevice *vdev,VirtQueue *vq) {VirtIORNG *vrng =DO_UPCAST (VirtIORNG, vdev, vdev);
@@ -131,23 +120,6 @@ static void virtio_rng_save(QEMUFile *f, void *opaque)VirtIORNG *vrng = opaque;virtio_save (&vrng->vdev, f); - -qemu_put_byte (f, vrng->popped); -if (vrng->popped) { -int i; - -qemu_put_be32 (f, vrng->elem.index); - -qemu_put_be32 (f, vrng->elem.in_num); -for (i =0 ; i < vrng->elem.in_num; i++) { -qemu_put_be64 (f, vrng->elem.in_addr[i]); - } - -qemu_put_be32 (f, vrng->elem.out_num); -for (i =0 ; i < vrng->elem.out_num; i++) { -qemu_put_be64 (f, vrng->elem.out_addr[i]); - } - } }static int virtio_rng_load (QEMUFile *f,void *opaque,int version_id)
@@ -159,30 +131,6 @@ static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id) }virtio_load (&vrng->vdev, f); - vrng->popped =qemu_get_byte (f); -if (vrng->popped) { -int i; - - vrng->elem.index =qemu_get_be32 (f); - - vrng->elem.in_num =qemu_get_be32 (f); -g_assert (vrng->elem.in_num <VIRTQUEUE_MAX_SIZE ); -for (i =0 ; i < vrng->elem.in_num; i++) { - vrng->elem.in_addr[i] =qemu_get_be64 (f); - } - - vrng->elem.out_num =qemu_get_be32 (f); -g_assert (vrng->elem.out_num <VIRTQUEUE_MAX_SIZE ); -for (i =0 ; i < vrng->elem.out_num; i++) { - vrng->elem.out_addr[i] =qemu_get_be64 (f); - } - -virtqueue_map_sg (vrng->elem.in_sg, vrng->elem.in_addr, - vrng->elem.in_num,1 ); -virtqueue_map_sg (vrng->elem.out_sg, vrng->elem.out_addr, - vrng->elem.out_num,0 ); - } -/* We may have an element ready but couldn't process it due to a quota limit. Make sure to try again after live migration when the quota may have been reset.
@@ -232,7 +180,7 @@ VirtIODevice *virtio_rng_init(DeviceState *dev, VirtIORNGConf *conf) vrng->qdev = dev; vrng->conf = conf; - vrng->popped = false; + vrng->quota_remaining = vrng->conf->max_bytes; g_assert_cmpint(vrng->conf->max_bytes, <=, INT64_MAX);
Signature · 1 line
1.8.0
PATCH v2 2/4 virtio-rng: remove extra request for entropy
In reply to Amit ShahIf we got fewer bytes from the backend than requested, don't poke the backend for more bytes; the guest will ask for more (or if the guest has already asked for more, the backend knows about it via handle_input()).
--- hw/virtio-rng.c | 7 ------- 1 file changed, 7 deletions(-)
hw/virtio-rng.c +0 -7
@@ -80,13 +80,6 @@ static void chr_read(void *opaque, const void *buf, size_t size)virtqueue_push (vrng->vq, &elem, len); }virtio_notify (&vrng->vdev, vrng->vq); - -/* -* Lastly, if we had multiple elems queued by the guest, and we -* didn't have enough data to fill them all, indicate we want more -* data. -*/ -virtio_rng_process (vrng); }static void virtio_rng_process (VirtIORNG *vrng)
Signature · 1 line
1.8.0
PATCH v2 3/4 virtio-rng: disable timer on device removal
In reply to Amit ShahDisable the rate-limit timer on device remove (e.g. hot-unplug).
--- hw/virtio-rng.c | 2 ++ 1 file changed, 2 insertions(+)
hw/virtio-rng.c +2 -0
@@ -194,6 +194,8 @@ void virtio_rng_exit(VirtIODevice *vdev) {VirtIORNG *vrng =DO_UPCAST (VirtIORNG, vdev, vdev); +qemu_del_timer (vrng->rate_limit_timer); +qemu_free_timer (vrng->rate_limit_timer);unregister_savevm (vrng->qdev,"virtio-rng" , vrng);virtio_cleanup (vdev); }
Signature · 1 line
1.8.0
PATCH v2 4/4 virtio-rng: fix typos, comments
In reply to Amit ShahFix typos, whitespace and update comments to match current implementation.
--- hw/virtio-rng.c | 7 +++---- include/qemu/rng.h | 6 +++--- 2 files changed, 6 insertions(+), 7 deletions(-)
hw/virtio-rng.c +3 -4
@@ -23,7 +23,6 @@ typedef struct VirtIORNG {/* Only one vq - guest puts buffer(s) on it when it needs entropy */ VirtQueue *vq; -/* Config data for the device -- currently only chardev */ VirtIORNGConf *conf;RngBackend *rng;
@@ -125,9 +124,9 @@ static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)virtio_load (&vrng->vdev, f);/* We may have an element ready but couldn't process it due to a quota -limit. Make sure to try again after live migration when the quota may -have been reset. -*/ +* limit. Make sure to try again after live migration when the quota may +* have been reset. +*/ virtio_rng_process (vrng);return 0 ;
include/qemu/rng.h +3 -3
@@ -61,10 +61,10 @@ struct RngBackend* This function is used by the front-end to request entropy from an entropy * source. This function can be called multiple times before @receive_entropy * is invoked with different values of @receive_entropy and @opaque. The -* backend will queue each request and handle appropriate. +* backend will queue each request and handle appropriately. * * The backend does not need to pass the full amount of data to @receive_entropy -* but will pass at a value greater than 0. +* but will pass a value greater than 0. */ void rng_backend_request_entropy(RngBackend *s, size_t size, EntropyReceiveFunc *receive_entropy,
@@ -87,7 +87,7 @@ void rng_backend_cancel_requests(RngBackend *s);* * This function will open the backend if it is not already open. Calling this * function on an already opened backend will not result in an error. -*/ +*/ void rng_backend_open (RngBackend *s,Error **errp);#endif
Signature · 1 line
1.8.0