[Skiboot] [PATCH] xive/p9: Remove assert from xive_eq_for_target()

[Greg Kurz]

On Fri, 27 Nov 2020 09:32:43 +0100
Cédric Le Goater <clg at kaod.org> wrote:

> On 11/27/20 9:08 AM, Greg Kurz wrote:
> > On Fri, 27 Nov 2020 08:32:12 +0100
> > Cédric Le Goater <clg at kaod.org> wrote:
> > 
> >> XIVE VPs are structures describing the vCPUs of guests. When starting
> >> a guest, these are allocated and enabled and some checks are done on
> >> the location of the associated ENDs, which describe the event
> >> queues. If the block of the VP and the block of the ENDs do not match,
> >> the XIVE driver asserts.
> >>
> >> Unfortunately, there is no way to check that a VP identifier is part
> >> of a VP block that was previously allocated and it is relatively easy
> >> to crash the host with a bogus VP id. That can be done with a QEMU
> >> hack on a machine using vsmt.
> >>
> > 
> > Reported-by: Greg Kurz <groug at kaod.org>
> > 
> > :)
> > 
> >> Simply remove the assert, the OS should gracefully handle the error.
> >>
> > 
> > This seems to be reasonable when xive_eq_for_target() is called from
> > an OPAL call since they'd all return OPAL_PARAMETER to the OS in this
> > case.
> 
> Linux behaves correctly. There are errors but the rollback is correct.
> 

Cool.

>  
> > Some other paths maybe need more care though, eg:
> > 
> > xive_ipi_init()
> >     __xive_set_irq_config()
> >         xive_set_irq_targetting()
> >             xive_eq_for_target()
> > 
> > If xive_eq_for_target() fails to map the target to a valid EQ,
> > this ends up being ignored in xive_ipi_init() with this patch.
> > Is it okay ?
> 
> This is the P8 emulation layer fully handled by OPAL. It the VP
> is bogus, then it means that OPAL just screwed up. 
> 
> And this seems to be the case because it's using cpu->pir and not 
> a vp identifier ... If this is working, it's pure luck. Another 
> good reason to drop support for this emulation which is a source 
> of complexity. It's dropped for P10. 
> 
> See :
> 
>   patchwork.ozlabs.org/patch/1157193  xive/p9: remove XICS emulation
>   patchwork.ozlabs.org/patch/1157182  xive/p9: remove percpu logging
>   patchwork.ozlabs.org/patch/1157172  xive/p9: remove XICS emulation OPAL calls
> 
> I would be very happy to push these again and get rid of 1100 
> lines of useless bringup complexity. 
> 

Hmm... some existing setups might still be using power8 compat mode
for legitimate reasons, eg. workload only certified on POWER8. I'm
not sure you can get rid of the XICS emulation that easily.

> C.
> 
> 
> > 
> >> Signed-off-by: Cédric Le Goater <clg at kaod.org>
> >> ---
> >>  hw/xive.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/xive.c b/hw/xive.c
> >> index 7d4e029f19cb..c442ea5e30ed 100644
> >> --- a/hw/xive.c
> >> +++ b/hw/xive.c
> >> @@ -2152,7 +2152,7 @@ static inline bool xive_eq_for_target(uint32_t target, uint8_t prio,
> >>  	if (eq_blk != vp_blk) {
> >>  		xive_err(x, "eq_blk != vp_blk (%d vs. %d) for target 0x%08x/%d\n",
> >>  			 eq_blk, vp_blk, target, prio);
> >> -		assert(false);
> >> +		return false;
> >>  	}
> >>  
> >>  	if (out_eq_blk)
> > 
>