Re: [PATCH 1/2] amd_iommu: Honor DTE[IR] and DTE[IW] when DTE[Mode] is 0

Hi Daniel,

On 7/18/26 1:52 PM, Daniel Paziyski wrote:

> In an AMD IOMMU's Device Table Entry (DTE), the Mode field specifies the number
> of levels that the page table hierarchy for translating Guest Physical Addresses
> (GPAs) to System Physical Addresses (SPAs) will have. When the DTE is valid and
> translation is active (V = 1, TV = 1), and Mode is equal to 0, then the IOMMU
> will act as a "filter": GPAs are the same as SPAs, but, whether reads or writes
> are allowed depends on the values of I/O read permission (IR) and I/O write
> permission (IW) in the DTE.
> 
> In the current code, the values of IR and IW are considered only when traversing
> the page tables for translating GPAs to SPAs when Mode is not 0 (and are ANDed
> with their counterparts in the page tables to determine the final permission),
> and when the DTE for a device is in passthrough mode (Mode is 0), IR and IW
> are not taken into consideration, and access is given unconditionally, which
> conflicts with the specification.
> 
> This patch implements honoring of the IR and IW DTE bits when Mode is 0. To do
> so, both the iommu and iommu_nodma memory regions in the AMDVIAddressSpace
> struct are preserved, and a new field is added, passthrough_perms, which along
> with addr_translation will determine whether the iommu or the iommu_nodma memory
> region will be used.
> 
> After a DTE invalidation, passthrough_perms and addr_translation will be set
> accordingly with the new DTE configuration, and when it's time to switch address
> spaces in amdvi_switch_address_space:
> 
>     - Iff passthrough is desired and IR = 1 and IW = 1, use the iommu_nodma
>       memory region,
> 
>     - otherwise, use the iommu memory region.
> 
> The handler for translation (amdvi_translate), which is called when the iommu
> memory region is enabled, has a fastpath which handles passthrough mode and
> verifies permissions. Additionally, since the DTE is fetched in the page walker,
> and it is possible for it to have been updated concurrently, if passthrough mode
> is requested, it is handled properly. Moreover, care has been taken to not fill
> the IOTLB with entries handed out when passthrough is active.
> 

Thank you for the patches and the detailed description in the commit
message. While the problem you are describing is valid, I think the
solution in this series only addresses it partially. If my understanding is
correct, your proposed Mode=0 handling covers a scenario like the following:

- an earlier INVALIDATE_DEVTAB_ENTRY read DTE[Mode]>0 and set
addr_translation = true
- guest changes DTE in RAM to Mode=0
- The corresponding DTE invalidation has not yet been received/processed by
QEMU, so addr_translation still reflects the older DTE with Mode>0
- a DMA request misses the IOTLB and triggers a page walk
- amdvi_do_translate() reads the new Mode=0 DTE, which conflicts with the
older DTE state cached in addr_translation = true

I think the reverse scenario can also happen where a previous
addr_translation = false can select the passthrough code path while the
guest memory already has a DTE[Mode]>0.

There are other issues that the patch doesn't address e.g. for a VFIO
device, the replay() callback does not handle Mode=0. It calls
amdvi_sync_shadow_page_table_range()->fetch_pte() which rejects Mode=0, so
no MAP notifications would be generated in case of DTEs with RO or WO
permissions.

The larger problem is not with your patch, but that the current code has
different emulation paths making decisions based on different versions of
the DTE. e.g. the current code makes it possible to have addr_translation
(and passthrough_perms) derived from an "older" DTE state be inconsistent
with a newly retrieved DTE data from guest memory. Your change is fixing a
special case, but unfortunately I think we need a larger rework so that it
can be applied without introducing additional issues.

Based on my reading of the AMDVi documentation, the OS must issue
INVALIDATE_DEVTAB_ENTRY after modifying a DTE before it can rely on any the
updated field(s) being used. A guest can change the Device Table in its
RAM, but the synchronization point where the change starts affecting
behavior in the HW (or emulated HW in this case) is after the next
INVALIDATE_DEVTAB_ENTRY command.

The approach I am considering is to cache the relevant DTE state (i.e. only
the address translation related state) for each address space, and update
it when processing INVALIDATE_DEVTAB_ENTRY or a global invalidation
(INVALIDATE_IOMMU_ALL). THis way we avoid reading the DTE from guest memory
at various paths, and instead use the cached version, so the emulation code
makes decisions based on a consistent state.

This new DTE cache and the existing IOTLB cache should remain separate i.e.
INVALIDATE_DEVTAB_ENTRY would update the DTE state but shouldn't call
iommu_inval_iotlb(), based on my reading of Section 2.4.2
INVALIDATE_DEVTAB_ENTRY from the spec.

