Re: [PATCH 2/3] cxl/mbox: Wire up interrupts for background completion

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 3 Apr 2023 17:52:24 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 24 Feb 2023 11:44:42 -0800
> Davidlohr Bueso <dave@stgolabs.net> wrote:
> 
> > Notify when the background operation is done.
> > 
> > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>  
> Hi Davidlohr,
> 
> One trivial inline.  
> 
> Also, the interrupt setup for the PCI cap is missing I think.
> See handling in ct3_realize()

Should have said, for more minor stuff I'm fine just fixing these
up in my staging tree directly.  You can either feedback here or
when I post that series for upstream merge.  

Jonathan
> 
> 
> 
> Jonathan
> 
> > ---
> >  hw/cxl/cxl-device-utils.c   | 10 +++++++++-
> >  hw/cxl/cxl-mailbox-utils.c  | 11 +++++++++++
> >  include/hw/cxl/cxl_device.h |  1 +
> >  3 files changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> > index 4bb4e85dae19..a4a2c6a80004 100644
> > --- a/hw/cxl/cxl-device-utils.c
> > +++ b/hw/cxl/cxl-device-utils.c
> > @@ -272,10 +272,18 @@ static void device_reg_init_common(CXLDeviceState *cxl_dstate)
> >  
> >  static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate)
> >  {
> > -    /* 2048 payload size, with no interrupt */
> > +    const uint8_t msi_n = 9;
> > +
> > +    /* 2048 payload size */
> >      ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
> >                       PAYLOAD_SIZE, CXL_MAILBOX_PAYLOAD_SHIFT);
> >      cxl_dstate->payload_size = CXL_MAILBOX_MAX_PAYLOAD_SIZE;
> > +    /* irq support */
> > +    ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
> > +                     BG_INT_CAP, 1);
> > +    ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
> > +                     MSI_N, msi_n);
> > +    cxl_dstate->mbox_msi_n = msi_n;
> >  }
> >  
> >  static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) { }
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 82923bb84eb0..61f0b8d675bc 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -8,6 +8,8 @@
> >   */
> >  
> >  #include "qemu/osdep.h"
> > +#include "hw/pci/msi.h"
> > +#include "hw/pci/msix.h"
> >  #include "hw/cxl/cxl.h"
> >  #include "hw/cxl/cxl_events.h"
> >  #include "hw/pci/pci.h"
> > @@ -984,9 +986,18 @@ static void bg_timercb(void *opaque)
> >      cxl_dstate->mbox_reg_state64[R_CXL_DEV_BG_CMD_STS] = bg_status_reg;
> >  
> >      if (cxl_dstate->bg.complete_pct == 100) {
> > +        CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> > +        PCIDevice *pdev = &ct3d->parent_obj;  
> 
> Should be casting it rather than directly accessing the parent_obj
> PCI_DEVICE(ct3d) should work.
> 
> There is an open question about whether we should be doing similar for the
> cxl_dstate.  I've not figured out the answer yet, but it may well affect this.
> 
> 
> > +
> >          cxl_dstate->bg.starttime = 0;
> >          /* registers are updated, allow new bg-capable cmds */
> >          cxl_dstate->bg.runtime = 0;
> > +
> > +        if (msix_enabled(pdev)) {
> > +            msix_notify(pdev, cxl_dstate->mbox_msi_n);
> > +        } else if (msi_enabled(pdev)) {
> > +            msi_notify(pdev, cxl_dstate->mbox_msi_n);
> > +        }
> >      }
> >  }
> >  
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index dbb8a955723b..f986651b6ead 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -189,6 +189,7 @@ typedef struct cxl_device_state {
> >      struct {
> >          MemoryRegion mailbox;
> >          uint16_t payload_size;
> > +        uint8_t mbox_msi_n;
> >          union {
> >              uint8_t mbox_reg_state[CXL_MAILBOX_REGISTERS_LENGTH];
> >              uint16_t mbox_reg_state16[CXL_MAILBOX_REGISTERS_LENGTH / 2];  
> 
>