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

[Pankaj Gupta <pagupta@redhat.com>]

Hi Amit,

Thanks for the review.

> 
> 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.

Sorry! for not being clear on this. By rate limit I meant same.
I used a broader term.

> 
> > 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
    Tested this scenario. I am setting timer when first request comes.
    So, timer gets fired after (1 << 16) ms time. 

> * 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.

    I did not do  'dd if=/dev/hwrng of=/dev/null'.
    Did cat '/dev/hwrng' && '/dev/random'

> * Ensure quota restrictions are maintained, and we're not giving more
>   data than configured.
    Ensured. We are either giving < = requested data
> 
> 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.)

  Agree.
  Will test this as well.

> 
> 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!

Thanks.
> 
> I have one question:
> 
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> >  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 also had this query. If we don't call this after resetting 'vrng->quota_remaining' 
further requests does not work. It looks to me some limitation in earlier code when 
'vrng->quota_remaining' goes to < = 0. A self request is needed to reset things.

I will try to find the answer.

> 
> 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?
  No
> 
>                 Amit
> 
>