Page MenuHomeFreeBSD

use a better method to get BAR type in pci_resource_flags()
ClosedPublic

Authored by decui_microsoft.com on Sep 29 2016, 10:07 AM.

Details

Summary

FreeBSD supports lazy allocation of PCI BAR address, that is, when a device
driver's attach method is invoked and the device's BAR addresses haven't been
initialized, the attach method should invoke bus_alloc_resource_any(),
which invokes pci_alloc_resource() -> pci_alloc_multi_resource() ->
pci_reserve_map() -> pci_write_bar() to allocate a proper BAR address for the
bar and write the address into the BAR register.

This model works fine for native FreeBSD drivers, but for device drivers
shared with Linux (e.g. dev/mlx5/mlx5_core/mlx5_main.c and
ofed/drivers/net/mlx4/main.c. Both of them use pci_request_regions()), this
doesn't work, because pci_resource_type() -> _pci_get_rle() always returns
NULL, so pci_request_regions() doesn't have the opportunity to invoke
bus_alloc_resource_any().

Luckily, we can still use the per-device "struct pci_map" to get the BAR
type and hence the patch is made.

The patch should not cause any regression.

BTW, the background of the patch is: we're adding PCIe pass-through support
for FreeBSD VM running on Hyper-V (Windows Server 2016) by adding a front-end
PCI bridge driver and the driver is supposed to initialize the addresses of
the PCI BARs of the passed-through device. And, we need to use lazy
allocation of PCI BARs, otherwise, in "devinfo -r", the BAR resources would
be registered to the pci bridge driver rather than the real device driver of
the device, and we'll be forced to bear the burden of managing the resources,
i.e. alocate/release. With the proposed patch, the lazy allocation model can
work fine with the mlx drivers.

Please review the patch (v2).

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

decui_microsoft.com retitled this revision from to use a better method to get BAR type in pci_resource_flags().
decui_microsoft.com updated this object.
decui_microsoft.com edited the test plan for this revision. (Show Details)
hselasky edited edge metadata.Sep 29 2016, 10:10 AM

Hi,

I will test your change shortly.

This change should possibly be MFC'ed to 11-stable aswell.

I'll get back to you.

--HPS

Hi,

There is a bug that we should return a bitmask from pci_resource_flags() and not a value.

We should return (1<<SYS_RES_IOPORT) etc and the corresponding Linux defines should be updated aswell!

--HPS

Hi,
There is a bug that we should return a bitmask from pci_resource_flags() and not a value.
We should return (1<<SYS_RES_IOPORT) etc and the corresponding Linux defines should be updated aswell!
--HPS

Can you please fix the Linux defines first? Then I can rebase this patch.

BTW, we may need to check all the usages carefully, e.g., the below Line 308: the "type" should be indeed SYS_RES_MEMORY rather than (1 << SYS_RES_MEMORY).

302 static inline int
303 pci_request_region(struct pci_dev *pdev, int bar, const char *res_name)
304 {
305 int rid;
306 int type;
307
308 type = pci_resource_flags(pdev, bar);
309 if (type == 0)
310 return (-ENODEV);
311 rid = PCIR_BAR(bar);
312 if (bus_alloc_resource_any(pdev->dev.bsddev, type, &rid,
313 RF_ACTIVE) == NULL)
314 return (-EINVAL);
315 return (0);
316 }
317

Hi,

Please rebase your patch on top of:
https://svnweb.freebsd.org/changeset/base/306451

Thank you!

--HPS

Can you also check that the new defines you use, like SYS_RES_IOPORT, are defined for all platforms, and not only i386 and amd64, because the LinuxKPI is building for all platforms.

Hi,
Please rebase your patch on top of:
https://svnweb.freebsd.org/changeset/base/306451
Thank you!
--HPS

It looks the patch introduces the issue I mentioned just now: in pci_request_region(), passing (1 << SYS_RES_MEMORY) as "type" to bus_alloc_resource_any() is incorrect?

Hi,

pci_request_region() was changed to use new function pci_resource_type() which returns the regular SYS_RES_XXX while pci_resource_flags() returns the shifted version.

I think it should be fine?

--HPS

Can you also check that the new defines you use, like SYS_RES_IOPORT, are defined for all platforms, and not only i386 and amd64, because the LinuxKPI is building for all platforms.

Thanks for reminding me! I'll check this and rebase my patch later tomorrow -- sorry I can't do it right now, because it's late night here and I'll have to finish some family errands tomorrow first. :-)

Hi,
pci_request_region() was changed to use new function pci_resource_type() which returns the regular SYS_RES_XXX while pci_resource_flags() returns the shifted version.
I think it should be fine?
--HPS

Oh, I overlooked the patch.. sorry.

Can you also check that the new defines you use, like SYS_RES_IOPORT, are defined for all platforms, and not only i386 and amd64, because the LinuxKPI is building for all platforms.

Thanks for reminding me! I'll check this and rebase my patch later tomorrow -- sorry I can't do it right now, because it's late night here and I'll have to finish some family errands tomorrow first. :-)

I think the define exists in all the architectures:

Cscope tag: SYS_RES_IOPORT

  1. line filename / context / line 1 42 amd64/include/resource.h <<SYS_RES_IOPORT>> #define SYS_RES_IOPORT 4 2 43 arm/include/resource.h <<SYS_RES_IOPORT>> #define SYS_RES_IOPORT 4 3 43 arm64/include/resource.h <<SYS_RES_IOPORT>> #define SYS_RES_IOPORT 4 4 576 boot/kshim/bsd_kernel.h <<SYS_RES_IOPORT>> #define SYS_RES_IOPORT 4 5 42 i386/include/resource.h <<SYS_RES_IOPORT>> #define SYS_RES_IOPORT 4 6 44 mips/include/resource.h <<SYS_RES_IOPORT>> #define SYS_RES_IOPORT 4 7 42 powerpc/include/resource.h <<SYS_RES_IOPORT>> #define SYS_RES_IOPORT 4 8 43 riscv/include/resource.h <<SYS_RES_IOPORT>> #define SYS_RES_IOPORT 4 9 43 sparc64/include/resource.h <<SYS_RES_IOPORT>> #define SYS_RES_IOPORT 4

Well, I managed to rebase my patch. Will upload it.

decui_microsoft.com updated this object.
decui_microsoft.com edited edge metadata.
decui_microsoft.com removed rS FreeBSD src repository as the repository for this revision.
hselasky accepted this revision.Sep 29 2016, 3:42 PM
hselasky edited edge metadata.

Looks good. Do you have access to commit it?

This revision is now accepted and ready to land.Sep 29 2016, 3:42 PM

Looks good. Do you have access to commit it?

No. Please help to commit it. :-)

I will commit it today, unless hps beats me to it :)

Looks good. Do you have access to commit it?

I am handling it now. Need to build LINTs for i386/amd64 before commit.

Please add

MFC: 1 week

To the commit message.

Thank you!

This revision was automatically updated to reflect the committed changes.