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

[Xen-devel] [PATCH] x86/boot: Don't explicitly map the VGA region as UC-

Posted by Andrew Cooper 4 years, 7 months ago

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(-)

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
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/boot: Don't explicitly map the VGA region as UC-

Posted by Roger Pau Monné 4 years, 7 months ago

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/boot: Don't explicitly map the VGA region as UC-

Posted by Andrew Cooper 4 years, 7 months ago

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/boot: Don't explicitly map the VGA region as UC-

Posted by Jan Beulich 4 years, 7 months ago

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel