...
> > The attach path reads @revoked without holding memory_lock. Annotate it
> > with READ_ONCE() to document this intentional lockless access: the read
> > is a benign early-out, and a racing revocation is re-checked under
> > dmabuf->resv in vfio_pci_dma_buf_map() before any mapping is handed out.
>
> I believe this modification (@revoked) might be related to resetting the
> bit field segment and subsequently identifying a concurrency issue. It is
> recommended to submit this as an independent commit.
>
Thanks, I think the @revoked part is still closely related here. The lockless
attach-path read existed before this patch; this only annotates it
with READ_ONCE().
The bitfield-to-bool change is needed because READ_ONCE() cannot be used on a
bitfield. The real check remains the dmabuf->resv recheck in
vfio_pci_dma_buf_map(),
so I'd prefer to keep it in this patch.
...
> > +
> > + /*
> > + * Updates protected by dmabuf->resv, @revoked additionally
> > + * protected by memory_lock.
> > + */
> > + u16 tph_st_ext;
> > + u8 tph_st;
>
> how about: u16 tph_xst;
> u16 tph_st;
>
I'd prefer to keep tph_st/tph_st_ext if that works for you. They map directly to
ST and Extended ST, and non-Extended ST is 8-bit, so u8 tph_st reflects the
actual width.
> > + bool revoked;
>
> why not move revoked before or after tph fields if it don't take one bit
> field?
>
I kept @revoked there because the comment documents the locking for these
dma-buf state fields: TPH under dmabuf->resv, and @revoked under dmabuf->resv
plus memory_lock.
> > + u8 tph_st_valid:1;
> > + u8 tph_st_ext_valid:1;
>
> how about: u8 tph_xst_valid
>
I'd prefer tph_st_ext_valid since it matches VFIO_DMA_BUF_TPH_ST_EXT and the
"Extended ST" wording used in the documentation.
> > + u8 tph_ph:2;
> > };
>
> ...
>
> > +#define VFIO_DEVICE_FEATURE_DMA_BUF_TPH 13
> > +
> > +#define VFIO_DMA_BUF_TPH_ST (1 << 0)
> > +#define VFIO_DMA_BUF_TPH_ST_EXT (1 << 1)
> > +
> > +struct vfio_device_feature_dma_buf_tph {
> > + __s32 dmabuf_fd;
> > + __u32 flags;
> > + __u16 steering_tag_ext;
> > + __u8 steering_tag;
>
> how about:
> __u16 xst;
> __u8 st;
> and it corresponding to VFIO_DMA_BUF_TPH_ST/ST_EXT
>
> Thanks
>
If that's OK, I'd prefer steering_tag/steering_tag_ext in the uAPI. They
match the PCIe "Steering Tag" term and seem clearer for userspace than
st/xst.
Thanks a lot!
Zhiping
