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(-)
Add missing parameter names and make function declarations and definitions
consistent. No functional change.
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
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);
#undef FLAGS_MASK
- ASSERT(nf & _PAGE_PRESENT);
+ ASSERT(flags & _PAGE_PRESENT);
ASSERT(IS_ALIGNED(s, PAGE_SIZE) && s >= XEN_VIRT_START);
ASSERT(IS_ALIGNED(e, PAGE_SIZE) && e <= XEN_VIRT_END);
@@ -5925,7 +5925,7 @@ void init_or_livepatch modify_xen_mappings_lite(
if ( l2e_get_flags(l2e) & _PAGE_PSE )
{
- l2e_write_atomic(pl2e, l2e_from_intpte((l2e.l2 & ~fm) | nf));
+ l2e_write_atomic(pl2e, l2e_from_intpte((l2e.l2 & ~fm) | flags));
v += 1UL << L2_PAGETABLE_SHIFT;
continue;
@@ -5943,7 +5943,7 @@ void init_or_livepatch modify_xen_mappings_lite(
ASSERT(l1f & _PAGE_PRESENT);
- l1e_write_atomic(pl1e, l1e_from_intpte((l1e.l1 & ~fm) | nf));
+ l1e_write_atomic(pl1e, l1e_from_intpte((l1e.l1 & ~fm) | flags));
v += 1UL << L1_PAGETABLE_SHIFT;
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 8b9618609f..abd09a83d2 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -71,9 +71,10 @@
struct page_info;
-void put_page(struct page_info *);
-bool __must_check get_page(struct page_info *, const struct domain *);
-struct domain *__must_check page_get_owner_and_reference(struct page_info *);
+void put_page(struct page_info *page);
+bool __must_check get_page(struct page_info *page, const struct domain *domain);
+struct domain *__must_check page_get_owner_and_reference(
+ struct page_info *page);
/* Boot-time allocator. Turns into generic allocator after bootstrap. */
void init_boot_pages(paddr_t ps, paddr_t pe);
@@ -110,8 +111,9 @@ int map_pages_to_xen(
unsigned long nr_mfns,
unsigned int flags);
/* Alter the permissions of a range of Xen virtual address space. */
-int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags);
-void modify_xen_mappings_lite(unsigned long s, unsigned long e, unsigned int flags);
+int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf);
+void modify_xen_mappings_lite(unsigned long s, unsigned long e,
+ unsigned int nf);
int destroy_xen_mappings(unsigned long s, unsigned long e);
/* Retrieve the MFN mapped by VA in Xen virtual address space. */
mfn_t xen_map_to_mfn(unsigned long va);
@@ -135,7 +137,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order);
unsigned long avail_domheap_pages_region(
unsigned int node, unsigned int min_width, unsigned int max_width);
unsigned long avail_domheap_pages(void);
-unsigned long avail_node_heap_pages(unsigned int);
+unsigned long avail_node_heap_pages(unsigned int nodeid);
#define alloc_domheap_page(d,f) (alloc_domheap_pages(d,0,f))
#define free_domheap_page(p) (free_domheap_pages(p,0))
unsigned int online_page(mfn_t mfn, uint32_t *status);
@@ -528,7 +530,7 @@ static inline unsigned int get_order_from_pages(unsigned long nr_pages)
return order;
}
-void scrub_one_page(struct page_info *);
+void scrub_one_page(struct page_info *pg);
#ifndef arch_free_heap_page
#define arch_free_heap_page(d, pg) \
--
2.34.1On 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
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.
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
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.
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
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)
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"
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)
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
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 <federico.serafini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Hi, > On Oct 4, 2023, at 04:54, Stefano Stabellini <sstabellini@kernel.org> 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 <federico.serafini@bugseng.com> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
© 2016 - 2024 Red Hat, Inc.