On 5/20/2022 10:06 AM, Jan Beulich wrote:
On 20.05.2022 15:33, Chuck Zmudzinski wrote:
On 5/20/2022 5:41 AM, Jan Beulich wrote:
On 20.05.2022 10:30, Chuck Zmudzinski wrote:
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 want
leaving unaltered. When preparing my earlier patch I did
inspect them
and came to the conclusion that these all would also better
observe the
adjusted behavior (or else I couldn't have left
pat_enabled() as the
only 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
the
problematic 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.c
I wanted to be rather defensive in my changes, but I agree at
least
the
case 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?
Because that's, in my understanding, is the purpose of "nopat"
(not breaking the driver of course - that's a driver bug -, but
having an effect on the driver).
I wouldn't call it a driver bug, but an incorrect configuration of the
kernel by the user. I presume X86_FEATURE_PAT is required by the
i915 driver
The driver ought to work fine without PAT (and hence without being
able to make WC mappings). It would use UC instead and be slow, but
it ought to work.
I am not an expert, but I think the reason it failed on my box was
because of the requirements of CI. Maybe the driver would fall back
to UC if the add_taint_for_CI function did not halt the entire system
in response to the failed test for PAT when trying to use WC mappings.
and therefore the driver should refuse to disable
it if the user requests to disable it and instead warn the user that
the driver did not disable the feature, contrary to what the user
requested with the nopat option.
In any case, my test did not verify that when nopat is set in Linux
5.16,
the thread takes the same code path as when nopat is not set,
so I am not totally sure that the reason nopat does not break the
i915 driver in 5.16 is that static_cpu_has(X86_FEATURE_PAT)
returns true even when nopat is set. I could test it with a custom
log message in 5.16 if that is necessary.
Are you saying it was wrong for
to return true in 5.16 when the user requests nopat?
No, I'm not saying that. It was wrong for this construct to be used
in the driver, which was fixed for 5.17 (and which had caused the
regression I did observe, leading to the patch as a hopefully least
bad option).
Hmm, the patch I used to fix my box with 5.17.6 used
static_cpu_has(X86_FEATURE_PAT) so the driver could
continue to configure the hardware using WC. This is the
relevant part of the patch I used to fix my box, which includes
extra error logs, (against Debian's official build of 5.17.6):
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c 2022-05-09
03:16:33.000000000 -0400
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c 2022-05-19
15:55:40.339778818 -0400
...
@@ -430,17 +434,23 @@
err = i915_gem_object_wait_moving_fence(obj, true);
if (err) {
ptr = ERR_PTR(err);
+ DRM_ERROR("i915_gem_object_wait_moving_fence error, err
= %d\n", err);
goto err_unpin;
}
- if (GEM_WARN_ON(type == I915_MAP_WC && !pat_enabled()))
+ if (GEM_WARN_ON(type == I915_MAP_WC &&
+ !pat_enabled() && !static_cpu_has(X86_FEATURE_PAT))) {
+ DRM_ERROR("type == I915_MAP_WC && !pat_enabled(), err =
%d\n", -ENODEV);
ptr = ERR_PTR(-ENODEV);
+ }
else if (i915_gem_object_has_struct_page(obj))
ptr = i915_gem_object_map_page(obj, type);
else
ptr = i915_gem_object_map_pfn(obj, type);
- if (IS_ERR(ptr))
+ if (IS_ERR(ptr)) {
+ DRM_ERROR("IS_ERR(PTR) is true, returning a (ptr)
error\n");
goto err_unpin;
+ }
obj->mm.mapping = page_pack_bits(ptr, type);
}
As you can see, adding the static_cpu_has(X86_FEATURE_PAT)
function to the test for PAT restored the behavior of 5.16 on the
Xen hypervisor to 5.17, and that is how I discovered the solution
to this problem on 5.17 on my box.
I think that is
just permitting a bad configuration to break the driver that a
well-written operating system should not allow. The i915 driver
was, in my opinion, correctly ignoring the nopat option in 5.16
because that option is not compatible with the hardware the
i915 driver is trying to initialize and setup at boot time. At least
that is my understanding now, but I will need to test it on 5.16
to be sure I understand it correctly.
Also, AFAICT, your patch would break the driver when the nopat
option is set and only fix the regression introduced by bdd8b6c98239
when nopat is not set on my box, so your patch would
introduce a regression relative to Linux 5.16 and earlier for the
case when nopat is set on my box. I think your point would
be that it is not a regression if it is an incorrect user
configuration.
Again no - my view is that there's a separate, pre-existing issue
in the driver which was uncovered by the change. This may be a
perceived regression, but is imo different from a real one.
Maybe it is only a perceived regression if nopat is set, but
imo bdd8b6c98239 introduced a real regression in 5.17
relative to 5.16 for the correctly and identically configured
case when the nopat option is not set. That is why I still think
it should be reverted and the fix backported to 5.17 until the
regression for the case when nopat is not set is fixed. As I
said before, the i915 driver relies on the memory subsyste
to provide it with an accurate test for the x86 pat feature.
The test the driver used in bdd8b6c98239 gives the i915 driver
a false negative, and that caused a real regression when nopat
is not set. bdd8b6c98239 can be re-applied if we apply your
patch which corrects the false negative that pat_enabled() is
currently providing the i915 driver with. That false negative
from pat_enabled() is not an i915 bug, it is a bug in x86/pat.
Chuck
