Xen PV Device

This patch introduces a new Xen PV PCI device which will act as a new
binding point for PV drivers for Xen.
The device has parameterized vendor-id, device-id and revision to allow to
be configured as a binding point for any vendor's PV drivers.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 hw/xen/Makefile.objs     |    1 +
 hw/xen/xen_pvdevice.c    |  131 ++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci/pci_ids.h |    5 +-
 trace-events             |    4 ++
 4 files changed, 139 insertions(+), 2 deletions(-)
 create mode 100644 hw/xen/xen_pvdevice.c

On Wed, 3 Jul 2013, Paul Durrant wrote:
> This patch introduces a new Xen PV PCI device which will act as a new
> binding point for PV drivers for Xen.
> The device has parameterized vendor-id, device-id and revision to allow to
> be configured as a binding point for any vendor's PV drivers.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  hw/xen/Makefile.objs     |    1 +
>  hw/xen/xen_pvdevice.c    |  131 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci/pci_ids.h |    5 +-
>  trace-events             |    4 ++
>  4 files changed, 139 insertions(+), 2 deletions(-)
>  create mode 100644 hw/xen/xen_pvdevice.c
> 
> diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
> index 2017560..fd88003 100644
> --- a/hw/xen/Makefile.objs
> +++ b/hw/xen/Makefile.objs
> @@ -4,3 +4,4 @@ common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o
>  obj-$(CONFIG_XEN_I386) += xen_platform.o xen_apic.o
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
> +obj-$(CONFIG_XEN) += xen_pvdevice.o
> diff --git a/hw/xen/xen_pvdevice.c b/hw/xen/xen_pvdevice.c
> new file mode 100644
> index 0000000..dbc4bf5
> --- /dev/null
> +++ b/hw/xen/xen_pvdevice.c
> @@ -0,0 +1,131 @@
> +/* Copyright (c) Citrix Systems Inc.
> + * All rights reserved.

Like Anthony wrote before, All rights reserved contradicts what's
written below.
Aside from this, it looks OK to me.

I would like to see the libxl side patch.
Also it would be nice to have an ack from Andreas or another QOM expert.




> + * 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"
> +#include "trace.h"
> +
> +#define TYPE_XEN_PV_DEVICE  "xen-pvdevice"
> +
> +#define XEN_PV_DEVICE(obj) \
> +     OBJECT_CHECK(XenPVDevice, (obj), TYPE_XEN_PV_DEVICE)
> +
> +typedef struct XenPVDevice {
> +    /*< private >*/
> +    PCIDevice       parent_obj;
> +    /*< public >*/
> +    uint16_t        vendor_id;
> +    uint16_t        device_id;
> +    uint8_t         revision;
> +    uint32_t        size;
> +    MemoryRegion    mmio;
> +} XenPVDevice;
> +
> +static uint64_t xen_pv_mmio_read(void *opaque, hwaddr addr,
> +                                        unsigned size)
> +{
> +    trace_xen_pv_mmio_read(addr);
> +
> +    return ~(uint64_t)0;
> +}
> +
> +static void xen_pv_mmio_write(void *opaque, hwaddr addr,
> +                                     uint64_t val, unsigned size)
> +{
> +    trace_xen_pv_mmio_write(addr);
> +}
> +
> +static const MemoryRegionOps xen_pv_mmio_ops = {
> +    .read = &xen_pv_mmio_read,
> +    .write = &xen_pv_mmio_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static int xen_pv_init(PCIDevice *pci_dev)
> +{
> +    XenPVDevice *d = XEN_PV_DEVICE(pci_dev);
> +    uint8_t *pci_conf;
> +
> +    pci_conf = pci_dev->config;
> +
> +    pci_set_word(pci_conf + PCI_VENDOR_ID, d->vendor_id);
> +    pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, d->vendor_id);
> +    pci_set_word(pci_conf + PCI_DEVICE_ID, d->device_id);
> +    pci_set_word(pci_conf + PCI_SUBSYSTEM_ID, d->device_id);
> +    pci_set_byte(pci_conf + PCI_REVISION_ID, d->revision);
> +
> +    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_MEMORY);
> +
> +    pci_config_set_prog_interface(pci_conf, 0);
> +
> +    pci_conf[PCI_INTERRUPT_PIN] = 1;
> +
> +    memory_region_init_io(&d->mmio, &xen_pv_mmio_ops, d,
> +                          "xen-pvdevice-mmio", d->size);
> +
> +    pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
> +                     &d->mmio);
> +
> +    return 0;
> +}
> +
> +static Property xen_pv_props[] = {
> +    DEFINE_PROP_UINT16("vendor-id", XenPVDevice, vendor_id, PCI_VENDOR_ID_XEN),
> +    DEFINE_PROP_UINT16("device-id", XenPVDevice, device_id, PCI_DEVICE_ID_XEN_PVDEVICE),
> +    DEFINE_PROP_UINT8("revision", XenPVDevice, revision, 0x01),
> +    DEFINE_PROP_UINT32("size", XenPVDevice, size, 0x400000),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void xen_pv_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->init = xen_pv_init;
> +    k->class_id = PCI_CLASS_SYSTEM_OTHER;
> +    dc->desc = "Xen PV Device";
> +    dc->props = xen_pv_props;
> +}
> +
> +static const TypeInfo xen_pv_type_info = {
> +    .name          = TYPE_XEN_PV_DEVICE,
> +    .parent        = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(XenPVDevice),
> +    .class_init    = xen_pv_class_init,
> +};
> +
> +static void xen_pv_register_types(void)
> +{
> +    type_register_static(&xen_pv_type_info);
> +}
> +
> +type_init(xen_pv_register_types)
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index d8dc2f1..263bca3 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -142,8 +142,9 @@
>  
>  #define PCI_DEVICE_ID_INTEL_Q35_MCH      0x29c0
>  
> -#define PCI_VENDOR_ID_XEN               0x5853
> -#define PCI_DEVICE_ID_XEN_PLATFORM      0x0001
> +#define PCI_VENDOR_ID_XEN                0x5853
> +#define PCI_DEVICE_ID_XEN_PLATFORM       0x0001
> +#define PCI_DEVICE_ID_XEN_PVDEVICE       0x0002
>  
>  #define PCI_VENDOR_ID_NEC                0x1033
>  #define PCI_DEVICE_ID_NEC_UPD720200      0x0194
> diff --git a/trace-events b/trace-events
> index c5f1ccb..0445853 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1161,3 +1161,7 @@ kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
>  # qom/object.c
>  object_dynamic_cast_assert(const char *type, const char *target, const char *file, int line, const char *func) "%s->%s (%s:%d:%s)"
>  object_class_dynamic_cast_assert(const char *type, const char *target, const char *file, int line, const char *func) "%s->%s (%s:%d:%s)"
> +
> +# hw/xen/xen_pvdevice.c
> +xen_pv_mmio_read(uint64_t addr) "WARNING: read from Xen PV Device MMIO space (address %"PRIx64")"
> +xen_pv_mmio_write(uint64_t addr) "WARNING: write to Xen PV Device MMIO space (address %"PRIx64")"
> -- 
> 1.7.10.4
>

