[XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
Add missing parameter names and make function declarations and definitions
consistent. No functional change.
Signed-off-by: Federico Serafini
---
Changes in v3:
- used "nf" as parameter name to denote "new flags".
---
Changes in v2:
- propagated changes to the arm code.
---
xen/arch/arm/mm.c | 4 ++--
xen/arch/ppc/mm-radix.c | 2 +-
xen/arch/x86/include/asm/mm.h | 20 ++--
xen/arch/x86/mm.c | 12 ++--
xen/include/xen/mm.h | 16 +---
5 files changed, 28 insertions(+), 26 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index c34cc94c90..484f23140e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1246,12 +1246,12 @@ int destroy_xen_mappings(unsigned long s, unsigned long
e)
return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, 0);
}
-int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
+int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
{
ASSERT(IS_ALIGNED(s, PAGE_SIZE));
ASSERT(IS_ALIGNED(e, PAGE_SIZE));
ASSERT(s <= e);
-return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
+return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, nf);
}
/* Release all __init and __initdata ranges to be reused */
diff --git a/xen/arch/ppc/mm-radix.c b/xen/arch/ppc/mm-radix.c
index 11d0f27b60..daa411a6fa 100644
--- a/xen/arch/ppc/mm-radix.c
+++ b/xen/arch/ppc/mm-radix.c
@@ -271,7 +271,7 @@ void __init setup_initial_pagetables(void)
*/
unsigned long __read_mostly frametable_base_pdx;
-void put_page(struct page_info *p)
+void put_page(struct page_info *page)
{
BUG_ON("unimplemented");
}
diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
index 05dfe35502..a270f8ddd6 100644
--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -406,7 +406,7 @@ void put_page_type(struct page_info *page);
int get_page_type(struct page_info *page, unsigned long type);
int put_page_type_preemptible(struct page_info *page);
int get_page_type_preemptible(struct page_info *page, unsigned long type);
-int put_old_guest_table(struct vcpu *);
+int put_old_guest_table(struct vcpu *v);
int get_page_from_l1e(
l1_pgentry_t l1e, struct domain *l1e_owner, struct domain *pg_owner);
void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner);
@@ -557,7 +557,7 @@ void audit_domains(void);
void make_cr3(struct vcpu *v, mfn_t mfn);
pagetable_t update_cr3(struct vcpu *v);
-int vcpu_destroy_pagetables(struct vcpu *);
+int vcpu_destroy_pagetables(struct vcpu *v);
void *do_page_walk(struct vcpu *v, unsigned long addr);
/* Allocator functions for Xen pagetables. */
@@ -572,20 +572,20 @@ int __sync_local_execstate(void);
/* Arch-specific portion of memory_op hypercall. */
long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
long subarch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
-int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void));
-int compat_subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void));
+int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
+int compat_subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
#define NIL(type) ((type *)-sizeof(type))
#define IS_NIL(ptr) (!((uintptr_t)(ptr) + sizeof(*(ptr
-int create_perdomain_mapping(struct domain *, unsigned long va,
- unsigned int nr, l1_pgentry_t **,
- struct page_info **);
-void destroy_perdomain_mapping(struct domain *, unsigned long va,
+int create_perdomain_mapping(struct domain *d, unsigned long va,
+ unsigned int nr, l1_pgentry_t **pl1tab,
+ struct page_info **ppg);
+void destroy_perdomain_mapping(struct domain *d, unsigned long va,
unsigned int nr);
-void free_perdomain_mappings(struct domain *);
+void free_perdomain_mappings(struct domain *d);
-void __iomem *ioremap_wc(paddr_t, size_t);
+void __iomem *ioremap_wc(paddr_t pa, size_t len);
extern int memory_add(unsigned long spfn, unsigned long epfn, unsigned int
pxm);
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 39544bd9f9..7ae42ac59b 100644
--- 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;
/* Set of valid PTE bits which may be altered. */
#define FLAGS_MASK (_PAGE_NX|_PAGE_DIRTY|_PAGE_ACCESSED|_PAGE_RW|_PAGE_PRESENT)
fm = put_pte_flags(FLAGS_MASK);
-nf = put_pte_flags(_nf & FLAGS_MASK);
+flags = put_pte_flags(nf & FLAGS_MASK);
#unde
Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
On Tue, 3 Oct 2023, Federico Serafini wrote: > Add missing parameter names and make function declarations and definitions > consistent. No functional change. > > Signed-off-by: Federico Serafini Reviewed-by: Stefano Stabellini
Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
Hi, > On Oct 4, 2023, at 04:54, Stefano Stabellini wrote: > > On Tue, 3 Oct 2023, Federico Serafini wrote: >> Add missing parameter names and make function declarations and definitions >> consistent. No functional change. >> >> Signed-off-by: Federico Serafini > > Reviewed-by: Stefano Stabellini Release-acked-by: Henry Wang Kind regards, Henry
Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
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.
Furthermore since you alter the local variable, is there any reason
not to also change it to be "unsigned int", matching the function
argument it's set from?
Yet then - can't we just delete "nf" and rename "_nf" to "nf"? The
function parameter is only used in
nf = put_pte_flags(_nf & FLAGS_MASK);
Jan
Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
On 16/10/23 17:26, 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.
Furthermore since you alter the local variable, is there any reason
not to also change it to be "unsigned int", matching the function
argument it's set from?
If you are referring to the local variable 'flags', it is set using the
value returned from put_pte_flags() which is an intpte_t (typedef for
u64). Am I missing something?
Yet then - can't we just delete "nf" and rename "_nf" to "nf"? The
function parameter is only used in
nf = put_pte_flags(_nf & FLAGS_MASK);
Jan
I thought about it but it will introduce a violation of Rule 17.8:
"A function parameter should not be modified".
It is an advisory rule that is not currently accepted but one day
it may be.
--
Federico Serafini, M.Sc.
Software Engineer, BUGSENG (http://bugseng.com)
Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
On 18.10.2023 12:07, Federico Serafini wrote:
> On 16/10/23 17:26, 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.
>>
>> Furthermore since you alter the local variable, is there any reason
>> not to also change it to be "unsigned int", matching the function
>> argument it's set from?
>
> If you are referring to the local variable 'flags', it is set using the
> value returned from put_pte_flags() which is an intpte_t (typedef for
> u64). Am I missing something?
Oh, I'm sorry, I didn't look there carefully enough. Which means using
_nf and nf here was probably not a good idea in the first place, when
both are of different type and nature.
Jan
Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
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.
Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
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.
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?
Jan
Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
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.
Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
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.
Jan
Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
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.
--
Federico Serafini, M.Sc.
Software Engineer, BUGSENG (http://bugseng.com)
Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
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"
