| Message ID | 20230224194443.1990440-3-dave@stgolabs.net |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl: Background commands and device sanitation | expand |
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() 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];
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]; > >
On Mon, 03 Apr 2023, Jonathan Cameron wrote: >Also, the interrupt setup for the PCI cap is missing I think. >See handling in ct3_realize() Hmm how is this any different that what is already there for events and cpmu? Once msi is initialized for the device, I didn't expect any other setup necessary - and I'm also receiving irqs fine with this patch at the driver side. Thanks, Davidlohr
On Tue, 11 Apr 2023 19:22:44 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote: > On Mon, 03 Apr 2023, Jonathan Cameron wrote: > > >Also, the interrupt setup for the PCI cap is missing I think. > >See handling in ct3_realize() > > Hmm how is this any different that what is already there > for events and cpmu? Once msi is initialized for the device, > I didn't expect any other setup necessary - and I'm also > receiving irqs fine with this patch at the driver side. Ah. Seems you are sharing with the DOE for the compliance mailbox - probably not the intent but as we have no code that binds to that at the moment I guess you won't notice the sharing. It should be harmless but I'd put this on 11 and increase msix_num in ct3_realize. At somepoint I need to make all the msix code for all usecase deal with the OS asking for fewer vectors. Not a problem with Linux as it always grabs them all, but the emulation isn't currently spec compliant as those numbers should 'squish' into the available space if not enough is requested. When that's not happening I should add an enum for msix vectors so that we can more easily reorder the patches that add them. Jonathan > > Thanks, > Davidlohr
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; + 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];
Notify when the background operation is done. Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> --- 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(-)