| Message ID | f303a1782aeb67186c3181e01a51153fdce70cfb.1691676251.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: fix missing headers and static storage duration | expand |
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?
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,
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
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.
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,
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/
CC-ing the missing maintainers here, both from x86 and vm_event
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,
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,
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. */
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(+)