[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH][4.17] EFI: don't convert memory marked for runtime use to ordinary RAM
On 06.10.2022 10:39, Jan Beulich wrote: > On 05.10.2022 20:09, Julien Grall wrote: >> Hi Jan, >> >> On 05/10/2022 12:55, Jan Beulich wrote: >>> On 05.10.2022 12:44, Julien Grall wrote: >>>> On 04/10/2022 16:58, Jan Beulich wrote: >>>>> On 30.09.2022 14:51, Bertrand Marquis wrote: >>>>>>> On 30 Sep 2022, at 09:50, Jan Beulich <jbeulich@xxxxxxxx> 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 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@xxxxxxxx> >>>>>> >>>>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx> #arm >>>>> >>>>> Thanks. However ... >>>>> >>>>>>> --- >>>>>>> Partly RFC for Arm, for two reasons: >>>>>>> >>>>>>> On Arm I question the conversion of EfiACPIReclaimMemory, in two ways: >>>>>>> For one like on x86 such ranges would likely better be retained, as Dom0 >>>>>>> may (will?) have a need to look at tables placed there. Plus converting >>>>>>> such ranges to RAM even if EFI_MEMORY_WB is not set looks suspicious to >>>>>>> me as well. I'd be inclined to make the latter adjustment right here >>>>>>> (while the other change probably would better be separate, if there >>>>>>> aren't actually reasons for the present behavior). >>>>> >>>>> ... any views on this WB aspect at least (also Stefano or Julien)? Would >>>>> be >>>>> good to know before I send v2. >>>> >>>> I don't quite understand what you are questioning here. Looking at the >>>> code, EfiACPIReclaimMemory will not be converted to RAM but added in a >>>> separate array. >>>> >>>> Furthermore, all the EfiACPIReclaimMemory regions will be passed to dom0 >>>> (see acpi_create_efi_mmap_table()). >>>> >>>> So to me the code looks correct. >>> >>> Oh, I've indeed not paid enough attention to the first argument passed >>> to meminfo_add_bank(). I'm sorry for the extra noise. However, the >>> question I wanted to have addressed before sending out v3 was that >>> regarding the present using of memory when EFI_MEMORY_WB is not set. >>> Is that correct for the EfiACPIReclaimMemory case, i.e. is the >>> consumer (Dom0) aware that there might be a restriction? >> >> Looking at the code, we always set EFI_MEMORY_WB for the reclaimable >> region and the stage-2 mapping will be cachable. >> >> So it looks like there would be a mismatch if EFI_MEMORY_WB is not set. >> However, given the region is reclaimable, shouldn't this imply that the >> flag is always set? > > Possibly (but then again consider [perhaps hypothetical] systems with e.g. > just WT caches, where specifying WB simply wouldn't make sense). In any > event, even if that's the case, being on the safe side and doing > > if ( (desc_ptr->Attribute & EFI_MEMORY_RUNTIME) || > !(desc_ptr->Attribute & EFI_MEMORY_WB) ) > /* nothing */; > else if ( ... > > would seem better to me. However, if the mapping you mention above > would be adjusted and ... > >>> And would >>> this memory then be guaranteed to never be freed into the general pool >>> of RAM pages? >> >> The region is not treated as RAM by Xen and not owned by the dom0. >> Therefore, it should not be possible to free the page because >> get_page_from_gfn() would not be able to get a reference. > > ... the space cannot become ordinary RAM, then such a precaution > wouldn't be necessary. After all hiding EfiACPIReclaimMemory from > Dom0 just because it can't be mapped WB wouldn't be very nice > either. I guess I'll submit v2 with this part of the change left > as it was. And while already in the process of committing the patch I came to realize that if the WB conditional isn't supposed to move, isn't the change done for Arm then wrong as well? Shouldn't it then be if ( !(desc_ptr->Attribute & EFI_MEMORY_RUNTIME) && (desc_ptr->Attribute & EFI_MEMORY_WB) && (desc_ptr->Type == EfiConventionalMemory || ... leaving the EfiACPIReclaimMemory case entirely unaffected by the change? Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |