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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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:
>>> ACPI tables are free to list far more device coordinates than there are
>>> actual devices. By delaying the table allocations for most cases, and
>>> doing them only when an actual device is known to be present at a given
>>> position, overall memory used for the tables goes down from over 500k
>>> pages to just over 1k ones.
>> This is slightly awkward grammar.  While I don't think it is strictly
>> speaking incorrect, it would be more normal to phrase as "just over 1k
>> pages".
> Changed, albeit to me the double "pages" sounds odd as well. Would
> "of them" be any better than "ones"?

You are drawing a comparison between 500k pages, and 1k pages.

Changing the 'pages' qualifier introduced ambiguity - consider the case
where "ones" is substituted for "bytes", which would be a legitimate way
to describe the memory reduction, but is misleading due to the
difference in units.

Alternatively, the trailing qualifier can be dropped, and the sentence
end at "1k.", as the "pages" is implied.

>
>>> ---
>>> 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.

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.

~Andrew

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