Re: [PATCH v2 3/5] iommu/amd: Introduce helper functions to access and update 256-bit DTE

[Jason Gunthorpe]

On Fri, Sep 06, 2024 at 09:08:06PM +0700, Suthikulpanit, Suravee wrote:
> On 9/6/2024 1:21 AM, Jason Gunthorpe wrote:
> > > > I don't think you should restore, this should reflect a locking error
> > > > but we still need to move forward and put some kind of correct
> > > > data.. The code can't go backwards so it should try to move forwards..
> > > In case of error, what if we pr_warn and put the device in blocking mode
> > > since we need to prevent malicious DMAs.
> > IMHO a WARN_ON is fine, and alerts to the possible machine corruption
> > No need to do blocking, you should have a perfectly valid target DTE
> > that represents the state the HW is expected to be in. Resolve the
> > race by making it bin that state and move forwards.
> 
> What do you mean by "making it bin that state".

Sorry, "be in that state"

> > The guidelines in "2.2.2.2 Making Device Table Entry Changes" make
> > this clear. The indivudal CPU writes smaller than 256 bits have to be
> > sequenced right.
> 
> For the interrupt remapping part, no special step is needed if we can write
> do 64-bit write. 

Yes

> Similary, for the address translation part, no special step is
> needed if we can do 128-bit write.

Except for GuestPagingMode, as below.

> > This section looks like it was written before translation bits were
> > placed in the other 128 bit word - it assumes a single 128 bit write
> > is always sufficient which isn't true anymore.
> > 
> > So you still have the issue of having to decide if you write 128 bit
> > [0] or [1] first.
> 
> The GuestPagingMode bit is in effect when GV=1. So, the higher 128-bit
> (which contains GuestPagingMode bit) should be written first, and follow by
> lower 128-bit (which contans GV bit).

Yes, exactly. That is what I mean by ordering.

When clearing GV=0 you have to do the reverse ordering, write the low
128 then the high.

Jason