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

[David Gibson]

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.
Likewise the "highest pending priority" register or whatever it's
called is AFAICT calculated on read from the CPPR and IPB.

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

> 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.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature