| Message ID | 7e3abd4c0ef5127a07a60de1bf090a8aefac8e5c.1692717906.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,v2] xen/hypercalls: address violations of MISRA C:2012 Rule 8.3 | expand |
On 23.08.2023 09:04, Federico Serafini wrote: > Make function declarations and definitions consistent to address > violations of MISRA C:2012 Rule 8.3 ("All declarations of an object or > function shall use the same names and type qualifiers"). > > No functional change. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On 23/08/2023 8:04 am, Federico Serafini wrote: > Make function declarations and definitions consistent to address > violations of MISRA C:2012 Rule 8.3 ("All declarations of an object or > function shall use the same names and type qualifiers"). > > No functional change. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > --- > Changes in v2: > - change compat_grant_table_op() definition instead of the declaration; > - use unsigned int for multicall()'s parameter in accordance with XEN coding > style. Nack. You were correct in v1, and the request to change it was erroneous. > diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c > index 6d361ddfce..d1892fd14f 100644 > --- a/xen/include/hypercall-defs.c > +++ b/xen/include/hypercall-defs.c > @@ -135,8 +135,8 @@ xenoprof_op(int op, void *arg) > #ifdef CONFIG_COMPAT > prefix: compat > set_timer_op(uint32_t lo, int32_t hi) > -multicall(multicall_entry_compat_t *call_list, uint32_t nr_calls) > -memory_op(unsigned int cmd, void *arg) > +multicall(multicall_entry_compat_t *call_list, unsigned int nr_calls) This, unfortunately for many reasons, is an ABI with guests. It is buggy that the entire file doesn't use unsigned long (i.e. one full GPR width) to begin with. It these types alone which cause explicit truncation of the guest-provided hypercall parameter values. But it is even more buggy to take something that at least truncates to a fixed width, and replace it with something that explicitly does not have a fixed width. ~Andrew
On 23.08.2023 09:28, Andrew Cooper wrote: > On 23/08/2023 8:04 am, Federico Serafini wrote: >> Make function declarations and definitions consistent to address >> violations of MISRA C:2012 Rule 8.3 ("All declarations of an object or >> function shall use the same names and type qualifiers"). >> >> No functional change. >> >> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> >> --- >> Changes in v2: >> - change compat_grant_table_op() definition instead of the declaration; >> - use unsigned int for multicall()'s parameter in accordance with XEN coding >> style. > > Nack. Oh, I'm sorry, I committed this just before seeing your reply. I'll revert right away, until we can settle the discussion. > You were correct in v1, and the request to change it was erroneous. > >> diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c >> index 6d361ddfce..d1892fd14f 100644 >> --- a/xen/include/hypercall-defs.c >> +++ b/xen/include/hypercall-defs.c >> @@ -135,8 +135,8 @@ xenoprof_op(int op, void *arg) >> #ifdef CONFIG_COMPAT >> prefix: compat >> set_timer_op(uint32_t lo, int32_t hi) >> -multicall(multicall_entry_compat_t *call_list, uint32_t nr_calls) >> -memory_op(unsigned int cmd, void *arg) >> +multicall(multicall_entry_compat_t *call_list, unsigned int nr_calls) > > This, unfortunately for many reasons, is an ABI with guests. > > It is buggy that the entire file doesn't use unsigned long (i.e. one > full GPR width) to begin with. It these types alone which cause > explicit truncation of the guest-provided hypercall parameter values. > > But it is even more buggy to take something that at least truncates to a > fixed width, and replace it with something that explicitly does not have > a fixed width. I disagree on all the points you make. Handling compat guests is quite fine to use unsigned int in hypercall prototypes. Fixed width isn't needed (just like you don't demand uint64_t, but suggest unsigned long), and wider than 32 bits (i.e. long) isn't needed either because can't pass in wider values anyway. Jan
On Wed, 23 Aug 2023, Jan Beulich wrote: > On 23.08.2023 09:28, Andrew Cooper wrote: > > On 23/08/2023 8:04 am, Federico Serafini wrote: > >> Make function declarations and definitions consistent to address > >> violations of MISRA C:2012 Rule 8.3 ("All declarations of an object or > >> function shall use the same names and type qualifiers"). > >> > >> No functional change. > >> > >> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > >> --- > >> Changes in v2: > >> - change compat_grant_table_op() definition instead of the declaration; > >> - use unsigned int for multicall()'s parameter in accordance with XEN coding > >> style. > > > > Nack. > > Oh, I'm sorry, I committed this just before seeing your reply. I'll > revert right away, until we can settle the discussion. > > > You were correct in v1, and the request to change it was erroneous. > > > >> diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c > >> index 6d361ddfce..d1892fd14f 100644 > >> --- a/xen/include/hypercall-defs.c > >> +++ b/xen/include/hypercall-defs.c > >> @@ -135,8 +135,8 @@ xenoprof_op(int op, void *arg) > >> #ifdef CONFIG_COMPAT > >> prefix: compat > >> set_timer_op(uint32_t lo, int32_t hi) > >> -multicall(multicall_entry_compat_t *call_list, uint32_t nr_calls) > >> -memory_op(unsigned int cmd, void *arg) > >> +multicall(multicall_entry_compat_t *call_list, unsigned int nr_calls) > > > > This, unfortunately for many reasons, is an ABI with guests. > > > > It is buggy that the entire file doesn't use unsigned long (i.e. one > > full GPR width) to begin with. It these types alone which cause > > explicit truncation of the guest-provided hypercall parameter values. > > > > But it is even more buggy to take something that at least truncates to a > > fixed width, and replace it with something that explicitly does not have > > a fixed width. > > I disagree on all the points you make. Handling compat guests is quite > fine to use unsigned int in hypercall prototypes. Fixed width isn't > needed (just like you don't demand uint64_t, but suggest unsigned long), > and wider than 32 bits (i.e. long) isn't needed either because can't > pass in wider values anyway. There are two points that Andrew made: 1) uint32_t should have been register-width (AKA register_t on ARM) 2) unsigned int is worse than uint32_t because is not fixed-width As a general rule I would say that it is better not to introduce any ABI changes as part of a clean-up patch for MISRA. (ABI changes could be done but separately.) In regard to 1) I don't think it is a good idea to change nr_calls to a type of different size. Even if it is the right thing to do. If it is, let's discuss it as a separate patch: you wouldn't want this type of change to be hidden inside a innocuous MISRA C patch anyway. In regard to 2), assuming "unsigned int" is exactly equivalent to uint32_t on all supported arches, then I am in two minds here. Just one year ago I would have agreed with Andrew and strongly requested an explicitly-sized type as argument. Then I discovered that all the RISC-V APIs are based on long, int and similar, and it seems to be working quite well for them. See for instance: https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/riscv-sbi.pdf Looking at Xen public headers, we have many instances of unsigned long and unsigned int in use. So now I think both explicitly-sized and non-explicitly-sized types can work well as long as we are clear on what they mean, their sizes on all supported arches, and we use them consistently. (For sure all of the above could be improved in Xen.) Coming to unsigned int, it should be 32-bit on all supported arches, so it is down to consistency, which is a bit arbitrary. We have quite a good mix of unsigned int and uint32_t in hypercall-defs.c, specifically: 10 uint32_t 32 unsigned int By popular vote, I would go with unsigned int. So, I would keep the patch as is.
On 26.08.2023 00:21, Stefano Stabellini wrote: > Coming to unsigned int, it should be 32-bit on all supported arches, But this isn't a requirement, even if we're using "unsigned int" in the C declarations / definitions: If "unsigned int" was wider, all we'd demand is that hypercall entry code (likely written in assembly anyway) zero-extend incoming values suitably to fit whatever "unsigned int" is. Which is no different to Andrew (aiui) suggesting to use "unsigned long" instead: That'll too require suitable zero-extension up front. > so > it is down to consistency, which is a bit arbitrary. We have quite a > good mix of unsigned int and uint32_t in hypercall-defs.c, specifically: > 10 uint32_t > 32 unsigned int > > By popular vote, I would go with unsigned int. So, I would keep the > patch as is. Well, I wouldn't quite say "as is": It clearly wants splitting for the two entirely unrelated changes. Then the uncontroversial part can go in right away, while we settle on the controversial aspect. As to "popular vote" - ./CODING_STYLE also matters here, and favors non-fixed-width types over fixed-width ones. And as per above imo there's no technical reason to use fixed-width types here. Yet I understand Andrew takes a different perspective ... Jan
On Mon, 28 Aug 2023, Jan Beulich wrote: > On 26.08.2023 00:21, Stefano Stabellini wrote: > > Coming to unsigned int, it should be 32-bit on all supported arches, > > But this isn't a requirement, even if we're using "unsigned int" in the > C declarations / definitions: If "unsigned int" was wider, all we'd > demand is that hypercall entry code (likely written in assembly anyway) > zero-extend incoming values suitably to fit whatever "unsigned int" is. > Which is no different to Andrew (aiui) suggesting to use "unsigned > long" instead: That'll too require suitable zero-extension up front. What you wrote assumes that "unsigned int" can only be 32-bit or wider, such as 64-bit. However, I think that by the C standard there is no guarantee. For instance, it could be smaller, e.g. 16-bit. There is also another assumption: if "unsigned int" was indeed 64-bit we could detect that a guest is 32-bit, assume that a 32-bit guest would interpret "unsigned int" as 32-bit and only pass 32-bit, and zero-extend the rest. I don't think it is a good idea to make unwritten assumption about the "unsigned int" size of the guest and as you know different OSes can assign different sizes to the same C type. This is why I think that in general if we wanted to use non-fixed-width as part of an ABI we would need to do a better job at writing down all of these assumptions. Without anything written down, it is easier to use fixed-width types. That said, in this case, we have been using "unsigned int" for years and it is fixed width for all the arches we support, so I think we should continue to use it for consistency, but ideally also help improve the documentation to write down all the unwritten assumptions we rely on. > > so > > it is down to consistency, which is a bit arbitrary. We have quite a > > good mix of unsigned int and uint32_t in hypercall-defs.c, specifically: > > 10 uint32_t > > 32 unsigned int > > > > By popular vote, I would go with unsigned int. So, I would keep the > > patch as is. > > Well, I wouldn't quite say "as is": It clearly wants splitting for the > two entirely unrelated changes. Then the uncontroversial part can go > in right away, while we settle on the controversial aspect. OK from me > As to "popular vote" - ./CODING_STYLE also matters here, and favors > non-fixed-width types over fixed-width ones. And as per above imo > there's no technical reason to use fixed-width types here. Yet I > understand Andrew takes a different perspective ... I think internal interfaces and public ABIs can have different policies and I understand the point that a public ABI should use only fixed-width types. That's not where we are today, but I would understand that argument.
On 28.08.2023 23:52, Stefano Stabellini wrote: > On Mon, 28 Aug 2023, Jan Beulich wrote: >> On 26.08.2023 00:21, Stefano Stabellini wrote: >>> Coming to unsigned int, it should be 32-bit on all supported arches, >> >> But this isn't a requirement, even if we're using "unsigned int" in the >> C declarations / definitions: If "unsigned int" was wider, all we'd >> demand is that hypercall entry code (likely written in assembly anyway) >> zero-extend incoming values suitably to fit whatever "unsigned int" is. >> Which is no different to Andrew (aiui) suggesting to use "unsigned >> long" instead: That'll too require suitable zero-extension up front. > > What you wrote assumes that "unsigned int" can only be 32-bit or wider, > such as 64-bit. However, I think that by the C standard there is no > guarantee. For instance, it could be smaller, e.g. 16-bit. Of course, but in Xen (just like e.g. Linux does) we inherently assume sizeof(int) >= 4. Jan
diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c index f8177c84c0..e00bc24a34 100644 --- a/xen/common/compat/grant_table.c +++ b/xen/common/compat/grant_table.c @@ -57,7 +57,7 @@ CHECK_gnttab_cache_flush; #undef xen_gnttab_cache_flush int compat_grant_table_op( - unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) cmp_uop, unsigned int count) + unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count) { int rc = 0; unsigned int i, cmd_op; @@ -71,7 +71,7 @@ int compat_grant_table_op( { #define CASE(name) \ case GNTTABOP_##name: \ - if ( unlikely(!guest_handle_okay(guest_handle_cast(cmp_uop, \ + if ( unlikely(!guest_handle_okay(guest_handle_cast(uop, \ gnttab_##name##_compat_t), \ count)) ) \ rc = -EFAULT; \ @@ -119,7 +119,7 @@ int compat_grant_table_op( #undef CASE default: - return do_grant_table_op(cmd, cmp_uop, count); + return do_grant_table_op(cmd, uop, count); } if ( (int)count < 0 ) @@ -148,7 +148,7 @@ int compat_grant_table_op( case GNTTABOP_setup_table: if ( unlikely(count > 1) ) rc = -EINVAL; - else if ( unlikely(__copy_from_guest(&cmp.setup, cmp_uop, 1)) ) + else if ( unlikely(__copy_from_guest(&cmp.setup, uop, 1)) ) rc = -EFAULT; else if ( unlikely(!compat_handle_okay(cmp.setup.frame_list, cmp.setup.nr_frames)) ) rc = -EFAULT; @@ -193,7 +193,7 @@ int compat_grant_table_op( } while (0) XLAT_gnttab_setup_table(&cmp.setup, nat.setup); #undef XLAT_gnttab_setup_table_HNDL_frame_list - if ( unlikely(__copy_to_guest(cmp_uop, &cmp.setup, 1)) ) + if ( unlikely(__copy_to_guest(uop, &cmp.setup, 1)) ) rc = -EFAULT; else i = 1; @@ -203,7 +203,7 @@ int compat_grant_table_op( case GNTTABOP_transfer: for ( n = 0; n < COMPAT_ARG_XLAT_SIZE / sizeof(*nat.xfer) && i < count && rc == 0; ++i, ++n ) { - if ( unlikely(__copy_from_guest_offset(&cmp.xfer, cmp_uop, i, 1)) ) + if ( unlikely(__copy_from_guest_offset(&cmp.xfer, uop, i, 1)) ) rc = -EFAULT; else { @@ -222,7 +222,7 @@ int compat_grant_table_op( { XEN_GUEST_HANDLE_PARAM(gnttab_transfer_compat_t) xfer; - xfer = guest_handle_cast(cmp_uop, gnttab_transfer_compat_t); + xfer = guest_handle_cast(uop, gnttab_transfer_compat_t); guest_handle_add_offset(xfer, i); cnt_uop = guest_handle_cast(xfer, void); while ( n-- ) @@ -237,7 +237,7 @@ int compat_grant_table_op( case GNTTABOP_copy: for ( n = 0; n < COMPAT_ARG_XLAT_SIZE / sizeof(*nat.copy) && i < count && rc == 0; ++i, ++n ) { - if ( unlikely(__copy_from_guest_offset(&cmp.copy, cmp_uop, i, 1)) ) + if ( unlikely(__copy_from_guest_offset(&cmp.copy, uop, i, 1)) ) rc = -EFAULT; else { @@ -267,7 +267,7 @@ int compat_grant_table_op( { XEN_GUEST_HANDLE_PARAM(gnttab_copy_compat_t) copy; - copy = guest_handle_cast(cmp_uop, gnttab_copy_compat_t); + copy = guest_handle_cast(uop, gnttab_copy_compat_t); guest_handle_add_offset(copy, i); cnt_uop = guest_handle_cast(copy, void); while ( n-- ) @@ -285,7 +285,7 @@ int compat_grant_table_op( rc = -EINVAL; break; } - if ( unlikely(__copy_from_guest(&cmp.get_status, cmp_uop, 1) || + if ( unlikely(__copy_from_guest(&cmp.get_status, uop, 1) || !compat_handle_okay(cmp.get_status.frame_list, cmp.get_status.nr_frames)) ) { @@ -303,7 +303,7 @@ int compat_grant_table_op( if ( rc >= 0 ) { XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_compat_t) get = - guest_handle_cast(cmp_uop, + guest_handle_cast(uop, gnttab_get_status_frames_compat_t); if ( unlikely(__copy_field_to_guest(get, nat.get_status, diff --git a/xen/common/multicall.c b/xen/common/multicall.c index 1f0cc4cb26..5330997144 100644 --- a/xen/common/multicall.c +++ b/xen/common/multicall.c @@ -34,7 +34,7 @@ static void trace_multicall_call(multicall_entry_t *call) } ret_t do_multicall( - XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, uint32_t nr_calls) + XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, unsigned int nr_calls) { struct vcpu *curr = current; struct mc_state *mcs = &curr->mc_state; diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c index 6d361ddfce..d1892fd14f 100644 --- a/xen/include/hypercall-defs.c +++ b/xen/include/hypercall-defs.c @@ -135,8 +135,8 @@ xenoprof_op(int op, void *arg) #ifdef CONFIG_COMPAT prefix: compat set_timer_op(uint32_t lo, int32_t hi) -multicall(multicall_entry_compat_t *call_list, uint32_t nr_calls) -memory_op(unsigned int cmd, void *arg) +multicall(multicall_entry_compat_t *call_list, unsigned int nr_calls) +memory_op(unsigned int cmd, void *compat) #ifdef CONFIG_IOREQ_SERVER dm_op(domid_t domid, unsigned int nr_bufs, void *bufs) #endif
Make function declarations and definitions consistent to address violations of MISRA C:2012 Rule 8.3 ("All declarations of an object or function shall use the same names and type qualifiers"). No functional change. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- Changes in v2: - change compat_grant_table_op() definition instead of the declaration; - use unsigned int for multicall()'s parameter in accordance with XEN coding style. --- xen/common/compat/grant_table.c | 22 +++++++++++----------- xen/common/multicall.c | 2 +- xen/include/hypercall-defs.c | 4 ++-- 3 files changed, 14 insertions(+), 14 deletions(-)