From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38318) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uu1nk-0004LS-9K for qemu-devel@nongnu.org; Tue, 02 Jul 2013 10:43:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uu1nh-0000PO-SY for qemu-devel@nongnu.org; Tue, 02 Jul 2013 10:43:28 -0400 Received: from mail-gh0-f178.google.com ([209.85.160.178]:36515) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uu1nh-0000Ov-Hr for qemu-devel@nongnu.org; Tue, 02 Jul 2013 10:43:25 -0400 Received: by mail-gh0-f178.google.com with SMTP id g15so2498905ghb.9 for ; Tue, 02 Jul 2013 07:43:24 -0700 (PDT) From: Anthony Liguori In-Reply-To: <1372754385-10186-1-git-send-email-paul.durrant@citrix.com> References: <1372754385-10186-1-git-send-email-paul.durrant@citrix.com> Date: Tue, 02 Jul 2013 09:43:21 -0500 Message-ID: <87ehbhqaye.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH] Citrix PV Bus device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paul Durrant , qemu-devel@nongnu.org, xen-devel@lists.xen.org Paul Durrant 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 > --- > 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