x86/boot: Don't explicitly map the VGA region as UC-
diff mbox series

Message ID 20190905190418.15142-1-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86/boot: Don't explicitly map the VGA region as UC-
Related show

Commit Message

Andrew Cooper Sept. 5, 2019, 7:04 p.m. UTC
All 64-bit capable processors use PAT, and with PAT, it is explicitly
permitted to have mappings with a cacheability different to MTRRs.

On a native system with a real legacy VGA region, MTRRs will cause the region
to be UC-.  When booting virtualised, this range may be RAM and not a legacy
VGA region, at which point it wants to be WB.

Use WB mapping in the pagetables, such that in systems without a legacy VGA
region, the RAM between 0xa0000 and 0xc0000 doesn't become unnecesserily UC-.
However, we still must use small pages to avoid the undefined behaviour caused
by superpages crossing MTRRs of different cacheability.

While adjusting the pagetable construction, switch from pfn to idx for
consistency with all the other construction logic.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

As a future optimisation, Xen could rewrite l2_identmap with a superpage if it
finds that MTRRs are disabled.  However, this probably needs to wait until Xen
no longer unconditionally assumes a legacy VGA region to be present in the
e820 if it finds something other than a hole
---
 xen/arch/x86/boot/x86_64.S | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

Comments

Roger Pau Monné Sept. 6, 2019, 11:06 a.m. UTC | #1
On Thu, Sep 05, 2019 at 08:04:18PM +0100, Andrew Cooper wrote:
> All 64-bit capable processors use PAT, and with PAT, it is explicitly
> permitted to have mappings with a cacheability different to MTRRs.
> 
> On a native system with a real legacy VGA region, MTRRs will cause the region
> to be UC-.  When booting virtualised, this range may be RAM and not a legacy
> VGA region, at which point it wants to be WB.
> 
> Use WB mapping in the pagetables, such that in systems without a legacy VGA
> region, the RAM between 0xa0000 and 0xc0000 doesn't become unnecesserily UC-.
> However, we still must use small pages to avoid the undefined behaviour caused
> by superpages crossing MTRRs of different cacheability.
> 
> While adjusting the pagetable construction, switch from pfn to idx for
> consistency with all the other construction logic.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> As a future optimisation, Xen could rewrite l2_identmap with a superpage if it
> finds that MTRRs are disabled.  However, this probably needs to wait until Xen
> no longer unconditionally assumes a legacy VGA region to be present in the
> e820 if it finds something other than a hole

Is that still the case? I remember dealing with it when working on the
shim, and I thought the code to unconditionally set a hole in
0xa0000-0xc0000 was removed.

Roger.
Andrew Cooper Sept. 6, 2019, 11:29 a.m. UTC | #2
On 06/09/2019 12:06, Roger Pau Monné wrote:
> On Thu, Sep 05, 2019 at 08:04:18PM +0100, Andrew Cooper wrote:
>> All 64-bit capable processors use PAT, and with PAT, it is explicitly
>> permitted to have mappings with a cacheability different to MTRRs.
>>
>> On a native system with a real legacy VGA region, MTRRs will cause the region
>> to be UC-.  When booting virtualised, this range may be RAM and not a legacy
>> VGA region, at which point it wants to be WB.
>>
>> Use WB mapping in the pagetables, such that in systems without a legacy VGA
>> region, the RAM between 0xa0000 and 0xc0000 doesn't become unnecesserily UC-.
>> However, we still must use small pages to avoid the undefined behaviour caused
>> by superpages crossing MTRRs of different cacheability.
>>
>> While adjusting the pagetable construction, switch from pfn to idx for
>> consistency with all the other construction logic.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> As a future optimisation, Xen could rewrite l2_identmap with a superpage if it
>> finds that MTRRs are disabled.  However, this probably needs to wait until Xen
>> no longer unconditionally assumes a legacy VGA region to be present in the
>> e820 if it finds something other than a hole
> Is that still the case? I remember dealing with it when working on the
> shim, and I thought the code to unconditionally set a hole in
> 0xa0000-0xc0000 was removed.

copy_e820_map() still has the broken logic.

I don't recall exactly what state the proposed fixes were in, and I
certainly don't have time to dust them off right now.

~Andrew
Jan Beulich Sept. 6, 2019, 1:31 p.m. UTC | #3
On 05.09.2019 21:04, Andrew Cooper wrote:
> All 64-bit capable processors use PAT, and with PAT, it is explicitly
> permitted to have mappings with a cacheability different to MTRRs.
> 
> On a native system with a real legacy VGA region, MTRRs will cause the region
> to be UC-.

Minor correction: MTRRs can't be used to specify UC-. UC- is used in
PAT to allow WC from MTRRs to be retained, rather than becoming UC.
And hence sensible BIOSes would make this range WC in the MTRRs, not
UC.

The main question here is whether we can rely on firmware to actually
set MTRRs correctly.

>  When booting virtualised, this range may be RAM and not a legacy
> VGA region, at which point it wants to be WB.

To limit the effect of improper MTRR settings, did you consider using
WT instead? Furthermore, did you consider dynamically changing the
involved PTEs depending on whether we run virtualized ourselves?

> Use WB mapping in the pagetables, such that in systems without a legacy VGA
> region, the RAM between 0xa0000 and 0xc0000 doesn't become unnecesserily UC-.

May I suggest s/RAM/range/ ? There's no guarantee that when there's
no VGA there, that it would be RAM instead.

Jan

Patch
diff mbox series

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 5ab24d73fc..1ca986f882 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -51,19 +51,13 @@  GLOBAL(stack_start)
 /*
  * Mapping of first 2 megabytes of memory. This is mapped with 4kB mappings
  * to avoid type conflicts with fixed-range MTRRs covering the lowest megabyte
- * of physical memory. In any case the VGA hole should be mapped with type UC.
- * Uses 1x 4k page.
+ * of physical memory.  Uses 1x 4k page.
  */
 l1_identmap:
-        pfn = 0
+        idx = 0
         .rept L1_PAGETABLE_ENTRIES
-        /* VGA hole (0xa0000-0xc0000) should be mapped UC-. */
-        .if pfn >= 0xa0 && pfn < 0xc0
-        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_UCMINUS | MAP_SMALL_PAGES
-        .else
-        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
-        .endif
-        pfn = pfn + 1
+        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
+        idx = idx + 1
         .endr
         .size l1_identmap, . - l1_identmap