Re: [PATCH v2 18/20] amd_iommu: Toggle address translation mode on devtab entry invalidation

[Ethan MILON <ethan.milon@eviden.com>]

Hi,

On 5/2/25 4:16 AM, Alejandro Jimenez wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
> 
> 
> A guest must issue an INVALIDATE_DEVTAB_ENTRY command after changing a
> Device Table entry (DTE) e.g. after attaching a device and setting up its
> DTE. When intercepting this event, determine if the DTE has been configured
> for paging or not, and toggle the appropriate memory regions to allow DMA
> address translation for the address space if needed. Requires dma-remap=on.
> 
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
>  hw/i386/amd_iommu.c | 78 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index a2df73062bf7..75a92067f35f 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -991,18 +991,92 @@ static void amdvi_switch_address_space_all(AMDVIState *s)
>      }
>  }
> 
> +/*
> + * A guest driver must issue the INVALIDATE_DEVTAB_ENTRY command to the IOMMU
> + * after changing a Device Table entry. We can use this fact to detect when a
> + * Device Table entry is created for a device attached to a paging domain and
> + * enable the corresponding IOMMU memory region to allow for DMA translation if
> + * appropriate.
> + */
> +static void amdvi_update_addr_translation_mode(AMDVIState *s, uint16_t devid)
> +{
> +    uint8_t bus_num, devfn, dte_mode;
> +    AMDVIAddressSpace *as;
> +    uint64_t dte[4] = { 0 };
> +    IOMMUNotifier *n;
> +    int ret;
> +
> +    /*
> +     * Convert the devid encoded in the command to a bus and devfn in
> +     * order to retrieve the corresponding address space.
> +     */
> +    bus_num = PCI_BUS_NUM(devid);
> +    devfn = devid & 0xff;
> +
> +    /*
> +     * The main buffer of size (AMDVIAddressSpace *) * (PCI_BUS_MAX) has already
> +     * been allocated within AMDVIState, but must be careful to not access
> +     * unallocated devfn.
> +     */
> +    if (!s->address_spaces[bus_num] || !s->address_spaces[bus_num][devfn]) {
> +        return;
> +    }
> +    as = s->address_spaces[bus_num][devfn];
> +
> +    ret = amdvi_as_to_dte(as, dte);
> +
> +    if (!ret) {
> +        dte_mode = (dte[0] >> AMDVI_DEV_MODE_RSHIFT) & AMDVI_DEV_MODE_MASK;
> +    }
> +
> +    if ((ret < 0) || (!ret && !dte_mode)) {
> +        /*
> +         * The DTE could not be retrieved, it is not valid, or it is not setup
> +         * for paging. In either case, ensure that if paging was previously in
> +         * use then invalidate all existing mappings and then switch to use the
> +         * no_dma memory region.
> +         */

If the DTE is malformed or could not be retrieved, wouldn't it be safer
to default to the DMA region rather than falling back to direct access?
Or am I missing something?

Thanks,

Ethan

> +        if (as->addr_translation) {
> +            as->addr_translation = false;
> +
> +            IOMMU_NOTIFIER_FOREACH(n, &as->iommu) {
> +                amdvi_address_space_unmap(as, n);
> +            }
> +            amdvi_switch_address_space(as);
> +        }
> +    } else if (!as->addr_translation) {
> +        /*
> +         * Installing a DTE that enables translation where it wasn't previously
> +         * active. Activate the DMA memory region.
> +         */
> +        as->addr_translation = true;
> +        amdvi_switch_address_space(as);
> +        amdvi_address_space_sync(as);
> +    }
> +}
> +
>  /* log error without aborting since linux seems to be using reserved bits */
>  static void amdvi_inval_devtab_entry(AMDVIState *s, uint64_t *cmd)
>  {
>      uint16_t devid = cpu_to_le16((uint16_t)extract64(cmd[0], 0, 16));
> 
> +    trace_amdvi_devtab_inval(PCI_BUS_NUM(devid), PCI_SLOT(devid),
> +                             PCI_FUNC(devid));
> +
>      /* This command should invalidate internal caches of which there isn't */
>      if (extract64(cmd[0], 16, 44) || cmd[1]) {
>          amdvi_log_illegalcom_error(s, extract64(cmd[0], 60, 4),
>                                     s->cmdbuf + s->cmdbuf_head);
> +        return;
> +    }
> +
> +    /*
> +     * When DMA remapping capability is enabled, check if updated DTE is setup
> +     * for paging or not, and configure the corresponding memory regions.
> +     */
> +    if (s->dma_remap) {
> +        amdvi_update_addr_translation_mode(s, devid);
>      }
> -    trace_amdvi_devtab_inval(PCI_BUS_NUM(devid), PCI_SLOT(devid),
> -                             PCI_FUNC(devid));
>  }
> 
>  static void amdvi_complete_ppr(AMDVIState *s, uint64_t *cmd)
> --
> 2.43.5
>