The missing header included by this patch provides declarations for
the functions 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
that are defined in the source file. This also resolves violations
of MISRA C:2012 Rule 8.4.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs")
Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
Fixes: 9864841914c2 ("x86/vm_event: add support for VM_EVENT_REASON_INTERRUPT")
---
xen/arch/x86/vm_event.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 7027c08a926b..499b6b349d79 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -20,6 +20,7 @@
#include <xen/sched.h>
#include <xen/mem_access.h>
+#include <xen/vm_event.h>
#include <asm/vm_event.h>
/* Implicitly serialized by the domctl lock. */
--
2.34.1CC-ing the missing maintainers here, both from x86 and vm_event -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
On 11.08.2023 09:19, Nicola Vetrini wrote:
> The missing header included by this patch provides declarations for
> the functions 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
> that are defined in the source file. This also resolves violations
> of MISRA C:2012 Rule 8.4.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs")
> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
> Fixes: 9864841914c2 ("x86/vm_event: add support for VM_EVENT_REASON_INTERRUPT")
It's hard to see how it can be three commit here. The oldest one is at
fault, I would say.
Also please remember to Cc maintainers.
Jan
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -20,6 +20,7 @@
>
> #include <xen/sched.h>
> #include <xen/mem_access.h>
> +#include <xen/vm_event.h>
> #include <asm/vm_event.h>
>
> /* Implicitly serialized by the domctl lock. */
On 14/08/2023 09:39, Jan Beulich wrote:
> On 11.08.2023 09:19, Nicola Vetrini wrote:
>> The missing header included by this patch provides declarations for
>> the functions
>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>> that are defined in the source file. This also resolves violations
>> of MISRA C:2012 Rule 8.4.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs
>> and p2m_vm_event_fill_regs")
>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>> Fixes: 9864841914c2 ("x86/vm_event: add support for
>> VM_EVENT_REASON_INTERRUPT")
>
> It's hard to see how it can be three commit here. The oldest one is at
> fault, I would say.
Since the patch is concerned with more than one function then in a sense
I agree
with you (the headers should have been included in the proper way the
first time around), but
then more definitions have been added by adc75eba8b15 and 9864841914c2,
and these should have
triggered a refactoring too. I can leave just 975efd3baa8d in the Fixes
if the preferred way is to list just the first problematic commit
(perhaps with a little explanation after --- ).
>
> Also please remember to Cc maintainers.
Yes, sorry. I must have forgotten to run add_maintainers.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Hi,
On 14/08/2023 11:33, Nicola Vetrini wrote:
> On 14/08/2023 09:39, Jan Beulich wrote:
>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>> The missing header included by this patch provides declarations for
>>> the functions
>>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>>> that are defined in the source file. This also resolves violations
>>> of MISRA C:2012 Rule 8.4.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs
>>> and p2m_vm_event_fill_regs")
>>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>>> Fixes: 9864841914c2 ("x86/vm_event: add support for
>>> VM_EVENT_REASON_INTERRUPT")
>>
>> It's hard to see how it can be three commit here. The oldest one is at
>> fault, I would say.
>
> Since the patch is concerned with more than one function then in a sense
> I agree
> with you (the headers should have been included in the proper way the
> first time around), but
> then more definitions have been added by adc75eba8b15 and 9864841914c2,
> and these should have
> triggered a refactoring too. I can leave just 975efd3baa8d in the Fixes
> if the preferred way is to list just the first problematic commit
> (perhaps with a little explanation after --- ).
To be honest, I don't exactly see the value of using Fixes tag for those
patches. I agree they are technically issues, but they are unlikely
going to be backported.
So if it were me, I would just drop all the Fixes tags for missing
includes unless there is an actual bug associated
with them (e.g. a caller was miscalling the function because the
prototype was incorrect).
Cheers,
--
Julien Grall
Hi,
On 14/08/2023 13:01, Julien Grall wrote:
> Hi,
>
> On 14/08/2023 11:33, Nicola Vetrini wrote:
>> On 14/08/2023 09:39, Jan Beulich wrote:
>>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>>> The missing header included by this patch provides declarations for
>>>> the functions
>>>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>>>> that are defined in the source file. This also resolves violations
>>>> of MISRA C:2012 Rule 8.4.
>>>>
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs
>>>> and p2m_vm_event_fill_regs")
>>>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>>>> Fixes: 9864841914c2 ("x86/vm_event: add support for
>>>> VM_EVENT_REASON_INTERRUPT")
>>>
>>> It's hard to see how it can be three commit here. The oldest one is
>>> at
>>> fault, I would say.
>>
>> Since the patch is concerned with more than one function then in a
>> sense I agree
>> with you (the headers should have been included in the proper way the
>> first time around), but
>> then more definitions have been added by adc75eba8b15 and
>> 9864841914c2, and these should have
>> triggered a refactoring too. I can leave just 975efd3baa8d in the
>> Fixes if the preferred way is to list just the first problematic
>> commit (perhaps with a little explanation after --- ).
>
> To be honest, I don't exactly see the value of using Fixes tag for
> those patches. I agree they are technically issues, but they are
> unlikely going to be backported.
>
> So if it were me, I would just drop all the Fixes tags for missing
> includes unless there is an actual bug associated
> with them (e.g. a caller was miscalling the function because the
> prototype was incorrect).
>
> Cheers,
Adding those tags for this kind of situation was requested on the
previous discussion [1],
so in this series I kept the same strategy (though probably here I put
too many of them).
[1]
https://lore.kernel.org/xen-devel/cfbc7569-3714-2200-054c-49ba593d6903@suse.com/
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Hi Nicola,
On 14/08/2023 13:53, Nicola Vetrini wrote:
> On 14/08/2023 13:01, Julien Grall wrote:
>> Hi,
>>
>> On 14/08/2023 11:33, Nicola Vetrini wrote:
>>> On 14/08/2023 09:39, Jan Beulich wrote:
>>>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>>>> The missing header included by this patch provides declarations for
>>>>> the functions
>>>>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>>>>> that are defined in the source file. This also resolves violations
>>>>> of MISRA C:2012 Rule 8.4.
>>>>>
>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs
>>>>> and p2m_vm_event_fill_regs")
>>>>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>>>>> Fixes: 9864841914c2 ("x86/vm_event: add support for
>>>>> VM_EVENT_REASON_INTERRUPT")
>>>>
>>>> It's hard to see how it can be three commit here. The oldest one is at
>>>> fault, I would say.
>>>
>>> Since the patch is concerned with more than one function then in a
>>> sense I agree
>>> with you (the headers should have been included in the proper way the
>>> first time around), but
>>> then more definitions have been added by adc75eba8b15 and
>>> 9864841914c2, and these should have
>>> triggered a refactoring too. I can leave just 975efd3baa8d in the
>>> Fixes if the preferred way is to list just the first problematic
>>> commit (perhaps with a little explanation after --- ).
>>
>> To be honest, I don't exactly see the value of using Fixes tag for
>> those patches. I agree they are technically issues, but they are
>> unlikely going to be backported.
>>
>> So if it were me, I would just drop all the Fixes tags for missing
>> includes unless there is an actual bug associated
>> with them (e.g. a caller was miscalling the function because the
>> prototype was incorrect).
>>
>> Cheers,
>
> Adding those tags for this kind of situation was requested on the
> previous discussion [1],
> so in this series I kept the same strategy (though probably here I put
> too many of them).
I disagree with the suggestion made. They are just noise for this sort
of patch and require extra digging (I assume you spent 10-15min to
figure out the multiple fixes) for a limited benefits (I don't expect
anyone to backport the patches).
Cheers,
--
Julien Grall
On 14.08.2023 15:06, Julien Grall wrote:
> Hi Nicola,
>
> On 14/08/2023 13:53, Nicola Vetrini wrote:
>> On 14/08/2023 13:01, Julien Grall wrote:
>>> Hi,
>>>
>>> On 14/08/2023 11:33, Nicola Vetrini wrote:
>>>> On 14/08/2023 09:39, Jan Beulich wrote:
>>>>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>>>>> The missing header included by this patch provides declarations for
>>>>>> the functions
>>>>>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>>>>>> that are defined in the source file. This also resolves violations
>>>>>> of MISRA C:2012 Rule 8.4.
>>>>>>
>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs
>>>>>> and p2m_vm_event_fill_regs")
>>>>>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>>>>>> Fixes: 9864841914c2 ("x86/vm_event: add support for
>>>>>> VM_EVENT_REASON_INTERRUPT")
>>>>>
>>>>> It's hard to see how it can be three commit here. The oldest one is at
>>>>> fault, I would say.
>>>>
>>>> Since the patch is concerned with more than one function then in a
>>>> sense I agree
>>>> with you (the headers should have been included in the proper way the
>>>> first time around), but
>>>> then more definitions have been added by adc75eba8b15 and
>>>> 9864841914c2, and these should have
>>>> triggered a refactoring too. I can leave just 975efd3baa8d in the
>>>> Fixes if the preferred way is to list just the first problematic
>>>> commit (perhaps with a little explanation after --- ).
>>>
>>> To be honest, I don't exactly see the value of using Fixes tag for
>>> those patches. I agree they are technically issues, but they are
>>> unlikely going to be backported.
>>>
>>> So if it were me, I would just drop all the Fixes tags for missing
>>> includes unless there is an actual bug associated
>>> with them (e.g. a caller was miscalling the function because the
>>> prototype was incorrect).
>>>
>>> Cheers,
>>
>> Adding those tags for this kind of situation was requested on the
>> previous discussion [1],
>> so in this series I kept the same strategy (though probably here I put
>> too many of them).
>
> I disagree with the suggestion made. They are just noise for this sort
> of patch and require extra digging (I assume you spent 10-15min to
> figure out the multiple fixes) for a limited benefits (I don't expect
> anyone to backport the patches).
While I agree that Fixes: is primarily for marking of backporting
candidates, I don't think this is its exclusive purpose. As far as I'm
concerned, it also aids review in the specific cases here (this isn't
a commonly occurring aspect, though, I agree). Yet the primary reason
I asked for them to be added is because each of the omissions is an
at least latent bug.
Jan
Hi Jan,
On 14/08/2023 14:31, Jan Beulich wrote:
> On 14.08.2023 15:06, Julien Grall wrote:
>> Hi Nicola,
>>
>> On 14/08/2023 13:53, Nicola Vetrini wrote:
>>> On 14/08/2023 13:01, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 14/08/2023 11:33, Nicola Vetrini wrote:
>>>>> On 14/08/2023 09:39, Jan Beulich wrote:
>>>>>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>>>>>> The missing header included by this patch provides declarations for
>>>>>>> the functions
>>>>>>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>>>>>>> that are defined in the source file. This also resolves violations
>>>>>>> of MISRA C:2012 Rule 8.4.
>>>>>>>
>>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>>>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs
>>>>>>> and p2m_vm_event_fill_regs")
>>>>>>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>>>>>>> Fixes: 9864841914c2 ("x86/vm_event: add support for
>>>>>>> VM_EVENT_REASON_INTERRUPT")
>>>>>>
>>>>>> It's hard to see how it can be three commit here. The oldest one is at
>>>>>> fault, I would say.
>>>>>
>>>>> Since the patch is concerned with more than one function then in a
>>>>> sense I agree
>>>>> with you (the headers should have been included in the proper way the
>>>>> first time around), but
>>>>> then more definitions have been added by adc75eba8b15 and
>>>>> 9864841914c2, and these should have
>>>>> triggered a refactoring too. I can leave just 975efd3baa8d in the
>>>>> Fixes if the preferred way is to list just the first problematic
>>>>> commit (perhaps with a little explanation after --- ).
>>>>
>>>> To be honest, I don't exactly see the value of using Fixes tag for
>>>> those patches. I agree they are technically issues, but they are
>>>> unlikely going to be backported.
>>>>
>>>> So if it were me, I would just drop all the Fixes tags for missing
>>>> includes unless there is an actual bug associated
>>>> with them (e.g. a caller was miscalling the function because the
>>>> prototype was incorrect).
>>>>
>>>> Cheers,
>>>
>>> Adding those tags for this kind of situation was requested on the
>>> previous discussion [1],
>>> so in this series I kept the same strategy (though probably here I put
>>> too many of them).
>>
>> I disagree with the suggestion made. They are just noise for this sort
>> of patch and require extra digging (I assume you spent 10-15min to
>> figure out the multiple fixes) for a limited benefits (I don't expect
>> anyone to backport the patches).
>
> While I agree that Fixes: is primarily for marking of backporting
> candidates, I don't think this is its exclusive purpose. As far as I'm
> concerned, it also aids review in the specific cases here (this isn't
> a commonly occurring aspect, though, I agree). Yet the primary reason
> I asked for them to be added is because each of the omissions is an
> at least latent bug.
I agree they are latent bugs. But the question is whether the time
looking up the histoy is well spent?
In this situation, we have a lot of of similar patches where we need to
add Fixes. So there is quite a bit of time sinked into finding the
original commit message. Is it valuable? I don't believe so.
A least for Arm, I would not require the tag to be added for any missing
prototypes.
Cheers,
--
Julien Grall
On Fri, 11 Aug 2023, Nicola Vetrini wrote:
> The missing header included by this patch provides declarations for
> the functions 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
> that are defined in the source file. This also resolves violations
> of MISRA C:2012 Rule 8.4.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs")
> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
> Fixes: 9864841914c2 ("x86/vm_event: add support for VM_EVENT_REASON_INTERRUPT")
> ---
> xen/arch/x86/vm_event.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> index 7027c08a926b..499b6b349d79 100644
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -20,6 +20,7 @@
>
> #include <xen/sched.h>
> #include <xen/mem_access.h>
> +#include <xen/vm_event.h>
> #include <asm/vm_event.h>
>
> /* Implicitly serialized by the domctl lock. */
I think the problem here is that ./arch/x86/include/asm/vm_event.h,
differently from ./arch/arm/include/asm/vm_event.h, doesn't #include
<xen/vm_event.h>
>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c >> index 7027c08a926b..499b6b349d79 100644 >> --- a/xen/arch/x86/vm_event.c >> +++ b/xen/arch/x86/vm_event.c >> @@ -20,6 +20,7 @@ >> >> #include <xen/sched.h> >> #include <xen/mem_access.h> >> +#include <xen/vm_event.h> >> #include <asm/vm_event.h> >> >> /* Implicitly serialized by the domctl lock. */ > > I think the problem here is that ./arch/x86/include/asm/vm_event.h, > differently from ./arch/arm/include/asm/vm_event.h, doesn't #include > <xen/vm_event.h> I see your point. Do you think it would be better to include xen/vm_event.h in asm/vm_event.h for x86 or move the inclusion of xen/vm_event.h for arm to the source file, as done in the patch? -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
Hi, On 12/08/2023 10:53, Nicola Vetrini wrote: > >>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c >>> index 7027c08a926b..499b6b349d79 100644 >>> --- a/xen/arch/x86/vm_event.c >>> +++ b/xen/arch/x86/vm_event.c >>> @@ -20,6 +20,7 @@ >>> >>> #include <xen/sched.h> >>> #include <xen/mem_access.h> >>> +#include <xen/vm_event.h> >>> #include <asm/vm_event.h> >>> >>> /* Implicitly serialized by the domctl lock. */ >> >> I think the problem here is that ./arch/x86/include/asm/vm_event.h, >> differently from ./arch/arm/include/asm/vm_event.h, doesn't #include >> <xen/vm_event.h> > > I see your point. Do you think it would be better to include xen/vm_event.h > in asm/vm_event.h for x86 or move the inclusion of xen/vm_event.h for > arm to > the source file, as done in the patch? I think it is a bit odd require the C file to include the arch-specific header and the common one. It would be better to include only one. My preference would be to include <asm/...> from <xen/...> and then only include the latter in the C file. Cheers, -- Julien Grall
On 12.08.2023 13:37, Julien Grall wrote: > On 12/08/2023 10:53, Nicola Vetrini wrote: >>>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c >>>> index 7027c08a926b..499b6b349d79 100644 >>>> --- a/xen/arch/x86/vm_event.c >>>> +++ b/xen/arch/x86/vm_event.c >>>> @@ -20,6 +20,7 @@ >>>> >>>> #include <xen/sched.h> >>>> #include <xen/mem_access.h> >>>> +#include <xen/vm_event.h> >>>> #include <asm/vm_event.h> >>>> >>>> /* Implicitly serialized by the domctl lock. */ >>> >>> I think the problem here is that ./arch/x86/include/asm/vm_event.h, >>> differently from ./arch/arm/include/asm/vm_event.h, doesn't #include >>> <xen/vm_event.h> >> >> I see your point. Do you think it would be better to include xen/vm_event.h >> in asm/vm_event.h for x86 or move the inclusion of xen/vm_event.h for >> arm to >> the source file, as done in the patch? > > I think it is a bit odd require the C file to include the arch-specific > header and the common one. It would be better to include only one. > > My preference would be to include <asm/...> from <xen/...> and then only > include the latter in the C file. +1 Jan
© 2016 - 2024 Red Hat, Inc.