Re: [PATCH v4 4/6] hw/arm/virt: Introduce virt_get_high_memmap_enabled() helper

From: Gavin Shan <gshan@redhat.com>
To: eric.auger@redhat.com, Cornelia Huck <cohuck@redhat.com>,
	qemu-arm@nongnu.org
Cc: qemu-devel@nongnu.org, maz@kernel.org, zhenyzha@redhat.com,
	richard.henderson@linaro.org, peter.maydell@linaro.org,
	shan.gavin@gmail.com
Subject: Re: [PATCH v4 4/6] hw/arm/virt: Introduce virt_get_high_memmap_enabled() helper
Date: Wed, 12 Oct 2022 07:06:49 +0800	[thread overview]
Message-ID: <8e50093c-f2ee-f4b2-8caf-08412ceb847d@redhat.com> (raw)
In-Reply-To: <5d804a6b-c55d-8a7b-415b-dadf807152fa@redhat.com>

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