Re: [XEN PATCH v2] xen/hypercalls: address violations of MISRA C:2012 Rule 8.3

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.