Re: [Qemu-devel] [PATCH] virtio-rng: Bump up quota value only when guest requests entropy

[Amit Shah]

On (Fri) 10 Jul 2015 [15:04:00], Pankaj Gupta wrote:
>    Timer was added in virtio-rng to rate limit the 
> entropy. It used to trigger at regular intervals to 
> bump up the quota value. The value of quota and timer 
> slice is decided based on entropy source rate in host. 

It doesn't necessarily depnd on the source rate in the host - all we
want the quota+timer to do is to limit the amount of data a guest can
take from the host - to ensure one (potentially rogue) guest does not
use up all the entropy from the host.

> This resulted in triggring of timer even when quota 
> is not exhausted at all and resulting in extra processing.
> 
> This patch triggers timer only when guest requests for 
> entropy. As soon as first request from guest for entropy 
> comes we set the timer. Timer bumps up the quota value 
> when it gets triggered.

Can you say how you tested this?

Mainly interested in seeing the results in these cases:

* No quota/timer specified on command line
* Quota+timer specified on command line, and guest keeps asking host
  for unlimited entropy, e.g. by doing 'dd if=/dev/hwrng of=/dev/null'
  in the guest.
* Ensure quota restrictions are maintained, and we're not giving more
  data than configured.

For these tests, it's helpful to use the host's /dev/urandom as the
source, since that can give data faster to the guest than the default
/dev/random.  (Otherwise, if the host itself blocks on /dev/random,
the guest may not get entropy due to that reason vs it not getting
entropy due to rate-limiting.)

I tested one scenario using the trace events.  With some quota and a
timer value specified on the cmdline, before patch, I get tons of
trace events before the guest is even up.  After applying the patch, I
don't get any trace events.  So that's progress!

I have one question:

> Signed-off-by: Pankaj Gupta <address@hidden>
> ---
>  hw/virtio/virtio-rng.c         | 15 ++++++++-------
>  include/hw/virtio/virtio-rng.h |  1 +
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index 22b1d87..8774a0c 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -78,6 +78,12 @@ static void virtio_rng_process(VirtIORNG *vrng)
>          return;
>      }
>  
> +    if (vrng->activate_timer) {
> +        timer_mod(vrng->rate_limit_timer,
> +                   qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 
> vrng->conf.period_ms);
> +        vrng->activate_timer = false;
> +    }
> +
>      if (vrng->quota_remaining < 0) {
>          quota = 0;
>      } else {
> @@ -139,8 +145,7 @@ static void check_rate_limit(void *opaque)
>  
>      vrng->quota_remaining = vrng->conf.max_bytes;
>      virtio_rng_process(vrng);
> -    timer_mod(vrng->rate_limit_timer,
> -                   qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 
> vrng->conf.period_ms);
> +    vrng->activate_timer = true;
>  }

We're processing an older request first, and then firing the timer.
What's the use of doing it this way?  Why even do this?

I know this is how the code was written originally, but since you've
looked at it, do you know why this is the way it is?

                Amit