Re: [PATCH v2 5/9] pnv/xive2: Configure Virtualization Structure Tables through the PC

[Cédric Le Goater]

On 7/15/24 20:33, Michael Kowal wrote:
> From: Frederic Barrat <fbarrat@linux.ibm.com>
>
> Both the virtualization layer (VC) and presentation layer (PC) need to
> be configured to access the VSTs. Since the information is redundant,
> the xive model combines both into one set of tables and only the
> definitions going through the VC are kept. The definitions through the
> PC are ignored. That works well as long as firmware calls the VC for
> all the tables.
>
> For the NVG and NVC tables, it can make sense to only configure them
> with the PC, since they are only used by the presenter. So this patch
> allows firmware to configure the VST tables through the PC as well.
> The definitions are still shared, since the VST tables can be set
> through both the VC and/or PC, they are dynamically re-mapped in
> memory by first deleting the memory subregion.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Signed-off-by: Michael Kowal <kowal@linux.vnet.ibm.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/intc/pnv_xive2.c | 47 ++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 38 insertions(+), 9 deletions(-)
>
> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> index c72c66dd6a..9e45161869 100644
> --- a/hw/intc/pnv_xive2.c
> +++ b/hw/intc/pnv_xive2.c
> @@ -759,6 +759,9 @@ static void pnv_xive2_vst_set_exclusive(PnvXive2 *xive, 
> uint8_t type,
>            * entries provisioned by FW (such as skiboot) and resize the
>            * ESB window accordingly.
>            */
> +        if (memory_region_is_mapped(&xsrc->esb_mmio)) {
> +            memory_region_del_subregion(&xive->esb_mmio, &xsrc->esb_mmio);
> +        }
>           if (!(VSD_INDIRECT & vsd)) {
>               memory_region_set_size(&xsrc->esb_mmio, vst_tsize * SBE_PER_BYTE
>                                      * (1ull << xsrc->esb_shift));
> @@ -774,6 +777,9 @@ static void pnv_xive2_vst_set_exclusive(PnvXive2 *xive, 
> uint8_t type,
>           /*
>            * Backing store pages for the END.
>            */
> +        if (memory_region_is_mapped(&end_xsrc->esb_mmio)) {
> +            memory_region_del_subregion(&xive->end_mmio, &end_xsrc->esb_mmio);
> +        }
>           if (!(VSD_INDIRECT & vsd)) {
>               memory_region_set_size(&end_xsrc->esb_mmio, (vst_tsize / 
> info->size)
>                                      * (1ull << end_xsrc->esb_shift));
> @@ -798,13 +804,10 @@ static void pnv_xive2_vst_set_exclusive(PnvXive2 *xive, 
> uint8_t type,
>    * Both PC and VC sub-engines are configured as each use the Virtual
>    * Structure Tables
>    */
> -static void pnv_xive2_vst_set_data(PnvXive2 *xive, uint64_t vsd)
> +static void pnv_xive2_vst_set_data(PnvXive2 *xive, uint64_t vsd,
> +                                   uint8_t type, uint8_t blk)
>   {
>       uint8_t mode = GETFIELD(VSD_MODE, vsd);
> -    uint8_t type = GETFIELD(VC_VSD_TABLE_SELECT,
> -                            xive->vc_regs[VC_VSD_TABLE_ADDR >> 3]);
> -    uint8_t blk = GETFIELD(VC_VSD_TABLE_ADDRESS,
> -                           xive->vc_regs[VC_VSD_TABLE_ADDR >> 3]);
>       uint64_t vst_addr = vsd & VSD_ADDRESS_MASK;
>   
>       if (type > VST_ERQ) {
> @@ -839,6 +842,16 @@ static void pnv_xive2_vst_set_data(PnvXive2 *xive, 
> uint64_t vsd)
>       }
>   }
>   
> +static void pnv_xive2_vc_vst_set_data(PnvXive2 *xive, uint64_t vsd)
> +{
> +    uint8_t type = GETFIELD(VC_VSD_TABLE_SELECT,
> +                            xive->vc_regs[VC_VSD_TABLE_ADDR >> 3]);
> +    uint8_t blk = GETFIELD(VC_VSD_TABLE_ADDRESS,
> +                           xive->vc_regs[VC_VSD_TABLE_ADDR >> 3]);
> +
> +    pnv_xive2_vst_set_data(xive, vsd, type, blk);
> +}
> +
>   /*
>    * MMIO handlers
>    */
> @@ -1268,7 +1281,7 @@ static void pnv_xive2_ic_vc_write(void *opaque, hwaddr 
> offset,
>       case VC_VSD_TABLE_ADDR:
>          break;
>       case VC_VSD_TABLE_DATA:
> -        pnv_xive2_vst_set_data(xive, val);
> +        pnv_xive2_vc_vst_set_data(xive, val);
>           break;
>   
>       /*
> @@ -1487,6 +1500,16 @@ static uint64_t pnv_xive2_ic_pc_read(void *opaque, 
> hwaddr offset,
>       return val;
>   }
>   
> +static void pnv_xive2_pc_vst_set_data(PnvXive2 *xive, uint64_t vsd)
> +{
> +    uint8_t type = GETFIELD(PC_VSD_TABLE_SELECT,
> +                            xive->pc_regs[PC_VSD_TABLE_ADDR >> 3]);
> +    uint8_t blk = GETFIELD(PC_VSD_TABLE_ADDRESS,
> +                           xive->pc_regs[PC_VSD_TABLE_ADDR >> 3]);
> +
> +    pnv_xive2_vst_set_data(xive, vsd, type, blk);
> +}
> +
>   static void pnv_xive2_ic_pc_write(void *opaque, hwaddr offset,
>                                     uint64_t val, unsigned size)
>   {
> @@ -1497,12 +1520,18 @@ static void pnv_xive2_ic_pc_write(void *opaque, hwaddr 
> offset,
>       switch (offset) {
>   
>       /*
> -     * VSD table settings. Only taken into account in the VC
> -     * sub-engine because the Xive2Router model combines both VC and PC
> -     * sub-engines
> +     * VSD table settings.
> +     * The Xive2Router model combines both VC and PC sub-engines. We
> +     * allow to configure the tables through both, for the rare cases
> +     * where a table only really needs to be configured for one of
> +     * them (e.g. the NVG table for the presenter). It assumes that
> +     * firmware passes the same address to the VC and PC when tables
> +     * are defined for both, which seems acceptable.
>        */
>       case PC_VSD_TABLE_ADDR:
> +        break;
>       case PC_VSD_TABLE_DATA:
> +        pnv_xive2_pc_vst_set_data(xive, val);
>           break;
>   
>       case PC_NXC_PROC_CONFIG: