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

["Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>]

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

>>> On ordering, I don't know, is this OK?
>>>
>>> If you are leaving/entering nesting mode I think you have to write the
>>> [2] value in the right sequence, you don't want to have the viommu
>>> enabled unless the host page table is setup properly. So [2] is
>>> written last when enabling, and first when disabling. Flushes required
>>> after each write to ensure the HW doesn't see a cross-128 word bit
>>> tear.
>>>> GuestPagingMode also has to be sequenced correctly, the GCR3 table
>>> pointer should be invalid when it is changed, meaning you have to
>>> write it and flush before storing the GCR3 table, and the reverse to
>>> undo it.
>>>
>>> The ordering, including when DTE flushes are needed, is pretty
>>> hard. This is much simpler than, say, ARM, so I think you could open
>>> code it, but it should be a pretty sizable bit of logic to figure out
>>> what to do.
>> IOMMU hardware do not do partial interpret of the DTE and SW ensure DTE
>> flush after updating the DTE. Therefore, ordering should not be of a concern
>> here as long as the driver correctly program the entry.
> Even if the IOMMU HW does a perfect 256 bit atomic read you still have
> to order the CPU writes correctly. It just means you don't need to
> flush.
> 
> 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. Similary, for the address translation part, no 
special step is needed if we can do 128-bit write.

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

Thanks,
Suravee