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

[Paul Durrant]

> -----Original Message-----
> From: Stefano Stabellini [mailto:address@hidden
> Sent: 02 July 2013 17:42
> To: Paul Durrant
> Cc: address@hidden; address@hidden
> Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device [V3]
> 
> On Tue, 2 Jul 2013, Paul Durrant wrote:
> > 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.
> >
> > V3:
> > - Addresses comments from Anthony Liguori and Peter Maydell
> >
> > V2:
> > - Addresses comments from Andreas Farber and Paolo Bonzini
> >
> > Signed-off-by: Paul Durrant <address@hidden>
> 
> I do realize that this is v3 and I didn't step in the discussion
> before, sorry for the delay in my response. I admit I didn't read the
> entire thread, so excuse me if I repeat things already said before.
> 
> 
> >  hw/misc/Makefile.objs    |    2 +
> >  hw/misc/citrix_pv_bus.c  |  127
> ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/pci/pci_ids.h |    7 ++-
> >  trace-events             |    4 ++
> >  4 files changed, 138 insertions(+), 2 deletions(-)
> >  create mode 100644 hw/misc/citrix_pv_bus.c
> >
> > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> > index 2578e29..ffd29ea 100644
> > --- a/hw/misc/Makefile.objs
> > +++ b/hw/misc/Makefile.objs
> > @@ -41,3 +41,5 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o
> >  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
> >
> >  obj-$(CONFIG_PVPANIC) += pvpanic.o
> > +
> > +obj-$(CONFIG_XEN) += citrix-pv-bus.o
> > diff --git a/hw/misc/citrix_pv_bus.c b/hw/misc/citrix_pv_bus.c
> > new file mode 100644
> > index 0000000..76e5e13
> > --- /dev/null
> > +++ b/hw/misc/citrix_pv_bus.c
> > @@ -0,0 +1,127 @@
> > +
> > +/* Copyright (c) Citrix Systems Inc.
> > + * All rights reserved.
> > + *
> > + * 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"
> > +#include "trace.h"
> > +
> > +#define TYPE_CITRIX_PV_BUS_DEVICE  "citrix-pv"
> > +
> > +#define CITRIX_PV_BUS_DEVICE(obj) \
> > +     OBJECT_CHECK(CitrixPVBusDevice, (obj),
> TYPE_CITRIX_PV_BUS_DEVICE)
> > +
> > +typedef struct CitrixPVBusDevice {
> > +    /*< private >*/
> > +    PCIDevice       parent_obj;
> > +    /*< public >*/
> > +    uint8_t         revision;
> > +    uint32_t        size;
> > +    MemoryRegion    mmio;
> > +} CitrixPVBusDevice;
> > +
> > +static uint64_t citrix_pv_bus_mmio_read(void *opaque, hwaddr addr,
> > +                                        unsigned size)
> > +{
> > +    trace_citrix_pv_bus_mmio_read(addr);
> > +
> > +    return ~(uint64_t)0;
> > +}
> > +
> > +static void citrix_pv_bus_mmio_write(void *opaque, hwaddr addr,
> > +                                     uint64_t val, unsigned size)
> > +{
> > +    trace_citrix_pv_bus_mmio_write(addr);
> > +}
> > +
> > +static const MemoryRegionOps citrix_pv_bus_mmio_ops = {
> > +    .read = &citrix_pv_bus_mmio_read,
> > +    .write = &citrix_pv_bus_mmio_write,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +};
> > +
> > +static int citrix_pv_bus_init(PCIDevice *pci_dev)
> > +{
> > +    CitrixPVBusDevice *d = CITRIX_PV_BUS_DEVICE(pci_dev);
> > +    uint8_t *pci_conf;
> > +
> > +    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;
> > +
> > +    memory_region_init_io(&d->mmio, &citrix_pv_bus_mmio_ops, d,
> > +                          "citrix-bus-mmio", d->size);
> > +
> > +    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("size", CitrixPVBusDevice, size, 0x400000),
> > +    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          = TYPE_CITRIX_PV_BUS_DEVICE,
> > +    .parent        = TYPE_PCI_DEVICE,
> > +    .instance_size = sizeof(CitrixPVBusDevice),
> > +    .class_init    = citrix_pv_bus_class_init,
> > +};
> 
> I get that we need a new device because Windows drivers from Windows
> Update need to bind to it and we can't simply "force" an update to all
> the Windows HVM instances out there.
> For example one might have installed another vendor's Windows PV drivers
> and might not want to switch to the ones provided by Citrix.
> 
> However we simply can't add a new device in QEMU for each vendor that
> decides to implement Windows Xen PV drivers. We have to make this
> configurable.
> 
> Finally if possible we shouldn't assume that the admin knows beforehand
> what operating system is installed in the VM.
> 
> As a result I think that:
> 
> - it's a good idea to add a new device but at the same time we should
> also leave the old one there, so that existing installations (Linux,
> BSD, Solaris, etc.) will keep working without troubles and people that
> wants to manually install the Windows PV drivers from the installer
> can keep doing so;
> 

Absolutely, and since Alex Bligh's comment on my original xen-platform-2 patch 
I realise this had to be the case; hence the idea of the secondary device.

> - the new device should have configurable vendor and device ids, so that
> host admins can select which vendor's PV drivers are going to be
> automatically installed on all your Windows guests. This should probably
> be a VM config option (pvdevice=<gplpv|citrix|suse|oracle|etc>, libxl would
> then pass a vendor and device id pair to QEMU via command line.
> We can come up with a config syntax that would support both numeric
> pairs as well as simple labels.
> 
> - the Citrix [vendor id|device id] pair has to be different from the
> xenproject one (0x5853|0x0001). I am sure we can arrange the
> xenproject device id space so that Windows PV drivers vendors don't have
> to acquire their own PCI vendor ids.
> 

This all sounds fine; it's a generalization of what I've already coded up so 
I'm happy to refactor it. Any suggestions on a name? "xen-pvdevice", perhaps, 
to link it to your proposed VM config option?

> - Windows PV drivers vendors might want to consider continuing on
> providing a Windows PV drivers installer that binds to the current PCI
> vendor and device ids so that people can continue consuming them the
> same way they have been doing so far without requiring host admin
> intervention.
> 

We (Citrix) may well do that too. The build script for the win-xenbus driver 
takes the binding information as an argument so it's trivial for anyone to 
build that driver to bind to any PCI device.

  Paul