LinuxLists.cc - [PATCH 2/2] x86/pat: add functions to query specific cache mode availability

2022-05-03 17:27:51

Subject: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability

Some drivers are using pat_enabled() in order to test availability of
special caching modes (WC and UC-). This will lead to false negatives
in case the system was booted e.g. with the "nopat" variant and the
BIOS did setup the PAT MSR supporting the queried mode, or if the
system is running as a Xen PV guest.

Add test functions for those caching modes instead and use them at the
appropriate places.

For symmetry reasons export the already existing x86_has_pat_wp() for
modules, too.

Fixes: bdd8b6c98239 ("drm/i915: replace X86_FEATURE_PAT with pat_enabled()")
Fixes: ae749c7ab475 ("PCI: Add arch_can_pci_mmap_wc() macro")
Signed-off-by: Juergen Gross <[email protected]>
---
arch/x86/include/asm/memtype.h | 2 ++
arch/x86/include/asm/pci.h | 2 +-
arch/x86/mm/init.c | 25 +++++++++++++++++++++---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 8 ++++----
4 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/memtype.h b/arch/x86/include/asm/memtype.h
index 9ca760e430b9..d00e0be854d4 100644
--- a/arch/x86/include/asm/memtype.h
+++ b/arch/x86/include/asm/memtype.h
@@ -25,6 +25,8 @@ extern void memtype_free_io(resource_size_t start, resource_size_t end);
extern bool pat_pfn_immune_to_uc_mtrr(unsigned long pfn);

bool x86_has_pat_wp(void);
+bool x86_has_pat_wc(void);
+bool x86_has_pat_uc_minus(void);
enum page_cache_mode pgprot2cachemode(pgprot_t pgprot);

#endif /* _ASM_X86_MEMTYPE_H */
diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
index f3fd5928bcbb..a5742268dec1 100644
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -94,7 +94,7 @@ int pcibios_set_irq_routing(struct pci_dev *dev, int pin, int irq);

#define HAVE_PCI_MMAP
-#define arch_can_pci_mmap_wc() pat_enabled()
+#define arch_can_pci_mmap_wc() x86_has_pat_wc()
#define ARCH_GENERIC_PCI_MMAP_RESOURCE

#ifdef CONFIG_PCI
diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 71e182ebced3..b6431f714dc2 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -77,12 +77,31 @@ static uint8_t __pte2cachemode_tbl[8] = {
[__pte2cm_idx(_PAGE_PWT | _PAGE_PCD | _PAGE_PAT)] = _PAGE_CACHE_MODE_UC,
};

-/* Check that the write-protect PAT entry is set for write-protect */
+static bool x86_has_pat_mode(unsigned int mode)
+{
+ return __pte2cachemode_tbl[__cachemode2pte_tbl[mode]] == mode;
+}
+
+/* Check that PAT supports write-protect */
bool x86_has_pat_wp(void)
{
- return __pte2cachemode_tbl[__cachemode2pte_tbl[_PAGE_CACHE_MODE_WP]] ==
- _PAGE_CACHE_MODE_WP;
+ return x86_has_pat_mode(_PAGE_CACHE_MODE_WP);
+}
+EXPORT_SYMBOL_GPL(x86_has_pat_wp);
+
+/* Check that PAT supports WC */
+bool x86_has_pat_wc(void)
+{
+ return x86_has_pat_mode(_PAGE_CACHE_MODE_WC);
+}
+EXPORT_SYMBOL_GPL(x86_has_pat_wc);
+
+/* Check that PAT supports UC- */
+bool x86_has_pat_uc_minus(void)
+{
+ return x86_has_pat_mode(_PAGE_CACHE_MODE_UC_MINUS);
}
+EXPORT_SYMBOL_GPL(x86_has_pat_uc_minus);

