Re: [PATCH v2] xen/arm: Convert runstate address during hypcall

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

> On 1 Aug 2020, at 00:03, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Fri, 31 Jul 2020, Bertrand Marquis wrote:
>>> On 31 Jul 2020, at 12:18, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> 
>>> On 31.07.2020 12:12, Julien Grall wrote:
>>>> On 31/07/2020 07:39, Jan Beulich wrote:
>>>>> We're fixing other issues without breaking the ABI. Where's the
>>>>> problem of backporting the kernel side change (which I anticipate
>>>>> to not be overly involved)?
>>>> This means you can't take advantage of the runstage on existing Linux 
>>>> without any modification.
>>>> 
>>>>> If the plan remains to be to make an ABI breaking change,
>>>> 
>>>> For a theoritical PoV, this is a ABI breakage. However, I fail to see 
>>>> how the restrictions added would affect OSes at least on Arm.
>>> 
>>> "OSes" covering what? Just Linux?
>>> 
>>>> In particular, you can't change the VA -> PA on Arm without going 
>>>> through an invalid mapping. So I wouldn't expect this to happen for the 
>>>> runstate.
>>>> 
>>>> The only part that *may* be an issue is if the guest is registering the 
>>>> runstate with an initially invalid VA. Although, I have yet to see that 
>>>> in practice. Maybe you know?
>>> 
>>> I'm unaware of any such use, but this means close to nothing.
>>> 
>>>>> then I
>>>>> think this will need an explicit vote.
>>>> 
>>>> I was under the impression that the two Arm maintainers (Stefano and I) 
>>>> already agreed with the approach here. Therefore, given the ABI breakage 
>>>> is only affecting Arm, why would we need a vote?
>>> 
>>> The problem here is of conceptual nature: You're planning to
>>> make the behavior of a common hypercall diverge between
>>> architectures, and in a retroactive fashion. Imo that's nothing
>>> we should do even for new hypercalls, if _at all_ avoidable. If
>>> we allow this here, we'll have a precedent that people later
>>> may (and based on my experience will, sooner or later) reference,
>>> to get their own change justified.
> 
> Please let's avoid "slippery slope" arguments
> (https://en.wikipedia.org/wiki/Slippery_slope)
> 
> We shouldn't consider this instance as the first in a long series of bad
> decisions on hypercall compatibility. Each new case, if there will be
> any, will have to be considered based on its own merits. Also, let's
> keep in mind that there have been no other cases in the last 8 years. (I
> would like to repeat my support for hypercall ABI compatibility.)
> 
> 
> I would also kindly ask not to put the discussion on a "conceptual"
> level: there is no way to fix all guests and also keep compatibility.
> From a conceptual point of view, it is already game over :-)
> 
> 
>> After a discussion with Jan, he is proposing to have a guest config setting 
>> to
>> turn on or off the translation of the address during the hypercall and add a
>> global Xen command line parameter to set the global default behaviour. 
>> With this was done on arm could be done on x86 and the current behaviour
>> would be kept by default but possible to modify by configuration.
>> 
>> @Jan: please correct me if i said something wrong
>> @others: what is your view on this solution ?
> 
> Having options to turn on or off the new behavior could be good-to-have
> if we find a guest that actually requires the old behavior. Today we
> don't know of any such cases. We have strong reasons to believe that
> there aren't any on ARM (see Julien's explanation in regards to the
> temporary invalid mappings.) In fact, it is one of the factors that led
> us to think this patch is the right approach.
> 
> That said, I am also OK with adding such a parameter now, but we need to
> choose the default value carefully.

This would also mean keeping support in the code for old and new behaviour
which might make the code bigger and more complex.

> 
> 
> We need the new behavior as default on ARM because we need the fix to
> work for all guests. I don't think we want to explain how you always
> need to set config_foobar otherwise things don't work. It has to work
> out of the box.
> 
> It would be nice if we had the same default on x86 too, although I
> understand if Jan and Andrew don't want to make the same change on x86,
> at least initially.

So you mean here adding a parameter but only on Arm ?
Should it be a command line parameter ? a configuration parameter ? both ?

It seems that with this patch i touched some kind of sensible area.
Should i just abandon it and see later to work on adding a new hypercall using
a physical address as parameter ?

Cheers
Bertrand