| Message ID | 20220921231349.274049-4-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: Improve address assignment for high memory regions | expand |
Hi Gavin, 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? Eric > > No functional change intended. > > Signed-off-by: Gavin Shan <gshan@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; > } > } >
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. Thanks, Gavin >> >> No functional change intended. >> >> Signed-off-by: Gavin Shan <gshan@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; >> } >> } >> >
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 <gshan@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; >>> } >>> } >>> >> >
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. > > No functional change intended. > > Signed-off-by: Gavin Shan <gshan@redhat.com> Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric > --- > 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; > } > } >
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; } }
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. No functional change intended. Signed-off-by: Gavin Shan <gshan@redhat.com> --- hw/arm/virt.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)