Re: [PATCH v2 13/18] VT-d: allow use of superpage mappings

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.12.2021 12:54, Roger Pau Monné wrote:
> On Fri, Sep 24, 2021 at 11:52:47AM +0200, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -743,18 +743,37 @@ static int __must_check iommu_flush_iotl
>>      return iommu_flush_iotlb(d, INVALID_DFN, 0, 0);
>>  }
>>  
>> +static void queue_free_pt(struct domain *d, mfn_t mfn, unsigned int 
>> next_level)
> 
> Same comment as the AMD side patch, about naming the parameter just
> level.

Sure, will change.

>> @@ -1901,13 +1926,15 @@ static int __must_check intel_iommu_map_
>>      }
>>  
>>      page = (struct dma_pte *)map_vtd_domain_page(pg_maddr);
>> -    pte = &page[dfn_x(dfn) & LEVEL_MASK];
>> +    pte = &page[address_level_offset(dfn_to_daddr(dfn), level)];
>>      old = *pte;
>>  
>>      dma_set_pte_addr(new, mfn_to_maddr(mfn));
>>      dma_set_pte_prot(new,
>>                       ((flags & IOMMUF_readable) ? DMA_PTE_READ  : 0) |
>>                       ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0));
>> +    if ( IOMMUF_order(flags) )
> 
> You seem to use level > 1 in other places to check for whether the
> entry is intended to be a super-page. Is there any reason to use
> IOMMUF_order here instead?

"flags" is the original source of information here, so it seemed more
natural to use it. The following hunk uses "level > 1" to better
match the similar unmap logic as well as AMD code. Maybe I should
change those to also use "flags" (or "order" in the unmap case), as
that would allow re-using the local variable in the new patches in v3
doing the re-coalescing of present superpages (right now I'm using a
second, not very nicely named variable there).

I'll have to think about this some and check whether there are other
issues if I made such a change.

>> @@ -2328,6 +2361,11 @@ static int __init vtd_setup(void)
>>                 cap_sps_2mb(iommu->cap) ? ", 2MB" : "",
>>                 cap_sps_1gb(iommu->cap) ? ", 1GB" : "");
>>  
>> +        if ( !cap_sps_2mb(iommu->cap) )
>> +            large_sizes &= ~PAGE_SIZE_2M;
>> +        if ( !cap_sps_1gb(iommu->cap) )
>> +            large_sizes &= ~PAGE_SIZE_1G;
>> +
>>  #ifndef iommu_snoop
>>          if ( iommu_snoop && !ecap_snp_ctl(iommu->ecap) )
>>              iommu_snoop = false;
>> @@ -2399,6 +2437,9 @@ static int __init vtd_setup(void)
>>      if ( ret )
>>          goto error;
>>  
>> +    ASSERT(iommu_ops.page_sizes & PAGE_SIZE_4K);
> 
> Since you are adding the assert, it might be more complete to check no
> other page sizes are set, iommu_ops.page_sizes == PAGE_SIZE_4K?

Ah yes, would make sense. Let me change this.

Jan