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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.09.2019 16:59, Andrew Cooper wrote:
> 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.

But in the latter case we need the zeroing to work around errata 732
and 733; see parse_{event,ppr}_log_entry().

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

I'll add a boolean parameter to allocate_buffer().

Jan

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