From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51390) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UtyE6-0004Qi-96 for qemu-devel@nongnu.org; Tue, 02 Jul 2013 06:54:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UtyE4-0003aN-En for qemu-devel@nongnu.org; Tue, 02 Jul 2013 06:54:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30514) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UtyE4-0003aI-6E for qemu-devel@nongnu.org; Tue, 02 Jul 2013 06:54:24 -0400 Message-ID: <51D2B156.2080103@redhat.com> Date: Tue, 02 Jul 2013 12:54:14 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1372754385-10186-1-git-send-email-paul.durrant@citrix.com> <51D2AF66.8000507@suse.de> In-Reply-To: <51D2AF66.8000507@suse.de> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] Citrix PV Bus device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-15?Q?Andreas_F=E4rber?= Cc: Paul Durrant , "Michael S. Tsirkin" , qemu-devel@nongnu.org, xen-devel@lists.xen.org Il 02/07/2013 12:45, Andreas F=E4rber ha scritto: > Am 02.07.2013 10:39, schrieb Paul Durrant: >> This patch introduces a new PCI device which will act as the binding p= oint >> for Citrix branded PV drivers for Xen. >> The intention is that Citrix Windows PV drivers will be available on W= indows >> Update and thus using the existing Xen platform PCI device as an ancho= r >> point is not desirable as that device has been ubiquitous in HVM guest= s for >> a long time and thus existing HVM guests running Windows would start >> automatically downloading drivers from Windows Update when this may no= t 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 +=3D pc.o pc_piix.o pc_q35.o >> obj-$(CONFIG_XEN) +=3D xen_domainbuild.o xen_machine_pv.o >> =20 >> obj-y +=3D kvmvapic.o >> +obj-y +=3D citrix_pv_bus.o >=20 > So the reason to place the device here is TARGET_PAGE_SIZE... We really > need a way to access that value from common code, somewhere down my TOD= O > list. :/ Why does it need to be in pages rather than bytes? Paolo >> 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. >> + *=20 >> + * Redistribution and use in source and binary forms,=20 >> + * with or without modification, are permitted provided=20 >> + * that the following conditions are met: >> + *=20 >> + * * Redistributions of source code must retain the above=20 >> + * copyright notice, this list of conditions and the=20 >> + * following disclaimer. >> + * * Redistributions in binary form must reproduce the above=20 >> + * copyright notice, this list of conditions and the=20 >> + * following disclaimer in the documentation and/or other=20 >> + * materials provided with the distribution. >> + *=20 >> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND=20 >> + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,=20 >> + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF=20 >> + * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE=20 >> + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR=20 >> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,=20 >> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,=20 >> + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR=20 >> + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS=20 >> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,=20 >> + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING=20 >> + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE=20 >> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF=20 >> + * SUCH DAMAGE. >> + */ >> + >> +#include "hw/hw.h" >> +#include "hw/pci/pci.h" >> + >> +typedef struct _CitrixPVBusDevice { >=20 > Identifiers starting with underscore are supposedly reserved by > POSIX/C99, you can just use the same name as for the typedef. >=20 >> + PCIDevice dev; >=20 > /*< private >*/ > PCIDevice parent_obj; > /*< public >*/ >=20 > please. More although still incomplete guidelines at: >=20 > http://wiki.qemu.org/QOMConventions >=20 >> + 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); >=20 > Possibly use the qemu_log() macros or a tracepoint? >=20 >> + >> + 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); >> +} >> + >> +static const MemoryRegionOps citrix_pv_bus_mmio_ops =3D { >> + .read =3D &citrix_pv_bus_mmio_read, >> + .write =3D &citrix_pv_bus_mmio_write, >> + .endianness =3D DEVICE_NATIVE_ENDIAN, >> +}; >> + >> +static int citrix_pv_bus_init(PCIDevice *pci_dev) >> +{ >> + CitrixPVBusDevice *d =3D DO_UPCAST(CitrixPVBusDevice, dev, pci_de= v); >=20 > Please don't use DO_UPCAST() with QOM objects. Please introduce a > CITRIX_PV_BUS_DEVICE(obj) macro as described above and/or in > include/qom/object.h to avoid using the parent field's name in code. >=20 >> + uint8_t *pci_conf; >> + uint64_t size; >> + >> + pci_conf =3D 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] =3D 1; >> + >> + size =3D d->pages * TARGET_PAGE_SIZE; >> + memory_region_init_io(&d->mmio, &citrix_pv_bus_mmio_ops, d, >> + "citrix-bus-mmio", size); >=20 > FYI Paolo will shortly merge a series that adds an owner as second > argument, i.e. pci_dev here. >=20 >> + >> + pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH, >> + &d->mmio); >> + >> + return 0; >=20 > Otherwise thanks for using pci_dev rather than d->dev, that'll make > converting this to QOM realize easier later. >=20 >> +} >> + >> +static Property citrix_pv_bus_props[] =3D { >> + 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 =3D DEVICE_CLASS(klass); >> + PCIDeviceClass *k =3D PCI_DEVICE_CLASS(klass); >> + >> + k->init =3D citrix_pv_bus_init; >> + k->vendor_id =3D PCI_VENDOR_ID_CITRIX; >> + k->device_id =3D PCI_DEVICE_ID_CITRIX_PV_BUS; >> + k->class_id =3D PCI_CLASS_SYSTEM_OTHER; >> + k->subsystem_vendor_id =3D PCI_VENDOR_ID_CITRIX; >> + k->subsystem_id =3D PCI_DEVICE_ID_CITRIX_PV_BUS; >> + dc->desc =3D "Citrix PV Bus"; >> + dc->props =3D citrix_pv_bus_props; >> +} >> + >> +static const TypeInfo citrix_pv_bus_type_info =3D { >> + .name =3D "citrix-pv-bus", >=20 > As part of the requested cast macro, please use a TYPE_... constant for > the string. I would suggest to not let a device type name end in -bus t= o > distinguish from actual QEMU busses (PCIBus, etc.). >=20 > What exactly do you intend to do with this device? Are you planning to > derive further specialized devices? Having a bar that prints errors on > each access surely doesn't seem like the final goal? >=20 > Regards, > Andreas >=20 >> + .parent =3D TYPE_PCI_DEVICE, >> + .instance_size =3D sizeof(CitrixPVBusDevice), >> + .class_init =3D 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 >> =20 >> +#define PCI_VENDOR_ID_CITRIX 0x5853 >> +#define PCI_DEVICE_ID_CITRIX_PV_BUS 0x0002 >> + >> #endif >> >=20 >=20