Re: [PATCH v5 23/32] hw/arm/virt-acpi-build: Add IORT RMR regions to handle MSI nested binding

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Fri, 31 Oct 2025 10:49:56 +0000
Shameer Kolothum <skolothumtho@nvidia.com> wrote:

> From: Eric Auger <eric.auger@redhat.com>
> 
> To handle SMMUv3 accel=on mode(which configures the host SMMUv3 in nested
> mode), it is practical to expose the guest with reserved memory regions
> (RMRs) covering the IOVAs used by the host kernel to map physical MSI
> doorbells.
> 
> Those IOVAs belong to [0x8000000, 0x8100000] matching MSI_IOVA_BASE and
> MSI_IOVA_LENGTH definitions in kernel arm-smmu-v3 driver. This is the
> window used to allocate IOVAs matching physical MSI doorbells.
> 
> With those RMRs, the guest is forced to use a flat mapping for this range.
> Hence the assigned device is programmed with one IOVA from this range.
> Stage 1, owned by the guest has a flat mapping for this IOVA. Stage2,
> owned by the VMM then enforces a mapping from this IOVA to the physical
> MSI doorbell.
> 
> The creation of those RMR nodes is only relevant if nested stage SMMU is
> in use, along with VFIO. As VFIO devices can be hotplugged, all RMRs need
> to be created in advance.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

One small question inline on the id increment.

With that tidied up.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> @@ -447,10 +475,70 @@ static void create_rc_its_idmaps(GArray *its_idmaps, GArray *smmuv3_devs)
>      }
>  }
>  
> +static void
> +build_iort_rmr_nodes(GArray *table_data, GArray *smmuv3_devices, uint32_t *id)
> +{
> +    AcpiIortSMMUv3Dev *sdev;
> +    AcpiIortIdMapping *idmap;
> +    int i;
> +
> +    for (i = 0; i < smmuv3_devices->len; i++) {
> +        uint16_t rmr_len;
> +        int bdf;
> +
> +        sdev = &g_array_index(smmuv3_devices, AcpiIortSMMUv3Dev, i);
> +        if (!sdev->accel) {
> +            continue;
> +        }
> +
> +        /*
> +         * Spec reference:Arm IO Remapping Table(IORT), ARM DEN 0049E.d,
> +         * Section 3.1.1.5 "Reserved Memory Range node"
> +         */
> +        idmap = &g_array_index(sdev->rc_smmu_idmaps, AcpiIortIdMapping, 0);
> +        bdf = idmap->input_base;
> +        rmr_len = IORT_RMR_COMMON_HEADER_SIZE
> +                 + (IORT_RMR_NUM_ID_MAPPINGS * ID_MAPPING_ENTRY_SIZE)
> +                 + (IORT_RMR_NUM_MEM_RANGE_DESC * IORT_RMR_MEM_RANGE_DESC_SIZE);
> +
> +        /* Table 18 Reserved Memory Range Node */
> +        build_append_int_noprefix(table_data, 6 /* RMR */, 1); /* Type */
> +        /* Length */
> +        build_append_int_noprefix(table_data, rmr_len, 2);
> +        build_append_int_noprefix(table_data, 3, 1); /* Revision */
> +        build_append_int_noprefix(table_data, (*id)++, 4); /* Identifier */
So *id is incremented here and...
> +        /* Number of ID mappings */
> +        build_append_int_noprefix(table_data, IORT_RMR_NUM_ID_MAPPINGS, 4);
> +        /* Reference to ID Array */
> +        build_append_int_noprefix(table_data, IORT_RMR_COMMON_HEADER_SIZE, 4);
> +
> +        /* RMR specific data */
> +
> +        /* Flags */
> +        build_append_int_noprefix(table_data, IORT_RMR_FLAGS, 4);
> +        /* Number of Memory Range Descriptors */
> +        build_append_int_noprefix(table_data, IORT_RMR_NUM_MEM_RANGE_DESC, 4);
> +        /* Reference to Memory Range Descriptors */
> +        build_append_int_noprefix(table_data, IORT_RMR_COMMON_HEADER_SIZE +
> +                        (IORT_RMR_NUM_ID_MAPPINGS * ID_MAPPING_ENTRY_SIZE), 4);
> +        build_iort_id_mapping(table_data, bdf, idmap->id_count, sdev->offset,
> +                              1);
> +
> +        /* Table 19 Memory Range Descriptor */
> +
> +        /* Physical Range offset */
> +        build_append_int_noprefix(table_data, MSI_IOVA_BASE, 8);
> +        /* Physical Range length */
> +        build_append_int_noprefix(table_data, MSI_IOVA_LENGTH, 8);
> +        build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> +        *id += 1;
here. Why this second one? Perhaps a comment if this is intended.

> +    }
> +}