| Message ID | 1444663074-32617-1-git-send-email-marcel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 12, 2015 at 06:17:54PM +0300, 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. > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > > This is an RFC because all it does it adds the PCIe capability and nothing more. Express capability is easy. But if you go over express space you will see that a bunch of other capabilities are required, such as PM capability etc. These might need more work. > I post it early so I can get feedbacks on what is the best way to continue it. > > Any comments would be appreciated, > Thanks, > Marcel > > hw/virtio/virtio-pci.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 6703806..f7c93d9 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1591,6 +1591,17 @@ 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_MODERN) > + && pci_bus_is_express(pci_dev->bus)) { One point: we probably want to avoid doing this for integrated devices on root bus. Does pci_bus_is_express return true there? > + int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0, > + PCI_EXP_VER2_SIZEOF); > + > + if (pos > 0) { We probably want to assert on pos < 0 instead. That implies a code bug. > + pci_dev->exp.exp_cap = pos; > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > + } > + } > + > virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); > if (k->realize) { > k->realize(proxy, errp); > -- > 2.1.0
On Mo, 2015-10-12 at 18:42 +0300, Michael S. Tsirkin wrote: > On Mon, Oct 12, 2015 at 06:17:54PM +0300, 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. > > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > > --- > > > > This is an RFC because all it does it adds the PCIe capability and nothing more. > > Express capability is easy. > But if you go over express space you will see that a bunch of > other capabilities are required, such as PM capability etc. > These might need more work. Also what about the legacy io bar? I guess we'd better avoid that for express devices. Maybe it makes sense to add virtio-*-pcie devices (virtio-1.0 only, with pcie caps)? cheers, Gerd
On Tue, Oct 13, 2015 at 10:13:37AM +0200, Gerd Hoffmann wrote: > On Mo, 2015-10-12 at 18:42 +0300, Michael S. Tsirkin wrote: > > On Mon, Oct 12, 2015 at 06:17:54PM +0300, 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. > > > > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > > > --- > > > > > > This is an RFC because all it does it adds the PCIe capability and nothing more. > > > > Express capability is easy. > > But if you go over express space you will see that a bunch of > > other capabilities are required, such as PM capability etc. > > These might need more work. > > Also what about the legacy io bar? I guess we'd better avoid that for > express devices. This needs some thought. We definitely still want a way to enable it I think, e.g. for old guests. > Maybe it makes sense to add virtio-*-pcie devices (virtio-1.0 only, with > pcie caps)? > > cheers, > Gerd Personally I think it's ugly enough to remember the need to specify -pci. OTOH the need to specify upstream and downstream ports is even uglier. Any idea how to address both issues at the same time?
On Monday, October 12, 2015, Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Oct 12, 2015 at 06:17:54PM +0300, 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. > > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com <javascript:;>> > > --- > > > > This is an RFC because all it does it adds the PCIe capability and > nothing more. > > Express capability is easy. > But if you go over express space you will see that a bunch of > other capabilities are required, such as PM capability etc. > These might need more work. Sure, I'll look on the PCIe spec for the minimum requirements. > > > I post it early so I can get feedbacks on what is the best way to > continue it. > > > > Any comments would be appreciated, > > Thanks, > > Marcel > > > > hw/virtio/virtio-pci.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index 6703806..f7c93d9 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -1591,6 +1591,17 @@ 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_MODERN) > > + && pci_bus_is_express(pci_dev->bus)) { > > One point: we probably want to avoid doing this for integrated > devices on root bus. Does pci_bus_is_express return true there? Hmm, I'll check, but I think it does. Is this a must? > > > + int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0, > > + PCI_EXP_VER2_SIZEOF); > > + > > + if (pos > 0) { > > We probably want to assert on pos < 0 instead. > That implies a code bug. > > I was wondering what to do here, thanks for the idea! Thanks, Marcel > > > + pci_dev->exp.exp_cap = pos; > > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > > + } > > + } > > + > > virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); > > if (k->realize) { > > k->realize(proxy, errp); > > -- > > 2.1.0 > >
On Tue, Oct 13, 2015 at 05:31:27PM +0300, Marcel Apfelbaum wrote: > > > On Monday, October 12, 2015, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Oct 12, 2015 at 06:17:54PM +0300, 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. > > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > > --- > > > > This is an RFC because all it does it adds the PCIe capability and > nothing more. > > Express capability is easy. > But if you go over express space you will see that a bunch of > other capabilities are required, such as PM capability etc. > These might need more work. > > > Sure, I'll look on the PCIe spec for the minimum requirements. > > > > > I post it early so I can get feedbacks on what is the best way to > continue it. > > > > Any comments would be appreciated, > > Thanks, > > Marcel > > > > hw/virtio/virtio-pci.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index 6703806..f7c93d9 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -1591,6 +1591,17 @@ 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_MODERN) > > + && pci_bus_is_express(pci_dev->bus)) { > > One point: we probably want to avoid doing this for integrated > devices on root bus. Does pci_bus_is_express return true there? > > > Hmm, I'll check, but I think it does. Is this a must? It's probably a smart thing to do because you can't e.g. hot-unplug them. So supporting PCI E capability for integrated devices is probably more work than just making the PCI devices (which is not spec compliant but very popular so guests support this). > > > > + int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0, > > + PCI_EXP_VER2_SIZEOF); > > + > > + if (pos > 0) { > > We probably want to assert on pos < 0 instead. > That implies a code bug. > > > > I was wondering what to do here, thanks for the idea! > > Thanks, > Marcel > > > > > > + pci_dev->exp.exp_cap = pos; > > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > > + } > > + } > > + > > virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); > > if (k->realize) { > > k->realize(proxy, errp); > > -- > > 2.1.0 > >
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 6703806..f7c93d9 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1591,6 +1591,17 @@ 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_MODERN) + && pci_bus_is_express(pci_dev->bus)) { + int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0, + PCI_EXP_VER2_SIZEOF); + + if (pos > 0) { + pci_dev->exp.exp_cap = pos; + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; + } + } + virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); if (k->realize) { k->realize(proxy, errp);
The virtio devices are converted to PCI-Express if they are plugged into a PCI-Express bus and the 'modern' protocol is enabled. Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- This is an RFC because all it does it adds the PCIe capability and nothing more. I post it early so I can get feedbacks on what is the best way to continue it. Any comments would be appreciated, Thanks, Marcel hw/virtio/virtio-pci.c | 11 +++++++++++ 1 file changed, 11 insertions(+)