Re: [RFC PATCH v2 08/11] powerpc: Add "mask_lvl" paramater to MASKABLE_* macros

[Madhavan Srinivasan <maddy@linux.vnet.ibm.com>]

On Monday 01 August 2016 10:51 AM, Nicholas Piggin wrote:
> On Mon,  1 Aug 2016 00:36:26 +0530
> Madhavan Srinivasan <maddy@linux.vnet.ibm.com> wrote:
>
>> Make it explicit the interrupt masking level supported
>> by a gievn interrupt handler. Patch correspondingly
>> extends the MASKABLE_* macros with an addition's parameter.
>> "mask_lvl" parameter is passed to SOFTEN_TEST macro to decide
>> on masking the interrupt.
> Hey Madhavan,
>
> It looks like this has worked quite nicely. I think you've
> managed to avoid any additional instructions in fastpaths
> if I'm reading correctly.

Yes. This avoids condition checking for many cases.

>
> I will do a more comprehensive review, but I wanted to ask:
>
>
>> @@ -426,79 +426,81 @@ label##_relon_hv:						\
>>   #define SOFTEN_VALUE_0xe60	PACA_IRQ_HMI
>>   #define SOFTEN_VALUE_0xe62	PACA_IRQ_HMI
>>   
>> -#define __SOFTEN_TEST(h, vec)						\
>> +#define __SOFTEN_TEST(h, vec, mask_lvl)					\
>>   	lbz	r10,PACASOFTIRQEN(r13);					\
>> -	cmpwi	r10,IRQ_DISABLE_LEVEL_LINUX;				\
>> +	andi.	r10,r10,mask_lvl;					\
>>   	li	r10,SOFTEN_VALUE_##vec;					\
>> -	bge	masked_##h##interrupt
>> -#define _SOFTEN_TEST(h, vec)	__SOFTEN_TEST(h, vec)
>> +	bne	masked_##h##interrupt
>> +#define _SOFTEN_TEST(h, vec, mask_lvl)	__SOFTEN_TEST(h, vec, mask_lvl)
> We're talking about IRQ masking levels, but here it looks
> like you're actually treating it as a mask.

Yes. That is true. I started with "level", but then realized
that I am adding more branch condition checks to
retain the PMI as NMI incase.

> I don't have a strong preference. Mask is more flexible, but
> potentially constrained in how many interrupt types it can
> cope with. That said, I doubt we'll need more than 8 mask bits
> considering we've lived with one for years. So perhaps a mask
> is a better choice. Ben, others, any preferences?
>
> We should just use either "mask" or "level" everywhere, depending
> on what we go with.

Yep. will change it.

Maddy

>
> Thanks,
> Nick
>