Re: [Qemu-ppc] [PATCH v5 11/36] spapr/xive: use the VCPU id as a NVT identifier

[Cédric Le Goater]

[ ... ] 

>>>> +/*
>>>> + * The allocation of VP blocks is a complex operation in OPAL and the
>>>> + * VP identifiers have a relation with the number of HW chips, the
>>>> + * size of the VP blocks, VP grouping, etc. The QEMU sPAPR XIVE
>>>> + * controller model does not have the same constraints and can use a
>>>> + * simple mapping scheme of the CPU vcpu_id
>>>> + *
>>>> + * These identifiers are never returned to the OS.
>>>> + */
>>>> +
>>>> +#define SPAPR_XIVE_VP_BASE 0x400
>>>
>>> 0x400 == 1024.  Could we ever have the possibility of needing to
>>> consider both physical NVTs and PAPR NVTs at the same time?  
>>
>> They would not be in the same CAM line: OS ring vs. PHYS ring. 
> 
> Hm.  They still inhabit the same NVT number space though, don't they?

No. skiboot reserves the range of VPs for the HW at init.

https://github.com/open-power/skiboot/blob/master/hw/xive.c#L1093

> I'm thinking about the END->NVT stage of the process here, rather than
> the NVT->TCTX stage.
>
> Oh, also, you're using "VP" here which IIUC == "NVT".  Can we
> standardize on one, please.

VP is used in Linux/KVM Linux/Native and skiboot. Yes. it's a mess. 
Let's have consistent naming in QEMU and use NVT. 

