Alejandro,
On 3/11/2025 8:54 PM, Alejandro Jimenez wrote:
> The AMD I/O Virtualization Technology (IOMMU) Specification (see Table 8: V,
> TV, and GV Fields in Device Table Entry), specifies that a DTE with V=0,
> TV=1 does not contain a valid address translation information. If a request
> requires a table walk, the walk is terminated when this condition is
> encountered.
>
> Do not assume that addresses for a device with DTE[TV]=0 are passed through
> (i.e. not remapped) and instead terminate the page table walk early.
>
> Cc: qemu-sta...@nongnu.org
> Fixes: d29a09ca6842 ("hw/i386: Introduce AMD IOMMU")
> Signed-off-by: Alejandro Jimenez <alejandro.j.jime...@oracle.com>
I did double check and I think this patch matches HW behaviour. I did run few
tests w/ this series. It seems to work fine for me.
Reviewed-by: Vasant Hegde <vasant.he...@amd.com>
> ---
> hw/i386/amd_iommu.c | 88 +++++++++++++++++++++++++--------------------
> 1 file changed, 49 insertions(+), 39 deletions(-)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index cf00450ebe..31d5522a62 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -932,51 +932,61 @@ static void amdvi_page_walk(AMDVIAddressSpace *as,
> uint64_t *dte,
> uint64_t pte = dte[0], pte_addr, page_mask;
>
> /* make sure the DTE has TV = 1 */
> - if (pte & AMDVI_DEV_TRANSLATION_VALID) {
> - level = get_pte_translation_mode(pte);
> - if (level >= 7) {
> - trace_amdvi_mode_invalid(level, addr);
> + if (!(pte & AMDVI_DEV_TRANSLATION_VALID)) {
> + /*
> + * A DTE with V=1, TV=0 does not have a valid Page Table Root
> Pointer.
> + * An IOMMU processing a request that requires a table walk
> terminates
> + * the walk when it encounters this condition. Do the same and return
> + * instead of assuming that the address is forwarded without
> translation
> + * i.e. the passthrough case, as it is done for the case where
> DTE[V]=0.
> + */
> + return;
> + }
> +
> + level = get_pte_translation_mode(pte);
> + if (level >= 7) {
> + trace_amdvi_mode_invalid(level, addr);
> + return;
> + }
> + if (level == 0) {
> + goto no_remap;
> + }
> +
> + /* we are at the leaf page table or page table encodes a huge page */
> + do {
> + pte_perms = amdvi_get_perms(pte);
> + present = pte & 1;
> + if (!present || perms != (perms & pte_perms)) {
> + amdvi_page_fault(as->iommu_state, as->devfn, addr, perms);
> + trace_amdvi_page_fault(addr);
> return;
> }
> - if (level == 0) {
> - goto no_remap;
> - }
>
> - /* we are at the leaf page table or page table encodes a huge page */
> - do {
> - pte_perms = amdvi_get_perms(pte);
> - present = pte & 1;
> - if (!present || perms != (perms & pte_perms)) {
> - amdvi_page_fault(as->iommu_state, as->devfn, addr, perms);
> - trace_amdvi_page_fault(addr);
> - return;
> - }
> -
> - /* go to the next lower level */
> - pte_addr = pte & AMDVI_DEV_PT_ROOT_MASK;
> - /* add offset and load pte */
> - pte_addr += ((addr >> (3 + 9 * level)) & 0x1FF) << 3;
> - pte = amdvi_get_pte_entry(as->iommu_state, pte_addr, as->devfn);
> - if (!pte) {
> - return;
> - }
> - oldlevel = level;
> - level = get_pte_translation_mode(pte);
> - } while (level > 0 && level < 7);
> -
> - if (level == 0x7) {
> - page_mask = pte_override_page_mask(pte);
> - } else {
> - page_mask = pte_get_page_mask(oldlevel);
> + /* go to the next lower level */
> + pte_addr = pte & AMDVI_DEV_PT_ROOT_MASK;
> + /* add offset and load pte */
> + pte_addr += ((addr >> (3 + 9 * level)) & 0x1FF) << 3;
> + pte = amdvi_get_pte_entry(as->iommu_state, pte_addr, as->devfn);
> + if (!pte) {
> + return;
> }
> + oldlevel = level;
> + level = get_pte_translation_mode(pte);
Unrelated to this patch.. We may want to add check to make sure level is
decreasing. Something like
if (oldlevel <= level)
error out
Otherwise bad page table can cause the issue.
-Vasant
> + } while (level > 0 && level < 7);
>
> - /* get access permissions from pte */
> - ret->iova = addr & page_mask;
> - ret->translated_addr = (pte & AMDVI_DEV_PT_ROOT_MASK) & page_mask;
> - ret->addr_mask = ~page_mask;
> - ret->perm = amdvi_get_perms(pte);
> - return;
> + if (level == 0x7) {
> + page_mask = pte_override_page_mask(pte);
> + } else {
> + page_mask = pte_get_page_mask(oldlevel);
> }
> +
> + /* get access permissions from pte */
> + ret->iova = addr & page_mask;
> + 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;
