| Message ID | 746a33fff1386b2e76657b5f7cfb31f3b117a1fe.1702310368.git.maria.celeste.cesario@bugseng.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen: address violations of MISRA C:2012 Rule 14.4 | expand |
On 2023-12-13 17:10, Simone Ballarin wrote: > From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> > > The xen sources contain violations of MISRA C:2012 Rule 14.4 whose > headline states: > "The controlling expression of an if statement and the controlling > expression of an iteration-statement shall have essentially Boolean > type". > > Add comparisons to avoid using enum constants as controlling > expressions > to comply with Rule 14.4. > No functional change. > > Signed-off-by: Maria Celeste Cesario > <maria.celeste.cesario@bugseng.com> > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > --- > Changes in v2 > - rename prefix from AMD/IOMMU to x86/IOMMU > - move changes on msi.c and hpet.c in this patch. > --- > xen/arch/x86/hpet.c | 6 +++--- > xen/arch/x86/msi.c | 4 ++-- > xen/drivers/passthrough/amd/iommu_init.c | 4 ++-- > xen/drivers/passthrough/vtd/iommu.c | 4 ++-- > xen/drivers/passthrough/vtd/quirks.c | 2 +- > 5 files changed, 10 insertions(+), 10 deletions(-) > +Stefano Hi all, this patch seems not to have been committed into staging, unlike the other patch from this series. Since these are the only remaining violations for Rule 14.4, then I think these changes could be reviewed. This should apply cleanly on staging, but I can send a rebased version if there are problems. Additionally, despite having patches addressing R14.4 already in staging, the rule itself is not in docs/misra/rules.rst due to concerns on the patch that aimed to add it [1], so it's probably best to send a new version of that as well, if Stefano agrees. [1] https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308301840520.6458@ubuntu-linux-20-04-desktop/
On 05.02.2024 16:36, Nicola Vetrini wrote: > On 2023-12-13 17:10, Simone Ballarin wrote: >> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> >> >> The xen sources contain violations of MISRA C:2012 Rule 14.4 whose >> headline states: >> "The controlling expression of an if statement and the controlling >> expression of an iteration-statement shall have essentially Boolean >> type". >> >> Add comparisons to avoid using enum constants as controlling >> expressions >> to comply with Rule 14.4. >> No functional change. >> >> Signed-off-by: Maria Celeste Cesario >> <maria.celeste.cesario@bugseng.com> >> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >> --- >> Changes in v2 >> - rename prefix from AMD/IOMMU to x86/IOMMU >> - move changes on msi.c and hpet.c in this patch. >> --- >> xen/arch/x86/hpet.c | 6 +++--- >> xen/arch/x86/msi.c | 4 ++-- >> xen/drivers/passthrough/amd/iommu_init.c | 4 ++-- >> xen/drivers/passthrough/vtd/iommu.c | 4 ++-- >> xen/drivers/passthrough/vtd/quirks.c | 2 +- >> 5 files changed, 10 insertions(+), 10 deletions(-) >> > > +Stefano > > Hi all, > > this patch seems not to have been committed into staging, unlike the > other patch from this series. Since these are the only remaining > violations for Rule 14.4, then I think these changes could be reviewed. It's no surprise the change isn't committed yet, when it hasn't had any of the necessary tags. As far as I'm concerned, I seem to recall indicating clearly that I'm not happy with this change, and hence acks would need to come from elsewhere. Jan
On Wed, 13 Dec 2023, Simone Ballarin wrote: > From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> > > The xen sources contain violations of MISRA C:2012 Rule 14.4 whose > headline states: > "The controlling expression of an if statement and the controlling > expression of an iteration-statement shall have essentially Boolean type". > > Add comparisons to avoid using enum constants as controlling expressions > to comply with Rule 14.4. > No functional change. > > Signed-off-by: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> However it needs an ack from an x86 maintainer > --- > Changes in v2 > - rename prefix from AMD/IOMMU to x86/IOMMU > - move changes on msi.c and hpet.c in this patch. > --- > xen/arch/x86/hpet.c | 6 +++--- > xen/arch/x86/msi.c | 4 ++-- > xen/drivers/passthrough/amd/iommu_init.c | 4 ++-- > xen/drivers/passthrough/vtd/iommu.c | 4 ++-- > xen/drivers/passthrough/vtd/quirks.c | 2 +- > 5 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c > index 7be26c6a9b..d1ddc8ddf6 100644 > --- a/xen/arch/x86/hpet.c > +++ b/xen/arch/x86/hpet.c > @@ -279,7 +279,7 @@ static int hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg) > { > ch->msi.msg = *msg; > > - if ( iommu_intremap ) > + if ( iommu_intremap != iommu_intremap_off ) > { > int rc = iommu_update_ire_from_msi(&ch->msi, msg); > > @@ -353,7 +353,7 @@ static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch) > u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx)); > irq_desc_t *desc = irq_to_desc(ch->msi.irq); > > - if ( iommu_intremap ) > + if ( iommu_intremap != iommu_intremap_off ) > { > ch->msi.hpet_id = hpet_blockid; > ret = iommu_setup_hpet_msi(&ch->msi); > @@ -372,7 +372,7 @@ static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch) > ret = __hpet_setup_msi_irq(desc); > if ( ret < 0 ) > { > - if ( iommu_intremap ) > + if ( iommu_intremap != iommu_intremap_off ) > iommu_update_ire_from_msi(&ch->msi, NULL); > return ret; > } > diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c > index 7f8e794254..72dce2e4ab 100644 > --- a/xen/arch/x86/msi.c > +++ b/xen/arch/x86/msi.c > @@ -189,7 +189,7 @@ static int write_msi_msg(struct msi_desc *entry, struct msi_msg *msg) > { > entry->msg = *msg; > > - if ( iommu_intremap ) > + if ( iommu_intremap != iommu_intremap_off ) > { > int rc; > > @@ -555,7 +555,7 @@ int msi_free_irq(struct msi_desc *entry) > destroy_irq(entry[nr].irq); > > /* Free the unused IRTE if intr remap enabled */ > - if ( iommu_intremap ) > + if ( iommu_intremap != iommu_intremap_off ) > iommu_update_ire_from_msi(entry + nr, NULL); > } > > diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c > index 5515cb70fd..e02a09a9a7 100644 > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -1480,7 +1480,7 @@ int __init amd_iommu_init(bool xt) > goto error_out; > } > > - if ( iommu_intremap ) > + if ( iommu_intremap != iommu_intremap_off ) > register_keyhandler('V', &amd_iommu_dump_intremap_tables, > "dump IOMMU intremap tables", 0); > > @@ -1498,7 +1498,7 @@ int __init amd_iommu_init_late(void) > > /* Further initialize the device table(s). */ > pci_init = true; > - if ( iommu_intremap ) > + if ( iommu_intremap != iommu_intremap_off ) > rc = iterate_ivrs_mappings(amd_iommu_setup_device_table); > > for_each_amd_iommu ( iommu ) > diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c > index e13b7d99db..bd6d69a6f5 100644 > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -2543,7 +2543,7 @@ static int __must_check init_vtd_hw(bool resume) > /* > * Enable interrupt remapping > */ > - if ( iommu_intremap ) > + if ( iommu_intremap != iommu_intremap_off ) > { > int apic; > for ( apic = 0; apic < nr_ioapics; apic++ ) > @@ -2559,7 +2559,7 @@ static int __must_check init_vtd_hw(bool resume) > } > } > } > - if ( iommu_intremap ) > + if ( iommu_intremap != iommu_intremap_off ) > { > for_each_drhd_unit ( drhd ) > { > diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c > index 5a56565ea8..950dcd56ef 100644 > --- a/xen/drivers/passthrough/vtd/quirks.c > +++ b/xen/drivers/passthrough/vtd/quirks.c > @@ -392,7 +392,7 @@ void __init platform_quirks_init(void) > map_igd_reg(); > > /* Tylersburg interrupt remap quirk */ > - if ( iommu_intremap ) > + if ( iommu_intremap != iommu_intremap_off ) > tylersburg_intremap_quirk(); > } > > -- > 2.40.0 > >
On Mon, 5 Feb 2024, Jan Beulich wrote: > On 05.02.2024 16:36, Nicola Vetrini wrote: > > On 2023-12-13 17:10, Simone Ballarin wrote: > >> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> > >> > >> The xen sources contain violations of MISRA C:2012 Rule 14.4 whose > >> headline states: > >> "The controlling expression of an if statement and the controlling > >> expression of an iteration-statement shall have essentially Boolean > >> type". > >> > >> Add comparisons to avoid using enum constants as controlling > >> expressions > >> to comply with Rule 14.4. > >> No functional change. > >> > >> Signed-off-by: Maria Celeste Cesario > >> <maria.celeste.cesario@bugseng.com> > >> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > >> --- > >> Changes in v2 > >> - rename prefix from AMD/IOMMU to x86/IOMMU > >> - move changes on msi.c and hpet.c in this patch. > >> --- > >> xen/arch/x86/hpet.c | 6 +++--- > >> xen/arch/x86/msi.c | 4 ++-- > >> xen/drivers/passthrough/amd/iommu_init.c | 4 ++-- > >> xen/drivers/passthrough/vtd/iommu.c | 4 ++-- > >> xen/drivers/passthrough/vtd/quirks.c | 2 +- > >> 5 files changed, 10 insertions(+), 10 deletions(-) > >> > > > > +Stefano > > > > Hi all, > > > > this patch seems not to have been committed into staging, unlike the > > other patch from this series. Since these are the only remaining > > violations for Rule 14.4, then I think these changes could be reviewed. > > It's no surprise the change isn't committed yet, when it hasn't had any > of the necessary tags. As far as I'm concerned, I seem to recall > indicating clearly that I'm not happy with this change, and hence acks > would need to come from elsewhere. Thanks Jan for not blocking it. I gave my reviewed-by, let's see if one of the other x86 maintainers is OK to ack it.
On 07.02.2024 01:51, Stefano Stabellini wrote: > On Wed, 13 Dec 2023, Simone Ballarin wrote: >> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> >> >> The xen sources contain violations of MISRA C:2012 Rule 14.4 whose >> headline states: >> "The controlling expression of an if statement and the controlling >> expression of an iteration-statement shall have essentially Boolean type". >> >> Add comparisons to avoid using enum constants as controlling expressions >> to comply with Rule 14.4. >> No functional change. >> >> Signed-off-by: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> >> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > However it needs an ack from an x86 maintainer And from respective IOMMU ones. Jan
On Wed, Dec 13, 2023 at 05:10:50PM +0100, Simone Ballarin wrote: > From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> > > The xen sources contain violations of MISRA C:2012 Rule 14.4 whose > headline states: > "The controlling expression of an if statement and the controlling > expression of an iteration-statement shall have essentially Boolean type". > > Add comparisons to avoid using enum constants as controlling expressions > to comply with Rule 14.4. If we really want to go this route, we also need to amend the comment in iommu_intremap definition, as it's no longer valid: extern enum __packed iommu_intremap { /* * In order to allow traditional boolean uses of the iommu_intremap * variable, the "off" value has to come first (yielding a value of zero). */ iommu_intremap_off, We no longer allow traditional boolean uses of iommu_intremap. Thanks, Roger.
On 2024-03-12 12:01, Roger Pau Monné wrote: > On Wed, Dec 13, 2023 at 05:10:50PM +0100, Simone Ballarin wrote: >> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> >> >> The xen sources contain violations of MISRA C:2012 Rule 14.4 whose >> headline states: >> "The controlling expression of an if statement and the controlling >> expression of an iteration-statement shall have essentially Boolean >> type". >> >> Add comparisons to avoid using enum constants as controlling >> expressions >> to comply with Rule 14.4. > > If we really want to go this route, we also need to amend the comment > in iommu_intremap definition, as it's no longer valid: > > extern enum __packed iommu_intremap { > /* > * In order to allow traditional boolean uses of the iommu_intremap > * variable, the "off" value has to come first (yielding a value of > zero). > */ > iommu_intremap_off, > > We no longer allow traditional boolean uses of iommu_intremap. > > Thanks, Roger. Noted, thanks.
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c index 7be26c6a9b..d1ddc8ddf6 100644 --- a/xen/arch/x86/hpet.c +++ b/xen/arch/x86/hpet.c @@ -279,7 +279,7 @@ static int hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg) { ch->msi.msg = *msg; - if ( iommu_intremap ) + if ( iommu_intremap != iommu_intremap_off ) { int rc = iommu_update_ire_from_msi(&ch->msi, msg); @@ -353,7 +353,7 @@ static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch) u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx)); irq_desc_t *desc = irq_to_desc(ch->msi.irq); - if ( iommu_intremap ) + if ( iommu_intremap != iommu_intremap_off ) { ch->msi.hpet_id = hpet_blockid; ret = iommu_setup_hpet_msi(&ch->msi); @@ -372,7 +372,7 @@ static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch) ret = __hpet_setup_msi_irq(desc); if ( ret < 0 ) { - if ( iommu_intremap ) + if ( iommu_intremap != iommu_intremap_off ) iommu_update_ire_from_msi(&ch->msi, NULL); return ret; } diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c index 7f8e794254..72dce2e4ab 100644 --- a/xen/arch/x86/msi.c +++ b/xen/arch/x86/msi.c @@ -189,7 +189,7 @@ static int write_msi_msg(struct msi_desc *entry, struct msi_msg *msg) { entry->msg = *msg; - if ( iommu_intremap ) + if ( iommu_intremap != iommu_intremap_off ) { int rc; @@ -555,7 +555,7 @@ int msi_free_irq(struct msi_desc *entry) destroy_irq(entry[nr].irq); /* Free the unused IRTE if intr remap enabled */ - if ( iommu_intremap ) + if ( iommu_intremap != iommu_intremap_off ) iommu_update_ire_from_msi(entry + nr, NULL); } diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c index 5515cb70fd..e02a09a9a7 100644 --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -1480,7 +1480,7 @@ int __init amd_iommu_init(bool xt) goto error_out; } - if ( iommu_intremap ) + if ( iommu_intremap != iommu_intremap_off ) register_keyhandler('V', &amd_iommu_dump_intremap_tables, "dump IOMMU intremap tables", 0); @@ -1498,7 +1498,7 @@ int __init amd_iommu_init_late(void) /* Further initialize the device table(s). */ pci_init = true; - if ( iommu_intremap ) + if ( iommu_intremap != iommu_intremap_off ) rc = iterate_ivrs_mappings(amd_iommu_setup_device_table); for_each_amd_iommu ( iommu ) diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index e13b7d99db..bd6d69a6f5 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -2543,7 +2543,7 @@ static int __must_check init_vtd_hw(bool resume) /* * Enable interrupt remapping */ - if ( iommu_intremap ) + if ( iommu_intremap != iommu_intremap_off ) { int apic; for ( apic = 0; apic < nr_ioapics; apic++ ) @@ -2559,7 +2559,7 @@ static int __must_check init_vtd_hw(bool resume) } } } - if ( iommu_intremap ) + if ( iommu_intremap != iommu_intremap_off ) { for_each_drhd_unit ( drhd ) { diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c index 5a56565ea8..950dcd56ef 100644 --- a/xen/drivers/passthrough/vtd/quirks.c +++ b/xen/drivers/passthrough/vtd/quirks.c @@ -392,7 +392,7 @@ void __init platform_quirks_init(void) map_igd_reg(); /* Tylersburg interrupt remap quirk */ - if ( iommu_intremap ) + if ( iommu_intremap != iommu_intremap_off ) tylersburg_intremap_quirk(); }