Re: [PATCH v4 04/11] pnv/xive2: Add NVG and NVC to cache watch facility

[Nicholas Piggin]

On Thu Jul 25, 2024 at 7:21 AM AEST, Michael Kowal wrote:
> From: Frederic Barrat <fbarrat@linux.ibm.com>
>
> The cache watch facility uses the same register interface to handle
> entries in the NVP, NVG and NVC tables. A bit-field in the 'watchX
> specification' register tells the table type. So far, that bit-field
> was not read and the code assumed a read/write to the NVP table.
>
> This patch allows to read/write entries in the NVG and NVC table as
> well.
>
> 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@kaod.org>
> ---
>  hw/intc/pnv_xive2.c | 49 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> index 3dbbfddacb..dfb0927fd3 100644
> --- a/hw/intc/pnv_xive2.c
> +++ b/hw/intc/pnv_xive2.c
> @@ -465,10 +465,30 @@ static int pnv_xive2_write_nvp(Xive2Router *xrtr, 
> uint8_t blk, uint32_t idx,
>                                word_number);
>  }
>  
> -static int pnv_xive2_nvp_update(PnvXive2 *xive, uint8_t watch_engine)
> +static int pnv_xive2_nxc_to_table_type(uint8_t nxc_type, uint32_t 
> *table_type)
>  {
> -    uint8_t  blk;
> -    uint32_t idx;
> +    switch (nxc_type) {
> +    case PC_NXC_WATCH_NXC_NVP:
> +        *table_type = VST_NVP;
> +        break;
> +    case PC_NXC_WATCH_NXC_NVG:
> +        *table_type = VST_NVG;
> +        break;
> +    case PC_NXC_WATCH_NXC_NVC:
> +        *table_type = VST_NVC;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "XIVE: invalid table type for nxc operation\n");
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +static int pnv_xive2_nxc_update(PnvXive2 *xive, uint8_t watch_engine)
> +{
> +    uint8_t  blk, nxc_type;
> +    uint32_t idx, table_type = -1;
>      int i, spec_reg, data_reg;
>      uint64_t nxc_watch[4];
>  
> @@ -476,21 +496,24 @@ static int pnv_xive2_nvp_update(PnvXive2 *xive, uint8_t 
> watch_engine)
>  
>      spec_reg = (PC_NXC_WATCH0_SPEC + watch_engine * 0x40) >> 3;
>      data_reg = (PC_NXC_WATCH0_DATA0 + watch_engine * 0x40) >> 3;
> +    nxc_type = GETFIELD(PC_NXC_WATCH_NXC_TYPE, xive->pc_regs[spec_reg]);
>      blk = GETFIELD(PC_NXC_WATCH_BLOCK_ID, xive->pc_regs[spec_reg]);
>      idx = GETFIELD(PC_NXC_WATCH_INDEX, xive->pc_regs[spec_reg]);
>  
> +    assert(pnv_xive2_nxc_to_table_type(nxc_type, &table_type));

Hey Mike,

Was in a bit of a rush and forgot to let you know earlier (thanks for
reminding me) - I flipped the assert condition on these when merging.
IIRC I got an always-false warning on one of the CI tests there, but I
can't seem to find which one, or why it didn't usually warn.

I know you're part way through wrangling a bunch of other xive stuff
that makes this stuff difficult to test, so no worries.

Thanks,
Nick