Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1340438 > unrolled thread

[tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings tip-bot for Sai Praneeth <tipbot@zytor.com>@zytor.com - 2016-02-23 10:20 +0100
    Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-23 18:50 +0100
      Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-23 19:10 +0100
        Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Borislav Petkov <bp@alien8.de> - 2016-02-23 19:20 +0100
        Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings "H. Peter Anvin" <hpa@zytor.com> - 2016-02-24 03:20 +0100
        Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings "H. Peter Anvin" <hpa@zytor.com> - 2016-02-24 03:20 +0100
        Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Ingo Molnar <mingo@kernel.org> - 2016-02-25 10:00 +0100
      Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> - 2016-02-24 02:00 +0100
        Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-24 03:50 +0100
          Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-24 15:20 +0100
            Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Borislav Petkov <bp@alien8.de> - 2016-02-24 17:30 +0100
              Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-24 17:40 +0100
                Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-24 20:00 +0100
                  Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-24 20:50 +0100
                    Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-24 21:00 +0100
                    Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Sylvain Chouleur <sylvain.chouleur@gmail.com> - 2016-02-29 12:00 +0100
                      Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Matt Fleming <matt@codeblueprint.co.uk> - 2016-03-02 12:30 +0100
                Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-24 20:40 +0100
                  Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-24 21:00 +0100
                    Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Ingo Molnar <mingo@kernel.org> - 2016-02-25 10:10 +0100
                    Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-25 16:30 +0100
              Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-24 17:50 +0100
              Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Borislav Petkov <bp@alien8.de> - 2016-02-24 17:50 +0100
            Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-24 17:50 +0100
              Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-25 17:10 +0100

Page 1 of 2  [1] 2  Next page →

#1340438 — [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

Commit-ID:  397630150632639b3ca5b4414accd5011c45e276
Gitweb:     http://git.kernel.org/tip/397630150632639b3ca5b4414accd5011c45e276
Author:     Sai Praneeth <sai.praneeth.prakhya@intel.com>
AuthorDate: Wed, 17 Feb 2016 12:35:56 +0000
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 22 Feb 2016 08:26:26 +0100

x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

Since EFI page tables can be treated as kernel page tables they should
be global. All the other page mapping functions in pageattr.c set the
_PAGE_GLOBAL bit and we want to avoid inconsistencies when we map a page
in the EFI code paths, for example when that page is split in
__split_large_page(), etc. It also makes it easier to validate that the
EFI region mappings have the correct attributes because there are fewer
differences compared with regular kernel mappings.

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Luis R. Rodriguez <mcgrof@suse.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi Shankar <ravi.v.shankar@intel.com>
Cc: Ricardo Neri <ricardo.neri@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Toshi Kani <toshi.kani@hp.com>
Cc: linux-efi@vger.kernel.org
Link: http://lkml.kernel.org/r/1455712566-16727-4-git-send-email-matt@codeblueprint.co.uk
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/mm/pageattr.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 632d34d..bf312da 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -909,6 +909,20 @@ static void populate_pte(struct cpa_data *cpa,
 
 	pte = pte_offset_kernel(pmd, start);
 
+	/*
+	 * Set the GLOBAL flags only if the PRESENT flag is
+	 * set otherwise pte_present will return true even on
+	 * a non present pte. The canon_pgprot will clear
+	 * _PAGE_GLOBAL for the ancient hardware that doesn't
+	 * support it.
+	 */
+	if (pgprot_val(pgprot) & _PAGE_PRESENT)
+		pgprot_val(pgprot) |= _PAGE_GLOBAL;
+	else
+		pgprot_val(pgprot) &= ~_PAGE_GLOBAL;
+
+	pgprot = canon_pgprot(pgprot);
+
 	while (num_pages-- && start < end) {
 		set_pte(pte, pfn_pte(cpa->pfn, pgprot));
 

[toc] | [next] | [standalone]

#1340904 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Feb 23, 2016 1:09 AM, <&quot;tip-bot for Sai Praneeth
&lt;tipbot@zytor.com&gt;&quot;@zytor.com> wrote:

Something's wrong with tip-bot.  This should say:


commit 397630150632639b3ca5b4414accd5011c45e276
Author: Sai Praneeth <sai.praneeth.prakhya@intel.com>
Date:   Wed Feb 17 12:35:56 2016 +0000

    x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

    Since EFI page tables can be treated as kernel page tables they should
    be global. All the other page mapping functions in pageattr.c set the
    _PAGE_GLOBAL bit and we want to avoid inconsistencies when we map a page
    in the EFI code paths, for example when that page is split in
    __split_large_page(), etc. It also makes it easier to validate that the
    EFI region mappings have the correct attributes because there are fewer
    differences compared with regular kernel mappings.

But the actual patch is:


@@ -909,6 +909,20 @@ static void populate_pte(struct cpa_data *cpa,

        pte = pte_offset_kernel(pmd, start);

+       /*
+        * Set the GLOBAL flags only if the PRESENT flag is
+        * set otherwise pte_present will return true even on
+        * a non present pte. The canon_pgprot will clear
+        * _PAGE_GLOBAL for the ancient hardware that doesn't
+        * support it.
+        */
+       if (pgprot_val(pgprot) & _PAGE_PRESENT)
+               pgprot_val(pgprot) |= _PAGE_GLOBAL;
+       else
+               pgprot_val(pgprot) &= ~_PAGE_GLOBAL;
+
+       pgprot = canon_pgprot(pgprot);
+

The comment is confusing.  This code is setting GLOBAL if PRESENT is
set even if not requested, but the comment is about setting GLOBAL
*only* if PRESENT is set.

Can you explain:

a) Why this wasn't already broken. (were there no callers who set
GLOBAL but not PRESENT?  If there weren't any, why is that part
needed?)

b) Why setting GLOBAL for EFI mappings is useful.

c) Why setting GLOBAL for EFI mappings is safe.  Don't we unmap the
EFI mappings when we're not actively using them in new kernels?  If
so, don't we explicitly want them *not* to be GLOBAL to avoid needing
an extra-expensive global flush?

d) Why this doesn't break any non-EFI code.

--Andy

[toc] | [prev] | [next] | [standalone]

#1340921 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Tue, Feb 23, 2016 at 9:47 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Feb 23, 2016 1:09 AM, <&quot;tip-bot for Sai Praneeth
> &lt;tipbot@zytor.com&gt;&quot;@zytor.com> wrote:
>
> Something's wrong with tip-bot.  This should say:

Yeah, there's about 50 tipbot emails that are just pure garbage. They
don't even show in my mailers, because they are so corrupt.

The raw email has some insane encoding too, for reasons I can't begin to fathom.

This is an example of what tipbot *used* to send out in the headers:

  From: tip-bot for Dave Hansen <tipbot@zytor.com>
  Subject: [tip:mm/pkeys] mm/core, x86/mm/pkeys:
    Add execute-only protection keys support

and this is what it sent out in the last crazy setup:

  From:   =?UTF-8?B?dGlwLWJvdCBmb3IgU2FpIFByYW5lZXRoIDx0aXBib3RAenl0b3IuY29tPg==?=@zytor.com
  Subject:
   =?UTF-8?B?W3RpcDplZmkvY29yZV0geDg2L21tL3BhdDogVXNlIF9QQUdFX0dMT0JBTCBiaXQ=?=
   =?UTF-8?B?IGZvciBFRkkgcGFnZSB0YWJsZSBtYXBwaW5ncw==?=

despite neither subject nor author having any odd characters in them.

(That's just two header lines - all the other ones are corrupt in
similar ways too)

The thing that seems to really make things unreadable is that the
content encoding lines have this corrupted quoting too:

  MIME-Version: =?UTF-8?B?MS4w?=
  Content-Transfer-Encoding: =?UTF-8?B?OGJpdA==?=
  Content-Type: =?UTF-8?B?dGV4dC9wbGFpbjsgY2hhcnNldD1VVEYtOA==?=
  Content-Disposition: =?UTF-8?B?aW5saW5l?=

rather than what it *should* be:

  Content-Transfer-Encoding: 8bit
  Content-Type: text/plain; charset=UTF-8
  Content-Disposition: inline

so the whole header situation is a complete mess.

The fact that you can see the patch at all and comment on the
*contents* of the email is impressive. My mail reader just says "this
is garbage" and shows me nothing at all.

               Linus

[toc] | [prev] | [next] | [standalone]

#1340924 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Tue, Feb 23, 2016 at 10:08:06AM -0800, Linus Torvalds wrote:
> The fact that you can see the patch at all and comment on the
> *contents* of the email is impressive. My mail reader just says "this
> is garbage" and shows me nothing at all.

Yeah, mutt says:

[-- application/x-=?UTF-8?B?dGV4dC9wbGFpbjsgY2hhcnNldD1VVEYtOA==?= is unsupported (use 'v' to view this part) --]

in the mail body. But then one can open it and it defaults to text:

---Attachment: application/x-=?UTF-8?B?dGV4dC9wbGFpbjsgY2hhcnNldD1VVEYtOA==?= (all)
No matching mailcap entry found.  Viewing as text.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1341250

On February 23, 2016 6:09:19 PM PST, "H. Peter Anvin" <hpa@zytor.com> wrote:
>On February 23, 2016 10:08:06 AM PST, Linus Torvalds
><torvalds@linux-foundation.org> wrote:
>>On Tue, Feb 23, 2016 at 9:47 AM, Andy Lutomirski <luto@amacapital.net>
>>wrote:
>>> On Feb 23, 2016 1:09 AM, <&quot;tip-bot for Sai Praneeth
>>> &lt;tipbot@zytor.com&gt;&quot;@zytor.com> wrote:
>>>
>>> Something's wrong with tip-bot.  This should say:
>>
>>Yeah, there's about 50 tipbot emails that are just pure garbage. They
>>don't even show in my mailers, because they are so corrupt.
>>
>>The raw email has some insane encoding too, for reasons I can't begin
>>to fathom.
>>
>>This is an example of what tipbot *used* to send out in the headers:
>>
>>  From: tip-bot for Dave Hansen <tipbot@zytor.com>
>>  Subject: [tip:mm/pkeys] mm/core, x86/mm/pkeys:
>>    Add execute-only protection keys support
>>
>>and this is what it sent out in the last crazy setup:
>>
>>From:  
>>=?UTF-8?B?dGlwLWJvdCBmb3IgU2FpIFByYW5lZXRoIDx0aXBib3RAenl0b3IuY29tPg==?=@zytor.com
>>  Subject:
>>=?UTF-8?B?W3RpcDplZmkvY29yZV0geDg2L21tL3BhdDogVXNlIF9QQUdFX0dMT0JBTCBiaXQ=?=
>>   =?UTF-8?B?IGZvciBFRkkgcGFnZSB0YWJsZSBtYXBwaW5ncw==?=
>>
>>despite neither subject nor author having any odd characters in them.
>>
>>(That's just two header lines - all the other ones are corrupt in
>>similar ways too)
>>
>>The thing that seems to really make things unreadable is that the
>>content encoding lines have this corrupted quoting too:
>>
>>  MIME-Version: =?UTF-8?B?MS4w?=
>>  Content-Transfer-Encoding: =?UTF-8?B?OGJpdA==?=
>>  Content-Type: =?UTF-8?B?dGV4dC9wbGFpbjsgY2hhcnNldD1VVEYtOA==?=
>>  Content-Disposition: =?UTF-8?B?aW5saW5l?=
>>
>>rather than what it *should* be:
>>
>>  Content-Transfer-Encoding: 8bit
>>  Content-Type: text/plain; charset=UTF-8
>>  Content-Disposition: inline
>>
>>so the whole header situation is a complete mess.
>>
>>The fact that you can see the patch at all and comment on the
>>*contents* of the email is impressive. My mail reader just says "this
>>is garbage" and shows me nothing at all.
>>
>>               Linus
>
>Someone decided to change the behavior of the Perl module I used for
>encoding to unconditionally encode almost everything, claiming some
>kind of strict RFC compliance.  An upgrade caused this to happen.  I
>have switched modules to one which should do what one actually wants.

For the record: I implemented escaping only to placate vger's spam filters...
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

[toc] | [prev] | [next] | [standalone]

#1341252

On February 23, 2016 10:08:06 AM PST, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Tue, Feb 23, 2016 at 9:47 AM, Andy Lutomirski <luto@amacapital.net>
>wrote:
>> On Feb 23, 2016 1:09 AM, <&quot;tip-bot for Sai Praneeth
>> &lt;tipbot@zytor.com&gt;&quot;@zytor.com> wrote:
>>
>> Something's wrong with tip-bot.  This should say:
>
>Yeah, there's about 50 tipbot emails that are just pure garbage. They
>don't even show in my mailers, because they are so corrupt.
>
>The raw email has some insane encoding too, for reasons I can't begin
>to fathom.
>
>This is an example of what tipbot *used* to send out in the headers:
>
>  From: tip-bot for Dave Hansen <tipbot@zytor.com>
>  Subject: [tip:mm/pkeys] mm/core, x86/mm/pkeys:
>    Add execute-only protection keys support
>
>and this is what it sent out in the last crazy setup:
>
>From:  
>=?UTF-8?B?dGlwLWJvdCBmb3IgU2FpIFByYW5lZXRoIDx0aXBib3RAenl0b3IuY29tPg==?=@zytor.com
>  Subject:
>=?UTF-8?B?W3RpcDplZmkvY29yZV0geDg2L21tL3BhdDogVXNlIF9QQUdFX0dMT0JBTCBiaXQ=?=
>   =?UTF-8?B?IGZvciBFRkkgcGFnZSB0YWJsZSBtYXBwaW5ncw==?=
>
>despite neither subject nor author having any odd characters in them.
>
>(That's just two header lines - all the other ones are corrupt in
>similar ways too)
>
>The thing that seems to really make things unreadable is that the
>content encoding lines have this corrupted quoting too:
>
>  MIME-Version: =?UTF-8?B?MS4w?=
>  Content-Transfer-Encoding: =?UTF-8?B?OGJpdA==?=
>  Content-Type: =?UTF-8?B?dGV4dC9wbGFpbjsgY2hhcnNldD1VVEYtOA==?=
>  Content-Disposition: =?UTF-8?B?aW5saW5l?=
>
>rather than what it *should* be:
>
>  Content-Transfer-Encoding: 8bit
>  Content-Type: text/plain; charset=UTF-8
>  Content-Disposition: inline
>
>so the whole header situation is a complete mess.
>
>The fact that you can see the patch at all and comment on the
>*contents* of the email is impressive. My mail reader just says "this
>is garbage" and shows me nothing at all.
>
>               Linus

Someone decided to change the behavior of the Perl module I used for encoding to unconditionally encode almost everything, claiming some kind of strict RFC compliance.  An upgrade caused this to happen.  I have switched modules to one which should do what one actually wants.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

[toc] | [prev] | [next] | [standalone]

#1342987 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, Feb 23, 2016 at 9:47 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> > On Feb 23, 2016 1:09 AM, <&quot;tip-bot for Sai Praneeth
> > &lt;tipbot@zytor.com&gt;&quot;@zytor.com> wrote:
> >
> > Something's wrong with tip-bot.  This should say:
> 
> Yeah, there's about 50 tipbot emails that are just pure garbage. They don't even 
> show in my mailers, because they are so corrupt.

It should now all be fixed.

We were unlucky in that I just happened to push out a bunch of new commits after 
the script broke. Normally I'd have noticed this after just a few commits.

Sorry about this!

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1341212 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Tue, 2016-02-23 at 09:47 -0800, Andy Lutomirski wrote:
> On Feb 23, 2016 1:09 AM, <&quot;tip-bot for Sai Praneeth
> &lt;tipbot@zytor.com&gt;&quot;@zytor.com> wrote:
> 
> Something's wrong with tip-bot.  This should say:
> 
> 
> commit 397630150632639b3ca5b4414accd5011c45e276
> Author: Sai Praneeth <sai.praneeth.prakhya@intel.com>
> Date:   Wed Feb 17 12:35:56 2016 +0000
> 
>     x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings
> 
>     Since EFI page tables can be treated as kernel page tables they should
>     be global. All the other page mapping functions in pageattr.c set the
>     _PAGE_GLOBAL bit and we want to avoid inconsistencies when we map a page
>     in the EFI code paths, for example when that page is split in
>     __split_large_page(), etc. It also makes it easier to validate that the
>     EFI region mappings have the correct attributes because there are fewer
>     differences compared with regular kernel mappings.
> 
> But the actual patch is:
> 
> 
> @@ -909,6 +909,20 @@ static void populate_pte(struct cpa_data *cpa,
> 
>         pte = pte_offset_kernel(pmd, start);
> 
> +       /*
> +        * Set the GLOBAL flags only if the PRESENT flag is
> +        * set otherwise pte_present will return true even on
> +        * a non present pte. The canon_pgprot will clear
> +        * _PAGE_GLOBAL for the ancient hardware that doesn't
> +        * support it.
> +        */
> +       if (pgprot_val(pgprot) & _PAGE_PRESENT)
> +               pgprot_val(pgprot) |= _PAGE_GLOBAL;
> +       else
> +               pgprot_val(pgprot) &= ~_PAGE_GLOBAL;
> +
> +       pgprot = canon_pgprot(pgprot);
> +
> 
> The comment is confusing.  This code is setting GLOBAL if PRESENT is
> set even if not requested, but the comment is about setting GLOBAL
> *only* if PRESENT is set.

As you rightly said the code is about making a page GLOBAL if PRESENT is
set and we do set PRESENT bit before mapping so that page is GLOBAL.
This code was taken from the other parts of pageattr.c. The point is
that we don't want differences between whether things were mapped in the
EFI page tables directly (i.e. using populate_pte()) or later split from
large pages via the split_large_page() code path. If this is still
confusing could you please elaborate on it further.

> 
> Can you explain:
> 
> a) Why this wasn't already broken. (were there no callers who set
> GLOBAL but not PRESENT?  If there weren't any, why is that part
> needed?)
> 

I don't think previous implementation is broken and this is not a bug
fix as such. Before this patch some EFI region mappings had GLOBAL bit
set (which followed split large page path) and some aren't (which used
populate_pte). This patch just aligns the mappings done via two
different code paths as mentioned above. As a whole it also maintains
consistency with kernel mappings.

> b) Why setting GLOBAL for EFI mappings is useful.

We did this for consistency among EFI mappings. This has some advantages
as mentioned in the commit message. It also makes it less confusing when
starting at the PGT_DUMP traces if _PAGE_GLOBAL is used consistently.

We don't actually do anything special with _PAGE_GLOBAL in EFI.

> c) Why setting GLOBAL for EFI mappings is safe.  Don't we unmap the
> EFI mappings when we're not actively using them in new kernels?  If
> so, don't we explicitly want them *not* to be GLOBAL to avoid needing
> an extra-expensive global flush?

This is a valid point. I know that EFI runtime regions persist during
and after boot if we have a UEFI firmware and other commits made EFI
regions have separate page table but I am not clear about the effect of
global flush. I think Matt/Boris could comment on it.

> d) Why this doesn't break any non-EFI code.

We touch this code path only when mapping EFI runtime regions to VA
space, i.e. we added pgd field in cpa only as a support for mapping efi
runtime regions.

> --Andy

[toc] | [prev] | [next] | [standalone]

#1341262 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Tue, Feb 23, 2016 at 4:50 PM, Sai Praneeth Prakhya
<sai.praneeth.prakhya@intel.com> wrote:
> On Tue, 2016-02-23 at 09:47 -0800, Andy Lutomirski wrote:
>> On Feb 23, 2016 1:09 AM, <&quot;tip-bot for Sai Praneeth
>> &lt;tipbot@zytor.com&gt;&quot;@zytor.com> wrote:
>>
>> Something's wrong with tip-bot.  This should say:
>>
>>
>> commit 397630150632639b3ca5b4414accd5011c45e276
>> Author: Sai Praneeth <sai.praneeth.prakhya@intel.com>
>> Date:   Wed Feb 17 12:35:56 2016 +0000
>>
>>     x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings
>>
>>     Since EFI page tables can be treated as kernel page tables they should
>>     be global. All the other page mapping functions in pageattr.c set the
>>     _PAGE_GLOBAL bit and we want to avoid inconsistencies when we map a page
>>     in the EFI code paths, for example when that page is split in
>>     __split_large_page(), etc. It also makes it easier to validate that the
>>     EFI region mappings have the correct attributes because there are fewer
>>     differences compared with regular kernel mappings.
>>
>> But the actual patch is:
>>
>>
>> @@ -909,6 +909,20 @@ static void populate_pte(struct cpa_data *cpa,
>>
>>         pte = pte_offset_kernel(pmd, start);
>>
>> +       /*
>> +        * Set the GLOBAL flags only if the PRESENT flag is
>> +        * set otherwise pte_present will return true even on
>> +        * a non present pte. The canon_pgprot will clear
>> +        * _PAGE_GLOBAL for the ancient hardware that doesn't
>> +        * support it.
>> +        */
>> +       if (pgprot_val(pgprot) & _PAGE_PRESENT)
>> +               pgprot_val(pgprot) |= _PAGE_GLOBAL;
>> +       else
>> +               pgprot_val(pgprot) &= ~_PAGE_GLOBAL;
>> +
>> +       pgprot = canon_pgprot(pgprot);
>> +
>>
>> The comment is confusing.  This code is setting GLOBAL if PRESENT is
>> set even if not requested, but the comment is about setting GLOBAL
>> *only* if PRESENT is set.
>
> As you rightly said the code is about making a page GLOBAL if PRESENT is
> set and we do set PRESENT bit before mapping so that page is GLOBAL.
> This code was taken from the other parts of pageattr.c. The point is
> that we don't want differences between whether things were mapped in the
> EFI page tables directly (i.e. using populate_pte()) or later split from
> large pages via the split_large_page() code path. If this is still
> confusing could you please elaborate on it further.

At least the comment should say "Set the GLOBAL flag if and only
if...".  But why is this code here in the first place?  What is
passing a pgprot with global unset into this code in the first place?

>
>>
>> Can you explain:
>>
>> a) Why this wasn't already broken. (were there no callers who set
>> GLOBAL but not PRESENT?  If there weren't any, why is that part
>> needed?)
>>
>
> I don't think previous implementation is broken and this is not a bug
> fix as such. Before this patch some EFI region mappings had GLOBAL bit
> set (which followed split large page path) and some aren't (which used
> populate_pte). This patch just aligns the mappings done via two
> different code paths as mentioned above. As a whole it also maintains
> consistency with kernel mappings.

