Page MenuHomeFreeBSD

Clear the MEM/PORT_EN bit when updating PCI BAR
ClosedPublic

Authored by decui_microsoft.com on Sep 18 2016, 2:59 AM.

Details

Summary

It's unsafe to update the BAR when the EN bit is set.

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 Clear the MEM/PORT_EN bit when updating PCI BAR.
decui_microsoft.com updated this object.
decui_microsoft.com edited the test plan for this revision. (Show Details)
jhb edited edge metadata.Sep 20 2016, 5:56 PM

In theory the callers should be doing this (e.g. during something like resume where we write all the BARs). In addition, the ROM BAR shouldn't toggle MEMEN for all other memory BARs since it has its own bit. I'd rather we fix the callers that need this changed. There are some places in pci.c that aren't quite correct. Are you using this in out-of-tree changes or are you having issues with the existing uses (e.g. during boot-time probe)?

In D7914#165049, @jhb wrote:

In theory the callers should be doing this (e.g. during something like resume where we write all the BARs). In addition, the ROM BAR shouldn't toggle MEMEN for all other memory BARs since it has its own bit. I'd rather we fix the callers that need this changed. There are some places in pci.c that aren't quite correct. Are you using this in out-of-tree changes or are you having issues with the existing uses (e.g. during boot-time probe)?

It's for WIP Hyper-V PCI bridge for PCI passthrough/SR-IOV. The WIP bridge driver will program the BARs when its children try to allocate RES_MEMORY, however, if the MEMEN is set (e.g. Upon the first RES_MEMORY request w/ RF_ACTIVE, the MEMEN will be set. When second RES_MEMORY request is received, normally for MSI-X, the MEMEN has been set), the hypervisor will ignore the programmed value, the exact path is:
pci_alloc_resource -> pci_reserve_map -> pci_write_bar.

Do you mean we should move the MEMEN clear/set to pci_reserve_map?

jhb added a comment.Sep 21 2016, 5:21 PM
In D7914#165049, @jhb wrote:

In theory the callers should be doing this (e.g. during something like resume where we write all the BARs). In addition, the ROM BAR shouldn't toggle MEMEN for all other memory BARs since it has its own bit. I'd rather we fix the callers that need this changed. There are some places in pci.c that aren't quite correct. Are you using this in out-of-tree changes or are you having issues with the existing uses (e.g. during boot-time probe)?

It's for WIP Hyper-V PCI bridge for PCI passthrough/SR-IOV. The WIP bridge driver will program the BARs when its children try to allocate RES_MEMORY, however, if the MEMEN is set (e.g. Upon the first RES_MEMORY request w/ RF_ACTIVE, the MEMEN will be set. When second RES_MEMORY request is received, normally for MSI-X, the MEMEN has been set), the hypervisor will ignore the programmed value, the exact path is:
pci_alloc_resource -> pci_reserve_map -> pci_write_bar.
Do you mean we should move the MEMEN clear/set to pci_reserve_map?

Yes. Truth be told, what we really should be doing is not setting MEMEN or IOEN unless/until all BARs of a given resource type are programmed/valid. I haven't gotten around to fixing that. For now, doing the disable/enable in pci_reserve_map() should be ok.

In D7914#165285, @jhb wrote:
In D7914#165049, @jhb wrote:

In theory the callers should be doing this (e.g. during something like resume where we write all the BARs). In addition, the ROM BAR shouldn't toggle MEMEN for all other memory BARs since it has its own bit. I'd rather we fix the callers that need this changed. There are some places in pci.c that aren't quite correct. Are you using this in out-of-tree changes or are you having issues with the existing uses (e.g. during boot-time probe)?

It's for WIP Hyper-V PCI bridge for PCI passthrough/SR-IOV. The WIP bridge driver will program the BARs when its children try to allocate RES_MEMORY, however, if the MEMEN is set (e.g. Upon the first RES_MEMORY request w/ RF_ACTIVE, the MEMEN will be set. When second RES_MEMORY request is received, normally for MSI-X, the MEMEN has been set), the hypervisor will ignore the programmed value, the exact path is:
pci_alloc_resource -> pci_reserve_map -> pci_write_bar.
Do you mean we should move the MEMEN clear/set to pci_reserve_map?

Yes. Truth be told, what we really should be doing is not setting MEMEN or IOEN unless/until all BARs of a given resource type are programmed/valid. I haven't gotten around to fixing that. For now, doing the disable/enable in pci_reserve_map() should be ok.

Yeah, agree w/ the complete fix.

We will move the MEMEN/IOEN to pci_reserve_map before the complete fix.

decui_microsoft.com edited edge metadata.
decui_microsoft.com removed rS FreeBSD src repository as the repository for this revision.

Please review v2.
I moved the MEMEN/PORTEN clear/restore from pci_write_bar() to pci_reserve_map().

jhb accepted this revision.Sep 26 2016, 5:36 PM
jhb edited edge metadata.
This revision is now accepted and ready to land.Sep 26 2016, 5:36 PM
This revision was automatically updated to reflect the committed changes.