Re: [PATCH 07/31] KVM: arm/arm64: vgic-v3: Add accessors for the ICH_APxRn_EL2 registers

[Marc Zyngier <marc.zyngier@arm.com>]

On 03/05/17 16:58, Marc Zyngier wrote:
> On 03/05/17 16:32, Mark Rutland wrote:
>> On Wed, May 03, 2017 at 11:45:42AM +0100, Marc Zyngier wrote:
>>> As we're about to access the Active Priority registers a lot more,
>>> let's define accessors that take the register number as a parameter.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>>  virt/kvm/arm/hyp/vgic-v3-sr.c | 116 ++++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 100 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
>>> index 32c3295929b0..990d9d1e85d0 100644
>>> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
>>> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
>>> @@ -118,6 +118,90 @@ static void __hyp_text __gic_v3_set_lr(u64 val, int lr)
>>>  	}
>>>  }
>>>  
>>> +static void __hyp_text __vgic_v3_write_ap0rn(u32 val, int n)
>>> +{
>>> +	switch (n) {
>>> +	case 0:
>>> +		write_gicreg(val, ICH_AP0R0_EL2);
>>> +		break;
>>> +	case 1:
>>> +		write_gicreg(val, ICH_AP0R1_EL2);
>>> +		break;
>>> +	case 2:
>>> +		write_gicreg(val, ICH_AP0R2_EL2);
>>> +		break;
>>> +	case 3:
>>> +		write_gicreg(val, ICH_AP0R3_EL2);
>>> +		break;
>>> +	}
>>
>> Is there any way we can get a build or runtime failure for an
>> out-of-bounds n value?
> 
> I'd rather avoid runtime failure on this path, because that's pretty
> terminal. Build-time is a possibility, to some extent.
> 
>>
>>> +}
>>
>> Given this is used with a constant n, you could make this:
>>
>> #define __vgic_v3_write_ap0rn(v, n) \
>> 	write_gicreg(v, ICH_AP0R##n##_EL2)
>>
>> ... which should also give you a warning for an out-of-bounds n.
>>
>> Similar could apply for the other helpers here.
>>
>> That would require some function -> macro conversion in later patches
>> though, so I can understand if you're not keen on that.
> 
> I don't mind reworking this if that makes it safer. But the real problem
> is that the register number and the group are not necessarily constants
> (see how this is used in __vgic_v3_get_highest_active_priority).
> 
> I'll have a look at how I can make that look a bit better.

So I had another look at that, and I'm not sure there is any way to make
it really nicer, other than expanding all of the apxrn accessors to deal
with non constant x and n (which makes the code look really awful).

I'm pretty confident that it is nigh impossible to get x or n out of
bounds so unless someone shouts, I plan on keeping this code mostly
unchanged for the next repost.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...