[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
On 5/20/2022 2:59 AM, Chuck Zmudzinski wrote: On 5/20/2022 2:05 AM, Jan Beulich wrote:On 20.05.2022 06:43, Chuck Zmudzinski wrote:On 5/4/22 5:14 AM, Juergen Gross wrote:On 04.05.22 10:31, Jan Beulich wrote:On 03.05.2022 15:22, Juergen Gross wrote:... these uses there are several more. You say nothing on why those wantleaving unaltered. When preparing my earlier patch I did inspect themand came to the conclusion that these all would also better observe theadjusted behavior (or else I couldn't have left pat_enabled() as theonly predicate). In fact, as said in the description of my earlier patch, in my debugging I did find the use in i915_gem_object_pin_map() to be theproblematic one, which you leave alone.Oh, I missed that one, sorry.That is why your patch would not fix my Haswell unless it also touches i915_gem_object_pin_map() in drivers/gpu/drm/i915/gem/i915_gem_pages.cI wanted to be rather defensive in my changes, but I agree at least thecase in arch_phys_wc_add() might want to be changed, too.I think your approach needs to be more aggressive so it will fix all the known false negatives introduced by bdd8b6c98239 such as the one in i915_gem_object_pin_map(). I looked at Jan's approach and I think it would fix the issue with my Haswell as long as I don't use the nopat option. I really don't have a strong opinion on that question, but I think the nopat option as a Linux kernel option, as opposed to a hypervisor option, should only affect the kernel, and if the hypervisor provides the pat feature, then the kernel should not override that,Hmm, why would the kernel not be allowed to override that? Such an override would affect only the single domain where the kernel runs; other domains could take their own decisions. Also, for the sake of completeness: "nopat" used when running on bare metal has the same bad effect on system boot, so there pretty clearly is an error cleanup issue in the i915 driver. But that's orthogonal, and I expect the maintainers may not even care (but tell us "don't do that then"). Actually I just did a test with the last official Debian kernel build of Linux 5.16, that is, a kernel before bdd8b6c98239 was applied. In fact, the nopat option does *not* break the i915 driver in 5.16. That is, with the nopat option, the i915 driver loads normally on both the bare metal and on the Xen hypervisor. That means your presumption (and the presumption of the author of bdd8b6c98239) that the "nopat" option was being observed by the i915 driver is incorrect. Setting "nopat" had no effect on my system with Linux 5.16. So after doing these tests, I am against the aggressive approach of breaking the i915 driver with the "nopat" option because prior to bdd8b6c98239, nopat did not break the i915 driver. Why break it now? Prior to bdd8b6c98239, the i915 driver used static_cpu_has(X86_FEATURE_PAT) to test for the PAT feature, and apparently this returns true even if nopat is set, but the new test, pat_enabled(), returns false on the Xen hypervisor even if nopat is not set. That is the only problem I see. The question of nopat should be irrelevant to the i915 driver. It was unfortunate that the author of bdd8b6c98239 mentioned nopat in the commit message when in fact nopat was never intended to be used to break the i915 driver. The i915 driver should ignore the nopat option and decide what to do based solely on the capability of the cpu, firmware, and the compiled options of the Linux kernel. That is how it behaved before bdd8b6c98239, and that behavior is what needs to be restored with a patch. Chuck
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |