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

[Julien Grall <julien@xen.org>]

Hi,

Sorry for the late answer.

On 14/08/2020 10:25, Bertrand Marquis wrote:
> 
> 
>> On 1 Aug 2020, at 00:03, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>
>> On Fri, 31 Jul 2020, Bertrand Marquis wrote:
>>>> On 31 Jul 2020, at 12:18, Jan Beulich <jbeulich@suse.com> 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.

I agree with that :).

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

I am concerned with that as well. However, this concern is also going to 
be true if we introduce an hypercall using a physical address as 
parameter. Indeed, the old hypercall will not go away.

If we introduce a second hypercall, you will also have to think about 
the interactions between the two. For instance:
     - The firmware may register the runstate using the old hypercall, 
while the OS may register using the new hypercall.
     - Can an OS use a mix of the two hypercalls?

For more details, you can have a look at the original attempt for a new 
hypercall (see [1]).

The approach you discussed with Jan has the advantage to not require any 
change in the guest software stack. So this would be my preference over 
a new hypercall.

>>
>>
>> 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 ?

I would suggest to mention the thread in the next community call.

Cheers,

[1] <1558721577-13958-3-git-send-email-andrii.anisov@gmail.com>


-- 
Julien Grall