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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 02/07/2013 12:45, Andreas Färber ha scritto:
> Am 02.07.2013 10:39, schrieb Paul Durrant:
>> 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
> 
> 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 TODO
> 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.
>> + * 
>> + * 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 {
> 
> Identifiers starting with underscore are supposedly reserved by
> POSIX/C99, you can just use the same name as for the typedef.
> 
>> +    PCIDevice       dev;
> 
>     /*< private >*/
>     PCIDevice parent_obj;
>     /*< public >*/
> 
> please. More although still incomplete guidelines at:
> 
> http://wiki.qemu.org/QOMConventions
> 
>> +    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);
> 
> Possibly use the qemu_log() macros or a tracepoint?
> 
>> +
>> +    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 = {
>> +    .read = &citrix_pv_bus_mmio_read,
>> +    .write = &citrix_pv_bus_mmio_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static int citrix_pv_bus_init(PCIDevice *pci_dev)
>> +{
>> +    CitrixPVBusDevice *d = DO_UPCAST(CitrixPVBusDevice, dev, pci_dev);
> 
> 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.
> 
>> +    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);
> 
> FYI Paolo will shortly merge a series that adds an owner as second
> argument, i.e. pci_dev here.
> 
>> +
>> +    pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
>> +                     &d->mmio);
>> +
>> +    return 0;
> 
> Otherwise thanks for using pci_dev rather than d->dev, that'll make
> converting this to QOM realize easier later.
> 
>> +}
>> +
>> +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",
> 
> 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 to
> distinguish from actual QEMU busses (PCIBus, etc.).
> 
> 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?
> 
> Regards,
> Andreas
> 
>> +    .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
>>
> 
>