[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] do_multicall and MISRA Rule 8.3\



On Mon, 11 Mar 2024, Julien Grall wrote:
> On 11/03/2024 11:32, George Dunlap wrote:
> > On Sat, Mar 9, 2024 at 1:59 AM Stefano Stabellini
> > <sstabellini@xxxxxxxxxx> wrote:
> > > 
> > > I would like to resurrect this thread and ask other opinions.
> > > 
> > > 
> > > On Thu, 23 Nov 2023, Jan Beulich wrote:
> > > > On 22.11.2023 22:46, Stefano Stabellini wrote:
> > > > > Two out of three do_multicall definitions/declarations use uint32_t as
> > > > > type for the "nr_calls" parameters. Change the third one to be
> > > > > consistent with the other two.
> > > > > 
> > > > > Link:
> > > > > https://lore.kernel.org/xen-devel/7e3abd4c0ef5127a07a60de1bf090a8aefac8e5c.1692717906.git.federico.serafini@xxxxxxxxxxx/
> > > > > Link:
> > > > > https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308251502430.6458@ubuntu-linux-20-04-desktop/
> > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > > > > ---
> > > > > Note that a previous discussion showed disagreement between
> > > > > maintainers
> > > > > on this topic. The source of disagreements are that we don't want to
> > > > > change a guest-visible ABI and we haven't properly documented how to
> > > > > use
> > > > > types for guest ABIs.
> > > > > 
> > > > > As an example, fixed-width types have the advantage of being explicit
> > > > > about their size but sometimes register-size types are required (e.g.
> > > > > unsigned long). The C specification says little about the size of
> > > > > unsigned long and today, and we even use unsigned int in guest ABIs
> > > > > without specifying the expected width of unsigned int on the various
> > > > > arches. As Jan pointed out, in Xen we assume sizeof(int) >= 4, but
> > > > > that's not written anywhere as far as I can tell.
> > > > > 
> > > > > I think the appropriate solution would be to document properly our
> > > > > expectations of both fixed-width and non-fixed-width types, and how to
> > > > > use them for guest-visible ABIs.
> > > > > 
> > > > > In this patch I used uint32_t for a couple of reasons:
> > > > > - until we have better documentation, I feel more confident in using
> > > > >    explicitly-sized integers in guest-visible ABIs
> > > > 
> > > > I disagree with this way of looking at it. Guests don't invoke these
> > > > functions directly, and our assembly code sitting in between already is
> > > > expected to (and does) guarantee that (in the case here) unsigned int
> > > > would be okay to use (as would be unsigned long, but at least on x86
> > > > that's slightly less efficient), in line with what ./CODING_STYLE says.
> > > > 
> > > > Otoh structure definitions in the public interface of course need to
> > > > use fixed with types (and still doesn't properly do so in a few cases).
> > 
> > You didn't address the other argument, which was that all the other
> > definitions have uint32_t; in particular,
> > common/multicall.c:do_multicall() takes uint32_t.  Surely that should
> > match the non-compat definition in include/hypercall-defs.c?
> > 
> > Whether they should both be `unsigned int` or `uint32_t` I don't
> > really feel like I have a good enough grasp of the situation to form a
> > strong opinion.
> 
> FWIW +1. We at least need some consistency.

Consistency is my top concern. Let's put the "unsigned int" vs
"uint32_t" argument aside.

do_multicall is not consistent with itself. We need
hypercall-defs.c:do_multicall and multicall.c:do_multicall to match.

Option1) We can change hypercall-defs.c:do_multicall to use uint32_t.

Option2) Or we can change multicall.c:do_multicall to use unsigned int.

I went with Option1. Andrew expressed his strong preference toward
Option1 in the past. George seems to prefer Option1.

Jan, can you accept Option1 and move on?

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.