Re: [Xen-devel] [PATCH v5 06/10] AMD/IOMMU: don't blindly allocate interrupt remapping tables

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.09.2019 13:36, Andrew Cooper wrote:
> On 18/09/2019 09:55, Jan Beulich wrote:
>> On 17.09.2019 15:10, Andrew Cooper wrote:
>>> On 06/08/2019 14:09, Jan Beulich wrote:
>>>> TBD: This retains prior (but suspicious) behavior of not calling
>>>>      amd_iommu_set_intremap_table() for "invalid" IVRS mapping entries.
>>>>      Since DTE.IV=0 means un-remapped interrupts, I wonder if this needs
>>>>      changing.
>>> How would an invalid entry get DTE.V set in the first place?
>> DTE.V gets set by amd_iommu_set_root_page_table(), which in turn gets
>> called from amd_iommu_setup_domain_device() alone. It's only the
>> latter function's callers which obtain (and possibly check) the
>> corresponding IVRS mappings entry. Hence to me there's a sufficient
>> disconnect between setting of DTE.V and DTE.IV.
>>
>> Plus, looking at e.g. amd_iommu_add_device(), there's ample room for
>> not even making it to amd_iommu_set_intremap_table(), due to earlier
>> errors.
>>
>>> Whatever the old behaviour may have been, we shouldn't have code which
>>> comes with a chance of having non-remapped interrupts by accident.
>> We can't make amd_iommu_set_root_page_table() set dte->iv to 1, as
>> it gets called only after amd_iommu_set_intremap_table() in
>> amd_iommu_add_device(). But we could of course make it do so when
>> it finds dte->it_root be zero. Yet I wonder if it wasn't more safe
>> to have DTEs start out with the field set this way.
>>
>> Along these lines I'm also not convinced it is a good idea for
>> amd_iommu_set_intremap_table() to have a "valid" parameter in the
>> first place: It's okay as long as all callers pass iommu_intremap,
>> but it would seem to me that - as already said - we'd want DTEs be
>> set this way right when a DT gets allocated. If you agree, I'll
>> happily add a patch doing so to the end of this series (there's
>> meanwhile a patch re-arranging DT allocation anyway).
> 
> I've been looking through the spec, and this is rather complicated.  We
> need to set V and TV to inhibit DMA, and IV and IntCtl to inhibit
> interrupts.

By "set V and TV", you don't mean setting both to 1, do you? My
reading of the respective tables is that we want V=1, TV=0, IV=1,
and IntCtl=0. The problem with setting V early is that some
other fields than also are assumed to be valid. I.e. along with
the above we'd also need SysMgt=0, EX=0, SD=?, Cache=?, IoCtl=0,
SA=?, SE=?, and I=0; question marks indicate either setting would
appear to be fine. In the end all zeros except V and IV would
look to be what we want, albeit setting on of SE or SA may be
worth considering.

> Why not initialise every entry in the device table when we create it to
> a safe, blocked state.  That way, the only way a device starts
> functioning appropriately is via a successful call to 
> amd_iommu_set_root_page_table() and amd_iommu_set_intremap_table(),
> which seems to be far safer behaviour overall.

I intend to add a respective patch to the series, if we manage to
agree (see above) what the initial settings should be.

Jan

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