On Tue, May 30, 2017 at 05:17:01PM +0100, Marc Zyngier wrote:
> 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:
> >>> +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.
Fair enough, thanks for taking a look anyhow.
Thanks,
Mark.
