Re: [Qemu-devel] [PATCH v6 08/37] ppc/xive: introduce a simplified XIVE presenter

[David Gibson]

On Fri, Dec 07, 2018 at 09:49:29AM +0100, Cédric Le Goater wrote:
> On 12/7/18 4:10 AM, David Gibson wrote:
> > On Thu, Dec 06, 2018 at 12:22:22AM +0100, Cédric Le Goater wrote:
> >> The last sub-engine of the XIVE architecture is the Interrupt
> >> Virtualization Presentation Engine (IVPE). On HW, the IVRE and the
> >> IVPE share elements, the Power Bus interface (CQ), the routing table
> >> descriptors, and they can be combined in the same HW logic. We do the
> >> same in QEMU and combine both engines in the XiveRouter for
> >> simplicity.
> >>
> >> When the IVRE has completed its job of matching an event source with a
> >> Notification Virtual Target (NVT) to notify, it forwards the event
> >> notification to the IVPE sub-engine. The IVPE scans the thread
> >> interrupt contexts of the Notification Virtual Targets (NVT)
> >> dispatched on the HW processor threads and if a match is found, it
> >> signals the thread. If not, the IVPE escalates the notification to
> >> some other targets and records the notification in a backlog queue.
> >>
> >> The IVPE maintains the thread interrupt context state for each of its
> >> NVTs not dispatched on HW processor threads in the Notification
> >> Virtual Target table (NVTT).
> >>
> >> The model currently only supports single NVT notifications.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  include/hw/ppc/xive.h      |  15 +++
> >>  include/hw/ppc/xive_regs.h |  24 ++++
> >>  hw/intc/xive.c             | 227 +++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 266 insertions(+)
> >>
> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >> index 74b547707b17..e9b06e75fc1c 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -327,6 +327,10 @@ typedef struct XiveRouterClass {
> >>                     XiveEND *end);
> >>      int (*write_end)(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> >>                       XiveEND *end, uint8_t word_number);
> >> +    int (*get_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> >> +                   XiveNVT *nvt);
> >> +    int (*write_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> >> +                     XiveNVT *nvt, uint8_t word_number);
> >>  } XiveRouterClass;
> >>  
> >>  void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon);
> >> @@ -337,6 +341,11 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t 
> >> end_blk, uint32_t end_idx,
> >>                          XiveEND *end);
> >>  int xive_router_write_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t 
> >> end_idx,
> >>                            XiveEND *end, uint8_t word_number);
> >> +int xive_router_get_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t 
> >> nvt_idx,
> >> +                        XiveNVT *nvt);
> >> +int xive_router_write_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t 
> >> nvt_idx,
> >> +                          XiveNVT *nvt, uint8_t word_number);
> >> +
> >>  
> >>  /*
> >>   * XIVE END ESBs
> >> @@ -393,6 +402,7 @@ typedef struct XiveTCTX {
> >>      qemu_irq    output;
> >>  
> >>      uint8_t     regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
> >> +    uint32_t    hw_cam;
> > 
> > I don't love having this as a separate field.  Since it also appears
> > within the register space, it's kind of redundant. 
> 
> yes.
> 
> > On the other hand,
> > I see that wiring up the property directly to the register space
> > doesn't really work.  Not sure how to deal with that one.
> 
> We could use get/set properties for "hw-cam" to assign WORD2 of the 
> physical ring and exclude it from reset, which makes some sense. The
> test on the PHYS ring in xive_presenter_tctx_match() would also look 
> like the other tests. I think this is better.

Ok sounds good.

> On a related topic, WORD2 of the OS ring is assigned by the hypervisor. 
> For the sPAPR machine, this is done when the sPAPR IRQ backend is 
> reseted. See patch 21 in v6.

Yes, I figured.

[snip]
> >> +/*
> >> + * The thread context register words are in big-endian format.
> >> + */
> >> +static int xive_presenter_tctx_match(XiveTCTX *tctx, uint8_t format,
> >> +                                     uint8_t nvt_blk, uint32_t nvt_idx,
> >> +                                     bool cam_ignore, uint32_t logic_serv)
> >> +{
> >> +    uint32_t cam = xive_nvt_cam_line(nvt_blk, nvt_idx);
> >> +    uint8_t *regs;
> >> +    uint32_t qw3w2;
> >> +    uint32_t qw2w2;
> >> +    uint32_t qw1w2;
> >> +    uint32_t qw0w2;
> >> +
> >> +    /* TODO (PowerNV): ignore low order bits of nvt id */
> >> +
> >> +    regs = &tctx->regs[TM_QW3_HV_PHYS];
> >> +    qw3w2 = be32_to_cpu(*((uint32_t *) &regs[TM_WORD2]));
> > 
> > This is one of the main places we access regs and we have to do
> > horrible casting.  Would it make more sense for it to be a uint32_t
> > array?  Or at least for the local *regs to be.
> 
> The register array is accessed by byte (patch 9) for the first two 
> words and by word for WORD2. I don't see any good solution apart 
> from a helper routine maybe : 
> 
>   static inline uint32_t xive_tctx_word2(int8_t *regs)
>   {
>       return be32_to_cpu(*((uint32_t *) &regs[TM_WORD2]));
>   }
> 
> which I need for xive_tctx_ring_print() also.

Well, you could at least make the regs local variable a uint32_t *,
since you're only accessing the 32-bit parts of the ring in this
function.

Alternatively, you could represent the regs not with a plain array,
but a structure which has some u8 fields and some u32 fields.

>  
> >> +    regs = &tctx->regs[TM_QW2_HV_POOL];
> >> +    qw2w2 = be32_to_cpu(*((uint32_t *) &regs[TM_WORD2]));
> >> +    regs = &tctx->regs[TM_QW1_OS];
> >> +    qw1w2 = be32_to_cpu(*((uint32_t *) &regs[TM_WORD2]));
> >> +    regs = &tctx->regs[TM_QW0_USER];
> >> +    qw0w2 = be32_to_cpu(*((uint32_t *) &regs[TM_WORD2]));
> >> +
> >> +    if (format == 0) {
> >> +        /* F=0 & i=1: Logical server notification */
> > 
> > I'm guessing the i=1 is the cam_ignore==true check?  Maybe put this
> > comment inside the if block to make that clearer.
> 
> yes. 
> 
> > 
> >> +        if (cam_ignore == true) {
> >> +            qemu_log_mask(LOG_UNIMP, "XIVE: no support for LS NVT 
> >> %x/%x\n",
> >> +                          nvt_blk, nvt_idx);
> >> +             return -1;
> >> +        }
> >> +
> >> +        /* F=0 & i=0: Specific NVT notification */
> >> +
> >> +        /* PHYS ring */
> >> +        if ((qw3w2 & TM_QW3W2_VT) &&
> >> +            tctx->hw_cam == hw_cam_line(nvt_blk, nvt_idx)) {
> >> +            return TM_QW3_HV_PHYS;
> >> +        }
> >> +
> >> +        /* HV POOL ring */
> >> +        if ((qw2w2 & TM_QW2W2_VP) &&
> >> +            cam == GETFIELD(TM_QW2W2_POOL_CAM, qw2w2)) {
> > 
> > Does that need to be a GETFIELD_BE32?
> 
> the qw[0123]w2 variables have been byteswapped already. But, that might
> not be a good idea. in that case, we should byteswap the V[TPOU] bit value 
> instead ? What's your opinion.

Actually I think it's fine as it is, I was just missing that the
locals were already byteswapped values.  As a rule I dislike
byteswapping constants rather than the variable part (at least partly
because it's a pattern that *only* works for bitwise operations).

> 
> we would get rid of the be32_to_cpu() above 
> 
> > 
> >> +            return TM_QW2_HV_POOL;
> >> +        }
> >> +
> >> +        /* OS ring */
> >> +        if ((qw1w2 & TM_QW1W2_VO) &&
> >> +            cam == GETFIELD(TM_QW1W2_OS_CAM, qw1w2)) {
> > 
> > And here.
> > 
> >> +            return TM_QW1_OS;
> >> +        }
> >> +    } else {
> >> +        /* F=1 : User level Event-Based Branch (EBB) notification */
> >> +
> >> +        /* USER ring */
> >> +        if  ((qw1w2 & TM_QW1W2_VO) &&
> >> +             (cam == GETFIELD(TM_QW1W2_OS_CAM, qw1w2)) &&
> > 
> > And here.
> > 
> >> +             (qw0w2 & TM_QW0W2_VU) &&
> >> +             (logic_serv == GETFIELD(TM_QW0W2_LOGIC_SERV, qw0w2))) {
> >> +            return TM_QW0_USER;
> >> +        }
> >> +    }
> >> +    return -1;
> >> +}
> >> +
> >> +typedef struct XiveTCTXMatch {
> >> +    XiveTCTX *tctx;
> >> +    uint8_t ring;
> >> +} XiveTCTXMatch;
> >> +
> >> +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)
> >> +{
> >> +    CPUState *cs;
> >> +
> >> +    /* TODO (PowerNV): handle chip_id overwrite of block field for
> >> +     * hardwired CAM compares */
> >> +
> >> +    CPU_FOREACH(cs) {
> >> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> >> +        XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
> >> +        int ring;
> >> +
> >> +        /*
> >> +         * HW checks that the CPU is enabled in the Physical Thread
> >> +         * Enable Register (PTER).
> >> +         */
> >> +
> >> +        /*
> >> +         * Check the thread context CAM lines and record matches. We
> >> +         * will handle CPU exception delivery later
> >> +         */
> >> +        ring = xive_presenter_tctx_match(tctx, format, nvt_blk, nvt_idx,
> >> +                                         cam_ignore, logic_serv);
> >> +        /*
> >> +         * Save the context and follow on to catch duplicates, that we
> >> +         * don't support yet.
> >> +         */
> >> +        if (ring != -1) {
> >> +            if (match->tctx) {
> >> +                qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a 
> >> thread "
> >> +                              "context NVT %x/%x\n", nvt_blk, nvt_idx);
> >> +                return false;
> >> +            }
> >> +
> >> +            match->ring = ring;
> >> +            match->tctx = tctx;
> >> +        }
> >> +    }
> >> +
> >> +    if (!match->tctx) {
> >> +        qemu_log_mask(LOG_UNIMP, "XIVE: NVT %x/%x is not dispatched\n",
> >> +                      nvt_blk, nvt_idx);
> >> +        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.
> >> + *
> >> + * It receives notification requests sent by the IVRE to find one
> >> + * matching NVT (or more) dispatched on the processor threads. In case
> >> + * of a single NVT notification, the process is abreviated and the
> >> + * thread is signaled if a match is found. In case of a logical server
> >> + * notification (bits ignored at the end of the NVT identifier), the
> >> + * IVPE and IVRE select a winning thread using different filters. This
> >> + * involves 2 or 3 exchanges on the PowerBus that the model does not
> >> + * support.
> >> + *
> >> + * The parameters represent what is sent on the PowerBus
> >> + */
> >> +static void xive_presenter_notify(XiveRouter *xrtr, uint8_t format,
> >> +                                  uint8_t nvt_blk, uint32_t nvt_idx,
> >> +                                  bool cam_ignore, uint8_t priority,
> >> +                                  uint32_t logic_serv)
> >> +{
> >> +    XiveNVT nvt;
> >> +    XiveTCTXMatch match = { 0 };
> > 
> > IIUC that's initializing the tctx pointer field of match, so should be
> > NULL, not 0 (yes, technically they're equivalent in C, but using 0 for
> > a pointer is confusing).
> 
> OK. I will clarify.
> 
> > 
> >> +    bool found;
> >> +
> >> +    /* NVT cache lookup */
> >> +    if (xive_router_get_nvt(xrtr, nvt_blk, nvt_idx, &nvt)) {
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: no NVT %x/%x\n",
> >> +                      nvt_blk, nvt_idx);
> >> +        return;
> >> +    }
> >> +
> >> +    if (!xive_nvt_is_valid(&nvt)) {
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: NVT %x/%x is invalid\n",
> >> +                      nvt_blk, nvt_idx);
> >> +        return;
> >> +    }
> >> +
> >> +    found = xive_presenter_match(xrtr, format, nvt_blk, nvt_idx, 
> >> cam_ignore,
> >> +                                 priority, logic_serv, &match);
> >> +    if (found) {
> >> +        return;
> >> +    }
> >> +
> >> +    /* If no matching NVT is dispatched on a HW thread :
> >> +     * - update the NVT structure if backlog is activated
> >> +     * - escalate (ESe PQ bits and EAS in w4-5) if escalation is
> >> +     *   activated
> >> +     */
> >> +}
> >> +
> >>  /*
> >>   * An END trigger can come from an event trigger (IPI or HW) or from
> >>   * another chip. We don't model the PowerBus but the END trigger
> >> @@ -1047,6 +1266,14 @@ static void xive_router_end_notify(XiveRouter 
> >> *xrtr, uint8_t end_blk,
> >>      /*
> >>       * Follows IVPE notification
> >>       */
> >> +    xive_presenter_notify(xrtr, format,
> >> +                          GETFIELD_BE32(END_W6_NVT_BLOCK, end.w6),
> >> +                          GETFIELD_BE32(END_W6_NVT_INDEX, end.w6),
> >> +                          GETFIELD_BE32(END_W7_F0_IGNORE, end.w7),
> >> +                          priority,
> >> +                          GETFIELD_BE32(END_W7_F1_LOG_SERVER_ID, end.w7));
> >> +
> >> +    /* TODO: Auto EOI. */
> >>  }
> >>  
> >>  static void xive_router_notify(XiveNotifier *xn, uint32_t lisn)
> > 
> 

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