Re: [Xen-devel] [PATCH v6 8/8] AMD/IOMMU: pre-fill all DTEs right after table allocation

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/09/2019 14:25, Jan Beulich wrote:
> Make sure we don't leave any DTEs unexpected requests through which
> would be passed through untranslated. Set V and IV right away (with
> all other fields left as zero), relying on the V and/or IV bits
> getting cleared only by amd_iommu_set_root_page_table() and
> amd_iommu_set_intremap_table() under special pass-through circumstances.
> Switch back to initial settings in amd_iommu_disable_domain_device().
>
> Take the liberty and also make the latter function static, constifying
> its first parameter at the same time, at this occasion.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v6: New.
>
> ---
>  xen/drivers/passthrough/amd/iommu_init.c    |   22 +++++++++++++++++++---
>  xen/drivers/passthrough/amd/pci_amd_iommu.c |   20 ++++++++++++++++----
>  2 files changed, 35 insertions(+), 7 deletions(-)
>
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1255,12 +1255,28 @@ static int __init amd_iommu_setup_device
>  
>      if ( !dt )
>      {
> +        unsigned int size = dt_alloc_size();
> +
>          /* allocate 'device table' on a 4K boundary */
>          dt = IVRS_MAPPINGS_DEVTAB(ivrs_mappings) =
> -            allocate_buffer(dt_alloc_size(), "Device Table");
> +            allocate_buffer(size, "Device Table");
> +        if ( !dt )
> +            return -ENOMEM;
> +
> +        /*
> +         * Prefill every DTE such that all kinds of requests will get 
> aborted.
> +         * Besides the two bits set to true below this builds upon
> +         * IOMMU_DEV_TABLE_SYS_MGT_DMA_ABORTED,
> +         * IOMMU_DEV_TABLE_IO_CONTROL_ABORTED, as well as
> +         * IOMMU_DEV_TABLE_INT_CONTROL_ABORTED all being zero, and us also
> +         * wanting at least TV, GV, I, and EX set to false.
> +         */
> +        for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf )
> +        {
> +            dt[bdf].v = true;
> +            dt[bdf].iv = true;
> +        }

The DT-BAR is generally 2MB in size.  It is inconvenient that we first
zero it, then walk it a second time setting two bits.

I'm not sure that allocate_buffer() needs to zero memory for any of its
callers.  The command ring writes a full entry at once, and the IOMMU
writes full event/page logs at once.

Dropping the memset() and changing this to be a loop of structure
assignments would be rather more efficient.

~Andrew

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