Re: [PATCH 4/4] ppc/xive: Add support for the PC MMIOs

[Frederic Barrat <fbarrat@linux.ibm.com>]

On 29/08/2023 16:32, Cédric Le Goater wrote:
> The XIVE interrupt contoller maintains various fields on interrupt
> targets in a structure called NVT. Each unit has a NVT cache, backed
> by RAM.
> 
> When the NVT structure is not local (in RAM) to the chip, the XIVE
> interrupt controller forwards the memory operation to the owning chip
> using the PC MMIO region configured for this purpose. QEMU does not
> need to be so precise since software shouldn't perform any of these
> operations. The model implementation is simplified to return the RAM
> address of the NVT structure which is then used by pnv_xive_vst_write
> or read to perform the operation in RAM.
> 
> Remove the last use of pnv_xive_get_remote().
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> -


Nice cleanup

Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>

   Fred


>   hw/intc/pnv_xive.c | 84 ++++++++++++++++++++++++++--------------------
>   1 file changed, 48 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index aae5cb6f607b..9b10e905195a 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -84,28 +84,6 @@ static uint8_t pnv_xive_block_id(PnvXive *xive)
>       return blk;
>   }
>   
> -/*
> - * Remote access to controllers. HW uses MMIOs. For now, a simple scan
> - * of the chips is good enough.
> - *
> - * TODO: Block scope support
> - */
> -static PnvXive *pnv_xive_get_remote(uint8_t blk)
> -{
> -    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> -    int i;
> -
> -    for (i = 0; i < pnv->num_chips; i++) {
> -        Pnv9Chip *chip9 = PNV9_CHIP(pnv->chips[i]);
> -        PnvXive *xive = &chip9->xive;
> -
> -        if (pnv_xive_block_id(xive) == blk) {
> -            return xive;
> -        }
> -    }
> -    return NULL;
> -}
> -
>   /*
>    * VST accessors for SBE, EAT, ENDT, NVT
>    *
> @@ -209,6 +187,42 @@ static uint64_t pnv_xive_vst_addr_indirect(PnvXive *xive, uint32_t type,
>       return pnv_xive_vst_addr_direct(xive, type, vsd, (idx % vst_per_page));
>   }
>   
> +/*
> + * This is a simplified model of operation forwarding on a remote IC.
> + *
> + * A PC MMIO address is built to identify the NVT structure. The load
> + * on the remote IC will return the address of the structure in RAM,
> + * which will then be used by pnv_xive_vst_write/read to perform the
> + * RAM operation.
> + */
> +static uint64_t pnv_xive_vst_addr_remote(PnvXive *xive, uint32_t type,
> +                                         uint64_t vsd, uint8_t blk,
> +                                         uint32_t idx)
> +{
> +    const XiveVstInfo *info = &vst_infos[type];
> +    uint64_t remote_addr = vsd & VSD_ADDRESS_MASK;
> +    uint64_t vst_addr;
> +    MemTxResult result;
> +
> +    if (type != VST_TSEL_VPDT) {
> +        xive_error(xive, "VST: invalid access on remote VST %s %x/%x !?",
> +                   info->name, blk, idx);
> +        return 0;
> +    }
> +
> +    remote_addr |= idx << xive->pc_shift;
> +
> +    vst_addr = address_space_ldq_be(&address_space_memory, remote_addr,
> +                                    MEMTXATTRS_UNSPECIFIED, &result);
> +    if (result != MEMTX_OK) {
> +        xive_error(xive, "VST: read failed at @0x%"  HWADDR_PRIx
> +                   " for NVT %x/%x\n", remote_addr, blk, idx);
> +        return 0;
> +    }
> +
> +    return vst_addr;
> +}
> +
>   static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
>                                     uint32_t idx)
>   {
> @@ -225,14 +239,7 @@ static uint64_t pnv_xive_vst_addr(PnvXive *xive, uint32_t type, uint8_t blk,
>   
>       /* Remote VST access */
>       if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
> -        if (type != VST_TSEL_VPDT) {
> -            xive_error(xive, "VST: invalid access on remote VST %s %x/%x !?",
> -                       info->name, blk, idx);
> -            return 0;
> -        }
> -        xive = pnv_xive_get_remote(blk);
> -
> -        return xive ? pnv_xive_vst_addr(xive, type, blk, idx) : 0;
> +        return pnv_xive_vst_addr_remote(xive, type, vsd, blk, idx);
>       }
>   
>       if (VSD_INDIRECT & vsd) {
> @@ -1785,16 +1792,20 @@ static const MemoryRegionOps pnv_xive_vc_ops = {
>   };
>   
>   /*
> - * Presenter Controller MMIO region. The Virtualization Controller
> - * updates the IPB in the NVT table when required. Not modeled.
> + * Presenter Controller MMIO region. Points to the NVT sets.
> + *
> + * HW implements all possible mem ops to the underlying NVT structure
> + * but QEMU does not need to be so precise. The model implementation
> + * simply returns the RAM address of the NVT structure which is then
> + * used by pnv_xive_vst_write/read to perform the RAM operation.
>    */
> -static uint64_t pnv_xive_pc_read(void *opaque, hwaddr addr,
> -                                 unsigned size)
> +static uint64_t pnv_xive_pc_read(void *opaque, hwaddr offset, unsigned size)
>   {
>       PnvXive *xive = PNV_XIVE(opaque);
> +    uint32_t nvt_idx = offset >> xive->pc_shift;
> +    uint8_t blk = pnv_xive_block_id(xive); /* TODO: VDT -> block xlate */
>   
> -    xive_error(xive, "PC: invalid read @%"HWADDR_PRIx, addr);
> -    return -1;
> +    return pnv_xive_vst_addr(xive, VST_TSEL_VPDT, blk, nvt_idx);
>   }
>   
>   static void pnv_xive_pc_write(void *opaque, hwaddr addr,
> @@ -2016,6 +2027,7 @@ static void pnv_xive_realize(DeviceState *dev, Error **errp)
>       /* Presenter Controller MMIO region (not modeled) */
>       memory_region_init_io(&xive->pc_mmio, OBJECT(xive), &pnv_xive_pc_ops, xive,
>                             "xive-pc", PNV9_XIVE_PC_SIZE);
> +    xive->pc_mmio.disable_reentrancy_guard = true;
>   
>       /* Thread Interrupt Management Area (Direct) */
>       memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &pnv_xive_tm_ops,