Re: [XEN PATCH] x86/guest_walk: address violations of MISRA C:2012 Rule 8.3

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

On 28/11/23 14:43, Jan Beulich wrote:
> On 28.11.2023 14:17, Andrew Cooper wrote:
> > On 28/11/2023 1:00 pm, Jan Beulich wrote:
> > > On 28.11.2023 10:46, Federico Serafini wrote:
> > > > Uniform declaration and definition of guest_walk_tables() using
> > > > parameter name "pfec_walk":
> > > > this name highlights the connection with PFEC_* constants and it is
> > > > consistent with the use of the parameter within function body.
> > > > No functional change.
> > > >
> > > > Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> > > I'm curious what other x86 maintainers think. I for one don't like this,
> > > but not enough to object if others are happy. That said, there was earlier
> > > discussion (and perhaps even a patch), yet without a reference I don't
> > > think I can locate this among all the Misra bits and pieces.
> > I looked at this and wanted a bit of time to think.
> >
> > Sadly, this code is half way through some cleanup, which started before
> > speculation and will continue in my copious free time.
> >
> > It's wrong to be passing PFEC_* constants, and that's why I renamed pfec
> > -> walk the last time I was fixing security bugs here  (indeed, passing
> > the wrong constant here *was* the security issue).  I missed the
> > prototype while fixing the implementation.
> >
> > At some point, PFEC_* will no longer be passed in.
> >
> > Therefore I'd far rather this was a one-line change for the declaration
> > changing pfec -> walk.
> So that was what Federico originally had. Did you see my response at
> https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg00122.html ?
> While I certainly agree with your plans (as far as I understand them),
> doing as you suggest would make it harder to spot what values are correct
> to pass to the function today. With a suitable new set of constants and
> perhaps even a proper typedef, such confusion would likely not arise.
Thanks to both for the information.

I take this opportunity to inform that we are really close to the end
with Rule 8.3 for x86, this is the situation:
- do_multicall(), Stefano sent a patch;
- guest_walk_tables(), Andrew will take care of it;
- xenmem_add_to_physmap_one(), this is the last one.

For the latter, I see you (x86) share the declaration with ARM,
where "gfn" is used for the last parameter instead of "gpfn".
Do you agree in changing the name in the definition from "gpfn"
to "gfn"?
If you agree, do you have any suggestions on how to rename
the local variable "gfn"?
Following your suggestions, I can do the renaming in two
steps to prevent bad things to happening.

--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)