Re: [PATCH v2 1/5] vtd: Drop (un)map_vtd_domain_page wrapper

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.10.2025 11:12, Teddy Astie wrote:
> Replace use of (un)map_vtd_domain_page with its general
> equivalents. Also take the opportunity to make iommu->root_page
> (previously root_maddr) use struct page_info instead of u64.

free_pgtable_maddr() also goes away as it seems.

> @@ -244,7 +245,7 @@ static unsigned int alloc_remap_entry(struct vtd_iommu 
> *iommu, unsigned int nr)
>          {
>              /* This entry across page boundry */
>              if ( iremap_entries )
> -                unmap_vtd_domain_page(iremap_entries);
> +                unmap_domain_page(iremap_entries);

Please can you also drop such redundant conditionals then? unmap_domain_page()
may validly be called with NULL. Overall it looks as if this patch could do
with splitting, as the unmap_vtd_domain_page() replacement is an entirely
mechanical one (and accounts for a fair part of the patch). I'm not going to
insist, though.

> @@ -366,8 +367,8 @@ struct iremap_entry {
>   */
>  #define GET_IREMAP_ENTRY(maddr, index, entries, entry)                       
>  \
>  do {                                                                         
>  \
> -    entries = (struct iremap_entry *)map_vtd_domain_page(                    
>  \
> -              (maddr) + (( (index) >> IREMAP_ENTRY_ORDER ) << PAGE_SHIFT ) 
> ); \
> +    entries = (struct iremap_entry *)map_domain_page(maddr_to_mfn(           
>  \
> +              (maddr) + (( (index) >> IREMAP_ENTRY_ORDER ) << PAGE_SHIFT ) 
> ));\
>      entry = &entries[(index) % (1 << IREMAP_ENTRY_ORDER)];                   
>  \
>  } while(0)

You drop casts elsewhere - why not here? Also, as you touch the 2nd of the 
lines,
can you please also drop the excess blanks? And then, while only theoretically 
at
risk of overflowing, the open-coded left shift may also want replacing by e.g.
pfn_to_paddr(), i.e. something which incorporates a suitable cast to paddr_t.

> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -18,6 +18,7 @@
>   */
>  
>  
> +#include <xen/domain_page.h>
>  #include <xen/sched.h>
>  #include <xen/iommu.h>
>  #include <xen/time.h>

Please could you take the opportunity and also get rid of one of the two blank
lines?

> @@ -99,38 +100,38 @@ void print_vtd_entries(struct vtd_iommu *iommu, int bus, 
> int devfn, u64 gmfn)
>             iommu->index, &PCI_SBDF(iommu->drhd->segment, bus, devfn),
>             gmfn);
>  
> -    if ( iommu->root_maddr == 0 )
> +    if ( iommu->root_page == NULL )
>      {
> -        printk("    iommu->root_maddr = 0\n");
> +        printk("    iommu->root_maddr = NULL\n");

root_page here then as well?

Jan