Re: [Qemu-devel] [PATCH 01/19] ppc/xive: hardwire the Physical CAM line of the thread context

["Cédric Le Goater" <clg@kaod.org>]

On 2/8/19 6:44 AM, David Gibson wrote:
> On Mon, Jan 28, 2019 at 10:46:07AM +0100, Cédric Le Goater wrote:
>> By default on P9, the HW CAM line (23bits) is hardwired to :
>>
>>       0x000||0b1||4Bit chip number||7Bit Thread number.
>>
>> When the block group mode is enabled at the controller level (PowerNV),
>> the CAM line is changed for CAM compares to :
>>
>>       4Bit chip number||0x001||7Bit Thread number
>>
>> This will require changes in xive_presenter_tctx_match() possibly.
>> This is a lowlevel functionality of the HW controller and it is not
>> strictly needed. Leave it for later.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/xive.h |  1 +
>>  hw/intc/xive.c        | 53 ++++++++++++++++++++++++++++++++++++++++---
>>  hw/ppc/pnv_core.c     | 16 ++++++++-----
>>  3 files changed, 61 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index ec3bb2aae45a..04d54e8315f7 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -319,6 +319,7 @@ typedef struct XiveTCTX {
>>      qemu_irq    output;
>>  
>>      uint8_t     regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
>> +    uint32_t    hw_cam;
> 
> I don't love the fact that the hw_cam field is mirroring something
> that's in the regs structure.  Any way you can have the property poke
> the right value directly into regs?  Or else split up regs so you can
> set the pieces individually.

We only need the PIR value to compute the HW CAM line. That's was the
initial proposal :

+static uint32_t xive_tctx_hw_cam_line(XiveTCTX *tctx, bool block_group)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(tctx->cs);
+    CPUPPCState *env = &cpu->env;
+    uint32_t pir = env->spr_cb[SPR_PIR].default_value;
+
+    return tctx_hw_cam_line(block_group, (pir >> 8) & 0xf, pir & 0x7f);
+}

but you didn't like either because it peeked into the CPUPPCState.

But I prefer the initial method. Less fuss. 

C.