>>> If so, does this base leave enough space for the physical ones?
>>
>> I only used 0x400 to map the VP identifier to the ones allocated by KVM. 
>> 0x0 would be fine but to exercise the model, it's better having a different 
>> base. 
>>
>>>> +uint32_t spapr_xive_nvt_to_target(sPAPRXive *xive, uint8_t nvt_blk,
>>>> +                                  uint32_t nvt_idx)
>>>> +{
>>>> +    return nvt_idx - SPAPR_XIVE_VP_BASE;
>>>> +}
>>>> +
>>>> +int spapr_xive_cpu_to_nvt(sPAPRXive *xive, PowerPCCPU *cpu,
>>>> +                          uint8_t *out_nvt_blk, uint32_t *out_nvt_idx)
>>>
>>> A number of these conversions will come out a bit simpler if we pass
>>> the block and index around as a single word in most places.
>>
>> Yes I have to check the whole patchset first. These prototype changes
>> are not too difficult in terms of code complexity but they do break
>> how patches apply and PowerNV is also using the idx and blk much more 
>> explicitly. the block has a meaning on bare metal. So I am a bit 
>> reluctant to do so. I will check.
> 
> Yeah, based on your comments here and earlier, I'm not sure that's a
> good idea any more either.
> 
>>>> +{
>>>> +    XiveRouter *xrtr = XIVE_ROUTER(xive);
>>>> +
>>>> +    if (!cpu) {
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    if (out_nvt_blk) {
>>>> +        /* For testing purpose, we could use 0 for nvt_blk */
>>>> +        *out_nvt_blk = xrtr->chip_id;
>>>
>>> I don't see any point using the chip_id here, which is currently
>>> always set to 0 for PAPR anyway.  If we just hardwire this to 0 it
>>> removes the only use here of xrtr, which will allow some further
>>> simplifications in the caller, I think.
>>
>> You are right about the simplification. It was one way to exercise 
>> the router model and remove any shortcuts in the indexing. I kept 
>> it to be sure I was not tempted to invent new ones. I think we can
>> remove it before merging. 
>>
>>>
>>>> +    }
>>>> +
>>>> +    if (out_nvt_blk) {
>>>> +        *out_nvt_idx = SPAPR_XIVE_VP_BASE + cpu->vcpu_id;
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +int spapr_xive_target_to_nvt(sPAPRXive *xive, uint32_t target,
>>>> +                             uint8_t *out_nvt_blk, uint32_t *out_nvt_idx)
>>>
>>> I suspect some, maybe most of these conversion functions could be static.
>>
>> static inline ? 
> 
> It's in a .c file so you don't need the "inline" - the compiler can
> work out whether it's a good idea to inline on its own.

It is used in the hcall file also. But we are going to change that.

Thanks,

C.

> 
>>>
>>>> +{
>>>> +    return spapr_xive_cpu_to_nvt(xive, spapr_find_cpu(target), 
>>>> out_nvt_blk,
>>>> +                                 out_nvt_idx);
>>>> +}
>>>> +
>>>> +/*
>>>> + * sPAPR END indexing uses a simple mapping of the CPU vcpu_id, 8
>>>> + * priorities per CPU
>>>> + */
>>>> +int spapr_xive_end_to_target(sPAPRXive *xive, uint8_t end_blk, uint32_t 
>>>> end_idx,
>>>> +                             uint32_t *out_server, uint8_t *out_prio)
>>>> +{
>>>> +    if (out_server) {
>>>> +        *out_server = end_idx >> 3;
>>>> +    }
>>>> +
>>>> +    if (out_prio) {
>>>> +        *out_prio = end_idx & 0x7;
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +int spapr_xive_cpu_to_end(sPAPRXive *xive, PowerPCCPU *cpu, uint8_t prio,
>>>> +                          uint8_t *out_end_blk, uint32_t *out_end_idx)
>>>> +{
>>>> +    XiveRouter *xrtr = XIVE_ROUTER(xive);
>>>> +
>>>> +    if (!cpu) {
>>>> +        return -1;
>>>
>>> Is there ever a reason this would be called with cpu == NULL?  If not
>>> might as well just assert() here rather than pushing the error
>>> handling back to the caller.
>>
>> ok. yes.
>>
>>>
>>>> +    }
>>>> +
>>>> +    if (out_end_blk) {
>>>> +        /* For testing purpose, we could use 0 for nvt_blk */
>>>> +        *out_end_blk = xrtr->chip_id;
>>>
>>> Again, I don't see any point to using the chip_id, which is pretty
>>> meaningless for PAPR.
>>>
>>>> +    }
>>>> +
>>>> +    if (out_end_idx) {
>>>> +        *out_end_idx = (cpu->vcpu_id << 3) + prio;
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +int spapr_xive_target_to_end(sPAPRXive *xive, uint32_t target, uint8_t 
>>>> prio,
>>>> +                             uint8_t *out_end_blk, uint32_t *out_end_idx)
>>>> +{
>>>> +    return spapr_xive_cpu_to_end(xive, spapr_find_cpu(target), prio,
>>>> +                                 out_end_blk, out_end_idx);
>>>> +}
>>>> +
>>>>  static const VMStateDescription vmstate_spapr_xive_end = {
>>>>      .name = TYPE_SPAPR_XIVE "/end",
>>>>      .version_id = 1,
>>>> @@ -263,6 +396,9 @@ static void spapr_xive_class_init(ObjectClass *klass, 
>>>> void *data)
>>>>      xrc->set_eas = spapr_xive_set_eas;
>>>>      xrc->get_end = spapr_xive_get_end;
>>>>      xrc->set_end = spapr_xive_set_end;
>>>> +    xrc->get_nvt = spapr_xive_get_nvt;
>>>> +    xrc->set_nvt = spapr_xive_set_nvt;
>>>> +    xrc->reset_tctx = spapr_xive_reset_tctx;
>>>>  }
>>>>  
>>>>  static const TypeInfo spapr_xive_info = {
>>>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>>>> index c49932d2b799..fc6ef5895e6d 100644
>>>> --- a/hw/intc/xive.c
>>>> +++ b/hw/intc/xive.c
>>>> @@ -481,6 +481,7 @@ static uint32_t xive_tctx_hw_cam_line(XiveTCTX *tctx, 
>>>> bool block_group)
>>>>  static void xive_tctx_reset(void *dev)
>>>>  {
>>>>      XiveTCTX *tctx = XIVE_TCTX(dev);
>>>> +    XiveRouterClass *xrc = XIVE_ROUTER_GET_CLASS(tctx->xrtr);
>>>>  
>>>>      memset(tctx->regs, 0, sizeof(tctx->regs));
>>>>  
>>>> @@ -495,6 +496,14 @@ static void xive_tctx_reset(void *dev)
>>>>       */
>>>>      tctx->regs[TM_QW1_OS + TM_PIPR] =
>>>>          ipb_to_pipr(tctx->regs[TM_QW1_OS + TM_IPB]);
>>>> +
>>>> +    /*
>>>> +     * QEMU sPAPR XIVE only. To let the controller model reset the OS
>>>> +     * CAM line with the VP identifier.
>>>> +     */
>>>> +    if (xrc->reset_tctx) {
>>>> +        xrc->reset_tctx(tctx->xrtr, tctx);
>>>> +    }
>>>
>>> AFAICT this whole function is only used from PAPR, so you can just
>>> move the whole thing to the papr code and avoid the hook function.
>>
>> Yes we could add a loop on all CPUs and reset all the XiveTCTX from
>> the machine or a spapr_irq->reset handler. We will need at some time
>> anyhow.
>>
>> Thanks,
>>
>> C.
>>
>>
>>>
>>>>  }
>>>>  
>>>>  static void xive_tctx_realize(DeviceState *dev, Error **errp)
>>>
>>
>