But your code also *clears* GLOBAL if PRESENT is clear, and your
comment talks about that.  Does this ever actually happen in practice?

I'd much rather see WARN_ON_ONCE((pgprot_val(pgprot) & (_PAGE_PRESENT
| _PAGE_GLOBAL)) == _PAGE_GLOBAL) in here, if that makes sense in the
context of the callers of the function.

>
>> b) Why setting GLOBAL for EFI mappings is useful.
>
> We did this for consistency among EFI mappings. This has some advantages
> as mentioned in the commit message. It also makes it less confusing when
> starting at the PGT_DUMP traces if _PAGE_GLOBAL is used consistently.
>
> We don't actually do anything special with _PAGE_GLOBAL in EFI.

You're making a choice of whether to set _PAGE_GLOBAL, and I think
you've made the wrong choice.

Normally, the only pages with are _PAGE_GLOBAL are those that are in
the normal kernel mappings (swapper_pg_dir and normal mm_struct pgds).
By allowing _PAGE_GLOBAL to be set in EFI mappings, you're breaking
that convention, which forces you to use extra-expensive
__flush_tlb_all calls in efi_call_virt.

I think you should explicitly *clear* _PAGE_GLOBAL in the EFI mappings
instead.  This would allow you to use write_cr3 by itself, which would
make the code simpler and faster.

>
>> c) Why setting GLOBAL for EFI mappings is safe.  Don't we unmap the
>> EFI mappings when we're not actively using them in new kernels?  If
>> so, don't we explicitly want them *not* to be GLOBAL to avoid needing
>> an extra-expensive global flush?
>
> This is a valid point. I know that EFI runtime regions persist during
> and after boot if we have a UEFI firmware and other commits made EFI
> regions have separate page table but I am not clear about the effect of
> global flush. I think Matt/Boris could comment on it.

It's straightfoward on existing kernels.  If _PAGE_GLOBAL is set, TLB
entries persist across cr3 writes.  If _PAGE_GLOBAL is clear, then TLB
entries are flushed by cr3 writes.

With PCID enabled (which is only in a not-quite-ready patch set I
have), the story is a bit more complicated, but it works essentially
the same way unless you explicitly opt out.

>
>> d) Why this doesn't break any non-EFI code.
>
> We touch this code path only when mapping EFI runtime regions to VA
> space, i.e. we added pgd field in cpa only as a support for mapping efi
> runtime regions.

populate_pgd is called from non-EFI code as well though, isn't it?

--Andy

[toc] | [prev] | [next] | [standalone]

#1342054 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Tue, 23 Feb, at 06:43:04PM, Andy Lutomirski wrote:
> On Tue, Feb 23, 2016 at 4:50 PM, Sai Praneeth Prakhya
> <sai.praneeth.prakhya@intel.com> wrote:
> >
> > As you rightly said the code is about making a page GLOBAL if PRESENT is
> > set and we do set PRESENT bit before mapping so that page is GLOBAL.
> > This code was taken from the other parts of pageattr.c. The point is
> > that we don't want differences between whether things were mapped in the
> > EFI page tables directly (i.e. using populate_pte()) or later split from
> > large pages via the split_large_page() code path. If this is still
> > confusing could you please elaborate on it further.
> 
> At least the comment should say "Set the GLOBAL flag if and only
> if...".  But why is this code here in the first place?  What is
> passing a pgprot with global unset into this code in the first place?
 
This comes from populate_pgd(),

  static int populate_pgd(struct cpa_data *cpa, unsigned long addr)
  {
	  pgprot_t pgprot = __pgprot(_KERNPG_TABLE);

> > I don't think previous implementation is broken and this is not a bug
> > fix as such. Before this patch some EFI region mappings had GLOBAL bit
> > set (which followed split large page path) and some aren't (which used
> > populate_pte). This patch just aligns the mappings done via two
> > different code paths as mentioned above. As a whole it also maintains
> > consistency with kernel mappings.
> 
> But your code also *clears* GLOBAL if PRESENT is clear, and your
> comment talks about that.  Does this ever actually happen in practice?
 
Not when mapping EFI regions, no (we explicitly set _PAGE_PRESENT in
kernel_map_pages_in_pgd(), but this logic is duplicated from
__split_large_page() which can be called from non-EFI code.

> I'd much rather see WARN_ON_ONCE((pgprot_val(pgprot) & (_PAGE_PRESENT
> | _PAGE_GLOBAL)) == _PAGE_GLOBAL) in here, if that makes sense in the
> context of the callers of the function.
 
I think that'd be a nice cleanup, along with pulling all the pgprot
twiddling out into a single function.

> > We did this for consistency among EFI mappings. This has some advantages
> > as mentioned in the commit message. It also makes it less confusing when
> > starting at the PGT_DUMP traces if _PAGE_GLOBAL is used consistently.
> >
> > We don't actually do anything special with _PAGE_GLOBAL in EFI.
> 
> You're making a choice of whether to set _PAGE_GLOBAL, and I think
> you've made the wrong choice.
> 
> Normally, the only pages with are _PAGE_GLOBAL are those that are in
> the normal kernel mappings (swapper_pg_dir and normal mm_struct pgds).
> By allowing _PAGE_GLOBAL to be set in EFI mappings, you're breaking
> that convention, which forces you to use extra-expensive
> __flush_tlb_all calls in efi_call_virt.
> 
> I think you should explicitly *clear* _PAGE_GLOBAL in the EFI mappings
> instead.  This would allow you to use write_cr3 by itself, which would
> make the code simpler and faster.
 
This is interesting.

When I suggested to Sai that he write this patch my main motivation
was consistency for all mappings. We've got that now, but perhaps it's
the wrong consistency ;)

