Re: [PATCH for-5.0 v5 04/23] ppc/pnv: Dump the XIVE NVT table

[Cédric Le Goater]

On 19/11/2019 23:06, David Gibson wrote:
> On Fri, Nov 15, 2019 at 05:24:17PM +0100, Cédric Le Goater wrote:
>> This is useful to dump the saved contexts of the vCPUs : configuration
>> of the base END index of the vCPU and the Interrupt Pending Buffer
>> register, which is updated when an interrupt can not be presented.
>>
>> When dumping the NVT table, we skip empty indirect pages which are not
>> necessarily allocated.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  include/hw/ppc/xive_regs.h |  2 ++
>>  hw/intc/pnv_xive.c         | 30 ++++++++++++++++++++++++++++++
>>  2 files changed, 32 insertions(+)
>>
>> diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h
>> index 1a5622f8ded8..94338b4b551e 100644
>> --- a/include/hw/ppc/xive_regs.h
>> +++ b/include/hw/ppc/xive_regs.h
>> @@ -252,6 +252,8 @@ typedef struct XiveNVT {
>>          uint32_t        w0;
>>  #define NVT_W0_VALID             PPC_BIT32(0)
>>          uint32_t        w1;
>> +#define NVT_W1_EQ_BLOCK          PPC_BITMASK32(0, 3)
>> +#define NVT_W1_EQ_INDEX          PPC_BITMASK32(4, 31)
>>          uint32_t        w2;
>>          uint32_t        w3;
>>          uint32_t        w4;
>> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
>> index a4d80fd5e79c..02faf4135e48 100644
>> --- a/hw/intc/pnv_xive.c
>> +++ b/hw/intc/pnv_xive.c
>> @@ -1554,6 +1554,27 @@ static const MemoryRegionOps pnv_xive_pc_ops = {
>>      },
>>  };
>>  
>> +/*
>> + * skiboot uses an indirect NVT table with 64k subpages
>> + */
>> +#define XIVE_NVT_COUNT          (1 << XIVE_NVT_SHIFT)
>> +#define XIVE_NVT_PER_PAGE       (0x10000 / sizeof(XiveNVT))

First of all, this is an optimization to skip ranges of NVTs when 
an indirect page is empty. two assumptions are made : indirect table
and page size.

> Hrm.  So skiboot uses 64kiB pages, but how does the hardware know
> that? Is the hw fixed to 64kiB pages, or is there some register
> controlling the subpage size?  

The indirect page size is a field of the VSD structure describing the
pages.

> In which case shouldn't you be looking at that rather than assuming
> 64kiB?

Something cleaner would be : 

+static uint64_t pnv_xive_vst_per_page(PnvXive *xive, uint32_t type)
+{
+    uint8_t blk = pnv_xive_block_id(xive);
+    uint64_t vsd = xive->vsds[type][blk];
+    const XiveVstInfo *info = &vst_infos[type];
+    uint64_t vsd_addr;
+    uint32_t page_shift;
+
+    /* Get the page size of the indirect table. */
+    vsd_addr = vsd & VSD_ADDRESS_MASK;
+    vsd = ldq_be_dma(&address_space_memory, vsd_addr);
+
+    if (!(vsd & VSD_ADDRESS_MASK)) {
+#ifdef XIVE_DEBUG
+        xive_error(xive, "VST: invalid %s entry %x !?", info->name, idx);
+#endif
+        return 0;
+    }
+
+    page_shift = GETFIELD(VSD_TSIZE, vsd) + 12;
+
+    if (!pnv_xive_vst_page_size_allowed(page_shift)) {
+        xive_error(xive, "VST: invalid %s page shift %d", info->name,
+                   page_shift);
+        return 0;
+    }
+
+    return (1ull << page_shift) / info->size;
+}
+
+
 #define XIVE_NVT_COUNT          (1 << XIVE_NVT_SHIFT)
-#define XIVE_NVT_PER_PAGE       (0x10000 / sizeof(XiveNVT))
+#define XIVE_NVT_PER_PAGE(xive) pnv_xive_vst_per_page(xive, VST_TSEL_VPDT)
 

I will rework that patch and try to keep the info pic command simple. 

C.

>> +static void xive_nvt_pic_print_info(XiveNVT *nvt, uint32_t nvt_idx,
>> +                                    Monitor *mon)
>> +{
>> +    uint8_t  eq_blk = xive_get_field32(NVT_W1_EQ_BLOCK, nvt->w1);
>> +    uint32_t eq_idx = xive_get_field32(NVT_W1_EQ_INDEX, nvt->w1);
>> +
>> +    if (!xive_nvt_is_valid(nvt)) {
>> +        return;
>> +    }
>> +
>> +    monitor_printf(mon, "  %08x end:%02x/%04x IPB:%02x\n", nvt_idx,
>> +                   eq_blk, eq_idx,
>> +                   xive_get_field32(NVT_W4_IPB, nvt->w4));
>> +}
>> +
>>  void pnv_xive_pic_print_info(PnvXive *xive, Monitor *mon)
>>  {
>>      XiveRouter *xrtr = XIVE_ROUTER(xive);
>> @@ -1562,6 +1583,7 @@ void pnv_xive_pic_print_info(PnvXive *xive, Monitor 
>> *mon)
>>      uint32_t nr_ipis = pnv_xive_nr_ipis(xive, blk);
>>      XiveEAS eas;
>>      XiveEND end;
>> +    XiveNVT nvt;
>>      int i;
>>  
>>      monitor_printf(mon, "XIVE[%x] Source %08x .. %08x\n", blk, srcno0,
>> @@ -1590,6 +1612,14 @@ void pnv_xive_pic_print_info(PnvXive *xive, Monitor 
>> *mon)
>>      while (!xive_router_get_end(xrtr, blk, i, &end)) {
>>          xive_end_eas_pic_print_info(&end, i++, mon);
>>      }
>> +
>> +    monitor_printf(mon, "XIVE[%x] NVTT %08x .. %08x\n", blk, 0,
>> +                   XIVE_NVT_COUNT - 1);
>> +    for (i = 0; i < XIVE_NVT_COUNT; i += XIVE_NVT_PER_PAGE) {
>> +        while (!xive_router_get_nvt(xrtr, blk, i, &nvt)) {
>> +            xive_nvt_pic_print_info(&nvt, i++, mon);
>> +        }
>> +    }
>>  }
>>  
>>  static void pnv_xive_reset(void *dev)
>