Re: [PATCH 19/26] hw/misc: pfsoc: Honor PolarFire service notification requests

On Thu, Jul 23, 2026 at 11:18:46PM +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.
> 
> 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]>

Thanks,
Chao

> ---
> 
>  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
> +         * 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 63658fbeb0..f348de6fd4 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -333,9 +333,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",
> @@ -489,6 +486,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",
> -- 
> 2.34.1
>