Re: [PATCH v5 5/8] remove remaining uses of iommu_legacy_map/unmap

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.09.2020 09:40, Paul Durrant wrote:
> NOTE: The code in memory_add() now sets 'ret' if iommu_map() or
>       iommu_iotlb_flush() fails. This seems to be have been missed before,
>       meaning the error path could actually return 0.

I agree this is the better way, but I'm not sure it really was
unintended: It could well have been considered a "best effort"
approach to get IOMMU mappings in place, back at the time.

> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one nit and one further remark:

> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1478,21 +1478,15 @@ int memory_add(unsigned long spfn, unsigned long 
> epfn, unsigned int pxm)
>           !iommu_use_hap_pt(hardware_domain) &&
>           !need_iommu_pt_sync(hardware_domain) )
>      {
> -        for ( i = spfn; i < epfn; i++ )
> -            if ( iommu_legacy_map(hardware_domain, _dfn(i), _mfn(i),
> -                                  1ul << PAGE_ORDER_4K,
> -                                  IOMMUF_readable | IOMMUF_writable) )
> -                break;
> -        if ( i != epfn )
> -        {
> -            while (i-- > old_max)
> -                /* If statement to satisfy __must_check. */
> -                if ( iommu_legacy_unmap(hardware_domain, _dfn(i),
> -                                        1ul << PAGE_ORDER_4K) )
> -                    continue;
> +        unsigned int flush_flags = 0;
> +        unsigned long n = epfn - spfn;
>  
> +        ret = iommu_map(hardware_domain, _dfn(i), _mfn(i), n,
> +                       IOMMUF_readable | IOMMUF_writable, &flush_flags);

There's one blank too little here.

> @@ -367,7 +344,7 @@ int iommu_iotlb_flush(struct domain *d, dfn_t dfn, 
> unsigned long page_count,
>      int rc;
>  
>      if ( !is_iommu_enabled(d) || !hd->platform_ops->iotlb_flush ||
> -         !page_count || !flush_flags )
> +         !page_count || !flush_flags || this_cpu(iommu_dont_flush_iotlb) )
>          return 0;

I'm still somewhat uneasy about this change, because there could in
theory be fallout. I don't think there is in practice, so I'm willing
to let it be this way. Long term we wan want to introduce a flag
into flush_flags to allow overriding this behavior.

Jan