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

[khng]

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 <[e-mail]>
MFC after: 2 weeks

Test Plan

FreeBSD guest

[khng]

Fix the differential

[cem]

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.

[alfredo]

sys/dev/virtio/pci/virtio_pci.c

381

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?

[khng]

sys/dev/virtio/pci/virtio_pci.c

381

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

[jhb]

sys/dev/virtio/pci/virtio_pci.c

335

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.

[khng]

• Mark virtio-pci devices MSI-X capable if all required resources are allocated

[khng]

• Fix formatting

[khng]

Re-include the whole history.

[jhb]

sys/dev/virtio/pci/virtio_pci.c

334–335

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.

337

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.

[alfredo]

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

[bryanv]

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.

[khng]

• Move all the msix_load logic into PCIY_MSIX capability check

[khng]

sys/dev/virtio/pci/virtio_pci.c

334–335

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

[jhb]

In D26933#617195, @bryanv 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?

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

334–335

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))

[bryanv]

In D26933#618091, @jhb wrote:

In D26933#617195, @bryanv 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;
}

[jhb]

In D26933#618110, @bryanv wrote:

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.

[khng]

• Address jhb@'s comment.

[khng]

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]

• Factor the MSI-X path out of vtpci_attach

[jhb]

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.

[khng]

• Shorten vtpci_attach_probe_msix and change return type to bool

[khng]

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.

[jhb]

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.

[number201724_me.com]

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

[alfredo]

In D26933#635874, @number201724_me.com wrote:

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).

[khng]

In D26933#636939, @alfredo wrote:

In D26933#635874, @number201724_me.com wrote:

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.

[khng]

This is abandoned and superseded by https://reviews.freebsd.org/D28817.