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