If we go with the no-PAGE_GLOBAL approach, we need changes to ensure
we never set _PAGE_GLOBAL for the EFI mappings, because before this
patch was applied sometimes we did and sometimes we didn't, depending
on whether a page was split or not.

I'm racking my brain to think of how your suggestion might have
unintended consequences because diagnosing stale TLB entry bugs is
simply the worst job ever. I can't think of anything. The only
scenarios where we'd see problems is if a) we have new global mappings
in the EFI page tables or b) we have different global mappings.

Since we reference swapper_pg_dir from the PMD level downwards b)
shouldn't be a problem, and the only differences between
swapper_pg_dir and efi_pgd should be the EFI mappings, which saves us
from a).

> > This is a valid point. I know that EFI runtime regions persist during
> > and after boot if we have a UEFI firmware and other commits made EFI
> > regions have separate page table but I am not clear about the effect of
> > global flush. I think Matt/Boris could comment on it.
> 
> It's straightfoward on existing kernels.  If _PAGE_GLOBAL is set, TLB
> entries persist across cr3 writes.  If _PAGE_GLOBAL is clear, then TLB
> entries are flushed by cr3 writes.
 
This is safe for EFI right now because of the big __flush_tlb_all() in
efi_call_virt().

> With PCID enabled (which is only in a not-quite-ready patch set I
> have), the story is a bit more complicated, but it works essentially
> the same way unless you explicitly opt out.
 
Hmm... is series that posted somewhere?

> > We touch this code path only when mapping EFI runtime regions to VA
> > space, i.e. we added pgd field in cpa only as a support for mapping efi
> > runtime regions.
> 
> populate_pgd is called from non-EFI code as well though, isn't it?

Nope. The "if (cpa->pgd)" guard ensures that we only call that
function for the EFI mapping code - no one else sets ->pgd.

[toc] | [prev] | [next] | [standalone]

#1342177 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Wed, Feb 24, 2016 at 02:10:46PM +0000, Matt Fleming wrote:
> > Normally, the only pages with are _PAGE_GLOBAL are those that are in
> > the normal kernel mappings (swapper_pg_dir and normal mm_struct pgds).
> > By allowing _PAGE_GLOBAL to be set in EFI mappings, you're breaking
> > that convention, which forces you to use extra-expensive
> > __flush_tlb_all calls in efi_call_virt.

Hold on, do you mean the __flush_tlb_all() in the CONFIG_EFI_MIXED code?

That's mixed mode. I think you mean the FLUSH_TLB_ALL in efi_call.
That's EFI on 64-bit but that is mandated by the spec, AFAIR.

So the EFI runtime crap should not change once it is mapped. And those
should be global. It is only natural.

> Nope. The "if (cpa->pgd)" guard ensures that we only call that
> function for the EFI mapping code - no one else sets ->pgd.

But it could - there's no guarantee. kernel_map_pages_in_pgd() is an
exported facility.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1342187 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Wed, Feb 24, 2016 at 8:20 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Feb 24, 2016 at 02:10:46PM +0000, Matt Fleming wrote:
>> > Normally, the only pages with are _PAGE_GLOBAL are those that are in
>> > the normal kernel mappings (swapper_pg_dir and normal mm_struct pgds).
>> > By allowing _PAGE_GLOBAL to be set in EFI mappings, you're breaking
>> > that convention, which forces you to use extra-expensive
>> > __flush_tlb_all calls in efi_call_virt.
>
> Hold on, do you mean the __flush_tlb_all() in the CONFIG_EFI_MIXED code?
>
> That's mixed mode. I think you mean the FLUSH_TLB_ALL in efi_call.
> That's EFI on 64-bit but that is mandated by the spec, AFAIR.

I mean the one in efi_call_virt.  Why would the spec mandate a TLB
flush at all?  EFI runtime services have no business touching the
paging structures directly.  Heck, the 32-bit ones don't even know the
*format* of the paging structures.

>
> So the EFI runtime crap should not change once it is mapped. And those
> should be global. It is only natural.

Why is it natural?

Long-term, I'd rather see EFI runtime services use an actual mm_struct
and use_mm.

--Andy

[toc] | [prev] | [next] | [standalone]

#1342343 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Wed, Feb 24, 2016 at 8:36 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> So the EFI runtime crap should not change once it is mapped. And those
>> should be global. It is only natural.
>
> Why is it natural?
>
> Long-term, I'd rather see EFI runtime services use an actual mm_struct
> and use_mm.

Definitely.

The EFI runtime page mapping may be unchanging, but that doesn't mean
we should be mapping it all the time - the mapping may not change, but
we will change away from it.

So marking those pages global is very wrong.

                 Linus

[toc] | [prev] | [next] | [standalone]

#1342383 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Wed, 24 Feb, at 10:56:13AM, Linus Torvalds wrote:
> On Wed, Feb 24, 2016 at 8:36 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> >>
> >> So the EFI runtime crap should not change once it is mapped. And those
> >> should be global. It is only natural.
> >
> > Why is it natural?
> >
> > Long-term, I'd rather see EFI runtime services use an actual mm_struct
> > and use_mm.
> 
> Definitely.
> 
> The EFI runtime page mapping may be unchanging, but that doesn't mean
> we should be mapping it all the time - the mapping may not change, but
> we will change away from it.
 