enum page_cache_mode pgprot2cachemode(pgprot_t pgprot)
{
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 0c5c43852e24..f43ecf3f63eb 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -76,7 +76,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
if (args->flags & ~(I915_MMAP_WC))
return -EINVAL;

- if (args->flags & I915_MMAP_WC && !pat_enabled())
+ if (args->flags & I915_MMAP_WC && !x86_has_pat_wc())
return -ENODEV;

obj = i915_gem_object_lookup(file, args->handle);
@@ -757,7 +757,7 @@ i915_gem_dumb_mmap_offset(struct drm_file *file,

if (HAS_LMEM(to_i915(dev)))
mmap_type = I915_MMAP_TYPE_FIXED;
- else if (pat_enabled())
+ else if (x86_has_pat_wc())
mmap_type = I915_MMAP_TYPE_WC;
else if (!i915_ggtt_has_aperture(to_gt(i915)->ggtt))
return -ENODEV;
@@ -813,7 +813,7 @@ i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
break;

case I915_MMAP_OFFSET_WC:
- if (!pat_enabled())
+ if (!x86_has_pat_wc())
return -ENODEV;
type = I915_MMAP_TYPE_WC;
break;
@@ -823,7 +823,7 @@ i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
break;

case I915_MMAP_OFFSET_UC:
- if (!pat_enabled())
+ if (!x86_has_pat_uc_minus())
return -ENODEV;
type = I915_MMAP_TYPE_UC;
break;
--
2.35.3

2022-05-04 19:58:46

On 25.05.22 10:37, Jan Beulich wrote:
> On 25.05.2022 09:45, Thorsten Leemhuis wrote:
>> On 24.05.22 20:32, Chuck Zmudzinski wrote:
>>> On 5/21/22 6:47 AM, Thorsten Leemhuis wrote:
>>>> I'm not a developer and I'm don't known the details of this thread and
>>>> the backstory of the regression, but it sounds like that's the approach
>>>> that is needed here until someone comes up with a fix for the regression
>>>> exposed by bdd8b6c98239.
>>>>
>>>> But if I'm wrong, please tell me.
>>>
>>> You are mostly right, I think. Reverting bdd8b6c98239 fixes
>>> it. There is another way to fix it, though.
>>
>> Yeah, I'm aware of it. But it seems...
>>
>>> The patch proposed
>>> by Jan Beulich also fixes the regression on my system, so as
>>> the person reporting this is a regression, I would also be satisfied
>>> with Jan's patch instead of reverting bdd8b6c98239 as a fix. Jan
>>> posted his proposed patch here:
>>>
>>> https://lore.kernel.org/lkml/[email protected]/
>>
>> ...that approach is not making any progress either?
>>
>> Jan, can could provide a short status update here? I'd really like to
>> get this regression fixed one way or another rather sooner than later,
>> as this is taken way to long already IMHO.
>
> What kind of status update could I provide? I've not heard back from
> anyone of the maintainers, so I have no way to know what (if anything)
> I need to do.

That is perfectly fine as a status update for me (I track a lot of
regression and it's easy to miss updated patches, discussion in other
places, and things like that).

Could you maybe send a reminder to the maintainer that this is a fix for
regression that is bothering people and needs to be handled with high
priority? Feel free to tell them the Linux kernel regression tracker is
pestering you because things are taken so long. :-D

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

P.S.: As the Linux kernel's regression tracker I deal with a lot of
reports and sometimes miss something important when writing mails like
this. If that's the case here, don't hesitate to tell me in a public
reply, it's in everyone's interest to set the public record straight.

2022-05-26 00:08:51

On 5/25/2022 3:45 AM, Thorsten Leemhuis wrote:
> On 24.05.22 20:32, Chuck Zmudzinski wrote:
>> On 5/21/22 6:47 AM, Thorsten Leemhuis wrote:
>>> On 20.05.22 16:48, Chuck Zmudzinski wrote:
>>>> 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.
>>>>>
>>>>>> 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 static_cpu_has(X86_FEATURE_PAT)
>>>>>> 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).
>>>>>
>>>>>> 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.
>>> Sorry, for you maybe, but I'm pretty sure for Linus it's not when it
>>> comes to the "no regressions rule". Just took a quick look at quotes
>>> from Linus
>>> https://www.kernel.org/doc/html/latest/process/handling-regressions.html
>>> and found this statement from Linus to back this up:
>>>
>>> ```
>>> One _particularly_ last-minute revert is the top-most commit (ignoring
>>> the version change itself) done just before the release, and while
>>> it's very annoying, it's perhaps also instructive.
>>>
>>> What's instructive about it is that I reverted a commit that wasn't
>>> actually buggy. In fact, it was doing exactly what it set out to do,
>>> and did it very well. In fact it did it _so_ well that the much
>>> improved IO patterns it caused then ended up revealing a user-visible
>>> regression due to a real bug in a completely unrelated area.
>>> ```
>>>
>>> He said that here:
>>> https://www.kernel.org/doc/html/latest/process/handling-regressions.html
>>>
>>> The situation is of course different here, but similar enough.
>>>
>>>> Since it is a regression, I think for now bdd8b6c98239 should
>>>> be reverted and the fix backported to Linux 5.17 stable until
>>>> the underlying memory subsystem can provide the i915 driver
>>>> with an updated test for the PAT feature that also meets the
>>>> requirements of the author of bdd8b6c98239 without breaking
>>>> the i915 driver.
>>> I'm not a developer and I'm don't known the details of this thread and
>>> the backstory of the regression, but it sounds like that's the approach
>>> that is needed here until someone comes up with a fix for the regression
>>> exposed by bdd8b6c98239.
>>>
>>> But if I'm wrong, please tell me.
>> You are mostly right, I think. Reverting bdd8b6c98239 fixes
>> it. There is another way to fix it, though.
> Yeah, I'm aware of it. But it seems...
>
>> The patch proposed
>> by Jan Beulich also fixes the regression on my system, so as
>> the person reporting this is a regression, I would also be satisfied
>> with Jan's patch instead of reverting bdd8b6c98239 as a fix. Jan
>> posted his proposed patch here:
>>
>> https://lore.kernel.org/lkml/[email protected]/
> ...that approach is not making any progress either?

Jan's approach does fix it on my system. There was some debate
about what the kernel nopat option should do, though. I don't
have a strong opinion on that and would accept Jan's patch
as a fix.

>
> Jan, can could provide a short status update here? I'd really like to
> get this regression fixed one way or another rather sooner than later,
> as this is taken way to long already IMHO.

I hope something is done soon also.

>
>> The only reservation I have about Jan's patch is that the commit
>> message does not clearly explain how the patch changes what
>> the nopat kernel boot option does. It doesn't affect me because
>> I don't use nopat, but it should probably be mentioned in the
>> commit message, as pointed out here:
>>
>> https://lore.kernel.org/lkml/[email protected]/
>>
>>
>> Whatever fix for the regression exposed by bdd8b6c98239 also
>> needs to be backported to the stable versions 5.17 and 5.18.
> Sure.
>
> BTW, as you seem to be familiar with the issue: there is another report
> about a regression WRT to Xen and i915 (that is also not making really
> progress):
> https://lore.kernel.org/lkml/Yn%2FTgj1Ehs%2FBdpHp@itl-email/
>
> It's just a wild guess, but bould this somehow be related?

It could be, but I do not run a GUI in my Xen Dom0, so I have not
seen that issue.

Best regards,

Chuck