Re: [PATCH] ppc/xive: Save/restore state of the External interrupt

[Cédric Le Goater]

> > > +{
> > > +    uint8_t *regs = &tctx->regs[TM_QW1_OS];
> > > +
> > > +    /*
> > > +     * Used when pulling an OS context.
> > > +     * Lower the External interrupt if it's pending. It is necessary
> > > +     * to avoid catching it in the hypervisor context. It should be
> > > +     * raised again when re-pushing the OS context.
> > > +     */
> > > +    if (regs[TM_PIPR] < regs[TM_CPPR]) {
> >
> > This seems useless. Since we want the signal down always.
>
> Agreed the test is ugly though it works and avoid calling qemu_irq_lower() when 
> it's not needed. I'll check whether qemu offers a way to test the state of a 
> signal (which is what we really want) otherwise I'll always lower the signal 
> unconditionnaly.

Not really, it's a function call. or test the NSR may be ?

Just lower it, I guess. I don't think this will impact much performance.

[ ... ]

> > >       return qw1w2;
> > >   }
> > > @@ -413,10 +430,13 @@ static void xive_tctx_need_resend(XiveRouter *xrtr, 
> > > XiveTCTX *tctx,
> > >           /* Reset the NVT value */
> > >           nvt.w4 = xive_set_field32(NVT_W4_IPB, nvt.w4, 0);
> > >           xive_router_write_nvt(xrtr, nvt_blk, nvt_idx, &nvt, 4);
> > > -
> > > -        /* Merge in current context */
> > > -        xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb);
> > >       }
> > > +    /*
> > > +     * Always merge in current context. Even if there's no escalation,
> > > +     * it will check with the IPB value restored before pushing the OS
> > > +     * context and set the External interrupt signal if needed.
> > > +     */
> > > +    xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb);
> >
> > That's another patch and I am not sure it is needed.
> >
> > An IPB value is recorded in the NVT when an interrupt is delivered
> > while a vCPU is not dispatched. With this change, you would force
> > the update in any case when the context is pushed.
> >
> > Is that to close a potential race window on an interrupt being
> > delivered to a vCPU just before it is dispatched by the HV ?
>
>
> Not quite, there's no race involved. Consider the following the scenario that I 
> can hit fairly easily:
>
> 1. an External interrupt is raised while the guest is on the CPU. We now have 
> IPB!=0 and PIPR!=0
>
> 2. the guest enters the interrupt handler but before it can ack the interrupt (special mmio read "TM_SPC_ACK_OS_REG"), another hypervisor interrupt is raised. For example the Hypervisor Decrementer (but I've also hit it with others). The hypervisor interrupt is delivered immediately in that case and forces a guest exit, so that the hypervisor can process the hypervisor interrupt. So we leave the guest and pull the OS context with IPB and PIPR!=0. 
>
> That's the state which is saved, either by KVM (P9)) 

yes.

> or by the hw (P10 with vp-save-restore).

yes.
 
> 3. Some time later, we enter the guest again and restore the interrupt state, 
> either from KVM (P9) or automatically (P10). So we call:
>
> xive_tm_push_os_ctx()
>    --> xive_tctx_need_resend()
>
> In xive_tctx_need_resend(), we check the NVT to see if we had an escalation. If 
> we do, then we update the IPB and PIPR and we are fine.
> But if we don't have an escalation, we don't call xive_tctx_ipb_update(). The 
> IPB value will be correct because it was just restored, but we won't recompute 
> the PIPR register.

Indeed.

> So it seems to me that irrespective of the separate issue of lowering/raising 
> the External interrupt signal when pulling/pushing an OS context, we already 
> need to always go through xive_tctx_ipb_update() to recompute the PIPR (and 
> it's agreed that's a reason enough to be a separate patch).

I think you just wrote the commit log :)

> Now, if on top of that, we add the fact that we may need to raise the External 
> interrupt signal when pushing an OS context, then calling 
> xive_tctx_ipb_update() is our natural way of doing that. So that's a second 
> reason where it helps.

I would be interested to know if you can start an emulated QEMU PowerNV
system (2cpus) with a KVM guest (1 vcpu) and sustain some network load
host<->guest with a ping -f for instance.

Thanks,

C.


> > >   }
> > >   /*
> > > diff --git a/hw/intc/xive2.c b/hw/intc/xive2.c
> > > index 3aff42a69e..b7f1917cd2 100644
> > > --- a/hw/intc/xive2.c
> > > +++ b/hw/intc/xive2.c
> > > @@ -269,6 +269,7 @@ uint64_t xive2_tm_pull_os_ctx(XivePresenter *xptr, XiveTCTX 
> > > *tctx,
> > >           xive2_tctx_save_os_ctx(xrtr, tctx, nvp_blk, nvp_idx);
> > >       }
> > > +    xive_tctx_eval_ext_signal(tctx);
> > >       return qw1w2;
> > >   }
> > > @@ -316,7 +317,6 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, 
> > > XiveTCTX *tctx,
> > >   {
> > >       Xive2Nvp nvp;
> > >       uint8_t ipb;
> > > -    uint8_t cppr = 0;
> > >       /*
> > >        * Grab the associated thread interrupt context registers in the
> > > @@ -337,7 +337,7 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, 
> > > XiveTCTX *tctx,
> > >       /* Automatically restore thread context registers */
> > >       if (xive2_router_get_config(xrtr) & XIVE2_VP_SAVE_RESTORE &&
> > >           do_restore) {
> > > -        cppr = xive2_tctx_restore_os_ctx(xrtr, tctx, nvp_blk, nvp_idx, &nvp);
> > > +        xive2_tctx_restore_os_ctx(xrtr, tctx, nvp_blk, nvp_idx, &nvp);
> > >       }
> > >       ipb = xive_get_field32(NVP2_W2_IPB, nvp.w2);
> > > @@ -346,10 +346,12 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, 
> > > XiveTCTX *tctx,
> > >           xive2_router_write_nvp(xrtr, nvp_blk, nvp_idx, &nvp, 2);
> > >       }
> > > -    /* An IPB or CPPR change can trigger a resend */
> > > -    if (ipb || cppr) {
> > > -        xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb);
> > > -    }
> > > +    /*
> > > +     * Always merge in current context. Even if there's no escalation,
> > > +     * it will check with the IPB value restored and set the External
> > > +     * interrupt signal if needed.
> > > +     */
> > > +    xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb);
> >
> >
> > I guess this fine. The test on the cppr is just an optimisation, if I am
> > correct. But it needs to be in another patch.
> >
> > Same previous comment for ibp.
>
>
> The scenario I described above still stands on P10. In which case checking the 
> CPPR value becomes a moot point if we always call xive_tctx_ipb_update(). And 
> the compiler yells if it's unused.
> Thanks!
>
>    Fred
>
>
> > Thanks,
> >
> > C.
> >
> > >   }
> > >   /*
> > > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> > > index 126e4e2c3a..91512ed5e6 100644
> > > --- a/include/hw/ppc/xive.h
> > > +++ b/include/hw/ppc/xive.h
> > > @@ -527,6 +527,7 @@ Object *xive_tctx_create(Object *cpu, XivePresenter *xptr, 
> > > Error **errp);
> > >   void xive_tctx_reset(XiveTCTX *tctx);
> > >   void xive_tctx_destroy(XiveTCTX *tctx);
> > >   void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t ipb);
> > > +void xive_tctx_eval_ext_signal(XiveTCTX *tctx);
> > >   /*
> > >    * KVM XIVE device helpers