On 13/03/18 03:22 PM, Sinan Kaya wrote: > It sounds like you have very tight hardware expectations for this to work > at this moment. You also don't want to generalize this code for others and > address the shortcomings. No, that's the way the community has pushed this work. Our original work was very general and we were told it was unacceptable to put the onus on the user and have things break if the hardware doesn't support it. I think that's a reasonable requirement. So the hardware use-cases were wittled down to the ones we can be confident about and support with reasonable changes to the kernel today. > To get you going, you should limit this change to the switch products that you have > validated via white-listing PCI vendor/device ids. Please do not enable this feature > for all other PCI devices or by default. This doesn't seem necessary. We know that all PCIe switches available today support P2P and we are highly confident that any switch that would ever be produced would support P2P. As long as devices are behind a switch you don't need any white listing. This is what the current patch set implements. If you want to start including root ports then you will need a white list (and solve all the other problems I mentioned earlier). > I think your code qualifies as a virus until this issue is resolved (so NAK). That seems a bit hyperbolic... "a virus"??!... please keep things constructive. > You are delivering a general purpose P2P code with a lot of holes in it and > expecting people to jump through it. No, the code prevents users from screwing it up. It just requires a switch in the hardware which is hardly a high bar to jump through (provided you are putting some design thought into your hardware). And given it requires semi-custom hardware today, it's not something that needs to be on by default in any distributor kernel. > Turning security off by default is also not acceptable. Linux requires ACS support > even though you don't care about it for your particular application. That's not true. Linux does not require ACS support. In fact it's already off by default until you specifically turn on the IOMMU. (Which is not always the most obvious thing to enable.) And the only thing it really supports is enforcing isolation between VMs that are using different pieces of hardware in the system. > I'd hate ACS to be broken due to some operating system enabling your CONFIG option. ACS isn't "broken" by enabling the config option. It just makes the IOMMU groups and isolation less granular. (ie. devices behind a switch will be in the same group and not isolated from each-other). Logan
