Hi Conor, On Wed, Sep 30, 2026 at 5:23 PM Conor Dooley <[email protected]> wrote: > > On Fri, Sep 04, 2026 at 11:57:35PM +0800, Bin Meng wrote: > > Polling firmware requests system services without requesting an > > interrupt. The old model ignored this distinction: > > > > 1. HSS requests the serial number with REQUEST set and NOTIFY clear. > > 2. QEMU synchronously fills the mailbox and clears REQUEST. > > 3. QEMU incorrectly asserts PLIC source 96. > > 4. HSS polls REQUEST and neither waits for nor handles the interrupt. > > 5. PLIC source 96 remains pending. > > 6. Linux registers the MPFS mailbox driver. > > 7. The stale interrupt immediately enters mpfs_mbox_inbox_isr(). > > 8. Linux has not submitted a request or installed its response pointer. > > 9. The ISR dereferences that null pointer and faults near address 0x10. > > This makes sense, and should probably have a fixes tag pointing to my > 2022 commit? Since I was direct kernel booting, I would never had > noticed anything of this nature.
Agreed. I've split the NOTIFY fix into a separate patch, limited to filtering completion interrupts. > > > Honor SERVICES_CR.NOTIFY, let IOSCB own the pending interrupt state, > > and route the SYSREG acknowledgement through an IOSCB clear input. > > > > Signed-off-by: Bin Meng <[email protected]> > > Reviewed-by: Chao Liu <[email protected]> > > --- > > > > (no changes since v1) > > > > include/hw/misc/mchp_pfsoc_ioscb.h | 2 ++ > > hw/misc/mchp_pfsoc_ioscb.c | 39 +++++++++++++++++++++++++++--- > > hw/misc/mchp_pfsoc_sysreg.c | 9 ++++++- > > hw/riscv/microchip_pfsoc.c | 6 ++--- > > 4 files changed, 49 insertions(+), 7 deletions(-) > > > > diff --git a/include/hw/misc/mchp_pfsoc_ioscb.h > > b/include/hw/misc/mchp_pfsoc_ioscb.h > > index fd31427304..e39d995b64 100644 > > --- a/include/hw/misc/mchp_pfsoc_ioscb.h > > +++ b/include/hw/misc/mchp_pfsoc_ioscb.h > > @@ -26,6 +26,7 @@ > > #include "hw/core/sysbus.h" > > > > #define MCHP_PFSOC_IOSCB_MAILBOX_SIZE 0x1000 > > +#define MCHP_PFSOC_IOSCB_IRQ_CLEAR "irq-clear" > > > > typedef struct MchpPfSoCIoscbState { > > SysBusDevice parent; > > @@ -53,6 +54,7 @@ typedef struct MchpPfSoCIoscbState { > > uint32_t services_sr; > > uint8_t mailbox_data[MCHP_PFSOC_IOSCB_MAILBOX_SIZE]; > > char *serial_number; > > + bool irq_pending; > > qemu_irq irq; > > } MchpPfSoCIoscbState; > > > > diff --git a/hw/misc/mchp_pfsoc_ioscb.c b/hw/misc/mchp_pfsoc_ioscb.c > > index 7c82b55986..ccc2201b7a 100644 > > --- a/hw/misc/mchp_pfsoc_ioscb.c > > +++ b/hw/misc/mchp_pfsoc_ioscb.c > > @@ -192,12 +192,13 @@ static const MemoryRegionOps > > mchp_pfsoc_io_calib_ddr_ops = { > > > > #define SERVICES_CR 0x50 > > #define SERVICES_CR_REQUEST BIT(0) > > +#define SERVICES_CR_NOTIFY BIT(3) > > #define SERVICES_CR_COMMAND_SHIFT 16 > > #define SERVICES_CR_COMMAND_WIDTH 8 > > #define SERVICES_CR_COMMAND_MASK \ > > MAKE_64BIT_MASK(SERVICES_CR_COMMAND_SHIFT, > > SERVICES_CR_COMMAND_WIDTH) > > #define SERVICES_CR_MASK \ > > - (SERVICES_CR_REQUEST | SERVICES_CR_COMMAND_MASK) > > + (SERVICES_CR_REQUEST | SERVICES_CR_NOTIFY | > > SERVICES_CR_COMMAND_MASK) > > #define SERVICES_SR 0x54 > > #define SERVICES_SR_STATUS_SHIFT 16 > > #define SERVICES_COMMAND_SERIAL_NUMBER 0 > > @@ -205,6 +206,21 @@ static const MemoryRegionOps > > mchp_pfsoc_io_calib_ddr_ops = { > > #define SERVICES_STATUS_FAILED 1 > > #define SERVICES_MAILBOX_RESPONSE_OFFSET 0 > > > > +static void mchp_pfsoc_ioscb_update_irq(MchpPfSoCIoscbState *s) > > +{ > > + qemu_set_irq(s->irq, s->irq_pending); > > +} > > + > > +static void mchp_pfsoc_ioscb_clear_irq(void *opaque, int n, int level) > > +{ > > + MchpPfSoCIoscbState *s = opaque; > > + > > + if (level) { > > + s->irq_pending = false; > > + mchp_pfsoc_ioscb_update_irq(s); > > + } > > +} > > + > > static void services_cr_write(MchpPfSoCIoscbState *s, uint32_t value) > > { > > uint32_t command; > > @@ -236,7 +252,15 @@ static void services_cr_write(MchpPfSoCIoscbState *s, > > uint32_t value) > > } > > > > s->services_sr = status << SERVICES_SR_STATUS_SHIFT; > > - qemu_irq_raise(s->irq); > > + /* > > + * HSS and U-Boot submit polling requests with REQUEST set and NOTIFY > > + * clear, then poll REQUEST/BUSY for completion. Linux sets both bits > > + * and expects completion through PLIC source 96. > > + */ > > + if (value & SERVICES_CR_NOTIFY) { > > + s->irq_pending = true; > > + mchp_pfsoc_ioscb_update_irq(s); > > + } > > } > > > > static uint64_t mchp_pfsoc_ctrl_read(void *opaque, hwaddr offset, > > @@ -325,7 +349,8 @@ static void mchp_pfsoc_ioscb_reset(DeviceState *dev) > > s->services_cr = 0; > > s->services_sr = 0; > > memset(s->mailbox_data, 0, sizeof(s->mailbox_data)); > > - qemu_irq_lower(s->irq); > > + s->irq_pending = false; > > + mchp_pfsoc_ioscb_update_irq(s); > > } > > > > static const Property mchp_pfsoc_ioscb_properties[] = { > > @@ -333,6 +358,13 @@ static const Property mchp_pfsoc_ioscb_properties[] = { > > MchpPfSoCIoscbState, serial_number), > > }; > > > > +static void mchp_pfsoc_ioscb_init(Object *obj) > > +{ > > + /* Accept service interrupt acknowledgements from SYSREG MESSAGE_INT */ > > + qdev_init_gpio_in_named(DEVICE(obj), mchp_pfsoc_ioscb_clear_irq, > > + MCHP_PFSOC_IOSCB_IRQ_CLEAR, 1); > > +} > > + > > static void mchp_pfsoc_ioscb_realize(DeviceState *dev, Error **errp) > > { > > MchpPfSoCIoscbState *s = MCHP_PFSOC_IOSCB(dev); > > @@ -456,6 +488,7 @@ static const TypeInfo mchp_pfsoc_ioscb_info = { > > .name = TYPE_MCHP_PFSOC_IOSCB, > > .parent = TYPE_SYS_BUS_DEVICE, > > .instance_size = sizeof(MchpPfSoCIoscbState), > > + .instance_init = mchp_pfsoc_ioscb_init, > > .class_init = mchp_pfsoc_ioscb_class_init, > > }; > > > > diff --git a/hw/misc/mchp_pfsoc_sysreg.c b/hw/misc/mchp_pfsoc_sysreg.c > > index 1d9154280a..899b485da6 100644 > > --- a/hw/misc/mchp_pfsoc_sysreg.c > > +++ b/hw/misc/mchp_pfsoc_sysreg.c > > @@ -77,7 +77,14 @@ static void mchp_pfsoc_sysreg_write(void *opaque, hwaddr > > offset, > > } > > break; > > case MESSAGE_INT: > > - qemu_irq_lower(s->irq); > > + /* > > + * A MESSAGE_INT write is an acknowledgement event, not a level > > that > > + * remains asserted. Model it as an active-high pulse. The rising > > edge > > I found this comment to be really confusing. Why would someone expect > that a write-zero-clear bit is an asserted level? Mentioning modelling > it as a pulse further makes it seem like software writing will generate an > interrupt, which it doesn't - it writes here to clear the interrupt. I > think this needs to be very clear that modelling this as an interrupt at > all is just being done for QEMU's sake. > > I think this needs to be split in two, with the fix for the problem > (which I think is limited to only raising the interrupt) kept apart from > this change. Part of that is because I don't actually understand the > reason for adding this "fake" interrupt and the commit message is > currently focused around the bug and doesn't actually provide > justification for it. Maybe it's obvious to people more familiar with > Qemu than I, but this complication makes no sense to me! I used another way to implement this in v3. I dropped the acknowledgement pulse and irq-clear GPIO. SYSREG now links to IOSCB and calls its pending-bit getter/setter. IOSCB owns the pending state and the PLIC output, so a write of zero directly clears the state and lowers the real interrupt input. There is no extra interrupt for the acknowledgement. > > > + * invokes IOSCB's irq-clear input with level 1, which clears > > + * irq_pending and lowers PLIC source 96. The falling edge invokes > > the > > + * input with level 0 and is ignored. > > + */ > > + qemu_irq_pulse(s->irq); > > break; > > default: > > qemu_log_mask(LOG_UNIMP, "%s: unimplemented device write " > > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c > > index fb0e18edba..87c9c89cb0 100644 > > --- a/hw/riscv/microchip_pfsoc.c > > +++ b/hw/riscv/microchip_pfsoc.c > > @@ -336,9 +336,6 @@ static void microchip_pfsoc_soc_realize(DeviceState > > *dev, Error **errp) > > sysbus_realize(SYS_BUS_DEVICE(&s->sysreg), errp); > > sysbus_mmio_map(SYS_BUS_DEVICE(&s->sysreg), 0, > > memmap[MICROCHIP_PFSOC_SYSREG].base); > > - sysbus_connect_irq(SYS_BUS_DEVICE(&s->sysreg), 0, > > - qdev_get_gpio_in(DEVICE(s->plic), > > - MICROCHIP_PFSOC_MAILBOX_IRQ)); > > > > /* AXISW */ > > create_unimplemented_device("microchip.pfsoc.axisw", > > @@ -492,6 +489,9 @@ static void microchip_pfsoc_soc_realize(DeviceState > > *dev, Error **errp) > > sysbus_connect_irq(SYS_BUS_DEVICE(&s->ioscb), 0, > > qdev_get_gpio_in(DEVICE(s->plic), > > MICROCHIP_PFSOC_MAILBOX_IRQ)); > > + sysbus_connect_irq(SYS_BUS_DEVICE(&s->sysreg), 0, > > + qdev_get_gpio_in_named(DEVICE(&s->ioscb), > > + MCHP_PFSOC_IOSCB_IRQ_CLEAR, 0)); > > > > /* FPGA Fabric */ > > create_unimplemented_device("microchip.pfsoc.fabricfic3", > > -- Regards, Bin
