WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [Qemu-devel] [PATCH 06/15] xen: Add the Xen platform pci

To: stefano.stabellini@xxxxxxxxxxxxx
Subject: [Xen-devel] Re: [Qemu-devel] [PATCH 06/15] xen: Add the Xen platform pci device
From: Blue Swirl <blauwirbel@xxxxxxxxx>
Date: Thu, 12 Aug 2010 18:26:49 +0000
Cc: Anthony.Perard@xxxxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxx, qemu-devel@xxxxxxxxxx
Delivery-date: Thu, 12 Aug 2010 11:28:52 -0700
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:mime-version:received:in-reply-to :references:from:date:message-id:subject:to:cc:content-type :content-transfer-encoding; bh=nYOmajZxkS8LHYB7aKvoVEtUYOALBVgLs5wpaKsGLT0=; b=FBrOqwyYhJESAa8wKLGhc7D8O0NPU0VXolokcUXctUDa+HGe2IbTnrn3k+MAD3yeAm iI3YmlZzxwNL12Evw3ND9mbb6uLMl8gxSaQSPe6XlZcggxKxmEyZnGctCC4u4p8B8u0m QjXoSeQUCg3ByoqHSQXu2HC/d/a+oIQ49rwt8=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:content-transfer-encoding; b=K1+IywDWbhgewa6dfSQ6AnIa9VE/TIOQxsSeMLsNkE/mCPK1ndRs7i3WlQZlDy0REr FVAJgXPLgKK1WKr3VwDd9Fd/9f8jB58ss0yybO1TeoO5J6G0Hg9DQkv73cCvGjyfjWBv Rj5GOHXawwIlR3mziiFAAKirTo6OJrWEtGs5M=
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1281622202-3453-6-git-send-email-stefano.stabellini@xxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <alpine.DEB.2.00.1008121244200.2545@kaball-desktop> <1281622202-3453-6-git-send-email-stefano.stabellini@xxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
On Thu, Aug 12, 2010 at 2:09 PM,  <stefano.stabellini@xxxxxxxxxxxxx> 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.

The code should be converted to qdev and VMState.

>
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
>  Makefile.target     |    1 +
>  hw/xen_machine_fv.c |    4 +
>  hw/xen_platform.c   |  452 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/xen_platform.h   |    9 +
>  4 files changed, 466 insertions(+), 0 deletions(-)
>  create mode 100644 hw/xen_platform.c
>  create mode 100644 hw/xen_platform.h
>
> diff --git a/Makefile.target b/Makefile.target
> index d1b63f2..1984cdd 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -324,6 +324,7 @@ obj-xen-y += pc.o
>  obj-xen-y += piix_pci.o
>  obj-xen-y += mc146818rtc.o
>  obj-xen-y += xenstore.o
> +obj-xen-y += xen_platform.o
>
>  obj-xen-y += xen_mapcache.o
>  obj-xen-y += stub-functions.o
> diff --git a/hw/xen_machine_fv.c b/hw/xen_machine_fv.c
> index 114addf..ec826e7 100644
> --- a/hw/xen_machine_fv.c
> +++ b/hw/xen_machine_fv.c
> @@ -35,6 +35,7 @@
>  #include "xen_common.h"
>  #include "xen_backend.h"
>  #include "xenstore.h"
> +#include "xen_platform.h"
>  #include "xen/hvm/hvm_info_table.h"
>
>  #define MAX_IDE_BUS 2
> @@ -93,6 +94,9 @@ static void xen_init_fv(ram_addr_t ram_size,
>
>     pc_vga_init(pci_bus);
>
> +    pci_xen_platform_init(pci_bus);
> +    platform_fixed_ioport_init();
> +
>     /* init basic PC hardware */
>     pc_basic_device_init(isa_irq, &floppy_controller, &rtc_state);
>
> diff --git a/hw/xen_platform.c b/hw/xen_platform.c
> new file mode 100644
> index 0000000..85d3f8b
> --- /dev/null
> +++ b/hw/xen_platform.c
> @@ -0,0 +1,452 @@
> +/*
> + * XEN platform pci device, formerly known as the event channel device
> + *
> + * Copyright (c) 2003-2004 Intel Corp.
> + * Copyright (c) 2006 XenSource
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "hw.h"
> +#include "pc.h"
> +#include "pci.h"
> +#include "irq.h"
> +#include "xen_common.h"
> +#include "net.h"
> +#include "xen_platform.h"
> +#include "xen_backend.h"
> +#include "qemu-log.h"
> +
> +#include <assert.h>
> +#include <xenguest.h>
> +
> +static int drivers_blacklisted;
> +static uint16_t driver_product_version;
> +static int throttling_disabled;
> +static char log_buffer[4096];
> +static int log_buffer_off;
> +
> +static uint8_t platform_flags;

A lot of static variables. Could you put these to PCIXenPlatformState?

> +
> +#define PFFLAG_ROM_LOCK 1 /* Sets whether ROM memory area is RW or RO */
> +
> +typedef struct PCIXenPlatformState
> +{
> +    PCIDevice  pci_dev;
> +} PCIXenPlatformState;
> +
> +
> +/* 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(unsigned count)
> +{
> +    static unsigned available;
> +    static struct timespec last_refil;
> +    static int started;
> +    static int warned;
> +
> +    struct timespec waiting_for, now;
> +    double delay;
> +    struct timespec ts;
> +
> +    if (throttling_disabled)
> +        return;
> +
> +    if (!started) {
> +        clock_gettime(CLOCK_MONOTONIC, &last_refil);
> +        available = BUCKET_MAX_SIZE;
> +        started = 1;
> +    }
> +
> +    if (count > BUCKET_MAX_SIZE) {
> +        fprintf(stderr, "tried to get %d tokens, but bucket size is %d\n",
> +                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) {
> +                fprintf(stderr, "throttling guest access to syslog");
> +                warned = 1;
> +            }
> +            while (nanosleep(&ts, &ts) < 0 && errno == EINTR)
> +                ;
> +        }
> +
> +        /* Refil */
> +        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;
> +        if (available > BUCKET_MAX_SIZE)
> +            available = BUCKET_MAX_SIZE;
> +        last_refil = now;
> +    }
> +
> +    assert(available >= count);
> +
> +    available -= count;
> +}
> +
> +#define UNPLUG_ALL_IDE_DISKS 1
> +#define UNPLUG_ALL_NICS 2
> +#define UNPLUG_AUX_IDE_DISKS 4

These should go to the top of the file. Are they even used, the
function below doesn't?

> +
> +static void platform_fixed_ioport_write2(void *opaque, uint32_t addr, 
> uint32_t val)
> +{
> +    switch (addr - 0x10) {

0x10 should be a #define, which should be used...

> +    case 0:
> +        /* Unplug devices.  Value is a bitmask of which devices to
> +           unplug, with bit 0 the IDE devices, bit 1 the network
> +           devices, and bit 2 the non-primary-master IDE devices. */
> +        break;
> +    case 2:
> +        switch (val) {
> +        case 1:
> +            fprintf(stderr, "Citrix Windows PV drivers loaded in guest\n");
> +            break;
> +        case 0:
> +            fprintf(stderr, "Guest claimed to be running PV product 0?\n");
> +            break;
> +        default:
> +            fprintf(stderr, "Unknown PV product %d loaded in guest\n", val);
> +            break;
> +        }
> +        driver_product_version = val;
> +        break;
> +    }
> +}
> +
> +static void platform_fixed_ioport_write4(void *opaque, uint32_t addr,
> +                                         uint32_t val)
> +{
> +    switch (addr - 0x10) {

... here ...

> +    case 0:
> +        /* PV driver version */
> +        break;
> +    }
> +}
> +
> +static void platform_fixed_ioport_write1(void *opaque, uint32_t addr, 
> uint32_t val)
> +{
> +    switch (addr - 0x10) {

... here ...

> +    case 0: /* Platform flags */ {
> +        hvmmem_type_t mem_type = (val & PFFLAG_ROM_LOCK) ?
> +            HVMMEM_ram_ro : HVMMEM_ram_rw;
> +        if (xc_hvm_set_mem_type(xen_xc, xen_domid, mem_type, 0xc0, 0x40))
> +            fprintf(stderr,"platform_fixed_ioport: unable to change ro/rw "
> +                    "state of ROM memory area!\n");

Please introduce a macro (DPRINTF) and use that.

> +        else {
> +            platform_flags = val & PFFLAG_ROM_LOCK;
> +            fprintf(stderr,"platform_fixed_ioport: changed ro/rw "
> +                    "state of ROM memory area. now is %s state.\n",
> +                    (mem_type == HVMMEM_ram_ro ? "ro":"rw"));
> +        }
> +        break;
> +    }
> +    case 2:
> +        /* Send bytes to syslog */
> +        if (val == '\n' || log_buffer_off == sizeof(log_buffer) - 1) {
> +            /* Flush buffer */
> +            log_buffer[log_buffer_off] = 0;
> +            throttle(log_buffer_off);
> +            fprintf(stderr, "%s\n", log_buffer);
> +            log_buffer_off = 0;
> +            break;
> +        }
> +        log_buffer[log_buffer_off++] = val;
> +        break;
> +    }
> +}
> +
> +static uint32_t platform_fixed_ioport_read2(void *opaque, uint32_t addr)
> +{
> +    switch (addr - 0x10) {

... here ...

> +    case 0:
> +        if (drivers_blacklisted) {
> +            /* The drivers will recognise this magic number and refuse
> +             * to do anything. */
> +            return 0xd249;
> +        } else {
> +            /* Magic value so that you can identify the interface. */
> +            return 0x49d2;
> +        }
> +    default:
> +        return 0xffff;
> +    }
> +}
> +
> +static uint32_t platform_fixed_ioport_read1(void *opaque, uint32_t addr)
> +{
> +    switch (addr - 0x10) {

... here ...

> +    case 0:
> +        /* Platform flags */
> +        return platform_flags;
> +    case 2:
> +        /* Version number */
> +        return 1;
> +    default:
> +        return 0xff;
> +    }
> +}
> +
> +static void platform_fixed_ioport_save(QEMUFile *f, void *opaque)
> +{
> +    qemu_put_8s(f, &platform_flags);
> +}
> +
> +static int platform_fixed_ioport_load(QEMUFile *f, void *opaque, int 
> version_id)
> +{
> +    uint8_t flags;
> +
> +    if (version_id > 1)
> +        return -EINVAL;
> +
> +    qemu_get_8s(f, &flags);
> +    platform_fixed_ioport_write1(NULL, 0x10, flags);
> +
> +    return 0;
> +}
> +
> +void platform_fixed_ioport_init(void)
> +{
> +    register_savevm(NULL, "platform_fixed_ioport", 0, 1, 
> platform_fixed_ioport_save,
> +                    platform_fixed_ioport_load, NULL);

Please use VMState instead.

> +
> +    register_ioport_write(0x10, 16, 4, platform_fixed_ioport_write4, NULL);

and here and below. In fact, just s/0x10/XEN_PLATFORM_IOPORT/g.

> +    register_ioport_write(0x10, 16, 2, platform_fixed_ioport_write2, NULL);
> +    register_ioport_write(0x10, 16, 1, platform_fixed_ioport_write1, NULL);
> +    register_ioport_read(0x10, 16, 2, platform_fixed_ioport_read2, NULL);
> +    register_ioport_read(0x10, 16, 1, platform_fixed_ioport_read1, NULL);
> +
> +    platform_fixed_ioport_write1(NULL, 0x10, 0);

Introduce a reset function which performs something similar.

> +}
> +
> +static uint32_t xen_platform_ioport_readb(void *opaque, uint32_t addr)
> +{
> +    addr &= 0xff;
> +
> +    return (addr == 0) ? platform_fixed_ioport_read1(NULL, 0x10) : ~0u;

Just use if.

> +}
> +
> +static void xen_platform_ioport_writeb(void *opaque, uint32_t addr, uint32_t 
> val)
> +{
> +    addr &= 0xff;
> +    val  &= 0xff;
> +
> +    switch (addr) {
> +    case 0: /* Platform flags */
> +        platform_fixed_ioport_write1(NULL, 0x10, val);
> +        break;
> +    case 8:
> +        {
> +            if (val == '\n' || log_buffer_off == sizeof(log_buffer) - 1) {
> +                /* Flush buffer */
> +                log_buffer[log_buffer_off] = 0;
> +                throttle(log_buffer_off);
> +                fprintf(stderr, "%s\n", log_buffer);
> +                log_buffer_off = 0;
> +                break;
> +            }
> +            log_buffer[log_buffer_off++] = val;
> +        }
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +static void platform_ioport_map(PCIDevice *pci_dev, int region_num, pcibus_t 
> addr, pcibus_t size, int type)
> +{
> +    PCIXenPlatformState *d = (PCIXenPlatformState *)pci_dev;

Useless cast in C. Moreover, you should use DO_UPCAST or container_of.

> +    register_ioport_write(addr, size, 1, xen_platform_ioport_writeb, d);
> +    register_ioport_read(addr, size, 1, xen_platform_ioport_readb, d);
> +}
> +
> +static uint32_t platform_mmio_read(void *opaque, target_phys_addr_t addr)
> +{
> +    static int warnings = 0;
> +    if (warnings < 5) {
> +        fprintf(stderr, "Warning: attempted read from physical address "
> +                "0x%"PRIx64" in xen platform mmio space\n", (uint64_t)addr);

Instead of the cast, you should use TARGET_FMT_plx.

> +        warnings++;
> +    }
> +    return 0;
> +}
> +
> +static void platform_mmio_write(void *opaque, target_phys_addr_t addr,
> +                                uint32_t val)
> +{
> +    static int warnings = 0;
> +    if (warnings < 5) {
> +        fprintf(stderr, "Warning: attempted write of 0x%x to physical "
> +                "address 0x%"PRIx64" in xen platform mmio space\n",
> +                val, (uint64_t)addr);
> +        warnings++;
> +    }
> +    return;
> +}
> +
> +static CPUReadMemoryFunc *platform_mmio_read_funcs[3] = {

These should be 'const'.

> +    platform_mmio_read,
> +    platform_mmio_read,
> +    platform_mmio_read,
> +};
> +
> +static CPUWriteMemoryFunc *platform_mmio_write_funcs[3] = {
> +    platform_mmio_write,
> +    platform_mmio_write,
> +    platform_mmio_write,
> +};
> +
> +static void platform_mmio_map(PCIDevice *d, int region_num,
> +                              pcibus_t addr, pcibus_t size, int type)
> +{
> +    int mmio_io_addr;
> +
> +    mmio_io_addr = cpu_register_io_memory(platform_mmio_read_funcs,
> +                                          platform_mmio_write_funcs, NULL);
> +
> +    cpu_register_physical_memory(addr, 0x1000000, mmio_io_addr);
> +}
> +
> +struct pci_config_header {
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +    uint16_t command;
> +    uint16_t status;
> +    uint8_t  revision;
> +    uint8_t  api;
> +    uint8_t  subclass;
> +    uint8_t  class;
> +    uint8_t  cache_line_size; /* Units of 32 bit words */
> +    uint8_t  latency_timer; /* In units of bus cycles */
> +    uint8_t  header_type; /* Should be 0 */
> +    uint8_t  bist; /* Built in self test */
> +    uint32_t base_address_regs[6];
> +    uint32_t reserved1;
> +    uint16_t subsystem_vendor_id;
> +    uint16_t subsystem_id;
> +    uint32_t rom_addr;
> +    uint32_t reserved3;
> +    uint32_t reserved4;
> +    uint8_t  interrupt_line;
> +    uint8_t  interrupt_pin;
> +    uint8_t  min_gnt;
> +    uint8_t  max_lat;
> +};

Why can't you use the facilities from pci.h?

> +
> +static void xen_pci_save(QEMUFile *f, void *opaque)
> +{
> +    PCIXenPlatformState *d = opaque;
> +    uint64_t t = 0;
> +
> +    pci_device_save(&d->pci_dev, f);
> +    qemu_put_be64s(f, &t);
> +}
> +
> +static int xen_pci_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +    PCIXenPlatformState *d = opaque;
> +    int ret;
> +
> +    if (version_id > 3)
> +        return -EINVAL;
> +
> +    ret = pci_device_load(&d->pci_dev, f);
> +    if (ret < 0)
> +        return ret;
> +
> +    if (version_id >= 2) {
> +        if (version_id == 2) {
> +            uint8_t flags;
> +            qemu_get_8s(f, &flags);
> +            xen_platform_ioport_writeb(d, 0, flags);
> +        }
> +        qemu_get_be64(f);
> +    }
> +
> +    return 0;
> +}
> +
> +void pci_xen_platform_init(PCIBus *bus)
> +{
> +    PCIXenPlatformState *d;
> +    struct pci_config_header *pch;
> +
> +    printf("Register xen platform.\n");
> +    d = (PCIXenPlatformState *)pci_register_device(
> +        bus, "xen-platform", sizeof(PCIXenPlatformState), -1, NULL, NULL);
> +    pch = (struct pci_config_header *)d->pci_dev.config;
> +    pch->vendor_id = 0x5853;

You should use pci_set_word etc. Please add 0x5853 to pci_ids.h.

> +    pch->device_id = 0x0001;
> +    pch->command = 3; /* IO and memory access */
> +    pch->revision = 1;
> +    pch->api = 0;
> +    pch->subclass = 0x80; /* Other */
> +    pch->class = 0xff; /* Unclassified device class */
> +    pch->header_type = 0;
> +    pch->interrupt_pin = 1;
> +
> +    /* Microsoft WHQL requires non-zero subsystem IDs. */
> +    /* http://www.pcisig.com/reflector/msg02205.html.  */
> +    pch->subsystem_vendor_id = pch->vendor_id; /* Duplicate vendor id.  */
> +    pch->subsystem_id        = 0x0001;         /* Hardcode sub-id as 1. */
> +
> +    pci_register_bar(&d->pci_dev, 0, 0x100,
> +                           PCI_BASE_ADDRESS_SPACE_IO, platform_ioport_map);
> +
> +    /* reserve 16MB mmio address for share memory*/
> +    pci_register_bar(&d->pci_dev, 1, 0x1000000,
> +                           PCI_BASE_ADDRESS_MEM_PREFETCH, platform_mmio_map);
> +
> +    register_savevm(NULL, "platform", 0, 3, xen_pci_save, xen_pci_load, d);
> +    printf("Done register platform.\n");
> +}
> +
> diff --git a/hw/xen_platform.h b/hw/xen_platform.h
> new file mode 100644
> index 0000000..6eeff22
> --- /dev/null
> +++ b/hw/xen_platform.h
> @@ -0,0 +1,9 @@
> +#ifndef XEN_PLATFORM_H
> +#define XEN_PLATFORM_H
> +
> +#include "hw/pci.h"
> +
> +void pci_xen_platform_init(PCIBus *bus);
> +void platform_fixed_ioport_init(void);
> +
> +#endif
> --
> 1.7.0.4
>
>
>

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

<Prev in Thread] Current Thread [Next in Thread>