Re: [PATCH v2 0/5] iommu/vt-d: Ensure memory ordering in context & root entry updates

[Dmytro Maluka <dmaluka@chromium.org>]

On Mon, Jan 05, 2026 at 03:14:10PM -0400, Jason Gunthorpe wrote:
> On Mon, Jan 05, 2026 at 07:54:53PM +0100, Dmytro Maluka wrote:
> > > Like AMD and ARM build the new PASID entry on the stack and then it
> > > should be copied to the DMA'able memory in a way that is consistent
> > > with the HW's atomicity granual, paying attention not to 'tear' it.
> > 
> > As I understand, the "consistent with the HW's atomicity granual, paying
> > attention not to 'tear' it" part is already fulfilled for PASID entries
> > (and with this series, for context entries as well):
> > 
> > static inline void pasid_set_bits(u64 *ptr, u64 mask, u64 bits)
> > {
> > 	u64 old;
> > 
> > 	old = READ_ONCE(*ptr);
> > 	WRITE_ONCE(*ptr, (old & ~mask) | bits);
> > }
> > 
> > I've been assuming it's ok to manipulate other bits in place as long as
> > we take care to only do that while the present bit it cleared (i.e.
> > while the entry is ignored by hardware)?
> 
> If these are only done while non-present then the only issue is
> missing a barrier before setting present, that should be a one line
> patch, no?

Yes, a barrier, or alternatively WRITE_ONCE for any updates of entries.
The latter is how it was already done for PASID entries, before this
series (so IIUC for PASID entries there was already no issue), and my
main goal was to fix the issue for context entries as well, so I just
did it similarly.

> > So IIUC the only problem with this approach is the redundancy: we do
> > this READ_ONCE+WRITE_ONCE for each invididual field in a PASID entry.
> 
> You don't need READ_ONCE if there isn't another thread concurrently
> writing,

Good point. But it was there before this series.

> and WRITE_ONCE is pointless if the HW is promising not to
> read it due to non-present.

As long as we use a barrier. And IIUC vice versa, if we use WRITE_ONCE
for any updates, a barrier is not necessary (on x86). And WRITE_ONCE for
any updates (for PASID entries) is what was already done before this
series.

Although, perhaps even with a barrier, WRITE_ONCE is still desirable to
prevent the compiler from doing strange but legal things that involve
transiently setting and then clearing the present bit behind our back?
(Not that I'm aware of any compilers doing that, but I'm no compiler
expert.)

> > So while I agree it would be more more natural to build whole entries,
> > and the existing way looks strange and not the most efficient, I'm
> > wondering if it is causing any actual correctness issues (apart from
> > those addressed by this series).
> 
> It prevents doing the replace operation, which is a correctness issue
> for VMs.
> 
> Jason