[Xen-devel] Re: [Qemu-devel] [PATCH RFC V3 04/12] xen: Add the Xen platform pci device

[Anthony PERARD <anthony@xxxxxxxxxx>]

On Fri, 17 Sep 2010, Blue Swirl wrote:

> On Fri, Sep 17, 2010 at 11:14 AM,  <anthony.perard@xxxxxxxxxx> wrote:
> > From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >
> > Introduce a new emulated PCI device, specific to fully virtualized Xen
> > guests.  The device is necessary for PV on HVM drivers to work.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > ---
> >  Makefile.target     |    1 +
> >  hw/hw.h             |    3 +
> >  hw/pci_ids.h        |    2 +
> >  hw/xen_machine_fv.c |    3 +
> >  hw/xen_platform.c   |  455 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/xen_platform.h   |    8 +
> >  6 files changed, 472 insertions(+), 0 deletions(-)
> >  create mode 100644 hw/xen_platform.c
> >  create mode 100644 hw/xen_platform.h

[...]

> > +/* We throttle access to dom0 syslog, to avoid DOS attacks.  This is
> > +   modelled as a token bucket, with one token for every byte of log.
> > +   The bucket size is 128KB (->1024 lines of 128 bytes each) and
> > +   refills at 256B/s.  It starts full.  The guest is blocked if no
> > +   tokens are available when it tries to generate a log message. */
> > +#define BUCKET_MAX_SIZE (128*1024)
> > +#define BUCKET_FILL_RATE 256
> > +
> > +static void throttle(PCIXenPlatformState *s, unsigned count)
> > +{
> > +    static unsigned available;
> > +    static struct timespec last_refil;
>
> last_refill
>
> > +    static int started;
> > +    static int warned;
> > +
> > +    struct timespec waiting_for, now;
> > +    double delay;
> > +    struct timespec ts;
> > +
> > +    if (s->throttling_disabled)
> > +        return;
>
> Braces should be added here and other places.
>
> > +
> > +    if (!started) {
> > +        clock_gettime(CLOCK_MONOTONIC, &last_refil);
> > +        available = BUCKET_MAX_SIZE;
> > +        started = 1;
> > +    }
> > +
> > +    if (count > BUCKET_MAX_SIZE) {
> > +        DPRINTF("tried to get %d tokens, but bucket size is %d\n",
>
> count is unsigned, so %u.
>
> > +                BUCKET_MAX_SIZE, count);
> > +        exit(1);
> > +    }
> > +
> > +    if (available < count) {
> > +        /* The bucket is empty.  Refil it */
> > +
> > +        /* When will it be full enough to handle this request? */
> > +        delay = (double)(count - available) / BUCKET_FILL_RATE;
> > +        waiting_for = last_refil;
> > +        waiting_for.tv_sec += delay;
> > +        waiting_for.tv_nsec += (delay - (int)delay) * 1e9;
> > +        if (waiting_for.tv_nsec >= 1000000000) {
> > +            waiting_for.tv_nsec -= 1000000000;
> > +            waiting_for.tv_sec++;
> > +        }
> > +
> > +        /* How long do we have to wait? (might be negative) */
> > +        clock_gettime(CLOCK_MONOTONIC, &now);
> > +        ts.tv_sec = waiting_for.tv_sec - now.tv_sec;
> > +        ts.tv_nsec = waiting_for.tv_nsec - now.tv_nsec;
> > +        if (ts.tv_nsec < 0) {
> > +            ts.tv_sec--;
> > +            ts.tv_nsec += 1000000000;
> > +        }
> > +
> > +        /* Wait for it. */
> > +        if (ts.tv_sec > 0 ||
> > +            (ts.tv_sec == 0 && ts.tv_nsec > 0)) {
> > +            if (!warned) {
> > +                DPRINTF("throttling guest access to syslog");
> > +                warned = 1;
> > +            }
> > +            while (nanosleep(&ts, &ts) < 0 && errno == EINTR)
> > +                ;
>
> braces
>
> > +        }
> > +
> > +        /* Refil */
>
> Refill
>
> > +        clock_gettime(CLOCK_MONOTONIC, &now);
> > +        delay = (now.tv_sec - last_refil.tv_sec) +
> > +            (now.tv_nsec - last_refil.tv_nsec) * 1.0e-9;
> > +        available += BUCKET_FILL_RATE * delay;
>
> We have muldiv64(), perhaps it could be used here?

Ok, I use it, and I also use qemu_get_clock_ns(rt_clock).

> > +        if (available > BUCKET_MAX_SIZE)
> > +            available = BUCKET_MAX_SIZE;
> > +        last_refil = now;
> > +    }
> > +
> > +    assert(available >= count);
>
> Is it possible to trigger this from the guest?

I don't think we can do that for every guest.

> > +
> > +    available -= count;
> > +}
> > +

[...]

> > +static uint32_t platform_mmio_read(void *opaque, target_phys_addr_t addr)
> > +{
> > +    static int warnings = 0;
> > +
> > +    if (warnings < 5) {
> > +        DPRINTF("Warning: attempted read from physical address "
> > +                "0x" TARGET_FMT_plx " in xen platform mmio space\n", addr);
> > +        warnings++;
>
> Since DPRINTF only works in a specially compiled version, I'd remove
> these checks. There could also be additional debug flags besides
> PLATFORM_DEBUG to enable these warnings if these are too noisy, like
> DEBUG_MMIO. I'd rename PLATFORM_DEBUG to DEBUG_PLATFORM.

I will fix the coding style issue.

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel