Re: [Qemu-devel] [PATCH] Citrix PV Bus device

[Anthony Liguori <anthony@codemonkey.ws>]

Paul Durrant <paul.durrant@citrix.com> writes:

> This patch introduces a new PCI device which will act as the binding point
> for Citrix branded PV drivers for Xen.
> The intention is that Citrix Windows PV drivers will be available on Windows
> Update and thus using the existing Xen platform PCI device as an anchor
> point is not desirable as that device has been ubiquitous in HVM guests for
> a long time and thus existing HVM guests running Windows would start
> automatically downloading drivers from Windows Update when this may not be
> desired by either the host or guest admin. This device therefore acts as
> an opt-in for those wishing to deploy Citrix PV drivers.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
>  hw/i386/Makefile.objs    |    1 +
>  hw/i386/citrix_pv_bus.c  |  122 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci/pci_ids.h |    3 ++
>  3 files changed, 126 insertions(+)
>  create mode 100644 hw/i386/citrix_pv_bus.c
>
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index 205d22e..8e28831 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -4,3 +4,4 @@ obj-y += pc.o pc_piix.o pc_q35.o
>  obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o
>  
>  obj-y += kvmvapic.o
> +obj-y += citrix_pv_bus.o
> diff --git a/hw/i386/citrix_pv_bus.c b/hw/i386/citrix_pv_bus.c
> new file mode 100644
> index 0000000..e1e0508
> --- /dev/null
> +++ b/hw/i386/citrix_pv_bus.c
> @@ -0,0 +1,122 @@
> +/* Copyright (c) Citrix Systems Inc.
> + * All rights reserved.

All rights reserved contradicts the grant of rights below.  Obviously
the later one is the one that wins but having the above statement is a
little silly IMHO.

> + * 
> + * Redistribution and use in source and binary forms, 
> + * with or without modification, are permitted provided 
> + * that the following conditions are met:
> + * 
> + * *   Redistributions of source code must retain the above 
> + *     copyright notice, this list of conditions and the 
> + *     following disclaimer.
> + * *   Redistributions in binary form must reproduce the above 
> + *     copyright notice, this list of conditions and the 
> + *     following disclaimer in the documentation and/or other 
> + *     materials provided with the distribution.
> + * 
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND 
> + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, 
> + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF 
> + * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE 
> + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR 
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, 
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, 
> + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR 
> + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS 
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, 
> + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING 
> + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE 
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF 
> + * SUCH DAMAGE.
> + */
> +
> +#include "hw/hw.h"
> +#include "hw/pci/pci.h"
> +
> +typedef struct _CitrixPVBusDevice {

Drop the '_' please.

> +    PCIDevice       dev;
> +    uint8_t         revision;
> +    uint32_t        pages;
> +    MemoryRegion    mmio;
> +} CitrixPVBusDevice;
> +
> +static uint64_t citrix_pv_bus_mmio_read(void *opaque, hwaddr addr,
> +                                        unsigned size)
> +{
> +    fprintf(stderr, "WARNING: read from address 0x" TARGET_FMT_plx
> +            " in Citrix PV Bus MMIO BAR\n", addr);
> +
> +    return ~(uint64_t)0;
> +}
> +
> +static void citrix_pv_bus_mmio_write(void *opaque, hwaddr addr,
> +                                     uint64_t val, unsigned size)
> +{
> +    fprintf(stderr, "WARNING: write to address 0x" TARGET_FMT_plx
> +            " in Citrix PV Bus MMIO BAR\n", addr);
> +}

Don't let guests trigger unconditional output to stdio.  If a management
tool logs stdio (libvirt does for instance), this can allow a guest to
mount a DoS attack on the host.

> +static const MemoryRegionOps citrix_pv_bus_mmio_ops = {
> +    .read = &citrix_pv_bus_mmio_read,
> +    .write = &citrix_pv_bus_mmio_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};

Please use DEVICE_LITTLE_ENDIAN.  Native endian shouldn't exist and is
there to deal with a few edge cases only.

> +static int citrix_pv_bus_init(PCIDevice *pci_dev)
> +{
> +    CitrixPVBusDevice *d = DO_UPCAST(CitrixPVBusDevice, dev, pci_dev);
> +    uint8_t *pci_conf;
> +    uint64_t size;
> +
> +    pci_conf = pci_dev->config;
> +
> +    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_MEMORY);
> +    pci_set_byte(pci_conf + PCI_REVISION_ID, d->revision);
> +
> +    pci_config_set_prog_interface(pci_conf, 0);
> +
> +    pci_conf[PCI_INTERRUPT_PIN] = 1;
> +
> +    size = d->pages * TARGET_PAGE_SIZE;
> +    memory_region_init_io(&d->mmio, &citrix_pv_bus_mmio_ops, d,
> +                          "citrix-bus-mmio", size);

:-/

This is a really bad interface.  Does this device support anything other
than x86?

On PPC, for instance, it's pretty normal to build a kernel with
PAGE_SIZE=4k, 16k, 64k, etc.

The page size that the kernel was compiled is a property of the kernel,
not anything architectural.  So what QEMU treats as TARGET_PAGE_SIZE
does not necessarily match the guest's PAGE_SIZE.

If this device only works on x86 today, you should fix the ABI at a
PAGE_SIZE=4096 or something like that.

Even hard coding 4096 here in QEMU would be better than using
TARGET_PAGE_SIZE.

Regards,

Anthony Liguori

> +
> +    pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
> +                     &d->mmio);
> +
> +    return 0;
> +}
> +
> +static Property citrix_pv_bus_props[] = {
> +    DEFINE_PROP_UINT8("revision", CitrixPVBusDevice, revision, 0x01),
> +    DEFINE_PROP_UINT32("pages", CitrixPVBusDevice, pages, 128),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void citrix_pv_bus_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->init = citrix_pv_bus_init;
> +    k->vendor_id = PCI_VENDOR_ID_CITRIX;
> +    k->device_id = PCI_DEVICE_ID_CITRIX_PV_BUS;
> +    k->class_id = PCI_CLASS_SYSTEM_OTHER;
> +    k->subsystem_vendor_id = PCI_VENDOR_ID_CITRIX;
> +    k->subsystem_id = PCI_DEVICE_ID_CITRIX_PV_BUS;
> +    dc->desc = "Citrix PV Bus";
> +    dc->props = citrix_pv_bus_props;
> +}
> +
> +static const TypeInfo citrix_pv_bus_type_info = {
> +    .name          = "citrix-pv-bus",
> +    .parent        = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(CitrixPVBusDevice),
> +    .class_init    = citrix_pv_bus_class_init,
> +};
> +
> +static void citrix_pv_bus_register_types(void)
> +{
> +    type_register_static(&citrix_pv_bus_type_info);
> +}
> +
> +type_init(citrix_pv_bus_register_types)
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index d8dc2f1..ed6a059 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -151,4 +151,7 @@
>  #define PCI_VENDOR_ID_TEWS               0x1498
>  #define PCI_DEVICE_ID_TEWS_TPCI200       0x30C8
>  
> +#define PCI_VENDOR_ID_CITRIX             0x5853
> +#define PCI_DEVICE_ID_CITRIX_PV_BUS      0x0002
> +
>  #endif
> -- 
> 1.7.10.4