| Message ID | 533a2d4f0c92d7fe92aa200b64434389de546f69.1710343652.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [XEN] amd/iommu: add fixed size to function parameter of array type | expand |
On 14.03.2024 08:42, Nicola Vetrini wrote: > The 'cmd' parameter of amd_iommu_send_guest_cmd is passed > to a function that expects arrays of size 4, therefore > specifying explicitly the size also in amd_iommu_send_guest_cmd > allows not to accidentally pass a smaller array or assume that > send_iommu_command handles array sizes >4 correctly. > > No functional change. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > All current users pass an array of size 4, hence this patch is addressing > a potential issue noticed by the analyzer in the context of Rule 17.5 > ("The function argument corresponding to a parameter declared to have an array > type shall have an appropriate number of elements"), not an actual problem in > the sources. While true, I think we want to consider alternatives. First one being to rip out this dead code (thus addressing other Misra concerns as well). Second, if we meant to keep it, to properly do away with the (ab)use of u32[]. Finally, if to be taken in this least-effort shape, ... > --- a/xen/drivers/passthrough/amd/iommu.h > +++ b/xen/drivers/passthrough/amd/iommu.h > @@ -346,7 +346,7 @@ void cf_check amd_iommu_crash_shutdown(void); > > /* guest iommu support */ > #ifdef CONFIG_HVM > -void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]); > +void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[4]); ... u32 here and ... > --- a/xen/drivers/passthrough/amd/iommu_cmd.c > +++ b/xen/drivers/passthrough/amd/iommu_cmd.c > @@ -390,7 +390,7 @@ void amd_iommu_flush_all_caches(struct amd_iommu *iommu) > flush_command_buffer(iommu, 0); > } > > -void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]) > +void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[4]) ... here would better be replaced by uint32_t at the same time, not the least because that's what ... > { > send_iommu_command(iommu, cmd); ... this function already takes afaics. Jan
On 2024-03-14 09:32, Jan Beulich wrote: > On 14.03.2024 08:42, Nicola Vetrini wrote: >> The 'cmd' parameter of amd_iommu_send_guest_cmd is passed >> to a function that expects arrays of size 4, therefore >> specifying explicitly the size also in amd_iommu_send_guest_cmd >> allows not to accidentally pass a smaller array or assume that >> send_iommu_command handles array sizes >4 correctly. >> >> No functional change. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> All current users pass an array of size 4, hence this patch is >> addressing >> a potential issue noticed by the analyzer in the context of Rule 17.5 >> ("The function argument corresponding to a parameter declared to have >> an array >> type shall have an appropriate number of elements"), not an actual >> problem in >> the sources. > > While true, I think we want to consider alternatives. First one being > to rip > out this dead code (thus addressing other Misra concerns as well). > Second, > if we meant to keep it, to properly do away with the (ab)use of u32[]. > I'm not understanding what you consider dead code. I see three users of amd_iommu_send_guest_cmd and seven for send_iommu_command. I can adjust u32 for sure. There are also other u32/uint32_t incosistencies in that header. > Finally, if to be taken in this least-effort shape, ... > >> --- a/xen/drivers/passthrough/amd/iommu.h >> +++ b/xen/drivers/passthrough/amd/iommu.h >> @@ -346,7 +346,7 @@ void cf_check amd_iommu_crash_shutdown(void); >> >> /* guest iommu support */ >> #ifdef CONFIG_HVM >> -void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]); >> +void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[4]); > > ... u32 here and ... > >> --- a/xen/drivers/passthrough/amd/iommu_cmd.c >> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c >> @@ -390,7 +390,7 @@ void amd_iommu_flush_all_caches(struct amd_iommu >> *iommu) >> flush_command_buffer(iommu, 0); >> } >> >> -void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]) >> +void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[4]) > > ... here would better be replaced by uint32_t at the same time, not the > least because that's what ... > >> { >> send_iommu_command(iommu, cmd); > > ... this function already takes afaics. > > Jan
On 14.03.2024 10:25, Nicola Vetrini wrote: > On 2024-03-14 09:32, Jan Beulich wrote: >> On 14.03.2024 08:42, Nicola Vetrini wrote: >>> The 'cmd' parameter of amd_iommu_send_guest_cmd is passed >>> to a function that expects arrays of size 4, therefore >>> specifying explicitly the size also in amd_iommu_send_guest_cmd >>> allows not to accidentally pass a smaller array or assume that >>> send_iommu_command handles array sizes >4 correctly. >>> >>> No functional change. >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> --- >>> All current users pass an array of size 4, hence this patch is >>> addressing >>> a potential issue noticed by the analyzer in the context of Rule 17.5 >>> ("The function argument corresponding to a parameter declared to have >>> an array >>> type shall have an appropriate number of elements"), not an actual >>> problem in >>> the sources. >> >> While true, I think we want to consider alternatives. First one being >> to rip >> out this dead code (thus addressing other Misra concerns as well). >> Second, >> if we meant to keep it, to properly do away with the (ab)use of u32[]. >> > > I'm not understanding what you consider dead code. iommu_guest.c:guest_iommu_{init,destroy,set_base,add_event_log}() have no callers; guest_iommu_add_ppr_log() having one is suspicious, but the function will still bail early due to domain_iommu() never returning non-NULL in practice. With that I'm pretty sure nothing else in the file is actually reachable. > I see three users of amd_iommu_send_guest_cmd All in said file. > and seven for send_iommu_command. Well, this one of course isn't dead. > I can adjust u32 for sure. There are also other u32/uint32_t > incosistencies in that header. Which we're going to take care of over time. Jan
On Thu, 14 Mar 2024, Jan Beulich wrote: > On 14.03.2024 10:25, Nicola Vetrini wrote: > > On 2024-03-14 09:32, Jan Beulich wrote: > >> On 14.03.2024 08:42, Nicola Vetrini wrote: > >>> The 'cmd' parameter of amd_iommu_send_guest_cmd is passed > >>> to a function that expects arrays of size 4, therefore > >>> specifying explicitly the size also in amd_iommu_send_guest_cmd > >>> allows not to accidentally pass a smaller array or assume that > >>> send_iommu_command handles array sizes >4 correctly. > >>> > >>> No functional change. > >>> > >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > >>> --- > >>> All current users pass an array of size 4, hence this patch is > >>> addressing > >>> a potential issue noticed by the analyzer in the context of Rule 17.5 > >>> ("The function argument corresponding to a parameter declared to have > >>> an array > >>> type shall have an appropriate number of elements"), not an actual > >>> problem in > >>> the sources. > >> > >> While true, I think we want to consider alternatives. First one being > >> to rip > >> out this dead code (thus addressing other Misra concerns as well). > >> Second, > >> if we meant to keep it, to properly do away with the (ab)use of u32[]. > >> > > > > I'm not understanding what you consider dead code. > > iommu_guest.c:guest_iommu_{init,destroy,set_base,add_event_log}() have > no callers; guest_iommu_add_ppr_log() having one is suspicious, but the > function will still bail early due to domain_iommu() never returning > non-NULL in practice. With that I'm pretty sure nothing else in the file > is actually reachable. I also prefer removing the code that is not used > > I see three users of amd_iommu_send_guest_cmd > > All in said file. > > > and seven for send_iommu_command. > > Well, this one of course isn't dead. > > > I can adjust u32 for sure. There are also other u32/uint32_t > > incosistencies in that header. > > Which we're going to take care of over time. > > Jan >
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h index 1b62c083ba67..be82bd950e30 100644 --- a/xen/drivers/passthrough/amd/iommu.h +++ b/xen/drivers/passthrough/amd/iommu.h @@ -346,7 +346,7 @@ void cf_check amd_iommu_crash_shutdown(void); /* guest iommu support */ #ifdef CONFIG_HVM -void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]); +void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[4]); void guest_iommu_add_ppr_log(struct domain *d, u32 entry[]); void guest_iommu_add_event_log(struct domain *d, u32 entry[]); int guest_iommu_init(struct domain* d); diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c index 49b9fcac9410..321a814eb810 100644 --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -390,7 +390,7 @@ void amd_iommu_flush_all_caches(struct amd_iommu *iommu) flush_command_buffer(iommu, 0); } -void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]) +void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[4]) { send_iommu_command(iommu, cmd); /* TBD: Timeout selection may require peeking into cmd[]. */
The 'cmd' parameter of amd_iommu_send_guest_cmd is passed to a function that expects arrays of size 4, therefore specifying explicitly the size also in amd_iommu_send_guest_cmd allows not to accidentally pass a smaller array or assume that send_iommu_command handles array sizes >4 correctly. No functional change. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- All current users pass an array of size 4, hence this patch is addressing a potential issue noticed by the analyzer in the context of Rule 17.5 ("The function argument corresponding to a parameter declared to have an array type shall have an appropriate number of elements"), not an actual problem in the sources. --- xen/drivers/passthrough/amd/iommu.h | 2 +- xen/drivers/passthrough/amd/iommu_cmd.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)