| Message ID | ca557de7-88e9-bf2b-0f5e-6a42dca3f9f7@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,4.17] EFI: don't convert memory marked for runtime use to ordinary RAM | expand |
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Subject: [PATCH v2][4.17] EFI: don't convert memory marked for runtime use > to ordinary RAM > > efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME > higher priority than the type of the range. To avoid accessing memory at > runtime which was re-used for other purposes, make > efi_arch_process_memory_map() follow suit. While on x86 in theory the > same would apply to EfiACPIReclaimMemory, we don't actually "reclaim" > E820_ACPI memory there (and it would be a bug if the Dom0 kernel tried > to do so, bypassing Xen's memory management), hence that type's handling > can be left alone. > > Fixes: bf6501a62e80 ("x86-64: EFI boot code") > Fixes: facac0af87ef ("x86-64: EFI runtime code") > Fixes: 6d70ea10d49f ("Add ARM EFI boot support") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> # Arm > Tested-By: Luca Fancellu <luca.fancellu@arm.com> # Arm > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> #arm Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry > --- > v2: Amend description.
On Thu, Oct 06, 2022 at 10:40:56AM +0200, Jan Beulich wrote: > efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME > higher priority than the type of the range. To avoid accessing memory at > runtime which was re-used for other purposes, make > efi_arch_process_memory_map() follow suit. While on x86 in theory the > same would apply to EfiACPIReclaimMemory, we don't actually "reclaim" > E820_ACPI memory there (and it would be a bug if the Dom0 kernel tried > to do so, bypassing Xen's memory management), hence that type's handling Strictly speaking I don't think dom0 needs to bypass Xen's memory management, just overwriting the page would be bad enough for runtime services to not work correctly I would think. > can be left alone. > > Fixes: bf6501a62e80 ("x86-64: EFI boot code") > Fixes: facac0af87ef ("x86-64: EFI runtime code") > Fixes: 6d70ea10d49f ("Add ARM EFI boot support") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> # Arm > Tested-By: Luca Fancellu <luca.fancellu@arm.com> # Arm > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> #arm Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 06.10.2022 10:53, Roger Pau Monné wrote: > On Thu, Oct 06, 2022 at 10:40:56AM +0200, Jan Beulich wrote: >> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME >> higher priority than the type of the range. To avoid accessing memory at >> runtime which was re-used for other purposes, make >> efi_arch_process_memory_map() follow suit. While on x86 in theory the >> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim" >> E820_ACPI memory there (and it would be a bug if the Dom0 kernel tried >> to do so, bypassing Xen's memory management), hence that type's handling > > Strictly speaking I don't think dom0 needs to bypass Xen's memory > management, just overwriting the page would be bad enough for runtime > services to not work correctly I would think. Then how about: "While on x86 in theory the same would apply to EfiACPIReclaimMemory, we don't actually "reclaim" or clobber E820_ACPI memory there (and it would be a bug if the Dom0 kernel tried to reclaim the range, bypassing Xen's memory management, plus it would be at least bogus if it clobbered that space), hence that type's handling can be left alone." I didn't think the clobbering aspect needed pointing out, as the same applies to all other memory which Dom0 is able to access beyond its actual allocation. >> can be left alone. >> >> Fixes: bf6501a62e80 ("x86-64: EFI boot code") >> Fixes: facac0af87ef ("x86-64: EFI runtime code") >> Fixes: 6d70ea10d49f ("Add ARM EFI boot support") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> # Arm >> Tested-By: Luca Fancellu <luca.fancellu@arm.com> # Arm >> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> #arm > > Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. Jan
On Thu, Oct 06, 2022 at 10:58:43AM +0200, Jan Beulich wrote: > On 06.10.2022 10:53, Roger Pau Monné wrote: > > On Thu, Oct 06, 2022 at 10:40:56AM +0200, Jan Beulich wrote: > >> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME > >> higher priority than the type of the range. To avoid accessing memory at > >> runtime which was re-used for other purposes, make > >> efi_arch_process_memory_map() follow suit. While on x86 in theory the > >> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim" > >> E820_ACPI memory there (and it would be a bug if the Dom0 kernel tried > >> to do so, bypassing Xen's memory management), hence that type's handling > > > > Strictly speaking I don't think dom0 needs to bypass Xen's memory > > management, just overwriting the page would be bad enough for runtime > > services to not work correctly I would think. > > Then how about: > > "While on x86 in theory the same would apply to EfiACPIReclaimMemory, we don't > actually "reclaim" or clobber E820_ACPI memory there (and it would be a bug if > the Dom0 kernel tried to reclaim the range, bypassing Xen's memory management, > plus it would be at least bogus if it clobbered that space), hence that type's > handling can be left alone." > > I didn't think the clobbering aspect needed pointing out, as the same applies > to all other memory which Dom0 is able to access beyond its actual allocation. I think it makes it clear that just clobbering it from dom0 could cause issues to runtime services. I guess it can be extrapolated that clobbering is also bad if reclaiming is. Thanks, Roger.
--- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -183,13 +183,15 @@ static EFI_STATUS __init efi_process_mem for ( Index = 0; Index < (mmap_size / desc_size); Index++ ) { - if ( desc_ptr->Attribute & EFI_MEMORY_WB && - (desc_ptr->Type == EfiConventionalMemory || - desc_ptr->Type == EfiLoaderCode || - desc_ptr->Type == EfiLoaderData || - (!map_bs && - (desc_ptr->Type == EfiBootServicesCode || - desc_ptr->Type == EfiBootServicesData))) ) + if ( desc_ptr->Attribute & EFI_MEMORY_RUNTIME ) + /* nothing */; + else if ( (desc_ptr->Attribute & EFI_MEMORY_WB) && + (desc_ptr->Type == EfiConventionalMemory || + desc_ptr->Type == EfiLoaderCode || + desc_ptr->Type == EfiLoaderData || + (!map_bs && + (desc_ptr->Type == EfiBootServicesCode || + desc_ptr->Type == EfiBootServicesData))) ) { if ( !meminfo_add_bank(&bootinfo.mem, desc_ptr) ) { --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -185,7 +185,9 @@ static void __init efi_arch_process_memo /* fall through */ case EfiLoaderCode: case EfiLoaderData: - if ( desc->Attribute & EFI_MEMORY_WB ) + if ( desc->Attribute & EFI_MEMORY_RUNTIME ) + type = E820_RESERVED; + else if ( desc->Attribute & EFI_MEMORY_WB ) type = E820_RAM; else case EfiUnusableMemory:
efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME higher priority than the type of the range. To avoid accessing memory at runtime which was re-used for other purposes, make efi_arch_process_memory_map() follow suit. While on x86 in theory the same would apply to EfiACPIReclaimMemory, we don't actually "reclaim" E820_ACPI memory there (and it would be a bug if the Dom0 kernel tried to do so, bypassing Xen's memory management), hence that type's handling can be left alone. Fixes: bf6501a62e80 ("x86-64: EFI boot code") Fixes: facac0af87ef ("x86-64: EFI runtime code") Fixes: 6d70ea10d49f ("Add ARM EFI boot support") Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> # Arm Tested-By: Luca Fancellu <luca.fancellu@arm.com> # Arm Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> #arm --- v2: Amend description.