Re: [PATCH v2] docs/misra: document the expected sizes of integer types

From: Stefano Stabellini <sstabellini@kernel.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	andrew.cooper3@citrix.com,  bertrand.marquis@arm.com,
	george.dunlap@citrix.com, julien@xen.org,  michal.orzel@amd.com,
	roger.pau@citrix.com, xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2] docs/misra: document the expected sizes of integer types
Date: Mon, 18 Mar 2024 21:37:10 -0600 (MDT)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2403181735410.853156@ubuntu-linux-20-04-desktop> (raw)
In-Reply-To: <7ab73379-b057-4568-9869-141cef185752@suse.com>

On Mon, 18 Mar 2024, Jan Beulich wrote:
> On 16.03.2024 01:07, Stefano Stabellini wrote:
> > On Fri, 15 Mar 2024, Jan Beulich wrote:
> >> On 14.03.2024 23:17, Stefano Stabellini wrote:
> >>> Xen makes assumptions about the size of integer types on the various
> >>> architectures. Document these assumptions.
> >>
> >> My prior reservation wrt exact vs minimum sizes remains.
> > 
> > We have to specify the exact size. In practice the size is predetermined
> > and exact with all our supported compilers given a architecture.
> 
> But that's not the purpose of this document; if it was down to what
> compilers offer, we could refer to compiler documentation (and iirc we
> already do for various aspects). The purpose of this document, aiui,
> is to document assumption we make in hypervisor code. And those should
> be >=, not ==.

Well... I guess the two of us are making different assumptions then :-)

Which is the reason why documenting assumptions is so important. More at
the bottom.

> > Most importantly, unfortunately we use non-fixed-size integer types in
> > C hypercall entry points and public ABIs. In my opinion, that is not
> > acceptable.
> 
> The problem is that I can't see the reason for you thinking so. The C
> entry points sit past assembly code doing (required to do) necessary
> adjustments, if any. If there was no assembly layer, whether to use
> fixed with types for such parameters would depend on what the
> architecture guarantees.

This could be the source of the disagreement. I see the little assembly
code as not important, I consider it just like a little trampoline to
me. As we describe the hypercalls in C header files, I consider the C
functions the "official" hypercall entry points.

Also, as this is an ABI, I consider mandatory to use clear width
definitions of all the types (whether with this document or with
fixed-width types, and fixed-width types are clearer and better) in both
the C header files that describe the ABI interfaces, as well as the C
entry points that corresponds to it. E.g. I think we have to use
the same types in both do_sched_op and the hypercall description in
xen/include/public/sched.h

> As to public ABIs - that's structure definitions, and I agree we ought
> to uniformly use fixed-width types there. We largely do; a few things
> still require fixing.

+1

> > We have two options:
> > 
> > 1) we go with this document, and we clarify that even if we specify
> >   "unsigned int", we actually mean a 32-bit integer
> > 
> > 2) we change all our public ABIs and C hypercall entry points to use
> >    fixed-size types (e.g. s/unsigned int/uint32_t/g)
> > 
> > 2) is preferred because it is clearer but it is more work. So I went
> > with 1). I also thought you would like 1) more.
> 
> For ABIs (i.e. structures) we ought to be making that change anyway.
> Leaving basic types in there is latently buggy.

I am glad we agree :-)

It is just that I also consinder the C hypercall entry points as part of
the ABI

> I'm happy to see a document like this added, for the purpose described
> above. But to me 1) and 2) and largely independent of one another.

Good that you are also happy with a document like this.

The remaining question is: what about the rest of the C functions in Xen
that are certainly not part of an ABI?

Those are less critical, still this document should apply uniformily to
them too. I don't understand why you are making the >= width assumption
you mentioned at the top of the file when actually it is impossible to
exercise or test this assumption on any compiler or any architecture
that works with Xen. If it cannot be enabled, it hasn't been tested, and
it probably won't work.