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

[Jan Beulich <jbeulich@suse.com>]

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.

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.

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 ...

> 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.

Jan