This makes the code much easier to understand, and avoids problems if
Xen's PAT ever changes in the future.
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
xen/common/efi/boot.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 8e880fe30c7541a202dec3e665300d6549953aa3..260997b251b09dae4b48c1b1db665778e02d760a 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1746,21 +1746,21 @@ void __init efi_init_memory(void)
if ( desc->Attribute & EFI_MEMORY_WB )
/* nothing */;
else if ( desc->Attribute & EFI_MEMORY_WT )
- prot |= _PAGE_PWT | MAP_SMALL_PAGES;
+ prot |= _PAGE_WT | MAP_SMALL_PAGES;
else if ( desc->Attribute & EFI_MEMORY_WC )
- prot |= _PAGE_PAT | MAP_SMALL_PAGES;
+ prot |= _PAGE_WC | MAP_SMALL_PAGES;
else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) )
- prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
+ prot |= _PAGE_UC | MAP_SMALL_PAGES;
else if ( efi_bs_revision >= EFI_REVISION(2, 5) &&
(desc->Attribute & EFI_MEMORY_WP) )
- prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES;
+ prot |= _PAGE_WP | MAP_SMALL_PAGES;
else
{
printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx%s\n",
smfn, emfn - 1, efi_map_uc ? ", assuming UC" : "");
if ( !efi_map_uc )
continue;
- prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
+ prot |= _PAGE_UC | MAP_SMALL_PAGES;
}
if ( desc->Attribute & (efi_bs_revision < EFI_REVISION(2, 5)
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things LabOn 06/12/2022 04:33, Demi Marie Obenour wrote:
> This makes the code much easier to understand, and avoids problems if
> Xen's PAT ever changes in the future.
>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> ---
> xen/common/efi/boot.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 8e880fe30c7541a202dec3e665300d6549953aa3..260997b251b09dae4b48c1b1db665778e02d760a 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1746,21 +1746,21 @@ void __init efi_init_memory(void)
> if ( desc->Attribute & EFI_MEMORY_WB )
> /* nothing */;
This is an implicit 0 case which wants changing to _PAGE_WB.
> else if ( desc->Attribute & EFI_MEMORY_WT )
> - prot |= _PAGE_PWT | MAP_SMALL_PAGES;
> + prot |= _PAGE_WT | MAP_SMALL_PAGES;
> else if ( desc->Attribute & EFI_MEMORY_WC )
> - prot |= _PAGE_PAT | MAP_SMALL_PAGES;
> + prot |= _PAGE_WC | MAP_SMALL_PAGES;
> else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) )
> - prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
> + prot |= _PAGE_UC | MAP_SMALL_PAGES;
> else if ( efi_bs_revision >= EFI_REVISION(2, 5) &&
> (desc->Attribute & EFI_MEMORY_WP) )
> - prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES;
> + prot |= _PAGE_WP | MAP_SMALL_PAGES;
Unrelated to the transformation. I'm unconvinced about the correctness
of using MAP_SMALL_PAGES here. There's nothing wrong with large pages
of reduced cache-ability, and the framebuffer is going to live in one of
these regions, probably a WC one...
~Andrew
> else
> {
> printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx%s\n",
> smfn, emfn - 1, efi_map_uc ? ", assuming UC" : "");
> if ( !efi_map_uc )
> continue;
> - prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
> + prot |= _PAGE_UC | MAP_SMALL_PAGES;
> }
>
> if ( desc->Attribute & (efi_bs_revision < EFI_REVISION(2, 5)
On Tue, Dec 06, 2022 at 11:17:20AM +0000, Andrew Cooper wrote: > On 06/12/2022 04:33, Demi Marie Obenour wrote: > > This makes the code much easier to understand, and avoids problems if > > Xen's PAT ever changes in the future. > > > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> > > --- > > xen/common/efi/boot.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > > index 8e880fe30c7541a202dec3e665300d6549953aa3..260997b251b09dae4b48c1b1db665778e02d760a 100644 > > --- a/xen/common/efi/boot.c > > +++ b/xen/common/efi/boot.c > > @@ -1746,21 +1746,21 @@ void __init efi_init_memory(void) > > if ( desc->Attribute & EFI_MEMORY_WB ) > > /* nothing */; > > This is an implicit 0 case which wants changing to _PAGE_WB. Good catch! I will make this change in v2, but I also will add BUILD_BUG_ON(_PAGE_WB), as at least Linux assumes that _PAGE_WB is 0. > > else if ( desc->Attribute & EFI_MEMORY_WT ) > > - prot |= _PAGE_PWT | MAP_SMALL_PAGES; > > + prot |= _PAGE_WT | MAP_SMALL_PAGES; > > else if ( desc->Attribute & EFI_MEMORY_WC ) > > - prot |= _PAGE_PAT | MAP_SMALL_PAGES; > > + prot |= _PAGE_WC | MAP_SMALL_PAGES; > > else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) ) > > - prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES; > > + prot |= _PAGE_UC | MAP_SMALL_PAGES; > > else if ( efi_bs_revision >= EFI_REVISION(2, 5) && > > (desc->Attribute & EFI_MEMORY_WP) ) > > - prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES; > > + prot |= _PAGE_WP | MAP_SMALL_PAGES; > > Unrelated to the transformation. I'm unconvinced about the correctness > of using MAP_SMALL_PAGES here. There's nothing wrong with large pages > of reduced cache-ability, and the framebuffer is going to live in one of > these regions, probably a WC one... I can make that a separate patch. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab
On 06.12.2022 12:17, Andrew Cooper wrote: > On 06/12/2022 04:33, Demi Marie Obenour wrote: >> --- a/xen/common/efi/boot.c >> +++ b/xen/common/efi/boot.c >> @@ -1746,21 +1746,21 @@ void __init efi_init_memory(void) >> if ( desc->Attribute & EFI_MEMORY_WB ) >> /* nothing */; > > This is an implicit 0 case which wants changing to _PAGE_WB. Oh, yes. Demi, feel free to retain my R-b with the adjustment. >> else if ( desc->Attribute & EFI_MEMORY_WT ) >> - prot |= _PAGE_PWT | MAP_SMALL_PAGES; >> + prot |= _PAGE_WT | MAP_SMALL_PAGES; >> else if ( desc->Attribute & EFI_MEMORY_WC ) >> - prot |= _PAGE_PAT | MAP_SMALL_PAGES; >> + prot |= _PAGE_WC | MAP_SMALL_PAGES; >> else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) ) >> - prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES; >> + prot |= _PAGE_UC | MAP_SMALL_PAGES; >> else if ( efi_bs_revision >= EFI_REVISION(2, 5) && >> (desc->Attribute & EFI_MEMORY_WP) ) >> - prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES; >> + prot |= _PAGE_WP | MAP_SMALL_PAGES; > > Unrelated to the transformation. I'm unconvinced about the correctness > of using MAP_SMALL_PAGES here. There's nothing wrong with large pages > of reduced cache-ability, Hmm, back in the 32-bit days we needed to be afraid of hardware issues in that area. Hence we had a global policy of never allowing non-WB large pages. Maybe we don't need to be concerned anymore ... > and the framebuffer is going to live in one of these regions, probably > a WC one... I very much hope it won't live anywhere there, unless you think of non- PCI devices supplying framebuffers. As long as it's described by a BAR, it better wouldn't be covered by a memory map entry. Jan
On 06.12.2022 05:33, Demi Marie Obenour wrote: > This makes the code much easier to understand, and avoids problems if > Xen's PAT ever changes in the future. > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
© 2016 - 2026 Red Hat, Inc.