RE: [PATCH] hw/arm/virt: Support larger highmem MMIO regions

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Matthew R. Ochs <mochs@nvidia.com>
> Sent: Tuesday, January 28, 2025 4:03 PM
> To: qemu-devel@nongnu.org; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; nathanc@nvidia.com
> Cc: ddutile@redhat.com; eric.auger@redhat.com; nicolinc@nvidia.com;
> ankita@nvidia.com
> Subject: [PATCH] hw/arm/virt: Support larger highmem MMIO regions
> 
> The MMIO region size required to support virtualized environments with
> large PCI BAR regions can exceed the hardcoded limit configured in QEMU.
> For example, a VM with multiple NVIDIA Grace-Hopper GPUs passed
> through
> requires more MMIO memory than the amount provided by
> VIRT_HIGH_PCIE_MMIO
> (currently 512GB). Instead of updating VIRT_HIGH_PCIE_MMIO, introduce a
> new parameter, highmem-mmio-size, that specifies the MMIO size required
> to support the VM configuration.
> 
> Example usage with 1TB MMIO region size:
>       -machine virt,gic-version=3,highmem-mmio-size=1099511627776

I guess you could do highmem-mmio-size=1024G as well.

> 
> Signed-off-by: Matthew R. Ochs <mochs@nvidia.com>
> ---
>  docs/system/arm/virt.rst |  4 ++++
>  hw/arm/virt.c            | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
> index e67e7f0f7c50..36344554788a 100644
> --- a/docs/system/arm/virt.rst
> +++ b/docs/system/arm/virt.rst
> @@ -138,6 +138,10 @@ highmem-mmio
>    Set ``on``/``off`` to enable/disable the high memory region for PCI MMIO.
>    The default is ``on``.
> 
> +highmem-mmio-size
> +  Set extended MMIO memory map size. Must be a power-of-2 and greater
> than
> +  the default size.
> +
>  gic-version
>    Specify the version of the Generic Interrupt Controller (GIC) to provide.
>    Valid values are:
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 49eb0355ef0c..43d47ffedd9a 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2773,6 +2773,35 @@ static void virt_set_highmem_mmio(Object *obj,
> bool value, Error **errp)
>      vms->highmem_mmio = value;
>  }
> 
> +static void virt_get_highmem_mmio_size(Object *obj, Visitor *v, const
> char *name,
> +                          void *opaque, Error **errp)
> +{
> +    uint64_t size = extended_memmap[VIRT_HIGH_PCIE_MMIO].size;
> +
> +    visit_type_size(v, name, &size, errp);
> +}
> +
> +static void virt_set_highmem_mmio_size(Object *obj, Visitor *v, const char
> *name,
> +                          void *opaque, Error **errp)
> +{
> +    uint64_t size;
> +
> +    if (!visit_type_size(v, name, &size, errp))
> +        return;

Qemu style mandates braces around.

> +
> +    if (!is_power_of_2(size)) {
> +        error_setg(errp, "highmem_mmio_size is not a power-of-2");
> +        return;
> +    }
> +
> +    if (size < extended_memmap[VIRT_HIGH_PCIE_MMIO].size) {

Not sure it is better to fallback to default size here instead of setting error.

> +        error_setg(errp, "highmem_mmio_size is less than the default (%lu)",
> +                   extended_memmap[VIRT_HIGH_PCIE_MMIO].size);
> +        return;
> +    }
> +
> +    extended_memmap[VIRT_HIGH_PCIE_MMIO].size = size;
> +}
> 
>  static bool virt_get_its(Object *obj, Error **errp)
>  {
> @@ -3446,6 +3475,13 @@ static void virt_machine_class_init(ObjectClass
> *oc, void *data)
>                                            "Set on/off to enable/disable high 
> "
>                                            "memory region for PCI MMIO");
> 
> +    object_class_property_add(oc, "highmem-mmio-size", "size",
> +                                   virt_get_highmem_mmio_size,
> +                                   virt_set_highmem_mmio_size,
> +                                   NULL, NULL);
> +    object_class_property_set_description(oc, "highmem-mmio-size",
> +                                          "Set extended MMIO memory map 
> size");
> +

I think this probably needs backward compatibility to keep migration happy.
Isn't it? See the no_highmem_compact handling.

Thanks,
Shameer