1. Patchew
  2. QEMU
  3. [PATCH v4 0/6] hw/arm/virt: Improve address assignment for high memory regions
  4. View patch

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

Gavin Shan posted 6 patches 3 years, 4 months ago
Maintainers: Peter Maydell <peter.maydell@linaro.org>
There is a newer version of this series
[PATCH v4 4/6] hw/arm/virt: Introduce virt_get_high_memmap_enabled() helper
Posted by Gavin Shan 3 years, 4 months ago 
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));
+
+    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;
     }
 }
-- 
2.23.0
Re: [PATCH v4 4/6] hw/arm/virt: Introduce virt_get_high_memmap_enabled() helper
Posted by Cornelia Huck 3 years, 4 months ago 
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];
> +}
> +
Re: [PATCH v4 4/6] hw/arm/virt: Introduce virt_get_high_memmap_enabled() helper
Posted by Gavin Shan 3 years, 4 months ago 
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
Re: [PATCH v4 4/6] hw/arm/virt: Introduce virt_get_high_memmap_enabled() helper
Posted by Eric Auger 3 years, 4 months ago 

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
>
Re: [PATCH v4 4/6] hw/arm/virt: Introduce virt_get_high_memmap_enabled() helper
Posted by Gavin Shan 3 years, 4 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.