[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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"
- 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()

     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.


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.