There is movement towards hanging the EFI memory map off of mm_struct
for x86. ARM and arm64 already do this and there were some patches
from Sylvain (Cc'd) to do this for the purposes of having a task
context that could be preempted while in the middle of an EFI runtime
call for some Intel platforms,

  https://lkml.kernel.org/r/1452702762-27216-4-git-send-email-sylvain.chouleur@gmail.com

Apart from the code simplification and not being required to open-code
the %cr3 diddling, are there other benefits of mm_struct and use_mm()
that make it appealing in the non-preemptible case?

Not that those aren't reasons enough.

> So marking those pages global is very wrong.

Ingo, Andy, how do you want to handle this patch? Maybe just drop it
from tip/efi/core while we prod around making all the EFI mappings
non-global? Nothing else depends on it, it can be dropped without any
harm.

[toc] | [prev] | [next] | [standalone]

#1342399 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Wed, Feb 24, 2016 at 11:45 AM, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Wed, 24 Feb, at 10:56:13AM, Linus Torvalds wrote:
>> On Wed, Feb 24, 2016 at 8:36 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>> >>
>> >> So the EFI runtime crap should not change once it is mapped. And those
>> >> should be global. It is only natural.
>> >
>> > Why is it natural?
>> >
>> > Long-term, I'd rather see EFI runtime services use an actual mm_struct
>> > and use_mm.
>>
>> Definitely.
>>
>> The EFI runtime page mapping may be unchanging, but that doesn't mean
>> we should be mapping it all the time - the mapping may not change, but
>> we will change away from it.
>
> There is movement towards hanging the EFI memory map off of mm_struct
> for x86. ARM and arm64 already do this and there were some patches
> from Sylvain (Cc'd) to do this for the purposes of having a task
> context that could be preempted while in the middle of an EFI runtime
> call for some Intel platforms,
>
>   https://lkml.kernel.org/r/1452702762-27216-4-git-send-email-sylvain.chouleur@gmail.com
>
> Apart from the code simplification and not being required to open-code
> the %cr3 diddling, are there other benefits of mm_struct and use_mm()
> that make it appealing in the non-preemptible case?
>
> Not that those aren't reasons enough.

If we add PCID support, then use_mm will get the benefits (~200ns
savings for a round trip) for free.

>
>> So marking those pages global is very wrong.
>
> Ingo, Andy, how do you want to handle this patch? Maybe just drop it
> from tip/efi/core while we prod around making all the EFI mappings
> non-global? Nothing else depends on it, it can be dropped without any
> harm.

If the patch is harmless as is, I'm okay with letting it stay.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1345643 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

2016-02-24 20:45 GMT+01:00 Matt Fleming <matt@codeblueprint.co.uk>:
> On Wed, 24 Feb, at 10:56:13AM, Linus Torvalds wrote:
>> On Wed, Feb 24, 2016 at 8:36 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>> >>
>> >> So the EFI runtime crap should not change once it is mapped. And those
>> >> should be global. It is only natural.
>> >
>> > Why is it natural?
>> >
>> > Long-term, I'd rather see EFI runtime services use an actual mm_struct
>> > and use_mm.
>>
>> Definitely.
>>
>> The EFI runtime page mapping may be unchanging, but that doesn't mean
>> we should be mapping it all the time - the mapping may not change, but
>> we will change away from it.
>
> There is movement towards hanging the EFI memory map off of mm_struct
> for x86. ARM and arm64 already do this and there were some patches
> from Sylvain (Cc'd) to do this for the purposes of having a task
> context that could be preempted while in the middle of an EFI runtime
> call for some Intel platforms,
>
>   https://lkml.kernel.org/r/1452702762-27216-4-git-send-email-sylvain.chouleur@gmail.com

I was thinking we could use the efi kthread to handle the efi services
generically, not only for the interruptible case, and have a way to decide if we
allow interruptions inside the efi call itself or not.

Then all runtime services would use an mm_struct. The drawback is that you will
need two context switchs to be able to execute the runtime service.

-- 
Sylvain

[toc] | [prev] | [next] | [standalone]

#1347966 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Mon, 29 Feb, at 11:56:56AM, Sylvain Chouleur wrote:
> 2016-02-24 20:45 GMT+01:00 Matt Fleming <matt@codeblueprint.co.uk>:
> > On Wed, 24 Feb, at 10:56:13AM, Linus Torvalds wrote:
> >> On Wed, Feb 24, 2016 at 8:36 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> >> >>
> >> >> So the EFI runtime crap should not change once it is mapped. And those
> >> >> should be global. It is only natural.
> >> >
> >> > Why is it natural?
> >> >
> >> > Long-term, I'd rather see EFI runtime services use an actual mm_struct
> >> > and use_mm.
> >>
> >> Definitely.
> >>
> >> The EFI runtime page mapping may be unchanging, but that doesn't mean
> >> we should be mapping it all the time - the mapping may not change, but
> >> we will change away from it.
> >
> > There is movement towards hanging the EFI memory map off of mm_struct
> > for x86. ARM and arm64 already do this and there were some patches
> > from Sylvain (Cc'd) to do this for the purposes of having a task
> > context that could be preempted while in the middle of an EFI runtime
> > call for some Intel platforms,
> >
> >   https://lkml.kernel.org/r/1452702762-27216-4-git-send-email-sylvain.chouleur@gmail.com
> 
> I was thinking we could use the efi kthread to handle the efi services
> generically, not only for the interruptible case, and have a way to decide if we
> allow interruptions inside the efi call itself or not.
> 
> Then all runtime services would use an mm_struct. The drawback is that you will
> need two context switchs to be able to execute the runtime service.

I would be surprised if the asynchronous nature of having a special
EFI kthread would buy you any benefit in general. And in fact, in the
efi-pstore code you can be invoked in IRQ context and you really don't
want to start talking to a kthread.

[toc] | [prev] | [next] | [standalone]

#1342371 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Wed, 24 Feb, at 08:36:33AM, Andy Lutomirski wrote:
> On Wed, Feb 24, 2016 at 8:20 AM, Borislav Petkov <bp@alien8.de> wrote:
> > On Wed, Feb 24, 2016 at 02:10:46PM +0000, Matt Fleming wrote:
> >> > Normally, the only pages with are _PAGE_GLOBAL are those that are in
> >> > the normal kernel mappings (swapper_pg_dir and normal mm_struct pgds).
> >> > By allowing _PAGE_GLOBAL to be set in EFI mappings, you're breaking
> >> > that convention, which forces you to use extra-expensive
> >> > __flush_tlb_all calls in efi_call_virt.
> >
> > Hold on, do you mean the __flush_tlb_all() in the CONFIG_EFI_MIXED code?
> >
> > That's mixed mode. I think you mean the FLUSH_TLB_ALL in efi_call.
> > That's EFI on 64-bit but that is mandated by the spec, AFAIR.
> 
> I mean the one in efi_call_virt.  Why would the spec mandate a TLB
> flush at all?  EFI runtime services have no business touching the
> paging structures directly.  Heck, the 32-bit ones don't even know the
> *format* of the paging structures.
 
Right, and it would necessitate copying out arguments because the
firmware won't understand where/how the kernel has mapped things.

No firmware is going to be doing that.

[toc] | [prev] | [next] | [standalone]

#1342401 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Wed, Feb 24, 2016 at 11:33 AM, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Wed, 24 Feb, at 08:36:33AM, Andy Lutomirski wrote:
>> On Wed, Feb 24, 2016 at 8:20 AM, Borislav Petkov <bp@alien8.de> wrote:
>> > On Wed, Feb 24, 2016 at 02:10:46PM +0000, Matt Fleming wrote:
>> >> > Normally, the only pages with are _PAGE_GLOBAL are those that are in
>> >> > the normal kernel mappings (swapper_pg_dir and normal mm_struct pgds).
>> >> > By allowing _PAGE_GLOBAL to be set in EFI mappings, you're breaking
>> >> > that convention, which forces you to use extra-expensive
>> >> > __flush_tlb_all calls in efi_call_virt.
>> >
>> > Hold on, do you mean the __flush_tlb_all() in the CONFIG_EFI_MIXED code?
>> >
>> > That's mixed mode. I think you mean the FLUSH_TLB_ALL in efi_call.
>> > That's EFI on 64-bit but that is mandated by the spec, AFAIR.
>>
>> I mean the one in efi_call_virt.  Why would the spec mandate a TLB
>> flush at all?  EFI runtime services have no business touching the
>> paging structures directly.  Heck, the 32-bit ones don't even know the
>> *format* of the paging structures.
>
> Right, and it would necessitate copying out arguments because the
> firmware won't understand where/how the kernel has mapped things.
>
> No firmware is going to be doing that.

Just so I understand correctly: could we get away with putting the EFI
virtual runtime mappings at positive (user) addresses for 64-bit UEFI,
or is there some reason that we need the high bit set?

If we could use positive addresses, then we could use the existing
use_mm infrastructure directly with no funny business at all except to
the extent that we might need to use unusual APIs to set up the VMAs
(if we use real VMAs) in the first place.  (We could cheat and
allocate a single monstrous VM_MIXEDMAP or VM_PFNMAP vma with a .fault
handler that always fails.)  If we have to use negative addresses,
then we'll always be stuck with a funny pgd, but we could still
probably use use_mm instead of manually fiddling with cr3.

Some day I want to experiment with calling runtime services at CPL 3,
too :)  We'd want to add some infrastructure to permit kernel threads
to run through the entry/exit code as if they were user processes, but
there's nothing conceptually wrong with that.  We already allow kernel
threads to call execve and "return" to real user mode, so it's not
much of a stretch.  The main issue would be dealing with signal
handling and such -- we'd want to report faults back to the kernel
thread's CPL3-invocation thunks rather than delivering a signal at CPL
3.

Hmm, now it's time to muse about how the interface would work.  If we
kept it in line with existing practice, we'd add an API to make a
special kernel thread with an attached user context.  To enter CPL3,
you'd return from the thread's main function.  When user mode was done
(fault or syscall), new hooks in the entry code (similar to seccomp
and the die notifier stuff) would re-enter the main function with some
arguments indicating what happened.

If we wanted to make it a bit easier to use, we'd have to allocate an
extra kernel sack, and we could have:

void invoke_cpl3(struct cpl3_context *ctx);

where ctx contains memory for an extra stack as well as a bunch of
data indicating the reason that it returned.

The latter is harder to implement but probably much easier to use.

If anyone wants to work on this, ping me and I'll help and do a bunch of review.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1343000 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

* Andy Lutomirski <luto@amacapital.net> wrote:

> >> I mean the one in efi_call_virt.  Why would the spec mandate a TLB flush at 
> >> all?  EFI runtime services have no business touching the paging structures 
> >> directly.  Heck, the 32-bit ones don't even know the *format* of the paging 
> >> structures.
> >
> > Right, and it would necessitate copying out arguments because the firmware 
> > won't understand where/how the kernel has mapped things.
> >
> > No firmware is going to be doing that.
> 
> Just so I understand correctly: could we get away with putting the EFI virtual 
> runtime mappings at positive (user) addresses for 64-bit UEFI, or is there some 
> reason that we need the high bit set?
> 
> If we could use positive addresses, then we could use the existing use_mm 
> infrastructure directly with no funny business at all except to the extent that 
> we might need to use unusual APIs to set up the VMAs (if we use real VMAs) in 
> the first place.  (We could cheat and allocate a single monstrous VM_MIXEDMAP or 
> VM_PFNMAP vma with a .fault handler that always fails.)  If we have to use 
> negative addresses, then we'll always be stuck with a funny pgd, but we could 
> still probably use use_mm instead of manually fiddling with cr3.

Would be nice to get an answer to these questions. The more we isolate firmware 
execution into 'regular' MM concepts, the more robust it all becomes.

> Some day I want to experiment with calling runtime services at CPL 3, too :)

That would be an interesting isolation method as well ...

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web