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 > > <sstabell...@kernel.org> 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.seraf...@bugseng.com/ > > > > > 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.stabell...@amd.com> > > > > > --- > > > > > 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?
