Re: [PATCH 6/7] VT-d: move dev_invalidate_iotlb() to the sole file it's used from

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 05, 2024 at 02:57:12PM +0100, Jan Beulich wrote:
> ..., thus allowing it and qinval_device_iotlb_sync() to become static.
> There's nothing x86-specific about the function anyway. While moving,
> adjust types to better match ./CODING_STYLE (albeit use of fixed-width
> types for parameters is retained to limit the effective change).
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> 
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -65,12 +65,6 @@ struct acpi_drhd_unit *ioapic_to_drhd(un
>  struct acpi_drhd_unit *hpet_to_drhd(unsigned int hpet_id);
>  struct acpi_rhsa_unit *drhd_to_rhsa(const struct acpi_drhd_unit *drhd);
>  
> -int dev_invalidate_iotlb(struct vtd_iommu *iommu, u16 did,
> -                         u64 addr, unsigned int size_order, u64 type);
> -
> -int __must_check qinval_device_iotlb_sync(struct vtd_iommu *iommu,
> -                                          struct pci_dev *pdev,
> -                                          u16 did, u16 size, u64 addr);
>  
>  uint64_t alloc_pgtable_maddr(unsigned long npages, nodeid_t node);
>  void free_pgtable_maddr(u64 maddr);
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -251,8 +251,9 @@ static int __must_check dev_invalidate_s
>      return rc;
>  }
>  
> -int qinval_device_iotlb_sync(struct vtd_iommu *iommu, struct pci_dev *pdev,
> -                             u16 did, u16 size, u64 addr)
> +static int qinval_device_iotlb_sync(struct vtd_iommu *iommu,
> +                                    struct pci_dev *pdev, uint16_t did,
> +                                    uint16_t size, paddr_t addr)
>  {
>      unsigned long flags;
>      unsigned int index;
> @@ -282,6 +283,101 @@ int qinval_device_iotlb_sync(struct vtd_
>      return dev_invalidate_sync(iommu, pdev, did);
>  }
>  
> +static bool device_in_domain(const struct vtd_iommu *iommu,
> +                             const struct pci_dev *pdev, uint16_t did)
> +{
> +    struct root_entry *root_entry;
> +    struct context_entry *ctxt_entry = NULL;
> +    unsigned int tt;
> +    bool found = false;
> +
> +    if ( unlikely(!iommu->root_maddr) )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return false;
> +    }
> +
> +    root_entry = map_vtd_domain_page(iommu->root_maddr);
> +    if ( !root_present(root_entry[pdev->bus]) )
> +        goto out;
> +
> +    ctxt_entry = map_vtd_domain_page(root_entry[pdev->bus].val);
> +    if ( context_domain_id(ctxt_entry[pdev->devfn]) != did )
> +        goto out;
> +
> +    tt = context_translation_type(ctxt_entry[pdev->devfn]);
> +    if ( tt != CONTEXT_TT_DEV_IOTLB )
> +        goto out;
> +
> +    found = true;
> + out:
> +    if ( root_entry )
> +        unmap_vtd_domain_page(root_entry);
> +
> +    if ( ctxt_entry )
> +        unmap_vtd_domain_page(ctxt_entry);
> +
> +    return found;
> +}
> +
> +static int dev_invalidate_iotlb(struct vtd_iommu *iommu, uint16_t did,
> +                                paddr_t addr, unsigned int size_order,
> +                                uint64_t type)
> +{
> +    struct pci_dev *pdev, *temp;
> +    int ret = 0;
> +
> +    if ( !ecap_dev_iotlb(iommu->ecap) )
> +        return ret;
> +
> +    list_for_each_entry_safe( pdev, temp, &iommu->ats_devices, ats.list )
> +    {
> +        bool sbit;
> +        int rc = 0;
> +
> +        switch ( type )
> +        {
> +        case DMA_TLB_DSI_FLUSH:
> +            if ( !device_in_domain(iommu, pdev, did) )
> +                break;
> +            /* fall through if DSI condition met */
> +        case DMA_TLB_GLOBAL_FLUSH:
> +            /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
> +            sbit = 1;
> +            addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;

Given the MISRA stuff, won't it be better to append 'UL' here while
moving?

Thanks, Roger.