| Message ID | 1372754385-10186-1-git-send-email-paul.durrant@citrix.com |
|---|---|
| State | New |
| Headers | show |
>>> On 02.07.13 at 10:39, Paul Durrant <paul.durrant@citrix.com> wrote: > --- 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 Is that really the right way to do things, considering that the same value is elsewhere used for PCI_VENDOR_ID_XEN? Jan > +#define PCI_DEVICE_ID_CITRIX_PV_BUS 0x0002 > + > #endif
On Tue, 2013-07-02 at 09:39 +0100, Paul Durrant wrote: > 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. What about <9AAE0902D5BC7E449B7C8E4E778ABCD00537EE@LONPEX01CL01.citrite.net> ?
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 02 July 2013 09:46 > To: Paul Durrant > Cc: xen-devel@lists.xen.org; qemu-devel@nongnu.org > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > >>> On 02.07.13 at 10:39, Paul Durrant <paul.durrant@citrix.com> wrote: > > --- 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 > > Is that really the right way to do things, considering that the same > value is elsewhere used for PCI_VENDOR_ID_XEN? > > Jan > > > +#define PCI_DEVICE_ID_CITRIX_PV_BUS 0x0002 > > + > > #endif > I opted to do this for clarity. The fact that the Xen platform device uses Citrix's vendor ID is historical; I wanted to be clear that this device was distinct. Paul
On Tue, 2013-07-02 at 08:57 +0000, Paul Durrant wrote: > > -----Original Message----- > > From: Jan Beulich [mailto:JBeulich@suse.com] > > Sent: 02 July 2013 09:46 > > To: Paul Durrant > > Cc: xen-devel@lists.xen.org; qemu-devel@nongnu.org > > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > > > >>> On 02.07.13 at 10:39, Paul Durrant <paul.durrant@citrix.com> wrote: > > > --- 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 > > > > Is that really the right way to do things, considering that the same > > value is elsewhere used for PCI_VENDOR_ID_XEN? > > > > Jan > > > > > +#define PCI_DEVICE_ID_CITRIX_PV_BUS 0x0002 > > > + > > > #endif > > > > I opted to do this for clarity. The fact that the Xen platform device > uses Citrix's vendor ID is historical; AFAIR this was XenSource's vendor ID (it is "XS" in ASCII) which was donated to the Xen community. I'll clarify this internally though. > I wanted to be clear that this device was distinct. I think giving two names to the same ID serves only to obfuscate. Ian.
> -----Original Message----- > From: Ian Campbell > Sent: 02 July 2013 10:02 > To: Paul Durrant > Cc: Jan Beulich; qemu-devel@nongnu.org; xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > On Tue, 2013-07-02 at 08:57 +0000, Paul Durrant wrote: > > > -----Original Message----- > > > From: Jan Beulich [mailto:JBeulich@suse.com] > > > Sent: 02 July 2013 09:46 > > > To: Paul Durrant > > > Cc: xen-devel@lists.xen.org; qemu-devel@nongnu.org > > > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > > > > > >>> On 02.07.13 at 10:39, Paul Durrant <paul.durrant@citrix.com> wrote: > > > > --- 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 > > > > > > Is that really the right way to do things, considering that the same > > > value is elsewhere used for PCI_VENDOR_ID_XEN? > > > > > > Jan > > > > > > > +#define PCI_DEVICE_ID_CITRIX_PV_BUS 0x0002 > > > > + > > > > #endif > > > > > > > I opted to do this for clarity. The fact that the Xen platform device > > uses Citrix's vendor ID is historical; > > AFAIR this was XenSource's vendor ID (it is "XS" in ASCII) which was > donated to the Xen community. I'll clarify this internally though. > The PCI SIG has it registered to Citrix. > > I wanted to be clear that this device was distinct. > > I think giving two names to the same ID serves only to obfuscate. > Ok. I don't mind dropping the definition if that's generally preferred. Paul
> -----Original Message----- > From: Ian Campbell > Sent: 02 July 2013 09:57 > To: Paul Durrant > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > On Tue, 2013-07-02 at 09:39 +0100, Paul Durrant wrote: > > 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. > > What about > <9AAE0902D5BC7E449B7C8E4E778ABCD00537EE@LONPEX01CL01.citrite.net> > ? > I had actually coded up a solution based on the existing Xen platform device, by having it synthesize a device ID based on the Xen version to which we could then host the xenbus driver, to allow us to deploy multiple versions of xenbus should compatibility (with things such as the shared info interface) become an issue. The co-installer for this driver could also spot existing PV driver installations and make sure they don't get trashed. This idea was rejected by Citrix product teams though, because we would not be able to prevent any Windows guest without some known PV drivers from downloading our new driver from Windows Update and this was seen as undesirable. Paul
On Tue, 2013-07-02 at 10:14 +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Ian Campbell > > Sent: 02 July 2013 09:57 > > To: Paul Durrant > > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org > > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > > > On Tue, 2013-07-02 at 09:39 +0100, Paul Durrant wrote: > > > 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. > > > > What about > > <9AAE0902D5BC7E449B7C8E4E778ABCD00537EE@LONPEX01CL01.citrite.net> > > ? > > > > I had actually coded up a solution based on the existing Xen platform > device, by having it synthesize a device ID based on the Xen version > to which we could then host the xenbus driver, to allow us to deploy > multiple versions of xenbus should compatibility (with things such as > the shared info interface) become an issue. The co-installer for this > driver could also spot existing PV driver installations and make sure > they don't get trashed. I think only this last bit of functionality is critical here, and it allows us to avoid having to carry multiple platform devices in upstream, doesn't it? > This idea was rejected by Citrix product teams though, because we > would not be able to prevent any Windows guest without some known PV > drivers from downloading our new driver from Windows Update and this > was seen as undesirable. Well, if your product requirements are at odds with doing the right thing upstream then I think it would be best for you to just carry patches to make XS behave how you want. I hope we can find a suitable compromise though. Ian.
At 10:56 +0100 on 02 Jul (1372762607), Ian Campbell wrote: > On Tue, 2013-07-02 at 10:14 +0100, Paul Durrant wrote: > > I had actually coded up a solution based on the existing Xen platform > > device, by having it synthesize a device ID based on the Xen version > > to which we could then host the xenbus driver, to allow us to deploy > > multiple versions of xenbus should compatibility (with things such as > > the shared info interface) become an issue. The co-installer for this > > driver could also spot existing PV driver installations and make sure > > they don't get trashed. > > I think only this last bit of functionality is critical here, and it > allows us to avoid having to carry multiple platform devices in > upstream, doesn't it? > > > This idea was rejected by Citrix product teams though, because we > > would not be able to prevent any Windows guest without some known PV > > drivers from downloading our new driver from Windows Update and this > > was seen as undesirable. > > Well, if your product requirements are at odds with doing the right > thing upstream then I think it would be best for you to just carry > patches to make XS behave how you want. I think it's a reasonable aim to have the WU drivers not spontaneously appear on VMs (on all Xen hosts, remember) where the admin has chosen not to install PV drivers. Generally, the more I think about this the more I'm convinced that _not_ installing the drivers on any existing systems without explicit permission is the most important thing. > I hope we can find a suitable compromise though. Well, the WU drivers could refuse to install except as upgrade to themselves (i.e. fail if there's any unknown driver bound to the xen platform device, and also fail if there's _no_ driver bound). Then the guest admin can choose to install the drivers by hand and get automatic updates after that. XS, XC and anyone else who chooses could carry a separate patch that changes the default to 'install if there are no drivers', signalling over xenstore, or ACPI, or a Windows domain policy, or whatever. Tim.
On Tue, 2013-07-02 at 11:15 +0100, Tim Deegan wrote: > At 10:56 +0100 on 02 Jul (1372762607), Ian Campbell wrote: > > On Tue, 2013-07-02 at 10:14 +0100, Paul Durrant wrote: > > > I had actually coded up a solution based on the existing Xen platform > > > device, by having it synthesize a device ID based on the Xen version > > > to which we could then host the xenbus driver, to allow us to deploy > > > multiple versions of xenbus should compatibility (with things such as > > > the shared info interface) become an issue. The co-installer for this > > > driver could also spot existing PV driver installations and make sure > > > they don't get trashed. > > > > I think only this last bit of functionality is critical here, and it > > allows us to avoid having to carry multiple platform devices in > > upstream, doesn't it? > > > > > This idea was rejected by Citrix product teams though, because we > > > would not be able to prevent any Windows guest without some known PV > > > drivers from downloading our new driver from Windows Update and this > > > was seen as undesirable. > > > > Well, if your product requirements are at odds with doing the right > > thing upstream then I think it would be best for you to just carry > > patches to make XS behave how you want. > > I think it's a reasonable aim to have the WU drivers not spontaneously > appear on VMs (on all Xen hosts, remember) where the admin has chosen > not to install PV drivers. > > Generally, the more I think about this the more I'm convinced that _not_ > installing the drivers on any existing systems without explicit > permission is the most important thing. Will WU install a completely fresh driver for a new (or indeed old) bit of hardware on native entirely without prompting? I'd have expected the old "Windows has found a driver for your device" dance. > > I hope we can find a suitable compromise though. > > Well, the WU drivers could refuse to install except as upgrade to > themselves (i.e. fail if there's any unknown driver bound to the xen > platform device, and also fail if there's _no_ driver bound). Then the > guest admin can choose to install the drivers by hand and get automatic > updates after that. That sounds reasonable. However I thought part of the point of getting things into WU was then that they could be "inbox" (either figuratively or literally) such that they would be installed by the Windows installer. Perhaps that's a separate thing though. > XS, XC and anyone else who chooses could carry a separate patch that > changes the default to 'install if there are no drivers', signalling > over xenstore, or ACPI, or a Windows domain policy, or whatever. Right.
> -----Original Message----- > From: Ian Campbell > Sent: 02 July 2013 11:24 > To: Tim (Xen.org) > Cc: Paul Durrant; qemu-devel@nongnu.org; xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > On Tue, 2013-07-02 at 11:15 +0100, Tim Deegan wrote: > > At 10:56 +0100 on 02 Jul (1372762607), Ian Campbell wrote: > > > On Tue, 2013-07-02 at 10:14 +0100, Paul Durrant wrote: > > > > I had actually coded up a solution based on the existing Xen platform > > > > device, by having it synthesize a device ID based on the Xen version > > > > to which we could then host the xenbus driver, to allow us to deploy > > > > multiple versions of xenbus should compatibility (with things such as > > > > the shared info interface) become an issue. The co-installer for this > > > > driver could also spot existing PV driver installations and make sure > > > > they don't get trashed. > > > > > > I think only this last bit of functionality is critical here, and it > > > allows us to avoid having to carry multiple platform devices in > > > upstream, doesn't it? > > > > > > > This idea was rejected by Citrix product teams though, because we > > > > would not be able to prevent any Windows guest without some known > PV > > > > drivers from downloading our new driver from Windows Update and > this > > > > was seen as undesirable. > > > > > > Well, if your product requirements are at odds with doing the right > > > thing upstream then I think it would be best for you to just carry > > > patches to make XS behave how you want. > > > > I think it's a reasonable aim to have the WU drivers not spontaneously > > appear on VMs (on all Xen hosts, remember) where the admin has chosen > > not to install PV drivers. > > > > Generally, the more I think about this the more I'm convinced that _not_ > > installing the drivers on any existing systems without explicit > > permission is the most important thing. > > Will WU install a completely fresh driver for a new (or indeed old) bit > of hardware on native entirely without prompting? > > I'd have expected the old "Windows has found a driver for your device" > dance. > > > > I hope we can find a suitable compromise though. > > > > Well, the WU drivers could refuse to install except as upgrade to > > themselves (i.e. fail if there's any unknown driver bound to the xen > > platform device, and also fail if there's _no_ driver bound). Then the > > guest admin can choose to install the drivers by hand and get automatic > > updates after that. > > That sounds reasonable. However I thought part of the point of getting > things into WU was then that they could be "inbox" (either figuratively > or literally) such that they would be installed by the Windows > installer. Perhaps that's a separate thing though. > No, that is the eventual aim so I don't think the 'upgrade only' options is really future-proof. > > XS, XC and anyone else who chooses could carry a separate patch that > > changes the default to 'install if there are no drivers', signalling > > over xenstore, or ACPI, or a Windows domain policy, or whatever. > > Right. > Surely having a new device for the purposes of hosting Citrix PV drivers is a cleaner option for opting in? Note that I'm not proposing the new device displaces the existing platform device in any way. Paul
> -----Original Message----- > From: Ian Campbell > Sent: 02 July 2013 11:24 > To: Tim (Xen.org) > Cc: Paul Durrant; qemu-devel@nongnu.org; xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > On Tue, 2013-07-02 at 11:15 +0100, Tim Deegan wrote: > > At 10:56 +0100 on 02 Jul (1372762607), Ian Campbell wrote: > > > On Tue, 2013-07-02 at 10:14 +0100, Paul Durrant wrote: > > > > I had actually coded up a solution based on the existing Xen platform > > > > device, by having it synthesize a device ID based on the Xen version > > > > to which we could then host the xenbus driver, to allow us to deploy > > > > multiple versions of xenbus should compatibility (with things such as > > > > the shared info interface) become an issue. The co-installer for this > > > > driver could also spot existing PV driver installations and make sure > > > > they don't get trashed. > > > > > > I think only this last bit of functionality is critical here, and it > > > allows us to avoid having to carry multiple platform devices in > > > upstream, doesn't it? > > > > > > > This idea was rejected by Citrix product teams though, because we > > > > would not be able to prevent any Windows guest without some known > PV > > > > drivers from downloading our new driver from Windows Update and > this > > > > was seen as undesirable. > > > > > > Well, if your product requirements are at odds with doing the right > > > thing upstream then I think it would be best for you to just carry > > > patches to make XS behave how you want. > > > > I think it's a reasonable aim to have the WU drivers not spontaneously > > appear on VMs (on all Xen hosts, remember) where the admin has chosen > > not to install PV drivers. > > > > Generally, the more I think about this the more I'm convinced that _not_ > > installing the drivers on any existing systems without explicit > > permission is the most important thing. > > Will WU install a completely fresh driver for a new (or indeed old) bit > of hardware on native entirely without prompting? > I believe it can be configured to do so. > I'd have expected the old "Windows has found a driver for your device" > dance. > > > > I hope we can find a suitable compromise though. > > I still think the extra device is the cleanest solution, and I have gone through all the alternatives I can think of. Paul
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. :/ > 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 >
On Tue, 2013-07-02 at 11:31 +0100, Paul Durrant wrote: > > > XS, XC and anyone else who chooses could carry a separate patch that > > > changes the default to 'install if there are no drivers', signalling > > > over xenstore, or ACPI, or a Windows domain policy, or whatever. > > > > Right. > > > > Surely having a new device for the purposes of hosting Citrix PV > drivers is a cleaner option for opting in? Note that I'm not proposing > the new device displaces the existing platform device in any way. A dislike for having to coordinate guest internal and guest external configuration changes in this way has been expressed by several people. However if you only intend to support your drivers on XenServer (and it is starting to seem like your constraints may end up forcing this option upon you) then you can equally well carry that new device in your patches too and manage it however your PMs require. Ian.
At 10:31 +0000 on 02 Jul (1372761105), Paul Durrant wrote: > > > Well, the WU drivers could refuse to install except as upgrade to > > > themselves (i.e. fail if there's any unknown driver bound to the xen > > > platform device, and also fail if there's _no_ driver bound). Then the > > > guest admin can choose to install the drivers by hand and get automatic > > > updates after that. > > > > That sounds reasonable. However I thought part of the point of getting > > things into WU was then that they could be "inbox" (either figuratively > > or literally) such that they would be installed by the Windows > > installer. Perhaps that's a separate thing though. > > > > No, that is the eventual aim so I don't think the 'upgrade only' > options is really future-proof. Well, you can have them install by default on platforms that want it, or on new enough Xen versions. Or, even better, on new enough _windows_ versions. > > > XS, XC and anyone else who chooses could carry a separate patch that > > > changes the default to 'install if there are no drivers', signalling > > > over xenstore, or ACPI, or a Windows domain policy, or whatever. > > > > Right. > > > > Surely having a new device for the purposes of hosting Citrix PV > drivers is a cleaner option for opting in? Only if it's OK that the _host_ admin has to be involved (which was the original objection). Upgrade-only but hooked to the existing ID lets a guest admin install the drivers manually without plumbing it through $CLOUDPROVIDER's toolstack, and without having it appear suddenly on existing VMs in the dead of night. If you could have it bind to either device then I guess a second PCI ID is another way of signalling that policy, but we should probably use one of the many other channels we have from the tools to the guest. Tim.
> -----Original Message----- > From: Andreas Färber [mailto:afaerber@suse.de] > Sent: 02 July 2013 11:46 > To: Paul Durrant > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Paolo Bonzini; > Michael S. Tsirkin > Subject: Re: [Qemu-devel] [PATCH] Citrix PV Bus device > > 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. :/ > > > 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. > Andreas, Thanks for the review. I'll modify that name as you suggest. > > + PCIDevice dev; > > /*< private >*/ > PCIDevice parent_obj; > /*< public >*/ > > please. More although still incomplete guidelines at: > > http://wiki.qemu.org/QOMConventions > Thanks for the pointer. > > + 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? > I'm just looking at tracepoints now. That does seem like a cleaner approach. > > + > > + 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. > Ok. > > + 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? > Yes, further devices will be enumerated by the driver that binds to the device in Xen HVM domains. The MMIO region is there to reserve address space for 'special' pages such as the Xen grant table so any access to the region that makes it through to the device emulation code is actually a bug in the guest - hence the warning. Paul
On Tue, Jul 02, 2013 at 11:48:46AM +0100, Ian Campbell wrote: > On Tue, 2013-07-02 at 11:31 +0100, Paul Durrant wrote: > > > > XS, XC and anyone else who chooses could carry a separate patch that > > > > changes the default to 'install if there are no drivers', signalling > > > > over xenstore, or ACPI, or a Windows domain policy, or whatever. > > > > > > Right. > > > > > > > Surely having a new device for the purposes of hosting Citrix PV > > drivers is a cleaner option for opting in? Note that I'm not proposing > > the new device displaces the existing platform device in any way. > > A dislike for having to coordinate guest internal and guest external > configuration changes in this way has been expressed by several people. > > However if you only intend to support your drivers on XenServer (and it > is starting to seem like your constraints may end up forcing this option > upon you) then you can equally well carry that new device in your > patches too and manage it however your PMs require. > It's also good to remember the Citrix XenServer Windows PV drivers are now opensource, and (more) easily usable on non-XenServer environments aswell. I hope they'd work on upstream Xen aswell. -- Pasi
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 >> > >
On Tue, 2013-07-02 at 11:49 +0100, Tim Deegan wrote: > At 10:31 +0000 on 02 Jul (1372761105), Paul Durrant wrote: > > > > Well, the WU drivers could refuse to install except as upgrade to > > > > themselves (i.e. fail if there's any unknown driver bound to the xen > > > > platform device, and also fail if there's _no_ driver bound). Then the > > > > guest admin can choose to install the drivers by hand and get automatic > > > > updates after that. > > > > > > That sounds reasonable. However I thought part of the point of getting > > > things into WU was then that they could be "inbox" (either figuratively > > > or literally) such that they would be installed by the Windows > > > installer. Perhaps that's a separate thing though. > > > > > > > No, that is the eventual aim so I don't think the 'upgrade only' > > options is really future-proof. > > Well, you can have them install by default on platforms that want it, or > on new enough Xen versions. Or, even better, on new enough _windows_ > versions. > > > > > XS, XC and anyone else who chooses could carry a separate patch that > > > > changes the default to 'install if there are no drivers', signalling > > > > over xenstore, or ACPI, or a Windows domain policy, or whatever. > > > > > > Right. > > > > > > > Surely having a new device for the purposes of hosting Citrix PV > > drivers is a cleaner option for opting in? > > Only if it's OK that the _host_ admin has to be involved (which was the > original objection). Upgrade-only but hooked to the existing ID lets a > guest admin install the drivers manually without plumbing it through > $CLOUDPROVIDER's toolstack, and without having it appear suddenly on > existing VMs in the dead of night. I think part of the problem here is that it is unclear who the target audience for these drivers are. Paul, are you intending that these drivers be only for XenServer users or are you intending for them to be used by the broader community on a variety of different Xen platforms? Ian.
> -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: 02 July 2013 11:54 > To: Andreas Färber > Cc: Paul Durrant; qemu-devel@nongnu.org; xen-devel@lists.xen.org; > Michael S. Tsirkin > Subject: Re: [Qemu-devel] [PATCH] Citrix PV Bus device > > 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? > It doesn't necessarily need to be in pages; it's just a more convenient quantity than bytes. Since it needs to be a power of 2 I could perhaps use an 'order' parameter instead? Paul
Il 02/07/2013 12:57, Paul Durrant ha scritto: >>> 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? > > It doesn't necessarily need to be in pages; it's just a more > convenient quantity than bytes. Since it needs to be a power of 2 I > could perhaps use an 'order' parameter instead? I would just use bytes, the power-of-2 requirement can be checked in the init function (actually it would just be caught by pci_register_bar). Paolo
On 2 July 2013 11:57, Paul Durrant <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >> > 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. :/ We probably don't, because it generally doesn't mean what you think it does. It's the smallest possible page size the guest CPU supports, which may not be the same as the actual page size the guest OS is using. >> Why does it need to be in pages rather than bytes? > It doesn't necessarily need to be in pages; it's just a more > convenient quantity than bytes. It isn't really more convienient, because the guest would have to tell QEMU what the page size was. (I'm told that virtio is planning to move to a simple "just use a byte count" approach.) thanks -- PMM
> -----Original Message----- > From: Ian Campbell > Sent: 02 July 2013 11:57 > To: Tim (Xen.org) > Cc: Paul Durrant; qemu-devel@nongnu.org; xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > On Tue, 2013-07-02 at 11:49 +0100, Tim Deegan wrote: > > At 10:31 +0000 on 02 Jul (1372761105), Paul Durrant wrote: > > > > > Well, the WU drivers could refuse to install except as upgrade to > > > > > themselves (i.e. fail if there's any unknown driver bound to the xen > > > > > platform device, and also fail if there's _no_ driver bound). Then the > > > > > guest admin can choose to install the drivers by hand and get > automatic > > > > > updates after that. > > > > > > > > That sounds reasonable. However I thought part of the point of getting > > > > things into WU was then that they could be "inbox" (either figuratively > > > > or literally) such that they would be installed by the Windows > > > > installer. Perhaps that's a separate thing though. > > > > > > > > > > No, that is the eventual aim so I don't think the 'upgrade only' > > > options is really future-proof. > > > > Well, you can have them install by default on platforms that want it, or > > on new enough Xen versions. Or, even better, on new enough _windows_ > > versions. > > > > > > > XS, XC and anyone else who chooses could carry a separate patch that > > > > > changes the default to 'install if there are no drivers', signalling > > > > > over xenstore, or ACPI, or a Windows domain policy, or whatever. > > > > > > > > Right. > > > > > > > > > > Surely having a new device for the purposes of hosting Citrix PV > > > drivers is a cleaner option for opting in? > > > > Only if it's OK that the _host_ admin has to be involved (which was the > > original objection). Upgrade-only but hooked to the existing ID lets a > > guest admin install the drivers manually without plumbing it through > > $CLOUDPROVIDER's toolstack, and without having it appear suddenly on > > existing VMs in the dead of night. > > I think part of the problem here is that it is unclear who the target > audience for these drivers are. > > Paul, are you intending that these drivers be only for XenServer users > or are you intending for them to be used by the broader community on a > variety of different Xen platforms? > My intention is that the drivers are widely available to all who want them, but the key word there is 'want'. No-one should get a surprise when we publish to Windows Update so having the drivers bind to a new device which can then be added to the VM config seems like the cleanest solution. I realise that this involves the VM provider having to do something to enable a VM to get drivers from Windows Update rather that the guest admin, but I think that's actually the right way to do it. Adding this device into a VM is essentially enabling new functionality in the same way that adding a new network device or storage device would be. Paul
> -----Original Message----- > From: Tim Deegan [mailto:tim@xen.org] > Sent: 02 July 2013 11:50 > To: Paul Durrant > Cc: Ian Campbell; qemu-devel@nongnu.org; xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > At 10:31 +0000 on 02 Jul (1372761105), Paul Durrant wrote: > > > > Well, the WU drivers could refuse to install except as upgrade to > > > > themselves (i.e. fail if there's any unknown driver bound to the xen > > > > platform device, and also fail if there's _no_ driver bound). Then the > > > > guest admin can choose to install the drivers by hand and get automatic > > > > updates after that. > > > > > > That sounds reasonable. However I thought part of the point of getting > > > things into WU was then that they could be "inbox" (either figuratively > > > or literally) such that they would be installed by the Windows > > > installer. Perhaps that's a separate thing though. > > > > > > > No, that is the eventual aim so I don't think the 'upgrade only' > > options is really future-proof. > > Well, you can have them install by default on platforms that want it, or > on new enough Xen versions. Or, even better, on new enough _windows_ > versions. > > > > > XS, XC and anyone else who chooses could carry a separate patch that > > > > changes the default to 'install if there are no drivers', signalling > > > > over xenstore, or ACPI, or a Windows domain policy, or whatever. > > > > > > Right. > > > > > > > Surely having a new device for the purposes of hosting Citrix PV > > drivers is a cleaner option for opting in? > > Only if it's OK that the _host_ admin has to be involved (which was the > original objection). Upgrade-only but hooked to the existing ID lets a > guest admin install the drivers manually without plumbing it through > $CLOUDPROVIDER's toolstack, and without having it appear suddenly on > existing VMs in the dead of night. > But for new VMs we *want* the drivers to download and install automatically; we just don't want them to do that for existing VMs. Paul
On Tue, 2013-07-02 at 13:35 +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Ian Campbell > > Sent: 02 July 2013 11:57 > > To: Tim (Xen.org) > > Cc: Paul Durrant; qemu-devel@nongnu.org; xen-devel@lists.xen.org > > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > > > On Tue, 2013-07-02 at 11:49 +0100, Tim Deegan wrote: > > > At 10:31 +0000 on 02 Jul (1372761105), Paul Durrant wrote: > > > > > > Well, the WU drivers could refuse to install except as upgrade to > > > > > > themselves (i.e. fail if there's any unknown driver bound to the xen > > > > > > platform device, and also fail if there's _no_ driver bound). Then the > > > > > > guest admin can choose to install the drivers by hand and get > > automatic > > > > > > updates after that. > > > > > > > > > > That sounds reasonable. However I thought part of the point of getting > > > > > things into WU was then that they could be "inbox" (either figuratively > > > > > or literally) such that they would be installed by the Windows > > > > > installer. Perhaps that's a separate thing though. > > > > > > > > > > > > > No, that is the eventual aim so I don't think the 'upgrade only' > > > > options is really future-proof. > > > > > > Well, you can have them install by default on platforms that want it, or > > > on new enough Xen versions. Or, even better, on new enough _windows_ > > > versions. > > > > > > > > > XS, XC and anyone else who chooses could carry a separate patch that > > > > > > changes the default to 'install if there are no drivers', signalling > > > > > > over xenstore, or ACPI, or a Windows domain policy, or whatever. > > > > > > > > > > Right. > > > > > > > > > > > > > Surely having a new device for the purposes of hosting Citrix PV > > > > drivers is a cleaner option for opting in? > > > > > > Only if it's OK that the _host_ admin has to be involved (which was the > > > original objection). Upgrade-only but hooked to the existing ID lets a > > > guest admin install the drivers manually without plumbing it through > > > $CLOUDPROVIDER's toolstack, and without having it appear suddenly on > > > existing VMs in the dead of night. > > > > I think part of the problem here is that it is unclear who the target > > audience for these drivers are. > > > > Paul, are you intending that these drivers be only for XenServer users > > or are you intending for them to be used by the broader community on a > > variety of different Xen platforms? > > > > My intention is that the drivers are widely available to all who want > them, but the key word there is 'want'. No-one should get a surprise > when we publish to Windows Update so having the drivers bind to a new > device which can then be added to the VM config seems like the > cleanest solution. Actually, it occurs to me now (and I have a feeling Tim was trying to say this and I didn't get it): It's not really appropriate for any individual vendor to upload a driver to Windows Update which binds to the default platform device ID anyway. There are several other sets of Xen PV drivers out there (including the GPL PV drivers and various companies commercial/proprietary drivers) and having one particular set of drivers in WU puts all of them at a disadvantage. Before doing that we would need consensus behind the particular set of drivers, which I don't think any of them currently have. All of which means that you do need your own device ID, for which you can upload support to WU. However I don't think it therefore follows that you need that device ID to be supported by upstream. Does this work: We assign a device ID to your drivers (and we would do the same for any vendor). You can then upload drivers supporting that ID to WU and arrange within your product to create the appropriate device. At the same time you produce a setup.exe installer which will install those same drivers but also sets up (or includes) the binding to the existing device IDs. So anyone running on XenServer gets the driver direct from WU and you can define your own policies around what that means and what the upgrade path and ties to the platform mean etc. People who want to use your drivers on non-XenServer platforms can choose to do so by manually installing from within the guest, with no need to modify their guest configuration. Does that make sense? Ian.
> -----Original Message----- > From: Ian Campbell > Sent: 02 July 2013 13:44 > To: Paul Durrant > Cc: Tim (Xen.org); qemu-devel@nongnu.org; xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > On Tue, 2013-07-02 at 13:35 +0100, Paul Durrant wrote: > > > -----Original Message----- > > > From: Ian Campbell > > > Sent: 02 July 2013 11:57 > > > To: Tim (Xen.org) > > > Cc: Paul Durrant; qemu-devel@nongnu.org; xen-devel@lists.xen.org > > > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > > > > > On Tue, 2013-07-02 at 11:49 +0100, Tim Deegan wrote: > > > > At 10:31 +0000 on 02 Jul (1372761105), Paul Durrant wrote: > > > > > > > Well, the WU drivers could refuse to install except as upgrade to > > > > > > > themselves (i.e. fail if there's any unknown driver bound to the > xen > > > > > > > platform device, and also fail if there's _no_ driver bound). Then > the > > > > > > > guest admin can choose to install the drivers by hand and get > > > automatic > > > > > > > updates after that. > > > > > > > > > > > > That sounds reasonable. However I thought part of the point of > getting > > > > > > things into WU was then that they could be "inbox" (either > figuratively > > > > > > or literally) such that they would be installed by the Windows > > > > > > installer. Perhaps that's a separate thing though. > > > > > > > > > > > > > > > > No, that is the eventual aim so I don't think the 'upgrade only' > > > > > options is really future-proof. > > > > > > > > Well, you can have them install by default on platforms that want it, or > > > > on new enough Xen versions. Or, even better, on new enough > _windows_ > > > > versions. > > > > > > > > > > > XS, XC and anyone else who chooses could carry a separate patch > that > > > > > > > changes the default to 'install if there are no drivers', signalling > > > > > > > over xenstore, or ACPI, or a Windows domain policy, or whatever. > > > > > > > > > > > > Right. > > > > > > > > > > > > > > > > Surely having a new device for the purposes of hosting Citrix PV > > > > > drivers is a cleaner option for opting in? > > > > > > > > Only if it's OK that the _host_ admin has to be involved (which was the > > > > original objection). Upgrade-only but hooked to the existing ID lets a > > > > guest admin install the drivers manually without plumbing it through > > > > $CLOUDPROVIDER's toolstack, and without having it appear suddenly > on > > > > existing VMs in the dead of night. > > > > > > I think part of the problem here is that it is unclear who the target > > > audience for these drivers are. > > > > > > Paul, are you intending that these drivers be only for XenServer users > > > or are you intending for them to be used by the broader community on a > > > variety of different Xen platforms? > > > > > > > My intention is that the drivers are widely available to all who want > > them, but the key word there is 'want'. No-one should get a surprise > > when we publish to Windows Update so having the drivers bind to a new > > device which can then be added to the VM config seems like the > > cleanest solution. > > Actually, it occurs to me now (and I have a feeling Tim was trying to > say this and I didn't get it): It's not really appropriate for any > individual vendor to upload a driver to Windows Update which binds to > the default platform device ID anyway. > > There are several other sets of Xen PV drivers out there (including the > GPL PV drivers and various companies commercial/proprietary drivers) and > having one particular set of drivers in WU puts all of them at a > disadvantage. Before doing that we would need consensus behind the > particular set of drivers, which I don't think any of them currently > have. > > All of which means that you do need your own device ID, for which you > can upload support to WU. However I don't think it therefore follows > that you need that device ID to be supported by upstream. > > Does this work: > > We assign a device ID to your drivers (and we would do the same for any > vendor). You can then upload drivers supporting that ID to WU and > arrange within your product to create the appropriate device. > > At the same time you produce a setup.exe installer which will install > those same drivers but also sets up (or includes) the binding to the > existing device IDs. > > So anyone running on XenServer gets the driver direct from WU and you > can define your own policies around what that means and what the upgrade > path and ties to the platform mean etc. People who want to use your > drivers on non-XenServer platforms can choose to do so by manually > installing from within the guest, with no need to modify their guest > configuration. > > Does that make sense? > Yes, that makes sense *but* I would still like to avoid carrying a private patch to QEMU (and potentially have to keep rebasing it), hence my posting the patch the qemu-devel. Having the code in QEMU does no harm, clearly reserves the device id, and any VM provider (with a suitable version of QEMU on their host) can then enable the device should they wish. Am I missing something? Paul
--On 2 July 2013 13:43:38 +0100 Ian Campbell <Ian.Campbell@citrix.com> wrote: > We assign a device ID to your drivers (and we would do the same for any > vendor). You can then upload drivers supporting that ID to WU and > arrange within your product to create the appropriate device. Let's say we do this, then Xirtic also produce PV drivers, and get assigned their own device ID. Will both of them be visible with a different device ID for the same device? Will windows cope with that? Or do we need to mediate which device IDs are exposed?
On Tue, 2013-07-02 at 14:36 +0100, Alex Bligh wrote: > > --On 2 July 2013 13:43:38 +0100 Ian Campbell <Ian.Campbell@citrix.com> > wrote: > > > We assign a device ID to your drivers (and we would do the same for any > > vendor). You can then upload drivers supporting that ID to WU and > > arrange within your product to create the appropriate device. > > Let's say we do this, then Xirtic also produce PV drivers, and get > assigned their own device ID. Will both of them be visible with > a different device ID for the same device? Different device ID for the same device isn't possible, a device has exactly one device ID. I assume you mean two devices visible one with each ID? > Will windows cope with that? > Or do we need to mediate which device IDs are exposed? I'd expect that only Xirtic products would provide a device with that device ID. They may choose to make it mutually exclusive with the standard device, or they may choose to allow it to co-exist. In the latter case it is down to them what the behaviour of their drivers is. In any case this is all down to the people who make that product and their requirements etc. For people running Xirtic drivers on non-Xirtic products the rest of my mail applies -- IOW those users would use a setup.exe which binds those drivers to the normal device, there is no Xirtic device ID present anywhere in this scenario. Ian.
On Tue, 2013-07-02 at 13:51 +0100, Paul Durrant wrote: > Yes, that makes sense *but* I would still like to avoid carrying a > private patch to QEMU (and potentially have to keep rebasing it), It's small and pretty self contained I think. Someone (Anthony?) recommended making it subclass the existing one, which ought to reduce the size of the patch to be maintained even further I think. > hence my posting the patch the qemu-devel. Having the code in QEMU > does no harm, clearly reserves the device id, FWIW I don't think QEMU should be the registry of these device IDs. Can you create a file somewhere in the Xen source base to serve as the registry, probably somewhere under xen/include/public. We should probably have some sort of scheme. How about we declare the topmost available bit of the device id to be the "vendor specific" bit? We could split the dev id into N-bits of "vendor" and M-bits of device, or add a "locally administered" bit, but that might be overkill? > and any VM provider (with a suitable version of QEMU on their host) > can then enable the device should they wish. I imagine they would be worried about you pushing new drivers which depend on new XenServer features. So unless they also intend to implement (and track) your version compatibility xenstore keys (or whatever mechanism) I'm not sure why they would want to do such a thing. Or are you proposing to maintain forward and backward compatibility? i.e. based on feature flags and negotiation with backends rather than versioned platform devices or other XenServer specific mechanisms? In any case they could also apply the patch. If it turns out that lots of people are doing so then maybe that is the time to consider it for upstream. But in the meantime this avoids anyone other than XenServer having to think about policy and/or mechanism WRT multiple platform devices. > Am I missing something? > > Paul
> -----Original Message----- > From: Ian Campbell > Sent: 02 July 2013 14:43 > To: Paul Durrant > Cc: Tim (Xen.org); qemu-devel@nongnu.org; xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > On Tue, 2013-07-02 at 13:51 +0100, Paul Durrant wrote: > > Yes, that makes sense *but* I would still like to avoid carrying a > > private patch to QEMU (and potentially have to keep rebasing it), > > It's small and pretty self contained I think. > It still has external interfaces, to tracing, QOM, etc that may change. > FWIW I don't think QEMU should be the registry of these device IDs. Can > you create a file somewhere in the Xen source base to serve as the > registry, probably somewhere under xen/include/public. I'll look at how best to do this. > > We should probably have some sort of scheme. How about we declare the > topmost available bit of the device id to be the "vendor specific" bit? > We could split the dev id into N-bits of "vendor" and M-bits of device, > or add a "locally administered" bit, but that might be overkill? > > > and any VM provider (with a suitable version of QEMU on their host) > > can then enable the device should they wish. > > I imagine they would be worried about you pushing new drivers which > depend on new XenServer features. So unless they also intend to > implement (and track) your version compatibility xenstore keys (or > whatever mechanism) I'm not sure why they would want to do such a thing. We already have people interested in doing this and we would look to QA in those non-XenServer environments, to ensure compatibility. > Or are you proposing to maintain forward and backward compatibility? > i.e. based on feature flags and negotiation with backends rather than > versioned platform devices or other XenServer specific mechanisms? > In general yes. Modifying the platform device revision is a mechanism of last resort (because of the inconvenience it causes) but one that we may still need at some point. > In any case they could also apply the patch. If it turns out that lots > of people are doing so then maybe that is the time to consider it for > upstream. > We already have interested parties and the device not being available upstream is an inconvenience. Paul
> -----Original Message----- > > FWIW I don't think QEMU should be the registry of these device IDs. Can > you create a file somewhere in the Xen source base to serve as the > registry, probably somewhere under xen/include/public. > Is the Xen source base actually the right place? The vendor ID belongs to Citrix. Does the PCI SIG hold a list of known device IDs for each vendor? If not then I guess we need some canonical place within Citrix to make sure device ids do not get re-used. Paul
On Tue, 2013-07-02 at 15:08 +0100, Paul Durrant wrote: > > -----Original Message----- > > > > FWIW I don't think QEMU should be the registry of these device IDs. Can > > you create a file somewhere in the Xen source base to serve as the > > registry, probably somewhere under xen/include/public. > > > > Is the Xen source base actually the right place? The vendor ID belongs > to Citrix. So far as I know XenSource gave this ID over for use by the Xen project and nothing about the acquisition will have changed that. So although the ID may technically be owned by Citrix it is in effect managed by the Xen project and "belongs" to Xen. We need to clarify this internally. > Does the PCI SIG hold a list of known device IDs for each vendor? If > not then I guess we need some canonical place within Citrix to make > sure device ids do not get re-used. > > Paul
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. > + * > + * 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
> -----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
Ian, --On 2 July 2013 14:42:41 +0100 Ian Campbell <Ian.Campbell@citrix.com> wrote: >> Let's say we do this, then Xirtic also produce PV drivers, and get >> assigned their own device ID. Will both of them be visible with >> a different device ID for the same device? > > Different device ID for the same device isn't possible, a device has > exactly one device ID. I assume you mean two devices visible one with > each ID? Yes. Two PCI devices talking to the same underlying thing. >> Will windows cope with that? >> Or do we need to mediate which device IDs are exposed? > > I'd expect that only Xirtic products would provide a device with that > device ID. They may choose to make it mutually exclusive with the > standard device, or they may choose to allow it to co-exist. In the > latter case it is down to them what the behaviour of their drivers is. > In any case this is all down to the people who make that product and > their requirements etc. > > For people running Xirtic drivers on non-Xirtic products the rest of my > mail applies -- IOW those users would use a setup.exe which binds those > drivers to the normal device, there is no Xirtic device ID present > anywhere in this scenario. OK. The thing I was suggesting we try to avoid was Citrix drivers binding to PCI-ID A and Xirtic drivers binding to PCI-ID B, and them both trying to work at once. Perhaps I should stop worrying about this as I'm finding it really hard to resist typing 'so don't use Windows' :-)
On Tue, 2013-07-02 at 10:05, Paul Durrant wrote: > > -----Original Message----- > > From: Ian Campbell > > Sent: 02 July 2013 10:02 > > To: Paul Durrant > > Cc: Jan Beulich; qemu-devel@nongnu.org; xen-devel@lists.xen.org > > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > > > On Tue, 2013-07-02 at 08:57 +0000, Paul Durrant wrote: > > > > -----Original Message----- > > > > From: Jan Beulich [mailto:JBeulich@suse.com] > > > > Sent: 02 July 2013 09:46 > > > > To: Paul Durrant > > > > Cc: xen-devel@lists.xen.org; qemu-devel@nongnu.org > > > > Subject: Re: [Xen-devel] [PATCH] Citrix PV Bus device > > > > > > > > >>> On 02.07.13 at 10:39, Paul Durrant <paul.durrant@citrix.com> > wrote: > > > > > --- 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 > > > > > > > > Is that really the right way to do things, considering that the same > > > > value is elsewhere used for PCI_VENDOR_ID_XEN? > > > > > > > > Jan > > > > > > > > > +#define PCI_DEVICE_ID_CITRIX_PV_BUS 0x0002 > > > > > + > > > > > #endif > > > > > > > > > > I opted to do this for clarity. The fact that the Xen platform device > > > uses Citrix's vendor ID is historical; > > > > AFAIR this was XenSource's vendor ID (it is "XS" in ASCII) which was > > donated to the Xen community. I'll clarify this internally though. > > > > The PCI SIG has it registered to Citrix. To be clear: the Xen PCI vendor ID (0x5853) is currently registered by Citrix on behalf of the Xen community. The ID is widely used in the community including within Linux and various vendor and community drivers for other OSs. Citrix has no intention to change this and will continue to fund this community resource through its PCI-SIG membership. Cheers, James (registered primary contact for Citrix's PCI-SIG membership) -- James Bulpin Senior Director, Technology, XenServer Citrix
On Tue, Jul 02, 2013 at 12:10:17PM +0100, Peter Maydell wrote: > On 2 July 2013 11:57, Paul Durrant <Paul.Durrant@citrix.com> wrote: > >> -----Original Message----- > >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > >> > 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. :/ > > We probably don't, because it generally doesn't mean what you > think it does. It's the smallest possible page size the guest > CPU supports, which may not be the same as the actual page > size the guest OS is using. > > >> Why does it need to be in pages rather than bytes? > > > It doesn't necessarily need to be in pages; it's just a more > > convenient quantity than bytes. > > It isn't really more convienient, because the guest would have > to tell QEMU what the page size was. (I'm told that virtio is > planning to move to a simple "just use a byte count" approach.) > > thanks > -- PMM Yes, sometime in a distant future ...
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. + * + * 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 { + 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); +} + +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); + 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); + + 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
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