Re: [Xen-devel] [PATCH 1/2] amd-iommu: add flush iommu_ops

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/11/2018 10:45, Paul Durrant wrote:
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c 
> b/xen/drivers/passthrough/amd/iommu_map.c
> index 04cb7b3182..c05b042821 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -631,6 +631,54 @@ int amd_iommu_unmap_page(struct domain *d, dfn_t dfn)
>      spin_unlock(&hd->arch.mapping_lock);
>  
>      amd_iommu_flush_pages(d, dfn_x(dfn), 0);
> +    return 0;
> +}
> +
> +static unsigned long flush_count(dfn_t dfn, unsigned int page_count,
> +                                 unsigned int order)
> +{
> +    unsigned long start = dfn_x(dfn) / (1u << order);
> +    unsigned long end = DIV_ROUND_UP(dfn_x(dfn) + page_count,
> +                                     (1u << order));
> +
> +    ASSERT(end > start);
> +    return end - start;
> +}
> +
> +int amd_iommu_flush_iotlb_pages(struct domain *d, dfn_t dfn,
> +                                unsigned int page_count)

What are the semantics here?  Why page_count rather than order?  Are we
guaranteed to be actually flushing consecutive dfn's ?

> +{
> +    /* Match VT-d semantics */
> +    if ( !page_count || dfn_eq(dfn, INVALID_DFN) ||

Do we really have callers passing these?  I'd argue that these should be
invalid to pass (accepting that we might need to tolerate them until
other cleanup can occur).

> +         dfn_lt(dfn_add(dfn, page_count), dfn) /* overflow */ )

Given that all users of dfn here want it in unsigned long form, I'd make
a local dfn_l and use that.  I'm not sure that we want to start
accumulating a sporadic set of binary operator functions for the
typesafe variables.

> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 3d78126801..da8294bac8 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -52,6 +52,11 @@ static inline bool_t dfn_eq(dfn_t x, dfn_t y)
>      return dfn_x(x) == dfn_x(y);
>  }
>  
> +static inline bool_t dfn_lt(dfn_t x, dfn_t y)

No new introductions of bool_t please.  dfn_eq() shouldn't have been
bool_t either.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel