diff mbox series

[5/6] x86/mm: only check uniform after calling mtrr_type_lookup()

Message ID 20230207072902.5528-6-jgross@suse.com
State New
Headers
Series x86/mtrr: fix handling with PAT but without MTRR |

Commit Message

Juergen Gross Feb. 7, 2023, 7:29 a.m. UTC 
  Today pud_set_huge() and pmd_set_huge() test for the MTRR type to be
WB or INVALID after calling mtrr_type_lookup(). Those tests can be
dropped, as the only reason to not use a large mapping would be
uniform being 0. Any MTRR type can be accepted as long as it applies
to the whole memory range covered by the mapping, as the alternative
would only be to map the same region with smaller pages instead using
the same PAT type as for the large mapping.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/mm/pgtable.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Borislav Petkov Feb. 7, 2023, 11:42 a.m. UTC | #1 
On Tue, Feb 07, 2023 at 08:29:01AM +0100, Juergen Gross wrote:
> Today pud_set_huge() and pmd_set_huge() test for the MTRR type to be
> WB or INVALID after calling mtrr_type_lookup(). Those tests can be
> dropped, as the only reason to not use a large mapping would be
> uniform being 0. Any MTRR type can be accepted as long as it applies
> to the whole memory range covered by the mapping, as the alternative
> would only be to map the same region with smaller pages instead using
> the same PAT type as for the large mapping.
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/mm/pgtable.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index e4f499eb0f29..7b9c5443d176 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -721,8 +721,7 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
>  	u8 mtrr, uniform;
>  
>  	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
> -	if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
> -	    (mtrr != MTRR_TYPE_WRBACK))
> +	if (!uniform)
>  		return 0;
>  
>  	/* Bail out if we are we on a populated non-leaf entry: */
> @@ -748,8 +747,7 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
>  	u8 mtrr, uniform;
>  
>  	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
> -	if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
> -	    (mtrr != MTRR_TYPE_WRBACK)) {
> +	if (!uniform) {
>  		pr_warn_once("%s: Cannot satisfy [mem %#010llx-%#010llx] with a huge-page mapping due to MTRR override.\n",
>  			     __func__, addr, addr + PMD_SIZE);
>  		return 0;
> -- 

See my reply here:

https://lore.kernel.org/all/Y+DLqV5MfuBJRnb6@zn.tnic

I understand it as WB is ok, for example, even if not uniform. That
thing in mtrr_type_lookup():

        /*
         * Look up the fixed ranges first, which take priority over
         * the variable ranges.
         */
        if ((start < 0x100000) &&
            (mtrr_state.have_fixed) &&
            (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) {
                is_uniform = 0;
                type = mtrr_type_lookup_fixed(start, end);
                goto out;
        }

If that can return WB, then I guess that says it is still ok. Can the
fixed ranges even cover a, at least a PMD? I guess I need to stare at
this more.

Lemme add Toshi who authored that code - he might have a comment or two.

Thx.
Juergen Gross Feb. 7, 2023, 11:54 a.m. UTC | #2 
On 07.02.23 12:42, Borislav Petkov wrote:
> On Tue, Feb 07, 2023 at 08:29:01AM +0100, Juergen Gross wrote:
>> Today pud_set_huge() and pmd_set_huge() test for the MTRR type to be
>> WB or INVALID after calling mtrr_type_lookup(). Those tests can be
>> dropped, as the only reason to not use a large mapping would be
>> uniform being 0. Any MTRR type can be accepted as long as it applies
>> to the whole memory range covered by the mapping, as the alternative
>> would only be to map the same region with smaller pages instead using
>> the same PAT type as for the large mapping.
>>
>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   arch/x86/mm/pgtable.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
>> index e4f499eb0f29..7b9c5443d176 100644
>> --- a/arch/x86/mm/pgtable.c
>> +++ b/arch/x86/mm/pgtable.c
>> @@ -721,8 +721,7 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
>>   	u8 mtrr, uniform;
>>   
>>   	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
>> -	if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
>> -	    (mtrr != MTRR_TYPE_WRBACK))
>> +	if (!uniform)
>>   		return 0;
>>   
>>   	/* Bail out if we are we on a populated non-leaf entry: */
>> @@ -748,8 +747,7 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
>>   	u8 mtrr, uniform;
>>   
>>   	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
>> -	if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
>> -	    (mtrr != MTRR_TYPE_WRBACK)) {
>> +	if (!uniform) {
>>   		pr_warn_once("%s: Cannot satisfy [mem %#010llx-%#010llx] with a huge-page mapping due to MTRR override.\n",
>>   			     __func__, addr, addr + PMD_SIZE);
>>   		return 0;
>> -- 
> 
> See my reply here:
> 
> https://lore.kernel.org/all/Y+DLqV5MfuBJRnb6@zn.tnic
> 
> I understand it as WB is ok, for example, even if not uniform. That
> thing in mtrr_type_lookup():
> 
>          /*
>           * Look up the fixed ranges first, which take priority over
>           * the variable ranges.
>           */
>          if ((start < 0x100000) &&
>              (mtrr_state.have_fixed) &&
>              (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) {
>                  is_uniform = 0;
>                  type = mtrr_type_lookup_fixed(start, end);
>                  goto out;
>          }
> 
> If that can return WB, then I guess that says it is still ok. Can the
> fixed ranges even cover a, at least a PMD? I guess I need to stare at
> this more.

Fixed MTRRs are all below 1MB. So no, they can't cover a PMD.


Juergen
Borislav Petkov Feb. 7, 2023, 12:21 p.m. UTC | #3 
On Tue, Feb 07, 2023 at 12:54:53PM +0100, Juergen Gross wrote:
> Fixed MTRRs are all below 1MB. So no, they can't cover a PMD.

Good, belongs in the commit message.

And we need more code staring like this to make sure nothing else
breaks. As said, upsetting the universe is not easy.

Thx.
Kani, Toshi Feb. 8, 2023, 1:13 a.m. UTC | #4 
On 07.02.23 12:42, Borislav Petkov wrote:
> On Tue, Feb 07, 2023 at 08:29:01AM +0100, Juergen Gross wrote:
> > Today pud_set_huge() and pmd_set_huge() test for the MTRR type to be
> > WB or INVALID after calling mtrr_type_lookup(). Those tests can be
> > dropped, as the only reason to not use a large mapping would be
> > uniform being 0. Any MTRR type can be accepted as long as it applies
> > to the whole memory range covered by the mapping, as the alternative
> > would only be to map the same region with smaller pages instead using
> > the same PAT type as for the large mapping.
:
> Lemme add Toshi who authored that code - he might have a comment or
> two.

The current mtrr_type_look() does not set 'uniform' for MTRR_TYPE_INVALID.
Please also update it to set 'uniform', if not done in other patches.

Toshi
diff mbox series

Patch

diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index e4f499eb0f29..7b9c5443d176 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -721,8 +721,7 @@  int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
 	u8 mtrr, uniform;
 
 	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
-	if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
-	    (mtrr != MTRR_TYPE_WRBACK))
+	if (!uniform)
 		return 0;
 
 	/* Bail out if we are we on a populated non-leaf entry: */
@@ -748,8 +747,7 @@  int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
 	u8 mtrr, uniform;
 
 	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
-	if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
-	    (mtrr != MTRR_TYPE_WRBACK)) {
+	if (!uniform) {
 		pr_warn_once("%s: Cannot satisfy [mem %#010llx-%#010llx] with a huge-page mapping due to MTRR override.\n",
 			     __func__, addr, addr + PMD_SIZE);
 		return 0;