Re: [PATCH v5 10/14] iommu/amd: Introduce logic to enable/disable IOPF

[Vasant Hegde <vasant.hegde@amd.com>]

Jason,


On 2/6/2024 10:06 PM, Jason Gunthorpe wrote:
> On Tue, Feb 06, 2024 at 09:49:36PM +0530, Vasant Hegde wrote:
> 
>>> This dte change is in the wrong place. When the domain is first
>>> attached we know if it requires PRI or not. At that moment the DTE
>>> should be set properly, it should not be set wrong and then changed
>>> later.
>>
>> We want to enable PPR support only after setting up the handler.
> 
> That's backwards. It means error handling can't really be sane..

Why do you think enabling feature only when we are really going to use it backwards?



> 
>>> (and again the whole dte setting flow needs cleaning, I think you
>>> should do that before trying to build more complex stuff on top)
>>
>> Yeah. I want to fix few things in that path. But that's outside this series.
> 
> I was looking at it and there are many security bugs in here now that
> iommufd can change the DTE at any time. The current design assumes DMA
> will be stopped and ignores the spec guidance on how to do a safe DTE
> update :(

What security issues are you referring? Can you elaborate?

> 
> I'm really not comfortable with adding more stuff here until the
> security issue is solved. Especially if the more stuff is drifting
> further from being correct. If you can keep the updates in set_dte
> then maybe with some reluctance. But not like this with random touches
> to the DTE all over the place.

Currently all DTE update is happening inside set_dte only (dirty bit enable is
an exception that may need to moved inside set_dte). This patch just invokes
that set_dte and invalidates cache.

-Vasant

> 
> I was looking at what it would take and got part way through. It is a
> very similar problem to ARM and I drafted a similar solution.
> 
> Jason