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

[Cédric Le Goater]

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.

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

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