Page MenuHomeFreeBSD

virtio_pci: Use the table BAR and PBA BAR from MSI-X cap
AbandonedPublic

Authored by khng on Oct 24 2020, 4:22 PM.

Details

Reviewers
alfredo
cem
bryanv
jhb
Group Reviewers
bhyve
Summary

The MSI-X resource shouldn't be assumed to be always on BAR1.
The Virtio v1.1 Spec did not specify that MSI-X table and PBA BAR has to
be BAR1 either.

Reported by: Yuan Rui <number201724@me.com>
MFC after: 2 weeks

Test Plan

FreeBSD guest

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 35488
Build 32394: arc lint + arc unit

Event Timeline

khng requested review of this revision.Oct 24 2020, 4:22 PM
khng created this revision.

Looks fine to me. If this compiles, these pointers are effectively unused other than for cleanup, and the whole point is the side effect of activating the resource(s).

The MSI-X doc explicitly says Table structure and PBA array may occupy the same, or distinct, BARs. So this is more correct than assuming both are in BAR 1.

This revision is now accepted and ready to land.Oct 24 2020, 5:12 PM
sys/dev/virtio/pci/virtio_pci.c
379

I'm not aware of all details but on line 315 'sc->vtpci_msix_pba_res' is populated with 'bus_alloc_resource_any' result only 'if (rid != table_bar)'. Do you think similar logic is required when 'bus_release_resource' as well?

sys/dev/virtio/pci/virtio_pci.c
379

It seems sufficient to me, as we never populate sc->vtpci_msix_pba_res otherwise.

jhb added inline comments.
sys/dev/virtio/pci/virtio_pci.c
336–337

If for some reason we allocated the table BAR but not the PBA BAR, this flag would be set incorrectly. Also, this will not work if the table or PBA are in BAR 0 since vtpci_res will have already claimed that BAR. I think you need to only allocate the extra BARs if they are both unique and not BAR 0, and you need to only set the flag if you successfully allocated all the BARs you needed to allocate.

  • Mark virtio-pci devices MSI-X capable if all required resources are allocated
This revision now requires review to proceed.Nov 17 2020, 10:50 AM

Re-include the whole history.

sys/dev/virtio/pci/virtio_pci.c
309–324

I would perhaps move msix_load into this block along with the test for it being '2' so that all of the MSI-X logic is conditional on their being a PCIY_MSIX cap explicitly.

336

I think you can just leave the them allocated and not worry about doing the cleanup until detach. The PCI bus keeps them allocated all the time anyway.

khng marked 2 inline comments as done.Dec 10 2020, 5:03 AM

I tested this patch on powerpc64 with virtio-legacy and it looks good to me.

This revision is now accepted and ready to land.Dec 15 2020, 3:24 PM

This looks odd to me having to push this down into the driver. I don't really like how to code flows with the 'msix_load' variable. I'm not sure what your VMM here is, but I'd much rather only apply this to transitional or modern devices.

  • Move all the msix_load logic into PCIY_MSIX capability check
This revision now requires review to proceed.Dec 15 2020, 4:30 PM
sys/dev/virtio/pci/virtio_pci.c
309–324

I forgot to submit the changes somehow. Now it is marked correctly.

This looks odd to me having to push this down into the driver. I don't really like how to code flows with the 'msix_load' variable. I'm not sure what your VMM here is, but I'd much rather only apply this to transitional or modern devices.

Where else would you put it? It's hard to do in the PCI bus itself because the resource is often used for other things in the driver and you can only bus_alloc_resource() "once" for a given BAR. Most hardware stores the table and PBA in fixed locations which don't require quite this amount of dynamic behavior. In terms of VirtIO drivers, this file seems like the right place to do it?

In terms of msix_load, it's perhaps not ideal, but to replace it you would need something quite convoluted like:

table_rid = pci_msix_table_bar(dev);
...
pba_rid = pci_msix_pba_bar(dev);
...

if ((table_rid ==  PCI_BAR(0) || sc->vtpci_msix_table_res != NULL) &&
    (pba_rid == PCI_BAR(0) || pba_rid == table_rid || sc->vtpci_msix_pba_res != NULL))
        sc->vtpci_flags &= ~VTPCI_FLAG_NO_MSIX;

