Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 17 Nov 2023, Federico Serafini wrote:
> On 20/10/23 08:35, Jan Beulich wrote:
> > On 19.10.2023 18:26, Stefano Stabellini wrote:
> > > On Thu, 19 Oct 2023, Jan Beulich wrote:
> > > > On 19.10.2023 00:43, Stefano Stabellini wrote:
> > > > > On Mon, 16 Oct 2023, Jan Beulich wrote:
> > > > > > On 03.10.2023 17:24, Federico Serafini wrote:
> > > > > > > --- a/xen/arch/x86/mm.c
> > > > > > > +++ b/xen/arch/x86/mm.c
> > > > > > > @@ -5901,17 +5901,17 @@ int destroy_xen_mappings(unsigned long s,
> > > > > > > unsigned long e)
> > > > > > >    * a problem.
> > > > > > >    */
> > > > > > >   void init_or_livepatch modify_xen_mappings_lite(
> > > > > > > -    unsigned long s, unsigned long e, unsigned int _nf)
> > > > > > > +    unsigned long s, unsigned long e, unsigned int nf)
> > > > > > >   {
> > > > > > > -    unsigned long v = s, fm, nf;
> > > > > > > +    unsigned long v = s, fm, flags;
> > > > > > 
> > > > > > While it looks correct, I consider this an unacceptably dangerous
> > > > > > change: What if by the time this is to be committed some new use of
> > > > > > the local "nf" appears, without resulting in fuzz while applying the
> > > > > > patch? Imo this needs doing in two steps: First nf -> flags, then
> > > > > > _nf -> nf.
> > > > > 
> > > > > Wouldn't it be sufficient for the committer to pay special attention
> > > > > when committing this patch? We are in code freeze anyway, the rate of
> > > > > changes affecting staging is low.
> > > > 
> > > > Any kind of risk excludes a patch from being a 4.18 candidate, imo.
> > > 
> > > I agree on that. I think it is best to commit it for 4.19 when the tree
> > > opens.
> > > 
> > > 
> > > > That was the case in early RCs already, and is even more so now. Paying
> > > > special attention is generally a possibility, yet may I remind you that
> > > > committing in general is intended to be a purely mechanical operation?
> > > 
> > > Sure, and I am not asking for a general process change. I am only
> > > suggesting that this specific concern on this patch is best solved in
> > > the simplest way: by a committer making sure the patch is correct on
> > > commit. It is meant to save time for everyone.
> > > 
> > > Jan, if you are OK with it, we could just trust you to commit it the
> > > right away as the earliest opportunity.
> > 
> > If you can get Andrew or Roger to ack this patch in its present shape,
> > I won't stand in the way. I'm not going to ack the change without the
> > indicated split.
> 
> I'll propose a new patch series where changes are splitted as indicated.
> I also noticed a discrepancy between Arm and x86 in the name of the
> last parameter of xenmem_add_to_physmap_one().
> Do you have any suggestions about how to solve it?
> If we reach an agreement, then I can put the changes related to the mm module
> in a single patch.

I think it should be "gfn"