Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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.
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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/9385fa60-fa5d-f559-a137-6608408f8...@suse.com/ >> >> ...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.
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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/9385fa60-fa5d-f559-a137-6608408f8...@suse.com/ > > ...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. Jan
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
On 25.05.22 09:45, 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.
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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. > >
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
On 5/3/22 9:22 AM, Juergen Gross wrote:
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
---
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;
This patch is advertised as a fix for
bdd8b6c98239 ("drm/i915: replace X86_FEATURE_PAT with pat_enabled()")
bdd8b6c98239 causes a serious regression on my system when
running Linux as a Dom0 on Xen.
The regression is that on my system, the
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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
Re: [REGRESSION} Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
On 5/20/2022 1:13 PM, Chuck Zmudzinski wrote: I think this summary of the regression is appropriate for a top-post. Details follow below. commit bdd8b6c98239: introduced what I call a real regression which persists in 5.17.x Jan's proposed patch: https://lore.kernel.org/lkml/9385fa60-fa5d-f559-a137-6608408f8...@suse.com/ Jan's patch would fix the real regression introduced by bdd8b6c98239 when the nopat option is not enabled, but when the nopat option is enabled, this patch would introduce what Jan calls a "perceived regression" that is really caused by the failure of the i915 driver to handle the case of the nopat option being provided on the command line properly. What I request: commit Jan's proposed patch, and backport it to 5.17. That would fix the real regression and only cause a perceived regression for the case when nopat is enabled. In that case, patches to the i915 driver would be helpful but necessary to fix a regression. Sorry again, I mean patches to i915 would be helpful but *not* necessary to fix a regression. Regards, Chuck Zmudzinski On 5/20/2022 11:46 AM, 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. 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
Re: [REGRESSION} Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
I think this summary of the regression is appropriate for a top-post. Details follow below. commit bdd8b6c98239: introduced what I call a real regression which persists in 5.17.x Jan's proposed patch: https://lore.kernel.org/lkml/9385fa60-fa5d-f559-a137-6608408f8...@suse.com/ Jan's patch would fix the real regression introduced by bdd8b6c98239 when the nopat option is not enabled, but when the nopat option is enabled, this patch would introduce what Jan calls a "perceived regression" that is really caused by the failure of the i915 driver to handle the case of the nopat option being provided on the command line properly. What I request: commit Jan's proposed patch, and backport it to 5.17. That would fix the real regression and only cause a perceived regression for the case when nopat is enabled. In that case, patches to the i915 driver would be helpful but necessary to fix a regression. Regard, Chuck Zmudzinski On 5/20/2022 11:46 AM, 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. 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
[REGRESSION} Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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.0 -0400
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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 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? 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. I respond by saying a well-written driver should refuse to honor the incorrect configuration requested by the user and instead warn the user that it did not honor the incorrect kernel option. I am only presuming what your patch would do on my box based on what I
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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). Jan
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 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? 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
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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:
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.
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
...
--- 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()
Besides this and ...
--- 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;
... 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").
Jan
but because of the confusion,
As I just wrote earlier, the confusion is whether or not "nopat"
means the kernel drivers will not use pat even if the firmware
and hypervisor provides it. I think you are correct to
point out that is the way the i915 driver behaved with the nopat
option before bdd8b6c98239 was applied, with the same
bad effects on bare metal as with the hypervisor. I think perhaps
dealing with the nopat option to fix bdd8b6c98239 is a solution in
search of a problem, at least as regards the i915 driver.
The only problem we have, as I see it, is with a false negative
when the nopat option is *not* enabled. But the forced disabling
of pat in Jan's patch when the nopat option is enabled is probably
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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:
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.
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
>>> ...
>>>
--- 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()
>>>
>>> Besides this and ...
>>>
--- 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;
>>>
>>> ... 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").
Jan
> but because of the confusion, maybe
> a warning could be printed with the nopat option when a
> hypervisor provides the feature so the user can at least have a
> knob to tweak if if does not behave the way the user intends.
> But I must admit, I don't know if the Xen hypervisor has an
> option also to disable pat. If not, then maybe Jan's more
> aggressive approach with nopat might be needed if for
>
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
On 5/20/22 12:43 AM, 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:
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.
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
...
... 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, but because of the confusion,
The confusion is: does "nopat" only mean the kernel does not
provide pat to device drivers, or does it mean kernel drivers
are not to use pat even if the hypervisor provides it?
I think the original purpose of bdd8b6c98239 was to
enable "nopat" to disable the use or pat in the i915 driver
even if the feature is present from either the kernel or the
hypervisor. This interpretation of the meaning of "nopat"
would favor Jan's approach, I think.
Chuck
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
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:
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.
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
...
--- 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()
Besides this and ...
--- 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;
... 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, but because of the confusion, maybe
a warning could be printed with the nopat option when a
hypervisor provides the feature so the user can at least have a
knob to tweak if if does not behave the way the user intends.
But I must admit, I don't know if the Xen hypervisor has an
option also to disable pat. If not, then maybe Jan's more
aggressive approach with nopat might be needed if for
some reason pat really needs to be disabled in the Linux
when Linux is running on Xen or another hypervisor, but I don't
know of any cases when that would be needed.
Chuck
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
On 5/19/22 10:15 PM, Chuck Zmudzinski wrote: On 5/3/22 9:22 AM, Juergen Gross wrote: 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. Hello, I am also getting a false positive Sorry, I meant false negative here, not false positive. Chuck in a Xen Dom0 from pat_enabled() where bdd8b6c98239 patched the file drivers/gpu/drm/i915/gem/i915_gem_pages.c I think this patch also needs to touch that file to fix the issue I am seeing. ...
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
On 5/3/22 9:22 AM, Juergen Gross wrote:
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.
Hello,
I am also getting a false positive in a Xen Dom0 from
pat_enabled() where bdd8b6c98239 patched the file
drivers/gpu/drm/i915/gem/i915_gem_pages.c
I think this patch also needs to touch that file to
fix the issue I am seeing.
Ever since bdd8b6c98239 was committed, I get the
following in the logs when running as a Dom0 on my
Haswell processor, including with the untainted
official Debian build of 5.17.6:
May 15 06:31:59 debian kernel: [ 3.721146] i915 :00:02.0:
[drm:add_taint_for_CI [i915]] CI tainted:0x9 by intel_gt_init+0xb6/0x2e0
[i915]
This causes the system to hang with the backlight on.
The only recovery is by hitting the reset button and
rebooting Linux Dom0 on Xen with Linux version 5.16
or earlier, or by rebooting Linux version 5.17 without
Xen.
I was able to fix it with a kernel that fixes the
false negative I was getting in
drivers/gpu/drm/i915/gem/i915_gem_pages.c
Can the patch also touch that file, replacing
pat_enabled() with x86_has_pat_wc() in the
place where bdd8b6c98239 patched that file?
Thanks,
Chuck Zmudzinski
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
---
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
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
On Tue, May 03, 2022 at 03:22:07PM +0200, Juergen Gross wrote: > 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. No, we never export unused functionality.
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
On 04.05.2022 11:14, Juergen Gross wrote:
> On 04.05.22 10:31, Jan Beulich wrote:
>> On 03.05.2022 15:22, Juergen Gross wrote:
>>> 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.
>>
>> While, as per my earlier patch, I agree with the Xen PV case, I'm not
>> convinced "nopat" is supposed to honor firmware-provided settings. In
>> fact in my patch I did arrange for "nopat" to also take effect under
>> Xen PV.
>
> Depends on what the wanted semantics for "nopat" are.
>
> Right now "nopat" will result in the PAT MSR left unchanged and the
> cache mode translation tables be initialized accordingly.
>
> So does "nopat" mean that the PAT MSR shouldn't be changed, or that
> PAGE_BIT_PAT will never be set?
According to the documentation for the option ("Disable PAT (page
attribute table extension of pagetables) support") I'd say the latter.
Jan
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
On 04.05.22 10:31, Jan Beulich wrote:
On 03.05.2022 15:22, Juergen Gross wrote:
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.
While, as per my earlier patch, I agree with the Xen PV case, I'm not
convinced "nopat" is supposed to honor firmware-provided settings. In
fact in my patch I did arrange for "nopat" to also take effect under
Xen PV.
Depends on what the wanted semantics for "nopat" are.
Right now "nopat" will result in the PAT MSR left unchanged and the
cache mode translation tables be initialized accordingly.
So does "nopat" mean that the PAT MSR shouldn't be changed, or that
PAGE_BIT_PAT will never be set?
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
I think this wants a Reported-by as well.
Okay.
--- 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()
Besides this and ...
--- 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;
... 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.
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.
kvm_is_mmio_pfn() should not really matter at least for the Xen case.
With the other use cases in memtype.c I'm rather on the edge.
In case the x86 maintainers think those should be changed, too, I agree
that your approach might be the better one.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 2/2] x86/pat: add functions to query specific cache mode availability
On 03.05.2022 15:22, Juergen Gross wrote:
> 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.
While, as per my earlier patch, I agree with the Xen PV case, I'm not
convinced "nopat" is supposed to honor firmware-provided settings. In
fact in my patch I did arrange for "nopat" to also take effect under
Xen PV.
> 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
I think this wants a Reported-by as well.
> --- 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()
Besides this and ...
> --- 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;
... 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.
Jan
[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
---
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