That seems like quite a mouthful compared to msix_load. Another way to handle it perhaps would be to not set VTPCI_FLAG_NO_MSIX by default but instead only set it if either of the bus_alloc_resource_any calls fail and in the else case when PCIY_MSIX is not present.

sys/dev/virtio/pci/virtio_pci.c
325–334

In theory the PBA could be back in BAR 0 but the table in BAR 1, so I think you'd have to write this as:

if (rid != table_rid && rid != PCI_BAR(0))
In D26933#618091, @jhb wrote:

This looks odd to me having to push this down into the driver. I don't really like how to code flows with the 'msix_load' variable. I'm not sure what your VMM here is, but I'd much rather only apply this to transitional or modern devices.

Where else would you put it? It's hard to do in the PCI bus itself because the resource is often used for other things in the driver and you can only bus_alloc_resource() "once" for a given BAR. Most hardware stores the table and PBA in fixed locations which don't require quite this amount of dynamic behavior. In terms of VirtIO drivers, this file seems like the right place to do it?

OK

In terms of msix_load, it's perhaps not ideal, but to replace it you would need something quite convoluted like:

My suggestion is to hoist the this MSIX related code into a separate function, that can do early returns instead of this counter-flow-control, so the attach method would read like

if (vtpci_something_msix(sc) != 0) {
   		sc->vtpci_flags |= VTPCI_FLAG_NO_MSIX;
}
In D26933#618091, @jhb wrote:

In terms of msix_load, it's perhaps not ideal, but to replace it you would need something quite convoluted like:

My suggestion is to hoist the this MSIX related code into a separate function, that can do early returns instead of this counter-flow-control, so the attach method would read like

if (vtpci_something_msix(sc) != 0) {
   		sc->vtpci_flags |= VTPCI_FLAG_NO_MSIX;
}

That works for me. I would make the helper function return bool.

  • Address jhb@'s comment.
khng marked an inline comment as done.Dec 17 2020, 7:06 PM

My suggestion is to hoist the this MSIX related code into a separate function, that can do early returns instead of this counter-flow-control, so the attach method would read like

if (vtpci_something_msix(sc) != 0) {
   		sc->vtpci_flags |= VTPCI_FLAG_NO_MSIX;
}

I would address this at the morning in my timezone.

khng marked an inline comment as done.Dec 17 2020, 7:07 PM
  • Factor the MSI-X path out of vtpci_attach

This looks ok to me. I would probably make it return bool, shorten the name, and invert the condition though so that the code in attach reads as:

if (!vtpci_probe_msix(dev, sc))
    sc->vtpci_flags |= VTPCI_FLAG_NO_MSIX;

We've generally taken to using bool for new code in the kernel when it makes sense. I think it's also a bit clearer if the function returns true when MSI-X is ok, rather than returning true when it is busted.

This revision is now accepted and ready to land.Dec 30 2020, 9:27 PM
  • Shorten vtpci_attach_probe_msix and change return type to bool
This revision now requires review to proceed.Dec 31 2020, 3:18 AM
In D26933#622385, @jhb wrote:

This looks ok to me. I would probably make it return bool, shorten the name, and invert the condition though so that the code in attach reads as:

if (!vtpci_probe_msix(dev, sc))
    sc->vtpci_flags |= VTPCI_FLAG_NO_MSIX;

We've generally taken to using bool for new code in the kernel when it makes sense. I think it's also a bit clearer if the function returns true when MSI-X is ok, rather than returning true when it is busted.

Yep. Done.

The diff is only the latest change instead of the total change I think, but I assume that was just due to the way it was uploaded.

This revision is now accepted and ready to land.Dec 31 2020, 11:41 PM

can we get it committed and merge before 13.0 release? This patch will support the FreeBSD operating system running on alibabacloud's VM. And need another patch: D26915

can we get it committed and merge before 13.0 release? This patch will support the FreeBSD operating system running on alibabacloud's VM. And need another patch: D26915

Hi!
Would you mind re-spin you patch? The driver has changed since recent merge of virtio-modern (V1.0).

can we get it committed and merge before 13.0 release? This patch will support the FreeBSD operating system running on alibabacloud's VM. And need another patch: D26915

Hi!
Would you mind re-spin you patch? The driver has changed since recent merge of virtio-modern (V1.0).

I will do the re-spin work.