On 10.05.2023 10:27, Roger Pau Monné wrote:
> On Tue, May 09, 2023 at 06:06:45PM +0200, Jan Beulich wrote:
>> 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.
>
> Hm, but the code in intel_iommu_lookup_page() is now wrong as it takes
> the bits in DMA_PTE_CONTIG_MASK as part of the physical address when
> doing the conversion to mfn? maddr_to_mfn() doesn't perform a any
> masking to remove the bits above PADDR_BITS.
Oh, right. But that's a missing dma_pte_addr() in intel_iommu_lookup_page()
then. (It would likely be better anyway to switch "uint64_t val" to
"struct dma_pte pte" there, to make more visible that it's a PTE we're
dealing with.) I indeed overlooked this aspect when doing the earlier
change.
Jan
