Re: [Qemu-devel] [PATCH v5 07/36] ppc/xive: introduce the XIVE interrupt thread context

[Cédric Le Goater]

On 11/27/18 6:07 AM, David Gibson wrote:
> On Sun, Nov 25, 2018 at 09:35:08PM +0100, Cédric Le Goater wrote:
>> On 11/23/18 6:08 AM, David Gibson wrote:
>>> On Fri, Nov 16, 2018 at 11:57:00AM +0100, Cédric Le Goater wrote:
>>>> Each POWER9 processor chip has a XIVE presenter that can generate four
>>>> different exceptions to its threads:
>>>>
>>>>   - hypervisor exception,
>>>>   - O/S exception
>>>>   - Event-Based Branch (EBB)
>>>>   - msgsnd (doorbell).
>>>>
>>>> Each exception has a state independent from the others called a Thread
>>>> Interrupt Management context. This context is a set of registers which
>>>> lets the thread handle priority management and interrupt acknowledgment
>>>> among other things. The most important ones being :
>>>>
>>>>   - Interrupt Priority Register  (PIPR)
>>>>   - Interrupt Pending Buffer     (IPB)
>>>>   - Current Processor Priority   (CPPR)
>>>>   - Notification Source Register (NSR)
>>>>
>>>> These registers are accessible through a specific MMIO region, called
>>>> the Thread Interrupt Management Area (TIMA), four aligned pages, each
>>>> exposing a different view of the registers. First page (page address
>>>> ending in 0b00) gives access to the entire context and is reserved for
>>>> the ring 0 security monitor. The second (page address ending in 0b01)
>>>> is for the hypervisor, ring 1. The third (page address ending in 0b10)
>>>> is for the operating system, ring 2. The fourth (page address ending
>>>> in 0b11) is for user level, ring 3.
>>>>
>>>> The thread interrupt context is modeled with a XiveTCTX object
>>>> containing the values of the different exception registers. The TIMA
>>>> region is mapped at the same address for each CPU.
>>>>
>>>> Signed-off-by: Cédric Le Goater <address@hidden>
>>>> ---
>>>>  include/hw/ppc/xive.h      |  36 +++
>>>>  include/hw/ppc/xive_regs.h |  82 +++++++
>>>>  hw/intc/xive.c             | 443 +++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 561 insertions(+)
>>>>
>>>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>>>> index 24301bf2076d..5987f26ddb98 100644
>>>> --- a/include/hw/ppc/xive.h
>>>> +++ b/include/hw/ppc/xive.h
>>>> @@ -238,4 +238,40 @@ typedef struct XiveENDSource {
>>>>  void xive_end_reset(XiveEND *end);
>>>>  void xive_end_pic_print_info(XiveEND *end, uint32_t end_idx, Monitor 
>>>> *mon);
>>>>  
>>>> +/*
>>>> + * XIVE Thread interrupt Management (TM) context
>>>> + */
>>>> +
>>>> +#define TYPE_XIVE_TCTX "xive-tctx"
>>>> +#define XIVE_TCTX(obj) OBJECT_CHECK(XiveTCTX, (obj), TYPE_XIVE_TCTX)
>>>> +
>>>> +/*
>>>> + * XIVE Thread interrupt Management register rings :
>>>> + *
>>>> + *   QW-0  User       event-based exception state
>>>> + *   QW-1  O/S        OS context for priority management, interrupt acks
>>>> + *   QW-2  Pool       hypervisor context for virtual processor being 
>>>> dispatched
>>>> + *   QW-3  Physical   for the security monitor to manage the entire 
>>>> context
>>>
>>> That last description is misleading, AIUI the hypervisor can and does
>>> make use of the physical ring as well as the pool ring.
>>
>> yes. The description is from the spec. I will rephrase. 
>>
>>>
>>>> + */
>>>> +#define TM_RING_COUNT           4
>>>> +#define TM_RING_SIZE            0x10
>>>> +
>>>> +typedef struct XiveTCTX {
>>>> +    DeviceState parent_obj;
>>>> +
>>>> +    CPUState    *cs;
>>>> +    qemu_irq    output;
>>>> +
>>>> +    uint8_t     regs[TM_RING_COUNT * TM_RING_SIZE];
>>>
>>> I'm a bit dubious about representing the state with a full buffer like
>>> this.  Isn't a fair bit of this space reserved or derived values which
>>> aren't backed by real state?
>>
>> Under sPAPR only the TM_QW1_OS ring is accessed but the TM_QW0_USER 
>> will also be when we support EBB.
>>
>> When running under the PowerNV machine, all rings could be accessed.
>> Today only 2 and 3 are.
> 
> It's not the fact that all rings are exposed that I'm talking about.
> What I mean is that some of the logical register address space isn't
> actually by actually modifiable registers.  For example the HW CAM
> bits are hardwired to a value which can be derived from elsewhere.

yes indeed.

> Likewise the "highest pending priority" register or whatever it's
> called is AFAICT calculated on read from the CPPR and IPB.

yes. the PIPR is computed from the IPB. (patch 10)

> So the thing I'm questioning is having actual storage bytes associated
> with logical registers which are actually derived from other state.

having storage for word2 and word3 on all rings is questionable I agree,
but I think the eight (NSR CPPR IPB LSMFB ACK# INC AGE PIPR) registers 
are good to have explicitly for the model.  

It fits well the HW and all the TIMA accessors which would be much
more complex if we only represented the interesting bits we need
to transfer. 

We also use the word0 and word1 to store the KVM state (os ring, maybe 
user one day) directly pulled from the KVM VCPU structure. word2 is also 
collected to print out the KVM VP identifier in the QEMU monitor.  

So, I think these four quad-words are useful and their storage is 
relatively small compared to the EAT (8bytes * nr_irqs) and ENDT 
(32bytes * 8 * nr_cpus).

Thanks,

C. 

>> It seemed correct to expose all registers under the thread interrupt
>> context model and filter the accesses with the TIMA. It fits the HW
>> well.
> 
>>>> +
>>>> +    XiveRouter  *xrtr;
>>>
>>> What's this for?  AFAIK a TCTX isn't associated with a particular
>>> routing unit.
>>
>> I should have added the pointer in patch 11 where it is used. This is 
>> to let the sPAPR XIVE controller model reset the OS CAM line with the 
>> VP identifier.
> 
>> The TCTX belong to the IVPE XIVE subengine and as the IVRE and IVPE are 
>> modeled under the XiveRouter, it's not suprising to see this '*xrtr' 
>> back pointer. But I agree we might not need.  Let's talk about it when 
>> you reach patch 11.
> 
> Ok.
>