[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio de
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices |
Date: |
Tue, 13 Oct 2015 17:47:22 +0300 |
On Tue, Oct 13, 2015 at 05:31:27PM +0300, Marcel Apfelbaum wrote:
>
>
> On Monday, October 12, 2015, Michael S. Tsirkin <address@hidden> 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 <address@hidden>
> > ---
> >
> > 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
>
>