> 
>>  } XiveTCTX;
>>  
>>  /*
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 2e9b8efd4342..f5642f2338de 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -469,6 +469,12 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>>      Object *obj;
>>      Error *local_err = NULL;
>>  
>> +    if (!tctx->hw_cam) {
>> +        error_setg(errp, "XIVE: HW CAM is not set for CPU %d",
>> +                   tctx->cs->cpu_index);
>> +        return;
>> +    }
>> +
>>      obj = object_property_get_link(OBJECT(dev), "cpu", &local_err);
>>      if (!obj) {
>>          error_propagate(errp, local_err);
>> @@ -509,11 +515,17 @@ static const VMStateDescription vmstate_xive_tctx = {
>>      },
>>  };
>>  
>> +static Property  xive_tctx_properties[] = {
>> +    DEFINE_PROP_UINT32("hw-cam", XiveTCTX, hw_cam, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>  static void xive_tctx_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>  
>>      dc->desc = "XIVE Interrupt Thread Context";
>> +    dc->props = xive_tctx_properties;
>>      dc->realize = xive_tctx_realize;
>>      dc->unrealize = xive_tctx_unrealize;
>>      dc->vmsd = &vmstate_xive_tctx;
>> @@ -526,8 +538,21 @@ static const TypeInfo xive_tctx_info = {
>>      .class_init    = xive_tctx_class_init,
>>  };
>>  
>> +/*
>> + * The HW CAM line (23bits) is hardwired to :
>> + *
>> + *   0x000||0b1||4Bit chip number||7Bit Thread number.
>> + */
>> +static uint32_t hw_cam_line(uint8_t chip_id, uint8_t tid)
>> +{
>> +    return 1 << 11 | (chip_id & 0xf) << 7 | (tid & 0x7f);
>> +}
>> +
>>  Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>>  {
>> +    CPUPPCState *env = &POWERPC_CPU(cpu)->env;
>> +    uint32_t pir = env->spr_cb[SPR_PIR].default_value;
>> +    uint32_t hw_cam = hw_cam_line((pir >> 8) & 0xf, pir & 0x7f);
>>      Error *local_err = NULL;
>>      Object *obj;
>>  
>> @@ -535,6 +560,10 @@ Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
>>      object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
>>      object_unref(obj);
>>      object_property_add_const_link(obj, "cpu", cpu, &error_abort);
>> +    object_property_set_int(obj, hw_cam, "hw-cam", &local_err);
>> +    if (local_err) {
>> +        goto error;
>> +    }
>>      object_property_set_bool(obj, true, "realized", &local_err);
>>      if (local_err) {
>>          goto error;
>> @@ -1112,14 +1141,28 @@ XiveTCTX *xive_router_get_tctx(XiveRouter *xrtr, CPUState *cs)
>>      return xrc->get_tctx(xrtr, cs);
>>  }
>>  
>> +/*
>> + * When the block grouping is enabled, the CAM line is changed to :
>> + *
>> + *   4Bit chip number||0x001||7Bit Thread number.
>> + */
>> +static bool xive_presenter_tctx_match_hw(XiveRouter *xrtr, XiveTCTX *tctx,
>> +                                         uint8_t nvt_blk, uint32_t nvt_idx)
>> +{
>> +    /* TODO: block group support */
>> +    return tctx->hw_cam == hw_cam_line(nvt_blk, nvt_idx);
>> +}
>> +
>>  /*
>>   * The thread context register words are in big-endian format.
>>   */
>> -static int xive_presenter_tctx_match(XiveTCTX *tctx, uint8_t format,
>> +static int xive_presenter_tctx_match(XiveRouter *xrtr,
>> +                                     XiveTCTX *tctx, uint8_t format,
>>                                       uint8_t nvt_blk, uint32_t nvt_idx,
>>                                       bool cam_ignore, uint32_t logic_serv)
>>  {
>>      uint32_t cam = xive_nvt_cam_line(nvt_blk, nvt_idx);
>> +    uint32_t qw3w2 = xive_tctx_word2(&tctx->regs[TM_QW3_HV_PHYS]);
>>      uint32_t qw2w2 = xive_tctx_word2(&tctx->regs[TM_QW2_HV_POOL]);
>>      uint32_t qw1w2 = xive_tctx_word2(&tctx->regs[TM_QW1_OS]);
>>      uint32_t qw0w2 = xive_tctx_word2(&tctx->regs[TM_QW0_USER]);
>> @@ -1142,7 +1185,11 @@ static int xive_presenter_tctx_match(XiveTCTX *tctx, uint8_t format,
>>  
>>          /* F=0 & i=0: Specific NVT notification */
>>  
>> -        /* TODO (PowerNV) : PHYS ring */
>> +        /* PHYS ring */
>> +        if ((be32_to_cpu(qw3w2) & TM_QW3W2_VT) &&
>> +            xive_presenter_tctx_match_hw(xrtr, tctx, nvt_blk, nvt_idx)) {
>> +            return TM_QW3_HV_PHYS;
>> +        }
>>  
>>          /* HV POOL ring */
>>          if ((be32_to_cpu(qw2w2) & TM_QW2W2_VP) &&
>> @@ -1199,7 +1246,7 @@ static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
>>           * Check the thread context CAM lines and record matches. We
>>           * will handle CPU exception delivery later
>>           */
>> -        ring = xive_presenter_tctx_match(tctx, format, nvt_blk, nvt_idx,
>> +        ring = xive_presenter_tctx_match(xrtr, tctx, format, nvt_blk, nvt_idx,
>>                                           cam_ignore, logic_serv);
>>          /*
>>           * Save the context and follow on to catch duplicates, that we
>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
>> index 7c806da720c6..f035522b4ec9 100644
>> --- a/hw/ppc/pnv_core.c
>> +++ b/hw/ppc/pnv_core.c
>> @@ -114,12 +114,6 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
>>          return;
>>      }
>>  
>> -    pcc->intc_create(chip, cpu, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> -        return;
>> -    }
>> -
>>      core_pir = object_property_get_uint(OBJECT(cpu), "core-pir", &error_abort);
>>  
>>      /*
>> @@ -129,6 +123,16 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
>>       */
>>      pir->default_value = core_pir + thread_index;
>>  
>> +    /*
>> +     * On P9, the interrupt presenter needs to hardwire the PIR value
>> +     * in the thread interrupt context of the CPU.
>> +     */
>> +    pcc->intc_create(chip, cpu, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>>      /* Set time-base frequency to 512 MHz */
>>      cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
>>  
>