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