Hi Gavin,
On 9/29/22 01:15, Gavin Shan wrote:
> Hi Eric,
>
> On 9/28/22 10:10 PM, Eric Auger wrote:
>> On 9/22/22 01:13, Gavin Shan wrote:
>>> This introduces variable 'region_base' for the base address of the
>>> specific high memory region. It's the preparatory work to optimize
>>> high memory region address assignment.
>> Why is it a preparatory work (same comment for previous patch, ie [2/5]
>> ). Are those changes really needed? why?
>>
>
> In PATCH[4/5], @base argument is added to virt_set_high_memmap(), to
> represent current global base address. With the optimization applied
> in PATCH[4/5], @base isn't unconditionally updated to the top of the
> iterated high memory region. So we need @region_base here (PATCH[3/5])
> to track the aligned base address for the iterated high memory region,
> which may or may be not updated to @base.
>
> Since we have @region_base in PATCH[3/5], it'd better to have
> @region_size
> in PATCH[2/5].
>
> Actually, PATCH[1-3/5] are all preparatory patches for PATCH[4/5]. My
> intention was to organize the patches in a way to keep the logical
> change part simple enough, for easier review.
OK fair enough
Eric
>
> Thanks,
> Gavin
>
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Gavin Shan <gs...@redhat.com>
>>> ---
>>> hw/arm/virt.c | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 187b3ee0e2..b0b679d1f4 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -1692,15 +1692,15 @@ static uint64_t
>>> virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
>>> static void virt_set_high_memmap(VirtMachineState *vms,
>>> hwaddr base, int pa_bits)
>>> {
>>> - hwaddr region_size;
>>> + hwaddr region_base, region_size;
>>> bool fits;
>>> int i;
>>> for (i = VIRT_LOWMEMMAP_LAST; i <
>>> ARRAY_SIZE(extended_memmap); i++) {
>>> + region_base = ROUND_UP(base, extended_memmap[i].size);
>>> region_size = extended_memmap[i].size;
>>> - base = ROUND_UP(base, region_size);
>>> - vms->memmap[i].base = base;
>>> + vms->memmap[i].base = region_base;
>>> vms->memmap[i].size = region_size;
>>> /*
>>> @@ -1709,9 +1709,9 @@ static void
>>> virt_set_high_memmap(VirtMachineState *vms,
>>> *
>>> * For each device that doesn't fit, disable it.
>>> */
>>> - fits = (base + region_size) <= BIT_ULL(pa_bits);
>>> + fits = (region_base + region_size) <= BIT_ULL(pa_bits);
>>> if (fits) {
>>> - vms->highest_gpa = base + region_size - 1;
>>> + vms->highest_gpa = region_base + region_size - 1;
>>> }
>>> switch (i) {
>>> @@ -1726,7 +1726,7 @@ static void
>>> virt_set_high_memmap(VirtMachineState *vms,
>>> break;
>>> }
>>> - base += region_size;
>>> + base = region_base + region_size;
>>> }
>>> }
>>>
>>
>