I think what I describe above should be a prerequisite for your patch to
then implement consistent handling of IR/IW permissions with Mode=0. It is
a much larger change, so I am still working through it, but you are welcome
to contribute now that I have hopefully explained the idea :).

Thank you,
Alejandro


> Also, for the sake of easier debugging, the trace_amdvi_page_fault tracepoint
> now also takes the BDF of the device which has caused the page fault.
> 
> Signed-off-by: Daniel Paziyski <[email protected]>
> ---
>  hw/i386/amd_iommu.c  | 136 +++++++++++++++++++++++++++++--------------
>  hw/i386/trace-events |   2 +-
>  2 files changed, 92 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 90252c52af..10d75a5088 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -54,6 +54,8 @@ struct AMDVIAddressSpace {
>      IOVATree *iova_tree;
>      /* DMA address translation active */
>      bool addr_translation;
> +    /* Permissions for direct map when DMA address translation is not active */
> +    unsigned passthrough_perms;
>  };
>  
>  /* AMDVI cache entry */
> @@ -1162,34 +1164,16 @@ static void amdvi_switch_address_space(AMDVIAddressSpace *amdvi_as)
>  {
>      AMDVIState *s = amdvi_as->iommu_state;
>  
> -    if (s->dma_remap && amdvi_as->addr_translation) {
> -        /* Enabling DMA region */
> -        memory_region_set_enabled(&amdvi_as->iommu_nodma, false);
> -        memory_region_set_enabled(MEMORY_REGION(&amdvi_as->iommu), true);
> -    } else {
> -        /* Disabling DMA region, using passthrough */
> -        memory_region_set_enabled(MEMORY_REGION(&amdvi_as->iommu), false);
> -        memory_region_set_enabled(&amdvi_as->iommu_nodma, true);
> +    if (!(s->dma_remap && amdvi_as->addr_translation)) {
> +        if (amdvi_as->passthrough_perms == (AMDVI_PERM_READ | AMDVI_PERM_WRITE)) {
> +            memory_region_set_enabled(MEMORY_REGION(&amdvi_as->iommu), false);
> +            memory_region_set_enabled(&amdvi_as->iommu_nodma, true);
> +            return;
> +        }
>      }
> -}
> -
> -/*
> - * For all existing address spaces managed by the IOMMU, enable/disable the
> - * corresponding memory regions to reset the address translation mode and
> - * use passthrough by default.
> - */
> -static void amdvi_reset_address_translation_all(AMDVIState *s)
> -{
> -    AMDVIAddressSpace *iommu_as;
> -    GHashTableIter as_it;
> -
> -    g_hash_table_iter_init(&as_it, s->address_spaces);
>  
> -    while (g_hash_table_iter_next(&as_it, NULL, (void **)&iommu_as)) {
> -        /* Use passthrough as default mode after reset */
> -        iommu_as->addr_translation = false;
> -        amdvi_switch_address_space(iommu_as);
> -    }
> +    memory_region_set_enabled(&amdvi_as->iommu_nodma, false);
> +    memory_region_set_enabled(MEMORY_REGION(&amdvi_as->iommu), true);
>  }
>  
>  static void enable_dma_mode(AMDVIAddressSpace *as, bool inval_current)
> @@ -1222,23 +1206,43 @@ static void enable_dma_mode(AMDVIAddressSpace *as, bool inval_current)
>   * - invalidate all existing mappings
>   * - switch to no_dma memory region
>   */
> -static void enable_nodma_mode(AMDVIAddressSpace *as)
> +static void enable_nodma_mode(AMDVIAddressSpace *as, unsigned perms)
>  {
>      IOMMUNotifier *n;
>  
> -    if (!as->addr_translation) {
> +    if (!as->addr_translation && as->passthrough_perms == perms) {
>          /* passthrough is already active, nothing to do */
>          return;
>      }
>  
> -    as->addr_translation = false;
>      IOMMU_NOTIFIER_FOREACH(n, &as->iommu) {
>          /* Drop all mappings for the address space */
>          amdvi_address_space_unmap(as, n);
>      }
> +
> +    as->addr_translation = false;
> +    as->passthrough_perms = perms;
>      amdvi_switch_address_space(as);
>  }
>  
> +/*
> + * For all existing address spaces managed by the IOMMU, enable/disable the
> + * corresponding memory regions to reset the address translation mode and
> + * use passthrough by default.
> + */
> +static void amdvi_reset_address_translation_all(AMDVIState *s)
> +{
> +    AMDVIAddressSpace *iommu_as;
> +    GHashTableIter as_it;
> +
> +    g_hash_table_iter_init(&as_it, s->address_spaces);
> +
> +    while (g_hash_table_iter_next(&as_it, NULL, (void **)&iommu_as)) {
> +        /* Use passthrough as default mode after reset */
> +        enable_nodma_mode(iommu_as, AMDVI_PERM_READ | AMDVI_PERM_WRITE);
> +    }
> +}
> +
>  /*
>   * 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
> @@ -1252,6 +1256,7 @@ static void amdvi_update_addr_translation_mode(AMDVIState *s, uint16_t devid)
>      AMDVIAddressSpace *as;
>      uint64_t dte[4] = { 0 };
>      int ret;
> +    unsigned perms;
>  
>      as = amdvi_get_as_by_devid(s, devid);
>      if (!as) {
> @@ -1268,14 +1273,16 @@ static void amdvi_update_addr_translation_mode(AMDVIState *s, uint16_t devid)
>      case 0:
>          /* DTE was successfully retrieved */
>          if (!dte_mode) {
> -            enable_nodma_mode(as); /* DTE[V]=1 && DTE[Mode]=0 => passthrough */
> +            /* DTE[V]=1 && DTE[Mode]=0 => passthrough */
> +            perms = amdvi_get_perms(dte[0]);
> +            enable_nodma_mode(as, perms);
>          } else {
>              enable_dma_mode(as, false); /* Enable DMA translation */
>          }
>          break;
>      case -AMDVI_FR_DTE_V:
>          /* DTE[V]=0, address is passed untranslated */
> -        enable_nodma_mode(as);
> +        enable_nodma_mode(as, AMDVI_PERM_READ | AMDVI_PERM_WRITE);
>          break;
>      case -AMDVI_FR_DTE_RTR_ERR:
>      case -AMDVI_FR_DTE_TV:
> @@ -1889,15 +1896,45 @@ static void amdvi_mmio_write(void *opaque, hwaddr addr, uint64_t val,
>      }
>  }
>  
> +static void amdvi_fill_iotlb_entry_passthrough(IOMMUTLBEntry *entry,
> +                                               AMDVIAddressSpace *as,
> +                                               unsigned passthrough_perms,
> +                                               hwaddr addr,
> +                                               IOMMUAccessFlags flag)
> +{
> +    unsigned perms = 0;
> +
> +    if (flag & IOMMU_RO) {
> +        perms |= AMDVI_PERM_READ;
> +    }
> +    if (flag & IOMMU_WO) {
> +        perms |= AMDVI_PERM_WRITE;
> +    }
> +
> +    if ((passthrough_perms & perms) != perms) {
> +        amdvi_page_fault(as->iommu_state, as->devfn, addr, perms);
> +        trace_amdvi_page_fault(addr, pci_bus_num(as->bus), PCI_SLOT(as->devfn),
> +                               PCI_FUNC(as->devfn), perms);
> +        return;
> +    }
> +
> +    entry->iova = addr & AMDVI_PAGE_MASK_4K;
> +    entry->translated_addr = addr & AMDVI_PAGE_MASK_4K;
> +    entry->addr_mask = ~AMDVI_PAGE_MASK_4K;
> +    entry->perm = flag & IOMMU_RW;
> +}
> +
>  static void amdvi_page_walk(AMDVIAddressSpace *as, uint64_t *dte,
>                              IOMMUTLBEntry *ret, unsigned perms,
> -                            hwaddr addr)
> +                            hwaddr addr, bool *update_iotlb)
>  {
>      hwaddr page_mask, pagesize = 0;
>      uint8_t mode;
>      uint64_t pte;
>      int fetch_ret;
>  
> +    *update_iotlb = false;
> +
>      /* make sure the DTE has TV = 1 */
>      if (!(dte[0] & AMDVI_DEV_TRANSLATION_VALID)) {
>          /*
> @@ -1916,7 +1953,9 @@ static void amdvi_page_walk(AMDVIAddressSpace *as, uint64_t *dte,
>          return;
>      }
>      if (mode == 0) {
> -        goto no_remap;
> +        amdvi_fill_iotlb_entry_passthrough(ret, as, amdvi_get_perms(dte[0]),
> +            addr, perms == AMDVI_PERM_WRITE ? IOMMU_WO : IOMMU_RO);
> +        return;
>      }
>  
>      /* Attempt to fetch the PTE to determine if a valid mapping exists */
> @@ -1931,7 +1970,8 @@ static void amdvi_page_walk(AMDVIAddressSpace *as, uint64_t *dte,
>          perms != (perms & amdvi_get_perms(pte))) {
>  
>          amdvi_page_fault(as->iommu_state, as->devfn, addr, perms);
> -        trace_amdvi_page_fault(addr);
> +        trace_amdvi_page_fault(addr, pci_bus_num(as->bus), PCI_SLOT(as->devfn),
> +                               PCI_FUNC(as->devfn), perms);
>          return;
>      }
>  
> @@ -1944,13 +1984,7 @@ static void amdvi_page_walk(AMDVIAddressSpace *as, uint64_t *dte,
>      ret->translated_addr = (pte & AMDVI_DEV_PT_ROOT_MASK) & page_mask;
>      ret->addr_mask = ~page_mask;
>      ret->perm = amdvi_get_perms(pte);
> -    return;
> -
> -no_remap:
> -    ret->iova = addr & AMDVI_PAGE_MASK_4K;
> -    ret->translated_addr = addr & AMDVI_PAGE_MASK_4K;
> -    ret->addr_mask = ~AMDVI_PAGE_MASK_4K;
> -    ret->perm = amdvi_get_perms(dte[0]);
> +    *update_iotlb = true;
>  }
>  
>  static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr,
> @@ -1961,6 +1995,7 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr,
>      AMDVIIOTLBEntry *iotlb_entry = amdvi_iotlb_lookup(s, addr, devid);
>      uint64_t entry[4];
>      int dte_ret;
> +    bool update_iotlb;
>  
>      if (iotlb_entry) {
>          trace_amdvi_iotlb_hit(PCI_BUS_NUM(devid), PCI_SLOT(devid),
> @@ -1983,10 +2018,14 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr,
>      }
>  
>      amdvi_page_walk(as, entry, ret,
> -                    is_write ? AMDVI_PERM_WRITE : AMDVI_PERM_READ, addr);
> +                    is_write ? AMDVI_PERM_WRITE : AMDVI_PERM_READ, addr,
> +                    &update_iotlb);
> +
> +    if (update_iotlb) {
> +        amdvi_update_iotlb(s, devid, addr, *ret,
> +                           entry[1] & AMDVI_DEV_DOMID_ID_MASK);
> +    }
>  
> -    amdvi_update_iotlb(s, devid, addr, *ret,
> -                       entry[1] & AMDVI_DEV_DOMID_ID_MASK);
>      return;
>  
>  out:
> @@ -2031,7 +2070,13 @@ static IOMMUTLBEntry amdvi_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
>          return ret;
>      }
>  
> -    amdvi_do_translate(as, addr, flag & IOMMU_WO, &ret);
> +    if (!as->addr_translation) {
> +        amdvi_fill_iotlb_entry_passthrough(&ret, as, as->passthrough_perms,
> +                                           addr, flag);
> +    } else {
> +        amdvi_do_translate(as, addr, flag & IOMMU_WO, &ret);
> +    }
> +
>      trace_amdvi_translation_result(pci_bus_num(as->bus), PCI_SLOT(as->devfn),
>              PCI_FUNC(as->devfn), addr, ret.translated_addr);
>      return ret;
> @@ -2423,6 +2468,7 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>          amdvi_dev_as->notifier_flags = IOMMU_NOTIFIER_NONE;
>          amdvi_dev_as->iova_tree = iova_tree_new();
>          amdvi_dev_as->addr_translation = false;
> +        amdvi_dev_as->passthrough_perms = AMDVI_PERM_READ | AMDVI_PERM_WRITE;
>          key->bus = bus;
>          key->devfn = devfn;
>  
> diff --git a/hw/i386/trace-events b/hw/i386/trace-events
> index a1a50d0910..df2d046000 100644
> --- a/hw/i386/trace-events
> +++ b/hw/i386/trace-events
> @@ -103,7 +103,7 @@ amdvi_dte_get_fail(uint64_t addr, uint32_t offset) "error: failed to access Devi
>  amdvi_invalid_dte(uint64_t addr) "PTE entry at 0x%"PRIx64" is invalid "
>  amdvi_get_pte_hwerror(uint64_t addr) "hardware error eccessing PTE at addr 0x%"PRIx64
>  amdvi_mode_invalid(uint8_t level, uint64_t addr)"error: translation level 0x%"PRIx8" translating addr 0x%"PRIx64
> -amdvi_page_fault(uint64_t addr) "error: page fault accessing guest physical address 0x%"PRIx64
> +amdvi_page_fault(uint64_t addr, uint8_t bus, uint8_t slot, uint8_t func, unsigned perms) "error: page fault accessing guest physical address 0x%"PRIx64" by devid %02x:%02x.%x with perms 0x%x"
>  amdvi_iotlb_hit(uint8_t bus, uint8_t slot, uint8_t func, uint64_t addr, uint64_t txaddr) "hit iotlb devid %02x:%02x.%x gpa 0x%"PRIx64" hpa 0x%"PRIx64
>  amdvi_translation_result(uint8_t bus, uint8_t slot, uint8_t func, uint64_t addr, uint64_t txaddr) "devid: %02x:%02x.%x gpa 0x%"PRIx64" hpa 0x%"PRIx64
>  amdvi_mem_ir_write_req(uint64_t addr, uint64_t val, uint32_t size) "addr 0x%"PRIx64" data 0x%"PRIx64" size 0x%"PRIx32
> -- 
> 2.55.0
>