| Message ID | d494980216b8f0f870083fcfae7269f45e779780.1701941924.git.maria.celeste.cesario@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: address violations of MISRA C:2012 Rule 14.4 | expand |
On 07.12.2023 10:48, Simone Ballarin wrote: > --- 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); > } > All of this would logically be part of patch 1. Is there a particular reason why it wasn't done right there? > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -1320,7 +1320,7 @@ x86_emulate( > ea.bytes = 2; > goto srcmem_common; > case SrcMem: > - if ( state->simd_size ) > + if ( state->simd_size != simd_none ) > break; > ea.bytes = (d & ByteOp) ? 1 : op_bytes; > srcmem_common: > @@ -1460,7 +1460,7 @@ x86_emulate( > /* Becomes a normal DstMem operation from here on. */ > case DstMem: > generate_exception_if(ea.type == OP_MEM && evex.z, X86_EXC_UD); > - if ( state->simd_size ) > + if ( state->simd_size != simd_none ) > { > generate_exception_if(lock_prefix, X86_EXC_UD); > break; > @@ -8176,7 +8176,7 @@ x86_emulate( > goto done; > } > > - if ( state->rmw ) > + if ( state->rmw != rmw_NONE ) > { > ea.val = src.val; > op_bytes = dst.bytes; > @@ -8205,7 +8205,7 @@ x86_emulate( > > dst.type = OP_NONE; > } > - else if ( state->simd_size ) > + else if ( state->simd_size != simd_none ) > { > generate_exception_if(!op_bytes, X86_EXC_UD); > generate_exception_if((vex.opcx && (d & TwoOp) && I'd be (somewhat reluctantly) okay with ack-ing this part of the patch. Jan
On 07/12/23 11:54, Jan Beulich wrote: > On 07.12.2023 10:48, Simone Ballarin wrote: >> --- 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); >> } >> > > All of this would logically be part of patch 1. Is there a particular reason > why it wasn't done right there? These changes and the ones in patch 1 are related, but still remain independent. Patch 1 can be accepted without patch 2 and vice versa. So we've decided to split the commits because patch 1 is in common code, while patch 2 is in x86-specific code. No other real reasons, but in any case we can move these changes to patch 1. > >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -1320,7 +1320,7 @@ x86_emulate( >> ea.bytes = 2; >> goto srcmem_common; >> case SrcMem: >> - if ( state->simd_size ) >> + if ( state->simd_size != simd_none ) >> break; >> ea.bytes = (d & ByteOp) ? 1 : op_bytes; >> srcmem_common: >> @@ -1460,7 +1460,7 @@ x86_emulate( >> /* Becomes a normal DstMem operation from here on. */ >> case DstMem: >> generate_exception_if(ea.type == OP_MEM && evex.z, X86_EXC_UD); >> - if ( state->simd_size ) >> + if ( state->simd_size != simd_none ) >> { >> generate_exception_if(lock_prefix, X86_EXC_UD); >> break; >> @@ -8176,7 +8176,7 @@ x86_emulate( >> goto done; >> } >> >> - if ( state->rmw ) >> + if ( state->rmw != rmw_NONE ) >> { >> ea.val = src.val; >> op_bytes = dst.bytes; >> @@ -8205,7 +8205,7 @@ x86_emulate( >> >> dst.type = OP_NONE; >> } >> - else if ( state->simd_size ) >> + else if ( state->simd_size != simd_none ) >> { >> generate_exception_if(!op_bytes, X86_EXC_UD); >> generate_exception_if((vex.opcx && (d & TwoOp) && > > I'd be (somewhat reluctantly) okay with ack-ing this part of the patch. > > Jan
On 07.12.2023 14:53, Simone Ballarin wrote: > On 07/12/23 11:54, Jan Beulich wrote: >> On 07.12.2023 10:48, Simone Ballarin wrote: >>> --- 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); >>> } >>> >> >> All of this would logically be part of patch 1. Is there a particular reason >> why it wasn't done right there? > > These changes and the ones in patch 1 are related, but still remain > independent. Patch 1 can be accepted without patch 2 and vice versa. > So we've decided to split the commits because patch 1 is in common > code, while patch 2 is in x86-specific code. Just to clarify: While not located under arch/x86/, what patch 1 touches is still x86-specific code. It's subject prefix also wrongly says AMD/IOMMU: when it also touches VT-d code. Especially with the changes here folded in, x86/IOMMU: might be more appropriate. Jan
On 07/12/23 15:15, Jan Beulich wrote: > On 07.12.2023 14:53, Simone Ballarin wrote: >> On 07/12/23 11:54, Jan Beulich wrote: >>> On 07.12.2023 10:48, Simone Ballarin wrote: >>>> --- 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); >>>> } >>>> >>> >>> All of this would logically be part of patch 1. Is there a particular reason >>> why it wasn't done right there? >> >> These changes and the ones in patch 1 are related, but still remain >> independent. Patch 1 can be accepted without patch 2 and vice versa. >> So we've decided to split the commits because patch 1 is in common >> code, while patch 2 is in x86-specific code. > > Just to clarify: While not located under arch/x86/, what patch 1 touches > is still x86-specific code. It's subject prefix also wrongly says > AMD/IOMMU: when it also touches VT-d code. Especially with the changes > here folded in, x86/IOMMU: might be more appropriate. > OK, then I'll move the changes and use x86/IOMMU. Thanks. > Jan >
On Thu, 7 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> I think it would have been better to put all the iommu_intremap changed in the same patch (patch #1). But anyway: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
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/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index cf780da501..00b7365ed3 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1320,7 +1320,7 @@ x86_emulate( ea.bytes = 2; goto srcmem_common; case SrcMem: - if ( state->simd_size ) + if ( state->simd_size != simd_none ) break; ea.bytes = (d & ByteOp) ? 1 : op_bytes; srcmem_common: @@ -1460,7 +1460,7 @@ x86_emulate( /* Becomes a normal DstMem operation from here on. */ case DstMem: generate_exception_if(ea.type == OP_MEM && evex.z, X86_EXC_UD); - if ( state->simd_size ) + if ( state->simd_size != simd_none ) { generate_exception_if(lock_prefix, X86_EXC_UD); break; @@ -8176,7 +8176,7 @@ x86_emulate( goto done; } - if ( state->rmw ) + if ( state->rmw != rmw_NONE ) { ea.val = src.val; op_bytes = dst.bytes; @@ -8205,7 +8205,7 @@ x86_emulate( dst.type = OP_NONE; } - else if ( state->simd_size ) + else if ( state->simd_size != simd_none ) { generate_exception_if(!op_bytes, X86_EXC_UD); generate_exception_if((vex.opcx && (d & TwoOp) &&