| Message ID | 20221004002627.59172-5-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: Improve address assignment for high memory regions | expand |
On Tue, Oct 04 2022, Gavin Shan <gshan@redhat.com> wrote: > This introduces virt_get_high_memmap_enabled() helper, which returns > the pointer to vms->highmem_{redists, ecam, mmio}. The pointer will > be used in the subsequent patches. > > No functional change intended. > > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > hw/arm/virt.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index b0b679d1f4..59de7b78b5 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1689,14 +1689,29 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) > return arm_cpu_mp_affinity(idx, clustersz); > } > > +static inline bool *virt_get_high_memmap_enabled(VirtMachineState *vms, > + int index) > +{ > + bool *enabled_array[] = { > + &vms->highmem_redists, > + &vms->highmem_ecam, > + &vms->highmem_mmio, > + }; > + > + assert(index - VIRT_LOWMEMMAP_LAST < ARRAY_SIZE(enabled_array)); I wonder whether we want an assert(ARRAY_SIZE(extended_memmap) == ARRAY_SIZE(enabled_array))? IIUC, we never want those two to get out of sync? > + > + return enabled_array[index - VIRT_LOWMEMMAP_LAST]; > +} > +
Hi Connie, On 10/4/22 6:41 PM, Cornelia Huck wrote: > On Tue, Oct 04 2022, Gavin Shan <gshan@redhat.com> wrote: > >> This introduces virt_get_high_memmap_enabled() helper, which returns >> the pointer to vms->highmem_{redists, ecam, mmio}. The pointer will >> be used in the subsequent patches. >> >> No functional change intended. >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> hw/arm/virt.c | 30 +++++++++++++++++------------- >> 1 file changed, 17 insertions(+), 13 deletions(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index b0b679d1f4..59de7b78b5 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -1689,14 +1689,29 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) >> return arm_cpu_mp_affinity(idx, clustersz); >> } >> >> +static inline bool *virt_get_high_memmap_enabled(VirtMachineState *vms, >> + int index) >> +{ >> + bool *enabled_array[] = { >> + &vms->highmem_redists, >> + &vms->highmem_ecam, >> + &vms->highmem_mmio, >> + }; >> + >> + assert(index - VIRT_LOWMEMMAP_LAST < ARRAY_SIZE(enabled_array)); > > I wonder whether we want an assert(ARRAY_SIZE(extended_memmap) == > ARRAY_SIZE(enabled_array))? IIUC, we never want those two to get out of > sync? > Yeah, It makes sense to ensure both arrays synchronized. I will add the extra check in next respin. >> + >> + return enabled_array[index - VIRT_LOWMEMMAP_LAST]; >> +} >> + Thanks, Gavin
On 10/5/22 00:47, Gavin Shan wrote: > Hi Connie, > > On 10/4/22 6:41 PM, Cornelia Huck wrote: >> On Tue, Oct 04 2022, Gavin Shan <gshan@redhat.com> wrote: >> >>> This introduces virt_get_high_memmap_enabled() helper, which returns >>> the pointer to vms->highmem_{redists, ecam, mmio}. The pointer will >>> be used in the subsequent patches. >>> >>> No functional change intended. >>> >>> Signed-off-by: Gavin Shan <gshan@redhat.com> >>> --- >>> hw/arm/virt.c | 30 +++++++++++++++++------------- >>> 1 file changed, 17 insertions(+), 13 deletions(-) >>> >>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>> index b0b679d1f4..59de7b78b5 100644 >>> --- a/hw/arm/virt.c >>> +++ b/hw/arm/virt.c >>> @@ -1689,14 +1689,29 @@ static uint64_t >>> virt_cpu_mp_affinity(VirtMachineState *vms, int idx) >>> return arm_cpu_mp_affinity(idx, clustersz); >>> } >>> +static inline bool *virt_get_high_memmap_enabled(VirtMachineState >>> *vms, >>> + int index) >>> +{ >>> + bool *enabled_array[] = { >>> + &vms->highmem_redists, >>> + &vms->highmem_ecam, >>> + &vms->highmem_mmio, >>> + }; >>> + >>> + assert(index - VIRT_LOWMEMMAP_LAST < ARRAY_SIZE(enabled_array)); >> >> I wonder whether we want an assert(ARRAY_SIZE(extended_memmap) == >> ARRAY_SIZE(enabled_array))? IIUC, we never want those two to get out of >> sync? >> > > Yeah, It makes sense to ensure both arrays synchronized. I will add > the extra check in next respin. With Connie's suggestion this looks good to me. Thanks Eric > >>> + >>> + return enabled_array[index - VIRT_LOWMEMMAP_LAST]; >>> +} >>> + > > Thanks, > Gavin >
On 10/12/22 12:45 AM, Eric Auger wrote: > On 10/5/22 00:47, Gavin Shan wrote: >> On 10/4/22 6:41 PM, Cornelia Huck wrote: >>> On Tue, Oct 04 2022, Gavin Shan <gshan@redhat.com> wrote: >>> >>>> This introduces virt_get_high_memmap_enabled() helper, which returns >>>> the pointer to vms->highmem_{redists, ecam, mmio}. The pointer will >>>> be used in the subsequent patches. >>>> >>>> No functional change intended. >>>> >>>> Signed-off-by: Gavin Shan <gshan@redhat.com> >>>> --- >>>> hw/arm/virt.c | 30 +++++++++++++++++------------- >>>> 1 file changed, 17 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>>> index b0b679d1f4..59de7b78b5 100644 >>>> --- a/hw/arm/virt.c >>>> +++ b/hw/arm/virt.c >>>> @@ -1689,14 +1689,29 @@ static uint64_t >>>> virt_cpu_mp_affinity(VirtMachineState *vms, int idx) >>>> return arm_cpu_mp_affinity(idx, clustersz); >>>> } >>>> +static inline bool *virt_get_high_memmap_enabled(VirtMachineState >>>> *vms, >>>> + int index) >>>> +{ >>>> + bool *enabled_array[] = { >>>> + &vms->highmem_redists, >>>> + &vms->highmem_ecam, >>>> + &vms->highmem_mmio, >>>> + }; >>>> + >>>> + assert(index - VIRT_LOWMEMMAP_LAST < ARRAY_SIZE(enabled_array)); >>> >>> I wonder whether we want an assert(ARRAY_SIZE(extended_memmap) == >>> ARRAY_SIZE(enabled_array))? IIUC, we never want those two to get out of >>> sync? >>> >> >> Yeah, It makes sense to ensure both arrays synchronized. I will add >> the extra check in next respin. > > With Connie's suggestion this looks good to me. > What we need is actually like below because the array (extended_memmap) starts from VIRT_LOWMEMMAP_LAST instead of zero. I'm adding the extra check into v5, which will be posted shortly. assert(ARRAY_SIZE(extended_memmap) - VIRT_LOWMEMMAP_LAST == ARRAY_SIZE(enabled_array)); >> >>>> + >>>> + return enabled_array[index - VIRT_LOWMEMMAP_LAST]; >>>> +} >>>> + Thanks, Gavin
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index b0b679d1f4..59de7b78b5 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1689,14 +1689,29 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) return arm_cpu_mp_affinity(idx, clustersz); } +static inline bool *virt_get_high_memmap_enabled(VirtMachineState *vms, + int index) +{ + bool *enabled_array[] = { + &vms->highmem_redists, + &vms->highmem_ecam, + &vms->highmem_mmio, + }; + + assert(index - VIRT_LOWMEMMAP_LAST < ARRAY_SIZE(enabled_array)); + + return enabled_array[index - VIRT_LOWMEMMAP_LAST]; +} + static void virt_set_high_memmap(VirtMachineState *vms, hwaddr base, int pa_bits) { hwaddr region_base, region_size; - bool fits; + bool *region_enabled, fits; int i; for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) { + region_enabled = virt_get_high_memmap_enabled(vms, i); region_base = ROUND_UP(base, extended_memmap[i].size); region_size = extended_memmap[i].size; @@ -1714,18 +1729,7 @@ static void virt_set_high_memmap(VirtMachineState *vms, vms->highest_gpa = region_base + region_size - 1; } - switch (i) { - case VIRT_HIGH_GIC_REDIST2: - vms->highmem_redists &= fits; - break; - case VIRT_HIGH_PCIE_ECAM: - vms->highmem_ecam &= fits; - break; - case VIRT_HIGH_PCIE_MMIO: - vms->highmem_mmio &= fits; - break; - } - + *region_enabled &= fits; base = region_base + region_size; } }
This introduces virt_get_high_memmap_enabled() helper, which returns the pointer to vms->highmem_{redists, ecam, mmio}. The pointer will be used in the subsequent patches. No functional change intended. Signed-off-by: Gavin Shan <gshan@redhat.com> --- hw/arm/virt.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)