Re: [PATCH 1/3] IOMMU: record the flushes skipped under iommu_dont_flush_iotlb

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 30/10/2026 à 17:53, Luca Di Maio a écrit :
> While iommu_dont_flush_iotlb is set, iommu_legacy_unmap() and the
> shared-EPT path of ept_set_entry() skip their IOTLB flush and drop the
> flags it would have used. Accumulate them in the per-CPU
> iommu_skipped_flush_flags, so that a caller deferring the flush can
> issue it with the same flags. The next patch does so for grant unmaps.
>
> Use a uint8_t, as a fixed width is meant: a byte holds every flag and
> fits in the padding after iommu_dont_flush_iotlb, so the per-CPU data
> that follows doesn't move.
>
> Assisted-by: Claude Code:claude-opus-5-5
> Signed-off-by: Luca Di Maio <luca@xxxxxxxxx>
>
> ---
>   xen/arch/x86/mm/p2m-ept.c       |  4 ++++
>   xen/drivers/passthrough/iommu.c | 12 +++++++++++-
>   xen/include/xen/iommu.h         |  1 +
>   3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 204bdb054a..1a87b25d94 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -981,6 +981,10 @@ out:
>                                      (iommu_flags ? IOMMU_FLUSHF_added : 0) |
>                                      (vtd_pte_present ? IOMMU_FLUSHF_modified
>                                                       : 0));
> +        else if ( iommu_use_hap_pt(d) )
> +            this_cpu(iommu_skipped_flush_flags) |=
> +                (iommu_flags ? IOMMU_FLUSHF_added : 0) |
> +                (vtd_pte_present ? IOMMU_FLUSHF_modified : 0);
>           else if ( need_iommu_pt_sync(d) )
>               rc = iommu_flags ?
>                   iommu_legacy_map(d, _dfn(gfn), mfn, 1ul << order, 
> iommu_flags) :
We now have

(iommu_flags ? IOMMU_FLUSHF_added : 0) |
(vtd_pte_present ? IOMMU_FLUSHF_modified : 0)

duplicated in 2 different (adjacent) places, I think we should collapse 
it in a variable. i.e something like
unsigned long flush_flags =
    (iommu_flags ? IOMMU_FLUSHF_added : 0) |
    (vtd_pte_present ? IOMMU_FLUSHF_modified : 0);

But more generally, given that iommu_dont_flush_iotlb is only 
set/cleared in paths that check it (directly or indirectly), what about 
moving this logic into iommu_iotlb_flush() to be suppressed by 
iommu_dont_flush_iotlb (and if suppressed, |= the 
iommu_skipped_flush_flags instead) ?
That would get rid of a lot of duplicated logic.

> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 4b22ecfe65..70ee45afb9 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -64,6 +64,11 @@ bool __read_mostly iommu_hap_pt_share = true;
>   bool __read_mostly iommu_debug;
>   
>   DEFINE_PER_CPU(bool, iommu_dont_flush_iotlb);
> +/*
> + * A byte holds every IOMMU_FLUSHF_*, and fits in the alignment padding the
> + * bool above leaves, instead of shifting the per-CPU data that follows.
> + */
> +DEFINE_PER_CPU(uint8_t, iommu_skipped_flush_flags);
>   
flush_flags in iotlb_flush is unsigned int in iommu_iotlb_flush(), so I 
would use it here as well instead of uint8_t.
>   static int __init cf_check parse_iommu_param(const char *s)
>   {
> @@ -458,7 +463,12 @@ int iommu_legacy_unmap(struct domain *d, dfn_t dfn, 
> unsigned long page_count)
>       unsigned int flush_flags = 0;
>       int rc = iommu_unmap(d, dfn, page_count, 0, &flush_flags);
>   
> -    if ( !this_cpu(iommu_dont_flush_iotlb) && !rc )
> +    if ( this_cpu(iommu_dont_flush_iotlb) )
> +    {
> +        BUILD_BUG_ON(IOMMU_FLUSHF_all > UINT8_MAX);
> +        this_cpu(iommu_skipped_flush_flags) |= flush_flags;
> +    }
> +    else if ( !rc )
>           rc = iommu_iotlb_flush(d, dfn, page_count, flush_flags);
>   
>       return rc;
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 37c4a1dc82..028f41bc15 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -501,6 +501,7 @@ void iommu_dev_iotlb_flush_timeout(struct domain *d, struct 
> pci_dev *pdev);
>    * the caller.
>    */
>   DECLARE_PER_CPU(bool, iommu_dont_flush_iotlb);
> +DECLARE_PER_CPU(uint8_t, iommu_skipped_flush_flags);
>   
>   bool arch_iommu_use_permitted(const struct domain *d);
>

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature