RE: [PATCH v5 25/27] vfio/cxl: Run the CXL reset at the vfio reset points

[Manish Honap]

> -----Original Message-----
> From: Alex Williamson <alex@xxxxxxxxxxx>
> Sent: Tuesday, September 22, 2026 7:43 AM
> 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 v5 25/27] vfio/cxl: Run the CXL reset at the vfio reset
> points
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 17 Sep 2026 00:05:38 +0530
> <mhonap@xxxxxxxxxx> wrote:
> 
> > From: Manish Honap <mhonap@xxxxxxxxxx>
> >
> > A CXL Type-2 function must not take an FLR: it resets the coherent
> > CXL.mem state and corrupts the HDM decoder. The PCI core already
> > reflects this, ordering cxl_reset ahead of flr in
> > pci_reset_fn_methods[], so a function reset of a CXL device runs the
> > DVSEC reset sequence rather than FLR.
> >
> > Route the vfio function-reset points (VFIO_DEVICE_RESET and the
> > virtualized PCIe/AF FLR writes) through a CXL reset op that runs
> > cxl_reset_dvsec_sequence(). The sequence resets the function, always
> > clearing device memory, and restores the HDM decoder and PCI config
> > state, so it is a complete replacement for pci_try_reset_function() on
> > a CXL device. The op runs under memory_lock and not the PCI device
> > lock, so
> > cxl_reset_dvsec_sequence() can take the device lock itself.
> >
> > Clear hdm_valid for the duration of the reset so a fault cannot insert
> > a PFN into a decoder that is being torn down, and restore it once the
> > sequence has put the decoder back.
> >
> > Assisted-by: LLM
> > Signed-off-by: Manish Honap <mhonap@xxxxxxxxxx>
> > ---
> >  drivers/vfio/pci/cxl/vfio_cxl_core.c | 41 +++++++++++++++
> >  drivers/vfio/pci/vfio_pci_config.c   | 49 +++++++++++++++---
> >  drivers/vfio/pci/vfio_pci_core.c     | 77 +++++++++++++++++++++++-----
> >  drivers/vfio/pci/vfio_pci_priv.h     |  1 +
> >  include/linux/vfio_pci_core.h        |  4 ++
> >  5 files changed, 152 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > index 55fa1f86850d..795362aea344 100644
> > --- a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > +++ b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > @@ -632,6 +632,45 @@ static void vfio_cxl_reset_done(struct
> vfio_pci_core_device *vdev)
> >               cxl->hdm_valid = false;
> >  }
> >
> > +/*
> > + * Run the CXL DVSEC reset sequence in place of a PCI function reset.
> > +A CXL
> > + * Type-2 function must not take an FLR (it would corrupt CXL.mem),
> > +so the vfio
> > + * reset points route here. The sequence resets the function, always
> > +clearing
> > + * device memory, and restores the HDM decoder. The caller holds
> > +memory_lock,
> > + * and this path does not hold the PCI device lock, so
> > +cxl_reset_dvsec_sequence()
> > + * can take it.
> > + */
> > +static int vfio_cxl_reset(struct vfio_pci_core_device *vdev) {
> > +     struct vfio_cxl_state *cxl = vdev->cxl;
> > +     int ret;
> > +
> > +     lockdep_assert_held_write(&vdev->memory_lock);
> > +
> > +     /* Host CPU access to the HDM range is unsafe until the decoder is back.
> */
> > +     cxl->hdm_valid = false;
> > +
> > +     ret = cxl_reset_dvsec_sequence(vdev->pdev);
> > +     if (!ret)
> > +             cxl->hdm_valid = true;
> > +
> > +     return ret;
> 
> Success oriented flow:
> 
>         if (ret)
>                 return ret;
> 
>         cxl->hdm_valid = true;
> 
>         return 0;
> 
> But I don't see how the case where hdm_valid remains false is really a valid
> place for the user to land.  We don't really have a "your device is now borked,
> give up" signal to the user.

Okay, I will use the success-oriented flow.

hdm_valid stays false only on a hardware failure: the device reports
Reset Error in Status2 (-EIO), does not finish within the timeout its
DVSEC advertises (-ETIMEDOUT), or the decoder restore fails.
The reset ioctl will return that error.

For the cxl_reset code, it leaves memory decode and
bus mastering disabled when the restore fails, as the host reset path
does. Due to this the HDM window stays closed, with faults returning
SIGBUS and its dma-buf revoked. I will not add a new signal for this case
of failure.

> 
> > +}
> > +
> > +/*
> > + * The HDM dma-buf may be armed only while the decoder is valid.
> > +After a failed
> > + * reset hdm_valid is clear, so the generic memory-enable re-arm must
> > +skip the
> > + * dma-buf rather than map DMA onto an unrestored decoder.
> > + */
> > +static bool vfio_cxl_hdm_active(struct vfio_pci_core_device *vdev) {
> > +     struct vfio_cxl_state *cxl = vdev->cxl;
> > +
> > +     lockdep_assert_held_write(&vdev->memory_lock);
> > +
> > +     return cxl->hdm_valid;
> > +}
> > +
> >  static const struct vfio_cxl_ops vfio_cxl_ops = {
> >       .init           = vfio_cxl_init_device,
> >       .release        = vfio_cxl_release_device,
> > @@ -639,6 +678,8 @@ static const struct vfio_cxl_ops vfio_cxl_ops = {
> >       .close_device   = vfio_cxl_close_device,
> >       .reset_prepare  = vfio_cxl_reset_prepare,
> >       .reset_done     = vfio_cxl_reset_done,
> > +     .reset          = vfio_cxl_reset,
> > +     .hdm_active     = vfio_cxl_hdm_active,
> >       .owner          = THIS_MODULE,
> >  };
> 
> It's vfio_cxl_ops, so it's unique to CXL, but still leaking HDM to vfio-pci-core as
> something it needs to care about is pretty ugly.
> It should be something generic, but also this should be wrapped in some
> abstraction in vfio-pci-core.
> 

Agreed. I will drop this in v6.

> >
> > diff --git a/drivers/vfio/pci/vfio_pci_config.c
> > b/drivers/vfio/pci/vfio_pci_config.c
> > index 9a020a768055..8a5a737efa31 100644
> > --- a/drivers/vfio/pci/vfio_pci_config.c
> > +++ b/drivers/vfio/pci/vfio_pci_config.c
> > @@ -630,7 +630,14 @@ static int vfio_basic_config_write(struct
> vfio_pci_core_device *vdev, int pos,
> >               *virt_cmd &= cpu_to_le16(~mask);
> >               *virt_cmd |= cpu_to_le16(new_cmd & mask);
> >
> > -             if (__vfio_pci_memory_enabled(vdev))
> > +             /*
> > +              * Re-arm the dma-bufs on memory-enable, but keep a CXL device's
> > +              * HDM dma-buf revoked while the decoder is unrestored (a failed
> > +              * reset leaves hdm_valid clear); re-arming would map DMA onto a
> > +              * decoder the fault path still gates. Plain vfio-pci is unchanged.
> > +              */
> > +             if (__vfio_pci_memory_enabled(vdev) &&
> > +                 (!vdev->cxl_ops || vdev->cxl_ops->hdm_active(vdev)))
> 
> HDM space and BAR MMIO space are governed by different things, how can
> we combine them here to say that dmabufs are invalid until both are active?
> That's not how the hardware works.  Does the HDM need a separate address
> space of dmabufs to toggle independently?

Okay. In v6 each vfio dma-buf records the region it was exported from.
vfio_pci_dma_buf_move() keeps moving the BAR dma-bufs on memory enable
changes, as upstream, and a new export revokes or re-arms the dma-bufs
of one device-specific region. vfio-cxl calls it around reset, bus
reset and low power.

I will revert the memory enable paths in vfio_pci_config.c back
to their upstream forms.


> 
> >                       vfio_pci_dma_buf_move(vdev, false);
> >               up_write(&vdev->memory_lock);
> >       }
> > @@ -720,7 +727,8 @@ static void vfio_lock_and_set_power_state(struct
> vfio_pci_core_device *vdev,
> >       }
> >
> >       vfio_pci_set_power_state(vdev, state);
> > -     if (__vfio_pci_memory_enabled(vdev))
> > +     if (__vfio_pci_memory_enabled(vdev) &&
> > +         (!vdev->cxl_ops || vdev->cxl_ops->hdm_active(vdev)))
> >               vfio_pci_dma_buf_move(vdev, false);
> >       up_write(&vdev->memory_lock);
> >  }
> > @@ -910,8 +918,14 @@ static int vfio_exp_config_write(struct
> vfio_pci_core_device *vdev, int pos,
> >               if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) {
> >                       vfio_pci_zap_and_down_write_memory_lock(vdev);
> >                       vfio_pci_dma_buf_move(vdev, true);
> > -                     pci_try_reset_function(vdev->pdev);
> > -                     if (__vfio_pci_memory_enabled(vdev))
> > +                     ret = vfio_pci_reset_function(vdev);
> > +                     /*
> > +                      * Keep the HDM dma-buf revoked if a CXL reset
> > +                      * failed; re-arming would map DMA onto an
> > +                      * unrestored decoder. Mirrors the reset ioctl.
> > +                      */
> 
> We don't have granularity of "the HDM dma-buf".

Okay; the per-dma-buf region index above will add this part.

> 
> > +                     if (__vfio_pci_memory_enabled(vdev) &&
> > +                         (!vdev->cxl_ops || !ret))
> >                               vfio_pci_dma_buf_move(vdev, false);
> >                       up_write(&vdev->memory_lock);
> >               }
> > @@ -995,8 +1009,14 @@ static int vfio_af_config_write(struct
> vfio_pci_core_device *vdev, int pos,
> >               if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) {
> >                       vfio_pci_zap_and_down_write_memory_lock(vdev);
> >                       vfio_pci_dma_buf_move(vdev, true);
> > -                     pci_try_reset_function(vdev->pdev);
> > -                     if (__vfio_pci_memory_enabled(vdev))
> > +                     ret = vfio_pci_reset_function(vdev);
> > +                     /*
> > +                      * Keep the HDM dma-buf revoked if a CXL reset
> > +                      * failed; re-arming would map DMA onto an
> > +                      * unrestored decoder. Mirrors the reset ioctl.
> > +                      */
> 
> Same.
> 
> > +                     if (__vfio_pci_memory_enabled(vdev) &&
> > +                         (!vdev->cxl_ops || !ret))
> >                               vfio_pci_dma_buf_move(vdev, false);
> >                       up_write(&vdev->memory_lock);
> >               }
> > @@ -1781,9 +1801,22 @@ static int vfio_cxl_dvsec_write(struct
> vfio_pci_core_device *vdev, int pos,
> >               status2 |= PCI_DVSEC_CXL_CACHE_INV;
> >       }
> >       if (ctrl2 & PCI_DVSEC_CXL_INIT_CXL_RST) {
> > +             int ret = 0;
> > +
> >               ctrl2 &= ~PCI_DVSEC_CXL_INIT_CXL_RST;
> > -             status2 &= ~PCI_DVSEC_CXL_RST_ERR;
> > -             status2 |= PCI_DVSEC_CXL_RST_DONE;
> > +
> > +             if (vdev->cxl_ops && vdev->cxl_ops->reset) {
> > +                     vfio_pci_zap_and_down_write_memory_lock(vdev);
> > +                     vfio_pci_dma_buf_move(vdev, true);
> > +                     ret = vfio_pci_reset_function(vdev);
> > +                     if (__vfio_pci_memory_enabled(vdev) &&
> > +                         (!vdev->cxl_ops || !ret))
> > +                             vfio_pci_dma_buf_move(vdev, false);
> > +                     up_write(&vdev->memory_lock);
> > +             }
> 
> Isn't this branch deterministic?  IIRC, we only install this handler when vdev-
> >cxl_ops and that ops always registers a reset function.

Yes. v6 will make the .reset mandatory: registration refuses ops without it,
and I can remove the checks.

> 
> > +
> > +             status2 &= ~(PCI_DVSEC_CXL_RST_DONE |
> PCI_DVSEC_CXL_RST_ERR);
> > +             status2 |= ret ? PCI_DVSEC_CXL_RST_ERR :
> > + PCI_DVSEC_CXL_RST_DONE;
> >       }
> >
> >       *pctrl2 = cpu_to_le16(ctrl2);
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c
> > b/drivers/vfio/pci/vfio_pci_core.c
> > index f02a5240aa71..8bd4db7afefe 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -643,8 +643,27 @@ int vfio_pci_core_enable(struct
> vfio_pci_core_device *vdev)
> >               goto out_power;
> >
> >       /* If reset fails because of the device lock, fail this path entirely */
> > -     ret = pci_try_reset_function(pdev);
> > -     if (ret == -EAGAIN)
> > +     if (vdev->cxl_ops && vdev->cxl_ops->reset) {
> > +             /*
> > +              * VM power-on resets a CXL Type-2 device through its DVSEC
> > +              * sequence. vconfig is not built yet here, so take memory_lock
> > +              * and call the op directly rather than the wrapper.
> > +              */
> 
> B does not follow from A: what's vconfig got to do with taking memory_lock
> here?

This is a mistake in the comment added here. I will update this in v6.

> 
> > +             down_write(&vdev->memory_lock);
> > +             ret = vdev->cxl_ops->reset(vdev);
> > +             up_write(&vdev->memory_lock);
> > +     } else {
> > +             ret = pci_try_reset_function(pdev);
> > +     }
> > +
> > +     /*
> > +      * -EAGAIN means the reset could not run. For a CXL device any reset
> > +      * error must also fail the open: a failed DVSEC reset can leave the HDM
> > +      * decoder cleared or unrestored, and continuing would expose the HDM
> > +      * region for host access through a decoder in an unknown state.
> > +      */
> > +     if (ret == -EAGAIN ||
> > +         (vdev->cxl_ops && vdev->cxl_ops->reset && ret))
> 
> This suggests to me that our abstraction is lacking.

With the HDM window closed after any failed reset, the enable path
will not need a CXL case. It fails the open only on -EAGAIN, as upstream, and
a device whose reset failed at open keeps its HDM window closed until a
reset succeeds.

> 
> >               goto out_disable_device;
> >
> >       vdev->reset_works = !ret;
> > @@ -845,16 +864,30 @@ void vfio_pci_core_disable(struct
> vfio_pci_core_device *vdev)
> >        * overwrite the previously restored configuration information.
> >        */
> >       if (vdev->reset_works) {
> > -             bridge = pci_upstream_bridge(pdev);
> > -             if (bridge && !pci_dev_trylock(bridge))
> > -                     goto out_restore_state;
> > -             if (pci_dev_trylock(pdev)) {
> > -                     if (!__pci_reset_function_locked(pdev))
> > +             if (vdev->cxl_ops && vdev->cxl_ops->reset) {
> > +                     /*
> > +                      * VM power-off resets a CXL Type-2 device through its
> > +                      * DVSEC sequence. The sequence takes its own device lock,
> > +                      * so run it outside the lock below.
> > +                      * vconfig is already freed here, so call the op directly
> > +                      * under memory_lock rather than the wrapper.
> 
> Again, the comment doesn't actually justify the behavior.  In the previous,
> vconfig is not setup, so take the lock, here vconfig is already freed, so take the
> lock... meaningless.
> 
> Also, are we dropping the try-lock semantics?  How's that justified?

Sorry for the churn; No, I am not dropping the try-lock semantics.
cxl_reset_dvsec_sequence() takes the device lock with pci_dev_trylock(); it returned
-EBUSY whereas pci_try_reset_function() returns -EAGAIN.

In v6, I will return -EAGAIN so callers see one errno for contention.

> 
> > +                      */
> > +                     down_write(&vdev->memory_lock);
> > +                     if (!vdev->cxl_ops->reset(vdev))
> >                               vdev->needs_reset = false;
> > -                     pci_dev_unlock(pdev);
> > +                     up_write(&vdev->memory_lock);
> > +             } else {
> > +                     bridge = pci_upstream_bridge(pdev);
> > +                     if (bridge && !pci_dev_trylock(bridge))
> > +                             goto out_restore_state;
> > +                     if (pci_dev_trylock(pdev)) {
> > +                             if (!__pci_reset_function_locked(pdev))
> > +                                     vdev->needs_reset = false;
> > +                             pci_dev_unlock(pdev);
> > +                     }
> > +                     if (bridge)
> > +                             pci_dev_unlock(bridge);
> 
> Abstraction leaves a lot to be desired here.

I will refactor this part for correct upstream shape.

> 
> >               }
> > -             if (bridge)
> > -                     pci_dev_unlock(bridge);
> >       }
> >
> >  out_restore_state:
> > @@ -1592,6 +1625,20 @@ static int vfio_pci_ioctl_set_irqs(struct
> vfio_pci_core_device *vdev,
> >       return ret;
> >  }
> >
> > +/*
> > + * Reset the function. A CXL device runs the CXL DVSEC reset sequence
> > +in place
> > + * of a PCI function reset: it replaces FLR (which would corrupt
> > +CXL.mem),
> > + * always clears device memory, and restores the HDM decoder. Callers
> > +hold
> > + * memory_lock for write.
> > + */
> > +int vfio_pci_reset_function(struct vfio_pci_core_device *vdev) {
> > +     if (!vdev->cxl_ops || !vdev->cxl_ops->reset)
> > +             return pci_try_reset_function(vdev->pdev);
> > +
> > +     return vdev->cxl_ops->reset(vdev);
> 
> This is all inverted logic and I don't understand why we're trying to leave the
> reset op optional, make it mandatory:
> 
>         if (vdev->cxl_ops)
>                 return vdev->cxl_ops->reset(vdev);
> 
>         return pci_try_reset_function(vdev->pdev);
> 
> But we're again losing the try semantics on the cxl path(?) and naming of the
> wrapper drops the try semantics as well.

Agreed. I will rename this to vfio_pci_try_reset_function(), and CXL path will keep the trylock.

> 
> > +}
> > +
> >  static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
> >                               void __user *arg)  { @@ -1614,8 +1661,14
> > @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
> >       vfio_pci_set_power_state(vdev, PCI_D0);
> >
> >       vfio_pci_dma_buf_move(vdev, true);
> > -     ret = pci_try_reset_function(vdev->pdev);
> > -     if (__vfio_pci_memory_enabled(vdev))
> > +     ret = vfio_pci_reset_function(vdev);
> > +     /*
> > +      * Re-arm the dma-bufs on success. A CXL device whose reset failed
> leaves
> > +      * the HDM decoder unrestored and hdm_valid clear, so re-arming its
> HDM
> > +      * dma-buf would map device DMA onto a decoder the fault path still
> gates;
> > +      * keep it revoked until a reset succeeds. Plain vfio-pci is unchanged.
> > +      */
> > +     if (__vfio_pci_memory_enabled(vdev) && (!vdev->cxl_ops || !ret))
> >               vfio_pci_dma_buf_move(vdev, false);
> 
> I don't think we're doing dmabufs correctly for CXL, I don't see how they're the
> same address space, or TBH, how we can have a reset fail so catastrophically.
> Thanks,
> 

In v6 I will separate them as described above.
A failed reset will leave the HDM window closed until a later reset succeeds.

> Alex
> 
> >       up_write(&vdev->memory_lock);
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_priv.h
> > b/drivers/vfio/pci/vfio_pci_priv.h
> > index c268c99aea82..e1ef21806a2f 100644
> > --- a/drivers/vfio/pci/vfio_pci_priv.h
> > +++ b/drivers/vfio/pci/vfio_pci_priv.h
> > @@ -78,6 +78,7 @@ int vfio_pci_set_power_state(struct
> vfio_pci_core_device *vdev,
> >                            pci_power_t state);
> >
> >  void vfio_pci_zap_and_down_write_memory_lock(struct
> > vfio_pci_core_device *vdev);
> > +int vfio_pci_reset_function(struct vfio_pci_core_device *vdev);
> >  u16 vfio_pci_memory_lock_and_enable(struct vfio_pci_core_device
> > *vdev);  void vfio_pci_memory_unlock_and_restore(struct
> vfio_pci_core_device *vdev,
> >                                       u16 cmd); diff --git
> > a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h index
> > 39a28cc6ae8c..231679dead45 100644
> > --- a/include/linux/vfio_pci_core.h
> > +++ b/include/linux/vfio_pci_core.h
> > @@ -74,6 +74,10 @@ struct vfio_cxl_ops {
> >       void    (*close_device)(struct vfio_pci_core_device *vdev);
> >       void    (*reset_prepare)(struct vfio_pci_core_device *vdev);
> >       void    (*reset_done)(struct vfio_pci_core_device *vdev);
> > +     /* Run the CXL reset (always clears CXL.mem) in place of FLR */
> > +     int     (*reset)(struct vfio_pci_core_device *vdev);
> > +     /* True while the HDM range is valid and its dma-buf may be armed */
> > +     bool    (*hdm_active)(struct vfio_pci_core_device *vdev);
> >       /* Pinned per bound CXL device so vfio-cxl cannot unload under usage */
> >       struct module *owner;
> >  };