Re: [PATCH 3/4] xen/version: Drop bogus return values for XENVER_platform_parameters

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.01.2023 13:14, Andrew Cooper wrote:
> On 06/01/2023 7:54 am, Jan Beulich wrote:
>> On 05.01.2023 23:17, Andrew Cooper wrote:
>>> On 05/01/2023 7:57 am, Jan Beulich wrote:
>>>> On 04.01.2023 20:55, Andrew Cooper wrote:
>>>>> On 04/01/2023 4:40 pm, Jan Beulich wrote:
>>>>>> On 03.01.2023 21:09, Andrew Cooper wrote:
>>>>>>> + * For all guest types using hardware virt extentions, Xen is not 
>>>>>>> mapped into
>>>>>>> + * the guest kernel virtual address space.  This now return 0, where it
>>>>>>> + * previously returned unrelated data.
>>>>>>> + */
>>>>>>>  #define XENVER_platform_parameters 5
>>>>>>>  struct xen_platform_parameters {
>>>>>>>      xen_ulong_t virt_start;
>>>>>> ... the field name tells me that all that is being conveyed is the 
>>>>>> virtual
>>>>>> address of where the hypervisor area starts.
>>>>> IMO, it doesn't matter what the name of the field is.  It dates from the
>>>>> days when 32bit PV was the only type of guest.
>>>>>
>>>>> 32bit PV guests really do have a variable split, so the guest kernel
>>>>> really does need to get this value from Xen.
>>>>>
>>>>> The split for 64bit PV guests is compile-time constant, hence why 64bit
>>>>> PV kernels don't care.
>>>> ... once we get to run Xen in 5-level mode, 4-level PV guests could also
>>>> gain a variable split: Like for 32-bit guests now, only the r/o M2P would
>>>> need to live in that area, and this may well occupy less than the full
>>>> range presently reserved for the hypervisor.
>>> ... you can't do this, because it only works for guests which have
>>> chosen to find the M2P using XENMEM_machphys_mapping (e.g. Linux), and
>>> doesn't for e.g. MiniOS which does:
>>>
>>> #define machine_to_phys_mapping ((unsigned long *)HYPERVISOR_VIRT_START)
>> Hmm, looks like a misunderstanding? I certainly wasn't thinking about
>> making the start of that region variable, but rather the end (i.e. not
>> exactly like for 32-bit compat).
> 
> But for what purpose?  You can't make 4-level guests have a variable range.

That entirely depends on the kernel. Linux, for example, could put its
direct map lower, ...

> The best you can do is make a 5-level-aware guest running on 4-level Xen
> have some new semantics, but a 4-level PV guest already owns the
> overwhelming majority of virtual address space, so being able to hand
> back a few extra TB is not interesting.  And for making that happen...

... allowing it to cover some more space. That's not a whole lot more,
sure.

>>>>> For compat HVM, it happens to pick up the -1 from:
>>>>>
>>>>> #ifdef CONFIG_PV32
>>>>>     HYPERVISOR_COMPAT_VIRT_START(d) =
>>>>>         is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
>>>>> #endif
>>>>>
>>>>> in arch_domain_create(), whereas for non-compat HVM, it gets a number in
>>>>> an address space it has no connection to in the slightest.  ARM guests
>>>>> end up getting XEN_VIRT_START (== 2M) handed back, but this absolutely
>>>>> an internal detail that guests have no business knowing.
>>>> Well, okay, this looks to be good enough an argument to make the adjustment
>>>> you propose for !PV guests.
>>> Right, HVM (on all architectures) is very cut and dry.
>>>
>>> But it feels wrong to not address the PV64 issue at the same time
>>> because it is similar level of broken, despite there being (in theory) a
>>> legitimate need for a PV guest kernel to know it.
>> To me it feels wrong to address the 64-bit PV issue by removing information,
>> when - as you also say - it is actually _missing_ information. To me the
>> proper course of action would be to expose the upper bound as well (such
>> that, down the road, it could become dynamic). There's also no info leak
>> there, as the two (static) bounds are part of the PV ABI anyway.
> 
> ... the absolute best you could do here is introduce a new
> XENVER_platform_parameters2 that has a different structure.

Indeed.

> Which still leaves us with XENVER_platform_parameters providing data
> which is somewhere between useless and actively unhelpful.

If it's useless, there's still no reason to remove / alter the information,
as you can never be absolutely certain that no-one uses this in some way.
And for the "actively unhelpful" aspect it looks like our views simply
differ. Is there no way we can settle on making the change affect HVM only?

Jan