qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/2] hw/arm/virt: Improve address assignment for highmem IO r

From: Gavin Shan
Subject: Re: [PATCH 1/2] hw/arm/virt: Improve address assignment for highmem IO regions
Date: Wed, 3 Aug 2022 23:11:01 +1000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.0 
Hi Eric,

On 8/3/22 6:44 PM, Eric Auger wrote:

On 8/3/22 05:01, Gavin Shan wrote:

On 8/2/22 7:41 PM, Eric Auger wrote:

On 8/2/22 08:45, Gavin Shan wrote:

There are 3 highmem IO regions as below. They can be disabled in
two situations: (a) The specific region is disabled by user. (b)
The specific region doesn't fit in the PA space. However, the base
address and highest_gpa are still updated no matter if the region
is enabled or disabled. It's incorrectly incurring waste in the PA
space.

If I am not wrong highmem_redists and highmem_mmio are not user
selectable

Only highmem ecam depends on machine type & ACPI setup. But I would say
that in server use case it is always set. So is that optimization really
needed?


There are two other cases you missed.

- highmem_ecam is enabled after virt-2.12, meaning it stays disabled
   before that.

Yes that's what I meant above by 'depends on machine type'


Ok.



- The high memory region can be disabled if user is asking large
   (normal) memory space through 'maxmem=' option. When the requested
   memory by 'maxmem=' is large enough, the high memory regions are
   disabled. It means the normal memory has higher priority than those
   high memory regions. This is the case I provided in (b) of the
   commit log.

yes but in such a case you don't "waste" IPA as you mention in the
commit log because you only ask for a VM dimensionned for the highest_gpa.
The only case where you would "waste" IPA is for high ecam which can
disabled by option combination but it is marginal.



Ok, I've explain this to Marc in another reply. In short, we possibly
have below combination. the 'highmem_mmio' region isn't enabled as
we expect. The reason is 'highmem_rdist2' and 'highmem_ecam' consumes
1GB, which is unnecessary because both regions are disabled in advance.

Note: system memory starts from 1GB.

   qemu -m 4096M -maxmem=511G

   IPA_LIMIT  = (1UL << 40)
   vms->highmem_rdist2 = false              /* 64MB  */
   vms->highmem_ecam   = false              /* 256MB */
   vms->highmem_mmio   = true               /* 512GB */



In the commit log, I was supposed to say something like below for
(a):

- The specific high memory region can be disabled through changing
   the code by user or developer. For example, 'vms->highmem_mmio'
   is changed from true to false in virt_instance_init().



Improve address assignment for highmem IO regions to avoid the waste
in the PA space by putting the logic into virt_memmap_fits().

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
   hw/arm/virt.c | 54
+++++++++++++++++++++++++++++----------------------
   1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9633f822f3..bc0cd218f9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1688,6 +1688,34 @@ static uint64_t
virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
       return arm_cpu_mp_affinity(idx, clustersz);
   }
   +static void virt_memmap_fits(VirtMachineState *vms, int index,
+                             bool *enabled, hwaddr *base, int pa_bits)
+{
+    hwaddr size = extended_memmap[index].size;
+
+    /* The region will be disabled if its size isn't given */
+    if (!*enabled || !size) {

In which case do you have !size?


Yeah, we don't have !size and the condition should be removed.


+        *enabled = false;
+        vms->memmap[index].base = 0;
+        vms->memmap[index].size = 0;

It looks dangerous to me to reset the region's base and size like that.
for instance fdt_add_gic_node() will add dummy data in the dt.


I would guess you missed that the high memory regions won't be exported
through device-tree if they have been disabled. We have a check there,
which is "if (nb_redist_regions == 1)"

OK I missed a check was added in virt_gicv3_redist_region_count.
Nevertheless, your comment "The region will be disabled if its size
isn't given */ is not obvious to me. To me the region is disabled if the
corresponding flag is not set. From your comment I have the impression
the size is checked to see if the region is exposed, it does not look
obvious.


Ok :)




+        return;
+    }
+
+    /*
+     * Check if the memory region fits in the PA space. The memory map
+     * and highest_gpa are updated if it fits. Otherwise, it's
disabled.
+     */
+    *enabled = (ROUND_UP(*base, size) + size <= BIT_ULL(pa_bits));

using a 'fits' local variable would make the code more obvious I think


Lets confirm if you're suggesting something like below?

         bool fits;

         fits = (ROUND_UP(*base, size) + size <= BIT_ULL(pa_bits));

         if (fits) {
            /* update *base, memory mapping, highest_gpa */
         } else {
            *enabled = false;
         }

yes that's what I suggested.


Yeah, it's more obvious. I would hold to post v2 to see if Marc will
have more comments.



I guess we can simply do

         if (ROUND_UP(*base, size) + size <= BIT_ULL(pa_bits)) {
            /* update *base, memory mapping, highest_gpa */
         } else {
            *enabled = false;
         }

Please let me know which one looks best to you.


+    if (*enabled) {
+        *base = ROUND_UP(*base, size);
+        vms->memmap[index].base = *base;
+        vms->memmap[index].size = size;
+        vms->highest_gpa = *base + size - 1;
+
+    *base = *base + size;
+    }
+}
+
   static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
   {
       MachineState *ms = MACHINE(vms);
@@ -1744,37 +1772,17 @@ static void virt_set_memmap(VirtMachineState
*vms, int pa_bits)
       vms->highest_gpa = memtop - 1;
         for (i = VIRT_LOWMEMMAP_LAST; i <
ARRAY_SIZE(extended_memmap); i++) {
-        hwaddr size = extended_memmap[i].size;
-        bool fits;
-
-        base = ROUND_UP(base, size);
-        vms->memmap[i].base = base;
-        vms->memmap[i].size = size;
-
-        /*
-         * Check each device to see if they fit in the PA space,
-         * moving highest_gpa as we go.
-         *
-         * For each device that doesn't fit, disable it.
-         */
-        fits = (base + size) <= BIT_ULL(pa_bits);
-        if (fits) {
-            vms->highest_gpa = base + size - 1;
-        }
-


we could avoid running the code below in case highmem is not set. We
would need to reset that flags though.



Yeah, I think it's not a big deal. My though is to update the flag in
virt_memmap_fits().


           switch (i) {
           case VIRT_HIGH_GIC_REDIST2:
-            vms->highmem_redists &= fits;
+            virt_memmap_fits(vms, i, &vms->highmem_redists, &base,
pa_bits);
               break;
           case VIRT_HIGH_PCIE_ECAM:
-            vms->highmem_ecam &= fits;
+            virt_memmap_fits(vms, i, &vms->highmem_ecam, &base,
pa_bits);
               break;
           case VIRT_HIGH_PCIE_MMIO:
-            vms->highmem_mmio &= fits;
+            virt_memmap_fits(vms, i, &vms->highmem_mmio, &base,
pa_bits);
               break;
           }
-
-        base += size;
       }
         if (device_memory_size > 0) {


Thanks,
Gavin
reply via email to
[Prev in Thread] Current Thread [Next in Thread]