Re: [PATCH for-5.0 v5 15/23] ppc/xive: Use the XiveFabric and XivePresenter interfaces

[Cédric Le Goater]

On 21/11/2019 08:30, Greg Kurz wrote:
> On Thu, 21 Nov 2019 08:01:44 +0100
> Cédric Le Goater <address@hidden> wrote:
> 
>> On 20/11/2019 19:30, Greg Kurz wrote:
>>> On Fri, 15 Nov 2019 17:24:28 +0100
>>> Cédric Le Goater <address@hidden> wrote:
>>>
>>>> Now that the machines have handlers implementing the XiveFabric and
>>>> XivePresenter interfaces, remove xive_presenter_match() and make use
>>>> of the 'match_nvt' handler of the machine.
>>>>
>>>> Signed-off-by: Cédric Le Goater <address@hidden>
>>>> ---
>>>>  hw/intc/xive.c | 48 +++++++++++++++++-------------------------------
>>>>  1 file changed, 17 insertions(+), 31 deletions(-)
>>>>
>>>
>>> Nice diffstat :)
>>>
>>>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>>>> index 1c9e58f8deac..ab62bda85788 100644
>>>> --- a/hw/intc/xive.c
>>>> +++ b/hw/intc/xive.c
>>>> @@ -1423,30 +1423,6 @@ int xive_presenter_tctx_match(XivePresenter *xptr, 
>>>> XiveTCTX *tctx,
>>>>      return -1;
>>>>  }
>>>>  
>>>> -static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
>>>> -                                 uint8_t nvt_blk, uint32_t nvt_idx,
>>>> -                                 bool cam_ignore, uint8_t priority,
>>>> -                                 uint32_t logic_serv, XiveTCTXMatch 
>>>> *match)
>>>> -{
>>>> -    XivePresenter *xptr = XIVE_PRESENTER(xrtr);
>>>> -    XivePresenterClass *xpc = XIVE_PRESENTER_GET_CLASS(xptr);
>>>> -    int count;
>>>> -
>>>> -    count = xpc->match_nvt(xptr, format, nvt_blk, nvt_idx, cam_ignore,
>>>> -                           priority, logic_serv, match);
>>>> -    if (count < 0) {
>>>> -        return false;
>>>> -    }
>>>> -
>>>> -    if (!match->tctx) {
>>>> -        qemu_log_mask(LOG_UNIMP, "XIVE: NVT %x/%x is not dispatched\n",
>>>> -                      nvt_blk, nvt_idx);
>>>
>>> Maybe keep this trace...
>>
>> It's in spapr_xive_match_nvt() now.
>>
> 
> Not really... spapr_xive_match_nvt() has a trace for the opposite case of 
> duplicate
> matches:

not that one. The one in spapr.c ... Yes I need to change the name.

C.

> 
>             if (match->tctx) {
>                 qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a thread "
>                               "context NVT %x/%x\n", nvt_blk, nvt_idx);
>                 return -1;
>             }
> 
>>>
>>>> -        return false;
>>>> -    }
>>>> -
>>>> -    return true;
>>>> -}
>>>> -
>>>>  /*
>>>>   * This is our simple Xive Presenter Engine model. It is merged in the
>>>>   * Router as it does not require an extra object.
>>>> @@ -1462,22 +1438,32 @@ static bool xive_presenter_match(XiveRouter *xrtr, 
>>>> uint8_t format,
>>>>   *
>>>>   * The parameters represent what is sent on the PowerBus
>>>>   */
>>>> -static bool xive_presenter_notify(XiveRouter *xrtr, uint8_t format,
>>>> +static bool xive_presenter_notify(uint8_t format,
>>>>                                    uint8_t nvt_blk, uint32_t nvt_idx,
>>>>                                    bool cam_ignore, uint8_t priority,
>>>>                                    uint32_t logic_serv)
>>>>  {
>>>> +    XiveFabric *xfb = XIVE_FABRIC(qdev_get_machine());
>>>> +    XiveFabricClass *xfc = XIVE_FABRIC_GET_CLASS(xfb);
>>>>      XiveTCTXMatch match = { .tctx = NULL, .ring = 0 };
>>>> -    bool found;
>>>> +    int count;
>>>>  
>>>> -    found = xive_presenter_match(xrtr, format, nvt_blk, nvt_idx, 
>>>> cam_ignore,
>>>> -                                 priority, logic_serv, &match);
>>>> -    if (found) {
>>>> +    /*
>>>> +     * Ask the machine to scan the interrupt controllers for a match
>>>> +     */
>>>> +    count = xfc->match_nvt(xfb, format, nvt_blk, nvt_idx, cam_ignore,
>>>> +                           priority, logic_serv, &match);
>>>> +    if (count < 0) {
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    /* handle CPU exception delivery */
>>>> +    if (count) {
>>>>          ipb_update(&match.tctx->regs[match.ring], priority);
>>>>          xive_tctx_notify(match.tctx, match.ring);
>>>>      }
>>>
>>> ... in an else block here ^^ ?
>>>
>>>>  
>>>> -    return found;
>>>> +    return count;
>>>
>>> Implicit cast is ok I guess, but !!count would ensure no paranoid
>>> compiler ever complains.
>>
>> yes. 
>>
>> Thanks,
>>
>> C.
>>
>>
>>>
>>>>  }
>>>>  
>>>>  /*
>>>> @@ -1590,7 +1576,7 @@ static void xive_router_end_notify(XiveRouter *xrtr, 
>>>> uint8_t end_blk,
>>>>          return;
>>>>      }
>>>>  
>>>> -    found = xive_presenter_notify(xrtr, format, nvt_blk, nvt_idx,
>>>> +    found = xive_presenter_notify(format, nvt_blk, nvt_idx,
>>>>                            xive_get_field32(END_W7_F0_IGNORE, end.w7),
>>>>                            priority,
>>>>                            xive_get_field32(END_W7_F1_LOG_SERVER_ID, 
>>>> end.w7));
>>>
>>
>