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

[Julien Grall <julien@xxxxxxx>]

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@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.
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@xxxxxxxxx>


--
Julien Grall