Re: [Qemu-devel] [PATCH 13/25] spapr: introduce the XIVE Event Queues

[Cédric Le Goater]

On 12/04/2017 02:09 AM, David Gibson wrote:
> On Fri, Dec 01, 2017 at 05:36:39PM +0100, Cédric Le Goater wrote:
>> On 12/01/2017 12:35 AM, David Gibson wrote:
>>> On Thu, Nov 30, 2017 at 02:06:27PM +0000, Cédric Le Goater wrote:
>>>> On 11/30/2017 04:38 AM, David Gibson wrote:
>>>>> On Thu, Nov 23, 2017 at 02:29:43PM +0100, Cédric Le Goater wrote:
>>>>>> The Event Queue Descriptor (EQD) table, also known as Event Notification
>>>>>> Descriptor (END), is one of the internal tables the XIVE interrupt
>>>>>> controller uses to redirect exception from event sources to CPU
>>>>>> threads.
>>>>>>
>>>>>> The EQD specifies on which Event Queue the event data should be posted
>>>>>> when an exception occurs (later on pulled by the OS) and which server
>>>>>> (VPD in XIVE terminology) to notify. The Event Queue is a much more
>>>>>> complex structure but we start with a simple model for the sPAPR
>>>>>> machine.
>>>>>
>>>>> Just to clarify my understanding a server / VPD in XIVE would
>>>>> typically correspond to a cpu - either real or virtual, yes?
>>>>
>>>> yes. VP for "virtual processor" and VPD for "virtual processor 
>>>> descriptor" which contains the XIVE interrupt state of the VP 
>>>> when not dispatched. It is still described in some documentation 
>>>> as an NVT : Notification Virtual Target.  
>>>>
>>>> XIVE concepts were renamed at some time but the old name perdured.
>>>> I am still struggling my way through all the names.
>>>>
>>>>
>>>>>> There is one XiveEQ per priority and the model chooses to store them
>>>>>> under the Xive Interrupt presenter model. It will be retrieved, just
>>>>>> like for XICS, through the 'intc' object pointer of the CPU.
>>>>>>
>>>>>> The EQ indexing follows a simple pattern:
>>>>>>
>>>>>>        (server << 3) | (priority & 0x7)
>>>>>>
>>>>>> Signed-off-by: Cédric Le Goater <address@hidden>
>>>>>> ---
>>>>>>  hw/intc/spapr_xive.c    | 56 
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  hw/intc/xive-internal.h | 50 +++++++++++++++++++++++++++++++++++++++++++
>>>>>>  2 files changed, 106 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>>>>>> index 554b25e0884c..983317a6b3f6 100644
>>>>>> --- a/hw/intc/spapr_xive.c
>>>>>> +++ b/hw/intc/spapr_xive.c
>>>>>> @@ -23,6 +23,7 @@
>>>>>>  #include "sysemu/dma.h"
>>>>>>  #include "monitor/monitor.h"
>>>>>>  #include "hw/ppc/spapr_xive.h"
>>>>>> +#include "hw/ppc/spapr.h"
>>>>>>  #include "hw/ppc/xics.h"
>>>>>>  
>>>>>>  #include "xive-internal.h"
>>>>>> @@ -34,6 +35,8 @@ struct sPAPRXiveICP {
>>>>>>      uint8_t   tima[TM_RING_COUNT * 0x10];
>>>>>>      uint8_t   *tima_os;
>>>>>>      qemu_irq  output;
>>>>>> +
>>>>>> +    XiveEQ    eqt[XIVE_PRIORITY_MAX + 1];
>>>>>>  };
>>>>>>  
>>>>>>  static uint64_t spapr_xive_icp_accept(sPAPRXiveICP *icp)
>>>>>> @@ -183,6 +186,13 @@ static const MemoryRegionOps spapr_xive_tm_ops = {
>>>>>>      },
>>>>>>  };
>>>>>>  
>>>>>> +static sPAPRXiveICP *spapr_xive_icp_get(sPAPRXive *xive, int server)
>>>>>> +{
>>>>>> +    PowerPCCPU *cpu = spapr_find_cpu(server);
>>>>>> +
>>>>>> +    return cpu ? SPAPR_XIVE_ICP(cpu->intc) : NULL;
>>>>>> +}
>>>>>> +
>>>>>>  static void spapr_xive_irq(sPAPRXive *xive, int lisn)
>>>>>>  {
>>>>>>  
>>>>>> @@ -632,6 +642,8 @@ static void spapr_xive_icp_reset(void *dev)
>>>>>>      sPAPRXiveICP *xicp = SPAPR_XIVE_ICP(dev);
>>>>>>  
>>>>>>      memset(xicp->tima, 0, sizeof(xicp->tima));
>>>>>> +
>>>>>> +    memset(xicp->eqt, 0, sizeof(xicp->eqt));
>>>>>>  }
>>>>>>  
>>>>>>  static void spapr_xive_icp_realize(DeviceState *dev, Error **errp)
>>>>>> @@ -683,6 +695,23 @@ static void spapr_xive_icp_init(Object *obj)
>>>>>>      xicp->tima_os = &xicp->tima[TM_QW1_OS];
>>>>>>  }
>>>>>>  
>>>>>> +static const VMStateDescription vmstate_spapr_xive_icp_eq = {
>>>>>> +    .name = TYPE_SPAPR_XIVE_ICP "/eq",
>>>>>> +    .version_id = 1,
>>>>>> +    .minimum_version_id = 1,
>>>>>> +    .fields = (VMStateField []) {
>>>>>> +        VMSTATE_UINT32(w0, XiveEQ),
>>>>>> +        VMSTATE_UINT32(w1, XiveEQ),
>>>>>> +        VMSTATE_UINT32(w2, XiveEQ),
>>>>>> +        VMSTATE_UINT32(w3, XiveEQ),
>>>>>> +        VMSTATE_UINT32(w4, XiveEQ),
>>>>>> +        VMSTATE_UINT32(w5, XiveEQ),
>>>>>> +        VMSTATE_UINT32(w6, XiveEQ),
>>>>>> +        VMSTATE_UINT32(w7, XiveEQ),
>>>>>
>>>>> Wow.  Super descriptive field names there, but I guess that's not your 
>>>>> fault.
>>>>
>>>> The defines in the "xive-internal.h" give a better view ... 
>>>>
>>>>>> +        VMSTATE_END_OF_LIST()
>>>>>> +    },
>>>>>> +};
>>>>>> +
>>>>>>  static bool vmstate_spapr_xive_icp_needed(void *opaque)
>>>>>>  {
>>>>>>      /* TODO check machine XIVE support */
>>>>>> @@ -696,6 +725,8 @@ static const VMStateDescription 
>>>>>> vmstate_spapr_xive_icp = {
>>>>>>      .needed = vmstate_spapr_xive_icp_needed,
>>>>>>      .fields = (VMStateField[]) {
>>>>>>          VMSTATE_BUFFER(tima, sPAPRXiveICP),
>>>>>> +        VMSTATE_STRUCT_ARRAY(eqt, sPAPRXiveICP, (XIVE_PRIORITY_MAX + 
>>>>>> 1), 1,
>>>>>> +                             vmstate_spapr_xive_icp_eq, XiveEQ),
>>>>>>          VMSTATE_END_OF_LIST()
>>>>>>      },
>>>>>>  };
>>>>>> @@ -755,3 +786,28 @@ bool spapr_xive_irq_unset(sPAPRXive *xive, uint32_t 
>>>>>> lisn)
>>>>>>      ive->w &= ~IVE_VALID;
>>>>>>      return true;
>>>>>>  }
>>>>>> +
>>>>>> +/*
>>>>>> + * Use a simple indexing for the EQs.
>>>>>
>>>>> Is this server+priority encoding architected anywhere?  
>>>>
>>>> no. This is a model shortcut.
>>>>
>>>>> Otherwise, why not use separate parameters?
>>>>
>>>> yes. spapr_xive_get_eq() could use separate parameters and it would
>>>> shorten the some of the hcalls.
>>>>
>>>> The result is stored in a single field of the IVE, EQ_INDEX. So I will 
>>>> still need mangle/demangle routines but these could be simple macros.
>>>> I will look at it.
>>>
>>> Hm, ok.  So it's architected in the sense that you're using the
>>> encoding from the EQ_INDEX field throughout.  That's could be a
>>> reasonable choice, I can't really tell yet.
>>>
>>> On the other hand, it might be easier to read if we use server and
>>> priority as separate parameters until the point we actually encode
>>> into the EQ_INDEX field.
>>
>> In the architecture, the EQ_INDEX field contains an index to an 
>> Event Queue Descriptor and the Event Queue Descriptor has a 
>> EQ_W6_NVT_INDEX field pointing to an Notification Virtual Target.
>> So there are two extra tables for the EQs and for the NVTs
>> used by the HW.
> 
> Ok.  In the PAPR interface is the EQ_INDEX ever exposed to the guest?

never. 

> Or does it just supply target/priority numbers and the hypervisor
> manages the mapping to queues internally?

Yes. target/priority numbers is the interface used by the hcalls. 

Same for baremetal. OPAL handles the EQ indexing because it creates 
the EQ table and register it in the controller   

C.