| Message ID | 1446119788-19722-1-git-send-email-marcel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 29, 2015 at 01:56:28PM +0200, Marcel Apfelbaum wrote: [...] > index 095de5d..0a08531 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -2,7 +2,51 @@ > #define HW_COMPAT_H > > #define HW_COMPAT_2_4 \ > - /* empty */ > + {\ > + .driver = "virtio-blk-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ Why not a single virtio-pci.disable-pcie=on entry, instead of one entry for each subclass? > + },{\ > + .driver = "virtio-scsi-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-net-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-input-host-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-keyboard-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-mouse-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-serial-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-tablet-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-gpu-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-balloon-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-rng-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + }, > > #define HW_COMPAT_2_3 \ > {\ > -- > 2.1.0 >
On 10/30/2015 05:20 PM, Eduardo Habkost wrote: > On Thu, Oct 29, 2015 at 01:56:28PM +0200, Marcel Apfelbaum wrote: > [...] >> index 095de5d..0a08531 100644 >> --- a/include/hw/compat.h >> +++ b/include/hw/compat.h >> @@ -2,7 +2,51 @@ >> #define HW_COMPAT_H >> >> #define HW_COMPAT_2_4 \ >> - /* empty */ >> + {\ >> + .driver = "virtio-blk-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ > > Why not a single virtio-pci.disable-pcie=on entry, instead of one entry > for each subclass? Hi, Thanks for pointing that out, that being said, I am a little embarrassed because I did think of it, but seeing HW_COMPAT 2_3 (any_layout) I assumed this would not work, my bad. By the way, HW_COMPAT_2_3 sets any-layout to off only for a few virtio devices, not for all. Does anybody know if is in purpose or can we do the same for them too? I'll send an updated version shortly, Thanks Marcel > >> + },{\ >> + .driver = "virtio-scsi-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-net-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-input-host-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-keyboard-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-mouse-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-serial-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-tablet-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-gpu-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-balloon-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-rng-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + }, >> >> #define HW_COMPAT_2_3 \ >> {\ >> -- >> 2.1.0 >> >
On Sun, 1 Nov 2015 11:15:24 +0200 Marcel Apfelbaum <marcel@redhat.com> wrote: > By the way, HW_COMPAT_2_3 sets any-layout to off only for a few virtio devices, > not for all. Does anybody know if is in purpose or can we do the same for them too? net and scsi already defaulted any_layout to true before. Are any others missing?
On 11/02/2015 11:07 AM, Cornelia Huck wrote: > On Sun, 1 Nov 2015 11:15:24 +0200 > Marcel Apfelbaum <marcel@redhat.com> wrote: > >> By the way, HW_COMPAT_2_3 sets any-layout to off only for a few virtio devices, >> not for all. Does anybody know if is in purpose or can we do the same for them too? > > net and scsi already defaulted any_layout to true before. > > Are any others missing? > Hi, The whole list is: name "virtio-blk-pci", bus PCI, alias "virtio-blk" name "virtio-scsi-pci", bus PCI name "virtio-net-pci", bus PCI, alias "virtio-net" name "virtio-input-host-pci", bus PCI name "virtio-keyboard-pci", bus PCI name "virtio-mouse-pci", bus PCI name "virtio-serial-pci", bus PCI, alias "virtio-serial" name "virtio-tablet-pci", bus PCI name "virtio-gpu-pci", bus PCI name "virtio-balloon-pci", bus PCI, alias "virtio-balloon" name "virtio-rng-pci", bus PCI HW_COMPAT_2_3 has: - virtio-blk-pci - virtio-balloon-pci - virtio-serial-pci - virtio-9p-pci - virtio-rng-pci You mentioned: - virtio-scsi-pci - virtio-net-pci I guess the remaining list is: - virtio-input-host-pci - virtio-keyboard-pci - virtio-mouse-pci - virtio-tablet-pci - virtio-gpu-pci I don't know if is is applicable to them, or if they have it defaulted to any_layout, however the above devices are part of QEMU 2.3. Thanks, Marcel + },{\ + .driver = "virtio-scsi-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-net-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-input-host-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-keyboard-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-mouse-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-serial-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-tablet-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-gpu-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-balloon-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-rng-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },
On Thu, 29 Oct 2015 13:56:28 +0200 Marcel Apfelbaum <marcel@redhat.com> wrote: > The virtio devices are converted to PCI-Express > if they are plugged into a PCI-Express bus and > the 'modern' protocol is enabled. > > Devices plugged directly into the Root Complex as > Integrated Endpoints remain PCI. > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > v2 -> v3: > - Addressed Michael S. Tsirkin's comments: > - enable pcie only for 2.5+ machines. > > v1 -> v2: > - Addressed Michael S. Tsirkin's comments: > - Added the minimum required capabilities for PCIe devices > - Integrated Endpoints remain PCI > > - Use pcie_endpoint_cap_init instead of manually creating the pcie capability. > > - Regarding Gerd Hoffman's comments: > - Creating virtio-pcie devices: > For the moment I prefer to not duplicate the virtio definitions, > at least until we don't have a consensus (Personally I don't like it) > - Removing the IO bar: > This would be my next patch on the "virtio to express" series, I plan > to remove it only for "modern" devices. > > Thanks, > Marcel > > hw/virtio/virtio-pci.c | 22 ++++++++++++++++++++++ > hw/virtio/virtio-pci.h | 2 ++ > include/hw/compat.h | 46 +++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 69 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index f55dd2b..a288d8b 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1592,6 +1592,26 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > > address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); > > + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) > + && !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) > + && pci_bus_is_express(pci_dev->bus) > + && !pci_bus_is_root(pci_dev->bus)) { > + int pos; > + > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > + pos = pcie_endpoint_cap_init(pci_dev, 0); > + assert(pos > 0); > + > + pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF); > + assert(pos > 0); > + > + /* > + * Indicates that this function complies with revision 1.2 of the > + * PCI Power Management Interface Specification. > + */ > + pci_set_word(pci_dev->config + pos + PCI_PM_PMC, 0x3); > + } > + > virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); > if (k->realize) { > k->realize(proxy, errp); > @@ -1622,6 +1642,8 @@ static Property virtio_pci_properties[] = { > VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false), > DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true), > + DEFINE_PROP_BIT("disable-pcie", VirtIOPCIProxy, flags, > + VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT, false), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h > index 801c23a..1a487fc 100644 > --- a/hw/virtio/virtio-pci.h > +++ b/hw/virtio/virtio-pci.h > @@ -72,8 +72,10 @@ typedef struct VirtioBusClass VirtioPCIBusClass; > /* virtio version flags */ > #define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2 > #define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 > +#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 4 > #define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT) > #define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT) > +#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT) > > typedef struct { > MSIMessage msg; > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 095de5d..0a08531 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -2,7 +2,51 @@ > #define HW_COMPAT_H > > #define HW_COMPAT_2_4 \ > - /* empty */ > + {\ > + .driver = "virtio-blk-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-scsi-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-net-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-input-host-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-keyboard-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-mouse-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-serial-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-tablet-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-gpu-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-balloon-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-rng-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + }, > What about "virtio-9p-pci" ? > #define HW_COMPAT_2_3 \ > {\
On 11/02/2015 11:42 AM, Greg Kurz wrote: > On Thu, 29 Oct 2015 13:56:28 +0200 > Marcel Apfelbaum <marcel@redhat.com> wrote: > >> The virtio devices are converted to PCI-Express >> if they are plugged into a PCI-Express bus and >> the 'modern' protocol is enabled. >> >> Devices plugged directly into the Root Complex as >> Integrated Endpoints remain PCI. >> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >> --- >> v2 -> v3: >> - Addressed Michael S. Tsirkin's comments: >> - enable pcie only for 2.5+ machines. >> >> v1 -> v2: >> - Addressed Michael S. Tsirkin's comments: >> - Added the minimum required capabilities for PCIe devices >> - Integrated Endpoints remain PCI >> >> - Use pcie_endpoint_cap_init instead of manually creating the pcie capability. >> >> - Regarding Gerd Hoffman's comments: >> - Creating virtio-pcie devices: >> For the moment I prefer to not duplicate the virtio definitions, >> at least until we don't have a consensus (Personally I don't like it) >> - Removing the IO bar: >> This would be my next patch on the "virtio to express" series, I plan >> to remove it only for "modern" devices. >> >> Thanks, >> Marcel >> >> hw/virtio/virtio-pci.c | 22 ++++++++++++++++++++++ >> hw/virtio/virtio-pci.h | 2 ++ >> include/hw/compat.h | 46 +++++++++++++++++++++++++++++++++++++++++++++- >> 3 files changed, 69 insertions(+), 1 deletion(-) >> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >> index f55dd2b..a288d8b 100644 >> --- a/hw/virtio/virtio-pci.c >> +++ b/hw/virtio/virtio-pci.c >> @@ -1592,6 +1592,26 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) >> >> address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); >> >> + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) >> + && !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) >> + && pci_bus_is_express(pci_dev->bus) >> + && !pci_bus_is_root(pci_dev->bus)) { >> + int pos; >> + >> + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; >> + pos = pcie_endpoint_cap_init(pci_dev, 0); >> + assert(pos > 0); >> + >> + pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF); >> + assert(pos > 0); >> + >> + /* >> + * Indicates that this function complies with revision 1.2 of the >> + * PCI Power Management Interface Specification. >> + */ >> + pci_set_word(pci_dev->config + pos + PCI_PM_PMC, 0x3); >> + } >> + >> virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); >> if (k->realize) { >> k->realize(proxy, errp); >> @@ -1622,6 +1642,8 @@ static Property virtio_pci_properties[] = { >> VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false), >> DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags, >> VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true), >> + DEFINE_PROP_BIT("disable-pcie", VirtIOPCIProxy, flags, >> + VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT, false), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h >> index 801c23a..1a487fc 100644 >> --- a/hw/virtio/virtio-pci.h >> +++ b/hw/virtio/virtio-pci.h >> @@ -72,8 +72,10 @@ typedef struct VirtioBusClass VirtioPCIBusClass; >> /* virtio version flags */ >> #define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2 >> #define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 >> +#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 4 >> #define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT) >> #define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT) >> +#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT) >> >> typedef struct { >> MSIMessage msg; >> diff --git a/include/hw/compat.h b/include/hw/compat.h >> index 095de5d..0a08531 100644 >> --- a/include/hw/compat.h >> +++ b/include/hw/compat.h >> @@ -2,7 +2,51 @@ >> #define HW_COMPAT_H >> >> #define HW_COMPAT_2_4 \ >> - /* empty */ >> + {\ >> + .driver = "virtio-blk-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-scsi-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-net-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-input-host-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-keyboard-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-mouse-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-serial-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-tablet-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-gpu-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-balloon-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-rng-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + }, >> > > What about "virtio-9p-pci" ? Thanks for pointing it out: 1. It was missed because: -device help 2>&1 | grep virtio | grep pci didn't return it. This is because my config didn't include VIRTFS. Next time I'll grep the code. 2. I sent V4 of this patch that applies the property to the "virtio-pci" base class rather than to individual devices. Thanks, Marcel > >> #define HW_COMPAT_2_3 \ >> {\ >
On Mon, 2 Nov 2015 11:20:50 +0200 Marcel Apfelbaum <marcel@redhat.com> wrote: > On 11/02/2015 11:07 AM, Cornelia Huck wrote: > > On Sun, 1 Nov 2015 11:15:24 +0200 > > Marcel Apfelbaum <marcel@redhat.com> wrote: > > > >> By the way, HW_COMPAT_2_3 sets any-layout to off only for a few virtio devices, > >> not for all. Does anybody know if is in purpose or can we do the same for them too? > > > > net and scsi already defaulted any_layout to true before. > > > > Are any others missing? > > > > Hi, > > The whole list is: > name "virtio-blk-pci", bus PCI, alias "virtio-blk" > name "virtio-scsi-pci", bus PCI > name "virtio-net-pci", bus PCI, alias "virtio-net" > name "virtio-input-host-pci", bus PCI > name "virtio-keyboard-pci", bus PCI > name "virtio-mouse-pci", bus PCI > name "virtio-serial-pci", bus PCI, alias "virtio-serial" > name "virtio-tablet-pci", bus PCI > name "virtio-gpu-pci", bus PCI > name "virtio-balloon-pci", bus PCI, alias "virtio-balloon" > name "virtio-rng-pci", bus PCI > > > HW_COMPAT_2_3 has: > - virtio-blk-pci > - virtio-balloon-pci > - virtio-serial-pci > - virtio-9p-pci > - virtio-rng-pci > > You mentioned: > - virtio-scsi-pci > - virtio-net-pci > > I guess the remaining list is: > - virtio-input-host-pci > - virtio-keyboard-pci > - virtio-mouse-pci > - virtio-tablet-pci > - virtio-gpu-pci > > I don't know if is is applicable to them, or if they have it defaulted to any_layout, > however the above devices are part of QEMU 2.3. Hm, unless I'm fumbling git commands here, they were all added post-2.3? (And all of them seem to force virtio-1 anyway.)
On 11/02/2015 11:54 AM, Cornelia Huck wrote: > On Mon, 2 Nov 2015 11:20:50 +0200 > Marcel Apfelbaum <marcel@redhat.com> wrote: > >> On 11/02/2015 11:07 AM, Cornelia Huck wrote: >>> On Sun, 1 Nov 2015 11:15:24 +0200 >>> Marcel Apfelbaum <marcel@redhat.com> wrote: >>> >>>> By the way, HW_COMPAT_2_3 sets any-layout to off only for a few virtio devices, >>>> not for all. Does anybody know if is in purpose or can we do the same for them too? >>> >>> net and scsi already defaulted any_layout to true before. >>> >>> Are any others missing? >>> >> >> Hi, >> >> The whole list is: >> name "virtio-blk-pci", bus PCI, alias "virtio-blk" >> name "virtio-scsi-pci", bus PCI >> name "virtio-net-pci", bus PCI, alias "virtio-net" >> name "virtio-input-host-pci", bus PCI >> name "virtio-keyboard-pci", bus PCI >> name "virtio-mouse-pci", bus PCI >> name "virtio-serial-pci", bus PCI, alias "virtio-serial" >> name "virtio-tablet-pci", bus PCI >> name "virtio-gpu-pci", bus PCI >> name "virtio-balloon-pci", bus PCI, alias "virtio-balloon" >> name "virtio-rng-pci", bus PCI >> >> >> HW_COMPAT_2_3 has: >> - virtio-blk-pci >> - virtio-balloon-pci >> - virtio-serial-pci >> - virtio-9p-pci >> - virtio-rng-pci >> >> You mentioned: >> - virtio-scsi-pci >> - virtio-net-pci >> >> I guess the remaining list is: >> - virtio-input-host-pci >> - virtio-keyboard-pci >> - virtio-mouse-pci >> - virtio-tablet-pci >> - virtio-gpu-pci >> >> I don't know if is is applicable to them, or if they have it defaulted to any_layout, >> however the above devices are part of QEMU 2.3. > > Hm, unless I'm fumbling git commands here, they were all added post-2.3? I misspoke, I wanted to say that when I run (latest master branch): <qemu-bin> -M pc-q35-2.3 -device help 2>&1 | grep virtio | grep pci the devices are there. > > (And all of them seem to force virtio-1 anyway.) OK. Thanks for looking into it! Marcel >
On Mon, 2 Nov 2015 12:01:06 +0200 Marcel Apfelbaum <marcel@redhat.com> wrote: > On 11/02/2015 11:54 AM, Cornelia Huck wrote: > > On Mon, 2 Nov 2015 11:20:50 +0200 > > Marcel Apfelbaum <marcel@redhat.com> wrote: > > > >> On 11/02/2015 11:07 AM, Cornelia Huck wrote: > >>> On Sun, 1 Nov 2015 11:15:24 +0200 > >>> Marcel Apfelbaum <marcel@redhat.com> wrote: > >>> > >>>> By the way, HW_COMPAT_2_3 sets any-layout to off only for a few virtio devices, > >>>> not for all. Does anybody know if is in purpose or can we do the same for them too? > >>> > >>> net and scsi already defaulted any_layout to true before. > >>> > >>> Are any others missing? > >>> > >> > >> Hi, > >> > >> The whole list is: > >> name "virtio-blk-pci", bus PCI, alias "virtio-blk" > >> name "virtio-scsi-pci", bus PCI > >> name "virtio-net-pci", bus PCI, alias "virtio-net" > >> name "virtio-input-host-pci", bus PCI > >> name "virtio-keyboard-pci", bus PCI > >> name "virtio-mouse-pci", bus PCI > >> name "virtio-serial-pci", bus PCI, alias "virtio-serial" > >> name "virtio-tablet-pci", bus PCI > >> name "virtio-gpu-pci", bus PCI > >> name "virtio-balloon-pci", bus PCI, alias "virtio-balloon" > >> name "virtio-rng-pci", bus PCI > >> > >> > >> HW_COMPAT_2_3 has: > >> - virtio-blk-pci > >> - virtio-balloon-pci > >> - virtio-serial-pci > >> - virtio-9p-pci > >> - virtio-rng-pci > >> > >> You mentioned: > >> - virtio-scsi-pci > >> - virtio-net-pci > >> > >> I guess the remaining list is: > >> - virtio-input-host-pci > >> - virtio-keyboard-pci > >> - virtio-mouse-pci > >> - virtio-tablet-pci > >> - virtio-gpu-pci > >> > >> I don't know if is is applicable to them, or if they have it defaulted to any_layout, > >> however the above devices are part of QEMU 2.3. > > > > Hm, unless I'm fumbling git commands here, they were all added post-2.3? > > I misspoke, I wanted to say that when I run (latest master branch): > <qemu-bin> -M pc-q35-2.3 -device help 2>&1 | grep virtio | grep pci > the devices are there. What's the word on compat machines and new device types, btw.? If we fire up a compat machine, we can still specify devices that were added with later machine versions, but of course we can't migrate to an old machine as the device types did not exist there. Do we want to give the user a hint here by disallowing new device types?
On 11/02/2015 02:05 PM, Cornelia Huck wrote: > On Mon, 2 Nov 2015 12:01:06 +0200 > Marcel Apfelbaum <marcel@redhat.com> wrote: > >> On 11/02/2015 11:54 AM, Cornelia Huck wrote: >>> On Mon, 2 Nov 2015 11:20:50 +0200 >>> Marcel Apfelbaum <marcel@redhat.com> wrote: >>> >>>> On 11/02/2015 11:07 AM, Cornelia Huck wrote: >>>>> On Sun, 1 Nov 2015 11:15:24 +0200 >>>>> Marcel Apfelbaum <marcel@redhat.com> wrote: >>>>> >>>>>> By the way, HW_COMPAT_2_3 sets any-layout to off only for a few virtio devices, >>>>>> not for all. Does anybody know if is in purpose or can we do the same for them too? >>>>> >>>>> net and scsi already defaulted any_layout to true before. >>>>> >>>>> Are any others missing? >>>>> >>>> >>>> Hi, >>>> >>>> The whole list is: >>>> name "virtio-blk-pci", bus PCI, alias "virtio-blk" >>>> name "virtio-scsi-pci", bus PCI >>>> name "virtio-net-pci", bus PCI, alias "virtio-net" >>>> name "virtio-input-host-pci", bus PCI >>>> name "virtio-keyboard-pci", bus PCI >>>> name "virtio-mouse-pci", bus PCI >>>> name "virtio-serial-pci", bus PCI, alias "virtio-serial" >>>> name "virtio-tablet-pci", bus PCI >>>> name "virtio-gpu-pci", bus PCI >>>> name "virtio-balloon-pci", bus PCI, alias "virtio-balloon" >>>> name "virtio-rng-pci", bus PCI >>>> >>>> >>>> HW_COMPAT_2_3 has: >>>> - virtio-blk-pci >>>> - virtio-balloon-pci >>>> - virtio-serial-pci >>>> - virtio-9p-pci >>>> - virtio-rng-pci >>>> >>>> You mentioned: >>>> - virtio-scsi-pci >>>> - virtio-net-pci >>>> >>>> I guess the remaining list is: >>>> - virtio-input-host-pci >>>> - virtio-keyboard-pci >>>> - virtio-mouse-pci >>>> - virtio-tablet-pci >>>> - virtio-gpu-pci >>>> >>>> I don't know if is is applicable to them, or if they have it defaulted to any_layout, >>>> however the above devices are part of QEMU 2.3. >>> >>> Hm, unless I'm fumbling git commands here, they were all added post-2.3? >> >> I misspoke, I wanted to say that when I run (latest master branch): >> <qemu-bin> -M pc-q35-2.3 -device help 2>&1 | grep virtio | grep pci >> the devices are there. > > What's the word on compat machines and new device types, btw.? If we > fire up a compat machine, we can still specify devices that were added > with later machine versions, but of course we can't migrate to an old > machine as the device types did not exist there. Do we want to give the > user a hint here by disallowing new device types? > I started to wonder about this too, so I added to this thread the migration maintainers that should be qualified to answer this :) Thanks, Marcel
On Mon, Nov 02, 2015 at 02:12:32PM +0200, Marcel Apfelbaum wrote: > On 11/02/2015 02:05 PM, Cornelia Huck wrote: [...] > >What's the word on compat machines and new device types, btw.? If we > >fire up a compat machine, we can still specify devices that were added > >with later machine versions, but of course we can't migrate to an old > >machine as the device types did not exist there. Do we want to give the > >user a hint here by disallowing new device types? > > > > I started to wonder about this too, so I added to this thread the migration > maintainers that should be qualified to answer this :) This looks no different from all other features that are available on newer QEMU versions and prevent migration to older hosts (even ones that are not guest-visible, like backend configuration). Management can easily detect the unavailability of those features in other hosts, long before trying migration (and have better ways to warn the user if necessary). Also, it looks like a potential nightmare for downstream maintainers that cherry-pick and rebase patches, so I hope we don't consider implementing that. :)
* Eduardo Habkost (ehabkost@redhat.com) wrote: > On Mon, Nov 02, 2015 at 02:12:32PM +0200, Marcel Apfelbaum wrote: > > On 11/02/2015 02:05 PM, Cornelia Huck wrote: > [...] > > >What's the word on compat machines and new device types, btw.? If we > > >fire up a compat machine, we can still specify devices that were added > > >with later machine versions, but of course we can't migrate to an old > > >machine as the device types did not exist there. Do we want to give the > > >user a hint here by disallowing new device types? > > > > > > > I started to wonder about this too, so I added to this thread the migration > > maintainers that should be qualified to answer this :) > > This looks no different from all other features that are available on > newer QEMU versions and prevent migration to older hosts (even ones that > are not guest-visible, like backend configuration). Management can > easily detect the unavailability of those features in other hosts, long > before trying migration (and have better ways to warn the user if > necessary). > > Also, it looks like a potential nightmare for downstream maintainers > that cherry-pick and rebase patches, so I hope we don't consider > implementing that. :) a) It's fine to add new devices and allow them to be used with old machine types b) The rule is that any old machine type used in the way it used to be used must stay the same. c) That also means it's fine to add new features that can be turned on with old machine types; as long as the default is that they behave just like they always did. d) If you know a new device just isn't going to work with an old machine type then please make it fail early with an obvious error. Having said all that; I have seen requests for some magic which would tell the management tool whether something is 'safe for migration'; so imagine that a user has a pile of hosts, some of which have qemu 2.n on and some have qemu 2.n+1 ; if he creates his VM on 2.n+1 and uses a feature that's new in 2.n+1 the management tool can't warn him because they've not yet expressed an interest in migrating to the 2.n machine. Dave > > -- > Eduardo -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 11/05/2015 08:22 PM, Dr. David Alan Gilbert wrote: > * Eduardo Habkost (ehabkost@redhat.com) wrote: >> On Mon, Nov 02, 2015 at 02:12:32PM +0200, Marcel Apfelbaum wrote: >>> On 11/02/2015 02:05 PM, Cornelia Huck wrote: >> [...] >>>> What's the word on compat machines and new device types, btw.? If we >>>> fire up a compat machine, we can still specify devices that were added >>>> with later machine versions, but of course we can't migrate to an old >>>> machine as the device types did not exist there. Do we want to give the >>>> user a hint here by disallowing new device types? >>>> >>> >>> I started to wonder about this too, so I added to this thread the migration >>> maintainers that should be qualified to answer this :) >> >> This looks no different from all other features that are available on >> newer QEMU versions and prevent migration to older hosts (even ones that >> are not guest-visible, like backend configuration). Management can >> easily detect the unavailability of those features in other hosts, long >> before trying migration (and have better ways to warn the user if >> necessary). >> >> Also, it looks like a potential nightmare for downstream maintainers >> that cherry-pick and rebase patches, so I hope we don't consider >> implementing that. :) > > a) It's fine to add new devices and allow them to be used with old machine > types > b) The rule is that any old machine type used in the way it used to be used > must stay the same. > c) That also means it's fine to add new features that can be turned on > with old machine types; as long as the default is that they behave > just like they always did. > d) If you know a new device just isn't going to work with an old > machine type then please make it fail early with an obvious > error. > > Having said all that; I have seen requests for some magic which would > tell the management tool whether something is 'safe for migration'; > so imagine that a user has a pile of hosts, some of which have qemu 2.n on > and some have qemu 2.n+1 ; if he creates his VM on 2.n+1 and uses > a feature that's new in 2.n+1 the management tool can't warn him > because they've not yet expressed an interest in migrating to > the 2.n machine. Exactly, so how can I do (d) ? A "migration possible" machine mapping qemu-pc-2.x -> qemu-pc-2.y is not enough, we need to compare also the QEMU versions and have a "minimum QEMU version per feature." Do we have a way to do this today in QEMU? Thanks, Marcel > > Dave > >> >> -- >> Eduardo > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
* Marcel Apfelbaum (marcel@redhat.com) wrote: > On 11/05/2015 08:22 PM, Dr. David Alan Gilbert wrote: > >* Eduardo Habkost (ehabkost@redhat.com) wrote: > >>On Mon, Nov 02, 2015 at 02:12:32PM +0200, Marcel Apfelbaum wrote: > >>>On 11/02/2015 02:05 PM, Cornelia Huck wrote: > >>[...] > >>>>What's the word on compat machines and new device types, btw.? If we > >>>>fire up a compat machine, we can still specify devices that were added > >>>>with later machine versions, but of course we can't migrate to an old > >>>>machine as the device types did not exist there. Do we want to give the > >>>>user a hint here by disallowing new device types? > >>>> > >>> > >>>I started to wonder about this too, so I added to this thread the migration > >>>maintainers that should be qualified to answer this :) > >> > >>This looks no different from all other features that are available on > >>newer QEMU versions and prevent migration to older hosts (even ones that > >>are not guest-visible, like backend configuration). Management can > >>easily detect the unavailability of those features in other hosts, long > >>before trying migration (and have better ways to warn the user if > >>necessary). > >> > >>Also, it looks like a potential nightmare for downstream maintainers > >>that cherry-pick and rebase patches, so I hope we don't consider > >>implementing that. :) > > > > a) It's fine to add new devices and allow them to be used with old machine > > types > > b) The rule is that any old machine type used in the way it used to be used > > must stay the same. > > c) That also means it's fine to add new features that can be turned on > > with old machine types; as long as the default is that they behave > > just like they always did. > > d) If you know a new device just isn't going to work with an old > > machine type then please make it fail early with an obvious > > error. > > > >Having said all that; I have seen requests for some magic which would > >tell the management tool whether something is 'safe for migration'; > >so imagine that a user has a pile of hosts, some of which have qemu 2.n on > >and some have qemu 2.n+1 ; if he creates his VM on 2.n+1 and uses > >a feature that's new in 2.n+1 the management tool can't warn him > >because they've not yet expressed an interest in migrating to > >the 2.n machine. > > Exactly, so how can I do (d) ? Note that (d) is talking about making it fail on a new version of qemu with an old machine type; e.g. if you know that your new device for some reason just won't work on pc-i440fx-2.4 or older then add a check in - I'm not sure if we've got any easy way to do that at the moment but it shouldn't be hard. However I don't think I'm aware of any device with that type of interaction; but maybe there is somewhere. > A "migration possible" machine mapping qemu-pc-2.x -> qemu-pc-2.y is not enough, we need to > compare also the QEMU versions and have a "minimum QEMU version per feature." > Do we have a way to do this today in QEMU? (d) is entirely separate from knowing that it won't work on an old machine type on an old qemu. No, I don't think we have anything for minimum version for features; but, management tools can probe for all features, so some management tool could group those feature sets together somewhere to know the features of all the hosts involved; but it doesn't sound that easy. Dave > > Thanks, > Marcel > > > > >Dave > > > >> > >>-- > >>Eduardo > >-- > >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Oct 29, 2015 at 01:56:28PM +0200, Marcel Apfelbaum wrote: > The virtio devices are converted to PCI-Express > if they are plugged into a PCI-Express bus and > the 'modern' protocol is enabled. > > Devices plugged directly into the Root Complex as > Integrated Endpoints remain PCI. > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> Looks ok. Two comments: > --- > v2 -> v3: > - Addressed Michael S. Tsirkin's comments: > - enable pcie only for 2.5+ machines. > > v1 -> v2: > - Addressed Michael S. Tsirkin's comments: > - Added the minimum required capabilities for PCIe devices > - Integrated Endpoints remain PCI > > - Use pcie_endpoint_cap_init instead of manually creating the pcie capability. > > - Regarding Gerd Hoffman's comments: > - Creating virtio-pcie devices: > For the moment I prefer to not duplicate the virtio definitions, > at least until we don't have a consensus (Personally I don't like it) > - Removing the IO bar: > This would be my next patch on the "virtio to express" series, I plan > to remove it only for "modern" devices. > > Thanks, > Marcel > > hw/virtio/virtio-pci.c | 22 ++++++++++++++++++++++ > hw/virtio/virtio-pci.h | 2 ++ > include/hw/compat.h | 46 +++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 69 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index f55dd2b..a288d8b 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1592,6 +1592,26 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > > address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); > > + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) > + && !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) > + && pci_bus_is_express(pci_dev->bus) > + && !pci_bus_is_root(pci_dev->bus)) { > + int pos; > + > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > + pos = pcie_endpoint_cap_init(pci_dev, 0); > + assert(pos > 0); > + > + pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF); > + assert(pos > 0); > + > + /* > + * Indicates that this function complies with revision 1.2 of the > + * PCI Power Management Interface Specification. > + */ > + pci_set_word(pci_dev->config + pos + PCI_PM_PMC, 0x3); > + } > + > virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); > if (k->realize) { > k->realize(proxy, errp); > @@ -1622,6 +1642,8 @@ static Property virtio_pci_properties[] = { > VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false), > DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true), > + DEFINE_PROP_BIT("disable-pcie", VirtIOPCIProxy, flags, > + VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT, false), It's preferable to call this one x-disable-pcie - this is the convention to mark it as "not for user". > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h > index 801c23a..1a487fc 100644 > --- a/hw/virtio/virtio-pci.h > +++ b/hw/virtio/virtio-pci.h > @@ -72,8 +72,10 @@ typedef struct VirtioBusClass VirtioPCIBusClass; > /* virtio version flags */ > #define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2 > #define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 > +#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 4 > #define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT) > #define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT) > +#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT) > > typedef struct { > MSIMessage msg; > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 095de5d..0a08531 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -2,7 +2,51 @@ > #define HW_COMPAT_H > > #define HW_COMPAT_2_4 \ > - /* empty */ > + {\ > + .driver = "virtio-blk-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-scsi-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-net-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-input-host-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-keyboard-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-mouse-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-serial-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-tablet-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-gpu-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-balloon-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-rng-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + }, Can't these properties be defined for type virtio-pci? If no - why not? > > #define HW_COMPAT_2_3 \ > {\ > -- > 2.1.0
On 11/08/2015 07:10 PM, Michael S. Tsirkin wrote: > On Thu, Oct 29, 2015 at 01:56:28PM +0200, Marcel Apfelbaum wrote: >> The virtio devices are converted to PCI-Express >> if they are plugged into a PCI-Express bus and >> the 'modern' protocol is enabled. >> >> Devices plugged directly into the Root Complex as >> Integrated Endpoints remain PCI. >> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > > > Looks ok. Two comments: > >> --- >> v2 -> v3: >> - Addressed Michael S. Tsirkin's comments: >> - enable pcie only for 2.5+ machines. >> >> v1 -> v2: >> - Addressed Michael S. Tsirkin's comments: >> - Added the minimum required capabilities for PCIe devices >> - Integrated Endpoints remain PCI >> >> - Use pcie_endpoint_cap_init instead of manually creating the pcie capability. >> >> - Regarding Gerd Hoffman's comments: >> - Creating virtio-pcie devices: >> For the moment I prefer to not duplicate the virtio definitions, >> at least until we don't have a consensus (Personally I don't like it) >> - Removing the IO bar: >> This would be my next patch on the "virtio to express" series, I plan >> to remove it only for "modern" devices. >> >> Thanks, >> Marcel >> >> hw/virtio/virtio-pci.c | 22 ++++++++++++++++++++++ >> hw/virtio/virtio-pci.h | 2 ++ >> include/hw/compat.h | 46 +++++++++++++++++++++++++++++++++++++++++++++- >> 3 files changed, 69 insertions(+), 1 deletion(-) >> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >> index f55dd2b..a288d8b 100644 >> --- a/hw/virtio/virtio-pci.c >> +++ b/hw/virtio/virtio-pci.c >> @@ -1592,6 +1592,26 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) >> >> address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); >> >> + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) >> + && !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) >> + && pci_bus_is_express(pci_dev->bus) >> + && !pci_bus_is_root(pci_dev->bus)) { >> + int pos; >> + >> + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; >> + pos = pcie_endpoint_cap_init(pci_dev, 0); >> + assert(pos > 0); >> + >> + pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF); >> + assert(pos > 0); >> + >> + /* >> + * Indicates that this function complies with revision 1.2 of the >> + * PCI Power Management Interface Specification. >> + */ >> + pci_set_word(pci_dev->config + pos + PCI_PM_PMC, 0x3); >> + } >> + >> virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); >> if (k->realize) { >> k->realize(proxy, errp); >> @@ -1622,6 +1642,8 @@ static Property virtio_pci_properties[] = { >> VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false), >> DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags, >> VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true), >> + DEFINE_PROP_BIT("disable-pcie", VirtIOPCIProxy, flags, >> + VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT, false), > > It's preferable to call this one x-disable-pcie - this > is the convention to mark it as "not for user". Sure, I'll change it. > >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h >> index 801c23a..1a487fc 100644 >> --- a/hw/virtio/virtio-pci.h >> +++ b/hw/virtio/virtio-pci.h >> @@ -72,8 +72,10 @@ typedef struct VirtioBusClass VirtioPCIBusClass; >> /* virtio version flags */ >> #define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2 >> #define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 >> +#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 4 >> #define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT) >> #define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT) >> +#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT) >> >> typedef struct { >> MSIMessage msg; >> diff --git a/include/hw/compat.h b/include/hw/compat.h >> index 095de5d..0a08531 100644 >> --- a/include/hw/compat.h >> +++ b/include/hw/compat.h >> @@ -2,7 +2,51 @@ >> #define HW_COMPAT_H >> >> #define HW_COMPAT_2_4 \ >> - /* empty */ >> + {\ >> + .driver = "virtio-blk-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-scsi-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-net-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-input-host-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-keyboard-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-mouse-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-serial-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-tablet-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-gpu-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-balloon-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-rng-pci",\ >> + .property = "disable-pcie",\ >> + .value = "on",\ >> + }, > > Can't these properties be defined for type virtio-pci? > If no - why not? You are right, I changed this in V4: https://patchwork.ozlabs.org/patch/538709/ Thanks, Marcel > >> >> #define HW_COMPAT_2_3 \ >> {\ >> -- >> 2.1.0
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index f55dd2b..a288d8b 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1592,6 +1592,26 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) + && !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) + && pci_bus_is_express(pci_dev->bus) + && !pci_bus_is_root(pci_dev->bus)) { + int pos; + + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; + pos = pcie_endpoint_cap_init(pci_dev, 0); + assert(pos > 0); + + pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF); + assert(pos > 0); + + /* + * Indicates that this function complies with revision 1.2 of the + * PCI Power Management Interface Specification. + */ + pci_set_word(pci_dev->config + pos + PCI_PM_PMC, 0x3); + } + virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); if (k->realize) { k->realize(proxy, errp); @@ -1622,6 +1642,8 @@ static Property virtio_pci_properties[] = { VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false), DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true), + DEFINE_PROP_BIT("disable-pcie", VirtIOPCIProxy, flags, + VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT, false), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index 801c23a..1a487fc 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h @@ -72,8 +72,10 @@ typedef struct VirtioBusClass VirtioPCIBusClass; /* virtio version flags */ #define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2 #define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 +#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 4 #define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT) #define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT) +#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT) typedef struct { MSIMessage msg; diff --git a/include/hw/compat.h b/include/hw/compat.h index 095de5d..0a08531 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -2,7 +2,51 @@ #define HW_COMPAT_H #define HW_COMPAT_2_4 \ - /* empty */ + {\ + .driver = "virtio-blk-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-scsi-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-net-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-input-host-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-keyboard-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-mouse-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-serial-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-tablet-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-gpu-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-balloon-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + },{\ + .driver = "virtio-rng-pci",\ + .property = "disable-pcie",\ + .value = "on",\ + }, #define HW_COMPAT_2_3 \ {\
The virtio devices are converted to PCI-Express if they are plugged into a PCI-Express bus and the 'modern' protocol is enabled. Devices plugged directly into the Root Complex as Integrated Endpoints remain PCI. Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- v2 -> v3: - Addressed Michael S. Tsirkin's comments: - enable pcie only for 2.5+ machines. v1 -> v2: - Addressed Michael S. Tsirkin's comments: - Added the minimum required capabilities for PCIe devices - Integrated Endpoints remain PCI - Use pcie_endpoint_cap_init instead of manually creating the pcie capability. - Regarding Gerd Hoffman's comments: - Creating virtio-pcie devices: For the moment I prefer to not duplicate the virtio definitions, at least until we don't have a consensus (Personally I don't like it) - Removing the IO bar: This would be my next patch on the "virtio to express" series, I plan to remove it only for "modern" devices. Thanks, Marcel hw/virtio/virtio-pci.c | 22 ++++++++++++++++++++++ hw/virtio/virtio-pci.h | 2 ++ include/hw/compat.h | 46 +++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 69 insertions(+), 1 deletion(-)