Re: [PATCH] iommu/vtd: fix address translation for superpages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.05.2023 12:41, Roger Pau Monne wrote:
> When translating an address that falls inside of a superpage in the
> IOMMU page tables the fetching of the PTE physical address field
> wasn't using dma_pte_addr(), which caused the returned data to be
> corrupt as it would contain bits not related to the address field.

I'm afraid I don't understand:

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -359,16 +359,18 @@ static uint64_t addr_to_dma_page_maddr(struct domain 
> *domain, daddr_t addr,
>  
>              if ( !alloc )
>              {
> -                pte_maddr = 0;
>                  if ( !dma_pte_present(*pte) )
> +                {
> +                    pte_maddr = 0;
>                      break;
> +                }
>  
>                  /*
>                   * When the leaf entry was requested, pass back the full PTE,
>                   * with the address adjusted to account for the residual of
>                   * the walk.
>                   */
> -                pte_maddr = pte->val +
> +                pte_maddr +=
>                      (addr & ((1UL << level_to_offset_bits(level)) - 1) &
>                       PAGE_MASK);

With this change you're now violating what the comment says (plus what
the comment ahead of the function says). And it says what it says for
a reason - see intel_iommu_lookup_page(), which I think your change is
breaking.

Note also the following code:

                if ( !target )
                    break;
            }

            pte_maddr = level - 1;

IOW the local variable is overwritten right away unless target == 0.

Jan