Am 03.07.2013 18:37, schrieb Stefano Stabellini:
> On Wed, 3 Jul 2013, Paul Durrant wrote:
>> This patch introduces a new Xen PV PCI device which will act as a new
>> binding point for PV drivers for Xen.
>> The device has parameterized vendor-id, device-id and revision to allow to
>> be configured as a binding point for any vendor's PV drivers.
>>
>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
>> ---
>>  hw/xen/Makefile.objs     |    1 +
>>  hw/xen/xen_pvdevice.c    |  131 ++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/pci/pci_ids.h |    5 +-
>>  trace-events             |    4 ++
>>  4 files changed, 139 insertions(+), 2 deletions(-)
>>  create mode 100644 hw/xen/xen_pvdevice.c
>>
>> diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
>> index 2017560..fd88003 100644
>> --- a/hw/xen/Makefile.objs
>> +++ b/hw/xen/Makefile.objs
>> @@ -4,3 +4,4 @@ common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o
>>  obj-$(CONFIG_XEN_I386) += xen_platform.o xen_apic.o
>>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
>>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
>> +obj-$(CONFIG_XEN) += xen_pvdevice.o
>> diff --git a/hw/xen/xen_pvdevice.c b/hw/xen/xen_pvdevice.c
>> new file mode 100644
>> index 0000000..dbc4bf5
>> --- /dev/null
>> +++ b/hw/xen/xen_pvdevice.c
>> @@ -0,0 +1,131 @@
>> +/* Copyright (c) Citrix Systems Inc.
>> + * All rights reserved.
> 
> Like Anthony wrote before, All rights reserved contradicts what's
> written below.
> Aside from this, it looks OK to me.
> 
> I would like to see the libxl side patch.
> Also it would be nice to have an ack from Andreas or another QOM expert.

From a QOM view it looks fine now. :) Thanks for inquiring.

Some other comments though:
* Now that it no longer depends on TARGET_PAGE_SIZE, is it possible to
use common-obj-$(CONFIG_XEN)? Then it would build only once rather than
separately for i386 and x86_64 and any future Xen platforms (e.g., arm).
* It looks as if the MMIO functions were renamed - the arguments no
longer align. That could be edited before you apply the patch to your
queue if there's nothing else - then feel free to add my Reviewed-by
independent of the other issue.
* Paolo had asked for new MemoryRegions not to include the device name -
can be renamed once they get the owner field though (not merged yet).
Don't have a better suggestion handy.

Also Paul, by my count this is [PATCH v4] - please use
--subject-prefix="PATCH v5" if you respin and include the change log
either below "---" or in a cover letter. We prefer to see it for patch
review but not in Git commit history.
Similarly, "Introduce a new Xen PV device..." would elegantly avoid
reading "This patch..." after it's been committed. ;)

Regards,
Andreas

> -----Original Message-----
> >
> > Like Anthony wrote before, All rights reserved contradicts what's
> > written below.

Like I said, it's part of all BSD licenses that I can find. It's certainly in the template on the OSI website and the FreeBSD license for instance.

> > Aside from this, it looks OK to me.
> >
> > I would like to see the libxl side patch.
> > Also it would be nice to have an ack from Andreas or another QOM expert.
> 
> From a QOM view it looks fine now. :) Thanks for inquiring.
> 
> Some other comments though:
> * Now that it no longer depends on TARGET_PAGE_SIZE, is it possible to
> use common-obj-$(CONFIG_XEN)? Then it would build only once rather than
> separately for i386 and x86_64 and any future Xen platforms (e.g., arm).

Sure, that sounds sensible.

> * It looks as if the MMIO functions were renamed - the arguments no
> longer align. That could be edited before you apply the patch to your
> queue if there's nothing else - then feel free to add my Reviewed-by
> independent of the other issue.

Thanks.

> * Paolo had asked for new MemoryRegions not to include the device name -
> can be renamed once they get the owner field though (not merged yet).
> Don't have a better suggestion handy.
> 

I guess this can be fixed up later.

> Also Paul, by my count this is [PATCH v4] - please use
> --subject-prefix="PATCH v5" if you respin and include the change log
> either below "---" or in a cover letter. We prefer to see it for patch
> review but not in Git commit history.

Ok. I was unsure what to do since this device was under a different name so I opted to reset the version back to 1. I'll call the next one v5 as you suggest. I'm still finding my way with git so thanks for the tips.

> Similarly, "Introduce a new Xen PV device..." would elegantly avoid
> reading "This patch..." after it's been committed. ;)
> 

Sure. Good point.

  Paul

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 03 July 2013 17:38
> To: Paul Durrant
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano Stabellini;
> afaerber@suse.de
> Subject: Re: [PATCH] Xen PV Device
> 
> On Wed, 3 Jul 2013, Paul Durrant wrote:
> > This patch introduces a new Xen PV PCI device which will act as a new
> > binding point for PV drivers for Xen.
> > The device has parameterized vendor-id, device-id and revision to allow to
> > be configured as a binding point for any vendor's PV drivers.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> > ---
> >  hw/xen/Makefile.objs     |    1 +
> >  hw/xen/xen_pvdevice.c    |  131
> ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/pci/pci_ids.h |    5 +-
> >  trace-events             |    4 ++
> >  4 files changed, 139 insertions(+), 2 deletions(-)
> >  create mode 100644 hw/xen/xen_pvdevice.c
> >
> > diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
> > index 2017560..fd88003 100644
> > --- a/hw/xen/Makefile.objs
> > +++ b/hw/xen/Makefile.objs
> > @@ -4,3 +4,4 @@ common-obj-$(CONFIG_XEN_BACKEND) +=
> xen_backend.o xen_devconfig.o
> >  obj-$(CONFIG_XEN_I386) += xen_platform.o xen_apic.o
> >  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
> >  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o
> xen_pt_config_init.o xen_pt_msi.o
> > +obj-$(CONFIG_XEN) += xen_pvdevice.o
> > diff --git a/hw/xen/xen_pvdevice.c b/hw/xen/xen_pvdevice.c
> > new file mode 100644
> > index 0000000..dbc4bf5
> > --- /dev/null
> > +++ b/hw/xen/xen_pvdevice.c
> > @@ -0,0 +1,131 @@
> > +/* Copyright (c) Citrix Systems Inc.
> > + * All rights reserved.
> 
> Like Anthony wrote before, All rights reserved contradicts what's
> written below.
> Aside from this, it looks OK to me.
> 
> I would like to see the libxl side patch.

Working on it, but it's not required to use the new device so I don't think the QEMU patch need be predicated on it.

  Paul