Re: [PATCH 1/2] iommu/vt-d: Ensure memory ordering in context entry updates

[Dmytro Maluka <dmaluka@chromium.org>]

On Sun, Dec 21, 2025 at 05:04:27PM +0800, Baolu Lu wrote:
> On 12/21/25 09:43, Dmytro Maluka wrote:
> > +static inline void context_set_bits(u64 *ptr, u64 mask, u64 bits)
> > +{
> > +	u64 old;
> > +
> > +	old = READ_ONCE(*ptr);
> > +	WRITE_ONCE(*ptr, (old & ~mask) | bits);
> > +}
> 
> Add a line to ensures that the input "bits" cannot overflow the assigned
> "mask".
> 
> static inline void context_set_bits(u64 *ptr, u64 mask, u64 bits)
> {
> 	u64 val;
> 
> 	val = READ_ONCE(*ptr);
> 	val &= ~mask;
> 	val |= (bits & mask);
> 	WRITE_ONCE(*ptr, val);
> }

Makes sense. And then worth doing the same in pasid_set_bits() as well?

Actually we can use the same helper for both context and pasid entries
(rename it e.g. to entry_set_bits()).

> >   static inline void context_set_present(struct context_entry *context)
> >   {
> > -	context->lo |= 1;
> > +	context_set_bits(&context->lo, 1 << 0, 1);
> >   }
> 
> How about adding a smp_wmb() before setting the present bit? Maybe it's
> unnecessary for x86 architecture, but at least it's harmless and more
> readable. Or not?
> 
> static inline void context_set_present(struct context_entry *context)
> {
> 	smp_wmb();
> 	context_set_bits(&context->lo, 1ULL << 0, 1ULL);
> }

Maybe... And if so, then in pasid_set_present() as well?

Actually this would make a slight behavioral difference even on x86:
right now this patch only provides ordering of context entry updates
between each other, while smp_wmb() would add a full compiler barrier
here.

So this barrier may be redundant as long as we always use these
context_*() helpers (and thus always use WRITE_ONCE) for any updates of
context entries. On the other hand, it might make it more robust if we
still occasionally do that without WRITE_ONCE, for example in
context_entry_set_pasid_table() which I also changed to use WRITE_ONCE
in this patch.