Re: [PATCH v2 13/13] SUPPORT.md: write down restriction of 32-bit tool stacks

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 15/07/2021 12:36, Jan Beulich wrote:
> On 15.07.2021 11:05, Julien Grall wrote:
> > On 15/07/2021 07:38, Jan Beulich wrote:
> > > On 14.07.2021 20:16, Julien Grall wrote:
> > > > On 05/07/2021 16:18, Jan Beulich wrote:
> > > > > Let's try to avoid giving the impression that 32-bit tool stacks are as
> > > > > capable as 64-bit ones.
> > > > Would you be able to provide a few examples of the known issues in the
> > > > commit message? This would be helpful for anyone to understand why we
> > > > decided to drop the support.
> > > Not sure how useful this is going to be.
> > It would at least be useful to me, so I can make an informed decision. I
> > suspect it would also be for anyone reading it in the future. This is
> > rather frustrating to find commit message with barely any rationale and
> > no-one remembering why this was done...
> Well, I've added "There are a number of cases there where 32-bit
> types are used to hold e.g. frame numbers." Not sure whether you
> consider this sufficient.
That's good enough for me in the commit message.

> Problematic code may be primarily in areas Arm doesn't
> care about (yet), like PCI pass-through or migration. But see e.g.
> - xc_map_foreign_range()'s "mfn" and "size" parameters,
> - xc_maximum_ram_page()'s "max_mfn" parameter,
> - libxl_dom.c:hvm_build_set_params()'s "store_mfn" and "console_mfn"
>    parameters,
> - xs_introduce_domain()'s "mfn" parameter,
> and quite a few more in particular in libxenguest.
That's quite a few :/. Thanks for listing them on the ML, they are 
useful to have them log on the ML.
> And then there are also subtle oddities like xc_set_mem_access_multi()
> having
>
>      xen_mem_access_op_t mao =
>      {
>          .op       = XENMEM_access_op_set_access_multi,
>          .domid    = domain_id,
>          .access   = XENMEM_access_default + 1, /* Invalid value */
>          .pfn      = ~0UL, /* Invalid GFN */
>          .nr       = nr,
>      };
>
> Clearly ~0UL won't have the intended effect even for 32-bit guests,
> when the field is uint64_t (we get away here because for whatever
> reason the hypervisor doesn't check that the field indeed is ~0UL).
> But I wouldn't be surprised to find uses where there would be a
> difference. One of the main aspects certainly is ...
Whoops :). One topic that came up on my series to drop the M2P helpers 
on Arm is the lack of a suitable define for invalid GFN.
However, at the first look, it didn't seem to be easy to introduce 
because the GFN is sometime stored in a 64-bit and other time in 
xen_ulong_t.
> > I vaguely recall a discussion about 64-bit hypercall ([1]). I assume the
> > decision to drop support is related to it, but I have no way to prove it
> > from the commit message.
> ... this. Some XENMEM_* may return 64-bit values, yet the hypercall
> interface is limited to "long" return types. Not even the multicall
> approach taken to work around the restriction to "int" would help
> here for x86-32, as struct multicall_entry also uses xen_ulong_t
> for its "result" field.
>
> > It is also not clear why adding the restriction is the way to go...
> >
> > > This would be pointing at the
> > > declarations / definitions of various tool stack internal variables or
> > > structure fields. Which also is why ...
> > ... is this because such issues are too widespread in libxc/libxl to fix
> > it in long term?
> Fixing is an option, but until it gets fixed (if anyone really cared
> to do so), spelling out the restriction looks to be an appropriate
> step to me (or else I wouldn't have followed the request and created
> this patch). Once suitably audited, fixed, and tested, I wouldn't see
> a reason not to remove the restriction again.
Agreed. I was mostly wondering whether this was a matter of couple 
patches and could be restricted to maybe libxl (IOW toolstack based on 
libxc may not be affected). But from what you wrote, the issue is quite 
widespread.
Anyway, this is enough to convince me that dropping support (until it is 
fixed) for 32-bit toolstack on 64-bit hypervisor.
Cheers,

--
Julien Grall