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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Anthony Liguori [mailto:anthony@xxxxxxxxxxxxx]
> Sent: 02 July 2013 15:43
> To: Paul Durrant; qemu-devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxx
> Cc: Paul Durrant
> Subject: Re: [Qemu-devel] [PATCH] Citrix PV Bus device
> 
> Paul Durrant <paul.durrant@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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.
> 

I lifted the license verbatim from the BSD 2-clause template at 
http://opensource.org/licenses/BSD-2-Clause

> > + *
> > + * 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.
> 

Done in the latest patch.

> > +    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.
> 

I went for trace points in the latest patch.

> > +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.
> 

Ok. I'll fix and re-post.

> > +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.
> 

I opted for a size parameter in the newer patch instead. I'll mark the next 
post v3 for clarity.

  Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel