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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.01.2023 21:52, Andrew Cooper wrote:
> 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.

I didn't by any means mean to suggest to bump. I was seeing us add new
5.0 entries, with the presence of the 3.0 ones indicating the backwards
compatible ABI. An option might then later be to make the compatible
ABI compile (kconfig) time conditional.

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

Right, that's what I was hinting at as a possible option to do right here,
or to do subsequently.

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

Indeed, but I understood your primary concern was the on-stack copy, so my
suggestion first focused on that.

Jan