Re: [PATCH v2 2/5] xen/version: Calculate xen_capabilities_info once at boot

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 16/01/2023 3:53 pm, Jan Beulich wrote:
> On 14.01.2023 00:08, Andrew Cooper wrote:
>> The arch_get_xen_caps() infrastructure is horribly inefficient, for something
>> that is constant after features have been resolved on boot.
>>
>> Every instance used snprintf() to format constants into a string (which gets
>> shorter when %d gets resolved!), which gets double buffered on the stack.
>>
>> Switch to using string literals with the "3.0" inserted - these numbers
>> haven't changed in 18 years (The Xen 3.0 release was Dec 5th 2005).
>>
>> Use initcalls to format the data into xen_cap_info, which is deliberately not
>> of type xen_capabilities_info_t because a 1k array is a silly overhead for
>> storing a maximum of 77 chars (the x86 version) and isn't liable to need any
>> more space in the forseeable future.
> So I was wondering if once we arrived at the new ABI (and hence the 3.0 one
> is properly legacy) we shouldn't declare Xen 5.0 and then also mark the new
> ABI's availability here by a string including "5.0" where at present we
> expose (only) "3.0".

"the new ABI" is is still two things.

The one part is changes to the in-guest ABI which does make it GPA based
(for HVM), but this does need to be broadly backwards compatible.  This
ABI string lives in the PV guest elfnotes (and is ultimately the thing
that distinguishes PV32pae vs PV64), but nowhere interesting for HVM
guests as far as I can see (furthermore, the 3 variations of hvm-3.0-
are bogus.

xen-3.0-x86_64la57 would probably be the least invasive way to extend PV
support to 5-level paging.

The other part is a stable tools API/ABI.  This can have any kind of
interface we choose, and frankly there are better interfaces than this
stringly typed one.


"xen-3.0" is even hardcoded in libelf.  I can't foresee a good reason to
bump 3 -> 5 and break all current PV guests.

>> If Xen had strncpy(), then the hunk in do_xen_version() could read:
>>
>>   if ( deny )
>>      memset(info, 0, sizeof(info));
>>   else
>>      strncpy(info, xen_cap_info, sizeof(info));
>>
>> to avoid double processing the start of the buffer, but given the ABI (must
>> write 1k chars into the guest), I cannot see any way of taking info off the
>> stack without some kind of strncpy_to_guest() API.
> How about using clear_guest() for the 1k range, then copy_to_guest() for
> merely the string? Plus - are we even required to clear the buffer past
> the nul terminator?

Well, we have previously always copied 1k bytes.  But this is always
been a NUL terminated API of a string persuasion, so I find it hard to
believe that any caller cares beyond the NUL.

Because of safe_strcpy(), xen_cap_info is guaranteed to be NUL
terminated, so if we don't care about padding the buffer with extra
zeroes, we don't even need the clear_guest().

Also, similar reasoning would apply to XENVER_cmdline which is typically
rather less than 1k in length (at least it's not on the stack, but it is
still an excessive copy).

~Andrew