| Message ID | 20190905190418.15142-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/boot: Don't explicitly map the VGA region as UC- | expand |
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.
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
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
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
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(-)