RE: [PATCH v4 16/27] vfio/cxl: Shadow the CXL DVSEC body at open

[Manish Honap]

> -----Original Message-----
> From: Alex Williamson <alex@xxxxxxxxxxx>
> Sent: Friday, August 28, 2026 8:40 PM
> To: Manish Honap <mhonap@xxxxxxxxxx>
> Cc: jgg@xxxxxxxx; Ankit Agrawal <ankita@xxxxxxxxxx>; jic23@xxxxxxxxxx;
> dave.jiang@xxxxxxxxx; alejandro.lucero-palau@xxxxxxx; Srirangan Madhavan
> <smadhavan@xxxxxxxxxx>; corbet@xxxxxxx; skhan@xxxxxxxxxxxxxxxxxxx;
> dave@xxxxxxxxxxxx; alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx;
> iweiny@xxxxxxxxxx; ming.li@xxxxxxxxxxxx; Yishai Hadas
> <yishaih@xxxxxxxxxx>; Shameer Kolothum Thodi
> <skolothumtho@xxxxxxxxxx>; kevin.tian@xxxxxxxxx; bhelgaas@xxxxxxxxxx;
> dmatlack@xxxxxxxxxx; kees@xxxxxxxxxx; gustavoars@xxxxxxxxxx; Neo Jia
> <cjia@xxxxxxxxxx>; Krishnakant Jaju <kjaju@xxxxxxxxxx>; Vikram Sethi
> <vsethi@xxxxxxxxxx>; Zhi Wang <zhiw@xxxxxxxxxx>; linux-
> doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx;
> linux-cxl@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> kselftest@xxxxxxxxxxxxxxx; linux-hardening@xxxxxxxxxxxxxxx; alex@xxxxxxxxxxx
> Subject: Re: [PATCH v4 16/27] vfio/cxl: Shadow the CXL DVSEC body at open
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 13 Aug 2026 15:06:20 +0530
> <mhonap@xxxxxxxxxx> wrote:
> 
> > From: Manish Honap <mhonap@xxxxxxxxxx>
> >
> > Sample the CXL DVSEC body into a per-open shadow when the guest opens
> > the device, and free it at close. Reading it here rather than at bind
> > picks up any change from a low-power transition, and gives the DVSEC
> > access handler added next a per-tenant copy to serve from.
> >
> > Annotate the shadow with __counted_by_ptr(dvsec_dwords) so its
> > accesses are bounds-checked against the recorded dword count.
> 
> It would be useful to describe why we want to shadow the DVSEC capability
> here.  Also, it's the whole DVSEC capability, not just the body.  We're again
> mentioning that low power transition that the earlier path prevented (but
> shouldn't have).
> 
> >
> > Signed-off-by: Manish Honap <mhonap@xxxxxxxxxx>
> > ---
> >  drivers/vfio/pci/cxl/vfio_cxl_core.c | 39
> > ++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >
> > diff --git a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > index d19fd638f538..2e516a0929c6 100644
> > --- a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > +++ b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> >  #include <linux/range.h>
> > +#include <linux/slab.h>
> >  #include <linux/vfio_pci_core.h>
> >  #include <cxl/cxl.h>
> >  #include <cxl/pci.h>
> > @@ -17,11 +18,19 @@
> >   * @cxlds: CXL device state; kept first for devm_cxl_dev_state_create()
> >   * @cxlmd: memory device joined to the CXL topology at bind
> >   * @hpa_range: host physical range of the HDM region
> > + * @dvsec: CXL device DVSEC config-space offset
> > + * @dvsec_len: length of the DVSEC body
> 
> Nit, not just the DVSEC body.
> 
> > + * @dvsec_dwords: dword count of @dvsec_shadow
> > + * @dvsec_shadow: guest view of the CXL DVSEC body, sampled at open
> >   */
> >  struct vfio_cxl_state {
> >       struct cxl_dev_state cxlds;
> >       struct cxl_memdev *cxlmd;
> >       struct range hpa_range;
> > +     u16 dvsec;
> > +     u32 dvsec_len;
> > +     u32 dvsec_dwords;
> > +     u32 *dvsec_shadow __counted_by_ptr(dvsec_dwords);
> 
> dvsec_len is bound by PCI_DVSEC_HEADER1_LEN, which is 12-bits, so it fits
> comfortably in a u16.  dvsec_dwords is therefore bound at 10-bits.
> Both of these fit comfortably in u16.  I'd argue that length is easily derived from
> dwords, but we end up with a hole in the data structure regardless, so it's
> arguably useful to keep both.  We can drop 4-bytes from the structure though,
> which actually turns into an 8-byte savings with the two holes above (2 + 4) vs
> one hole below (2):
> 
>         u16 dvsec;
>         u16 dvsec_len;
>         u16 dvsec_dwords;
>         u32 *dvsec_shadow __counted_by_ptr(dvsec_dwords);
> 
> >  };
> >
> >  static int vfio_cxl_init_device(struct vfio_pci_core_device *vdev) @@
> > -71,6 +80,8 @@ static int vfio_cxl_init_device(struct vfio_pci_core_device
> *vdev)
> >       if (!cxl)
> >               return -ENOMEM;
> >
> > +     cxl->dvsec = dvsec;
> > +
> >       /*
> >        * vfio-pci requests the whole component BAR when the guest opens the
> >        * device. Declare the BAR owned so the CXL core maps the
> > HDM/RAS @@ -102,11 +113,39 @@ static void
> > vfio_cxl_release_device(struct vfio_pci_core_device *vdev)
> >
> >  static int vfio_cxl_open_device(struct vfio_pci_core_device *vdev)  {
> > +     struct vfio_cxl_state *cxl = vdev->cxl;
> > +     struct pci_dev *pdev = vdev->pdev;
> > +     u32 hdr, *shadow;
> > +     int i, dwords;
> > +
> > +     /*
> > +      * Sample the DVSEC body now rather than at bind: a low-power
> > +      * transition could have changed it since the device was bound.
> > +      */
> > +     pci_read_config_dword(pdev, cxl->dvsec + PCI_DVSEC_HEADER1, &hdr);
> > +     cxl->dvsec_len = PCI_DVSEC_HEADER1_LEN(hdr);
> > +     dwords = cxl->dvsec_len / sizeof(u32);
> > +
> > +     shadow = kcalloc(dwords, sizeof(u32), GFP_KERNEL);
> > +     if (!shadow)
> > +             return -ENOMEM;
> 
> dvsec_len becomes inconsistent with the other fields if we take this return.
> 
> > +
> > +     for (i = 0; i < dwords; i++)
> > +             pci_read_config_dword(pdev, cxl->dvsec + i * sizeof(u32),
> > +                                   &shadow[i]);
> > +
> > +     cxl->dvsec_dwords = dwords;
> > +     cxl->dvsec_shadow = shadow;
> > +
> >       return 0;
> >  }
> >
> >  static void vfio_cxl_close_device(struct vfio_pci_core_device *vdev)
> > {
> > +     struct vfio_cxl_state *cxl = vdev->cxl;
> > +
> > +     kfree(cxl->dvsec_shadow);
> > +     cxl->dvsec_shadow = NULL;
> 
> This makes the counted-by field inconsistent.  All fields should be cleared.
> Thanks,
> 
> Alex

With the DVSEC handled in vfio_pci_config.c on top of vconfig (patch 17), there is no
separate shadow to set up. I will assess whether I can drop this patch entirely.

Manish
> 
> >  }
> >
> >  static const struct vfio_cxl_ops vfio_cxl_ops = {