Re: [PATCH 1/2] KVM: arm/arm64: Add save/restore support for firmware workaround state

[Marc Zyngier <marc.zyngier@arm.com>]

On Mon, 18 Feb 2019 11:29:57 +0000
André Przywara <andre.przywara@arm.com> wrote:

> On 18/02/2019 09:07, Marc Zyngier wrote:
> > On Fri, 15 Feb 2019 17:26:02 +0000
> > Dave Martin <Dave.Martin@arm.com> wrote:  
> 
> Hi,
> 
> >   
> >> On Fri, Feb 15, 2019 at 11:42:27AM +0000, Marc Zyngier wrote:  
> >>> On Fri, 15 Feb 2019 09:58:57 +0000,
> >>> Andre Przywara <andre.przywara@arm.com> wrote:    
> >>>>
> >>>> On Wed, 30 Jan 2019 11:39:00 +0000
> >>>> Andre Przywara <andre.przywara@arm.com> wrote:
> >>>>
> >>>> Peter, Marc, Christoffer,
> >>>>
> >>>> can we have an opinion on whether it's useful to introduce some
> >>>> common scheme for firmware workaround system registers (parts of
> >>>> KVM_REG_ARM_FW_REG(x)), which would allow checking them for
> >>>> compatibility between two kernels without specifically knowing about
> >>>> them?
> >>>> Dave suggested to introduce some kind of signed encoding in the 4
> >>>> LSBs for all those registers (including future ones), where 0 means
> >>>> UNKNOWN and greater values are better. So without knowing about the
> >>>> particular register, one could judge whether it's safe to migrate.
> >>>> I am just not sure how useful this is, given that QEMU seems to ask
> >>>> the receiving kernel about any sysreg, and doesn't particularly care
> >>>> about the meaning of those registers. And I am not sure we really
> >>>> want to introduce some kind of forward looking scheme in the kernel
> >>>> here, short of a working crystal ball. I think the kernel policy was
> >>>> always to be as strict as possible about those things.    
> >>>
> >>> I honestly don't understand how userspace can decide whether a given
> >>> configuration is migratable or not solely based on the value of such a
> >>> register. In my experience, the target system has a role to play, and
> >>> is the only place where we can find out about whether migration is
> >>> actually possible.    
> >>
> >> Both origin and target system need to be taken into account.  I don't
> >> think that's anything new.  
> > 
> > Well, that was what I understood from Andre's question.
> >   
> >>  
> >>> As you said, userspace doesn't interpret the data, nor should it. It
> >>> is only on the receiving end that compatibility is assessed and
> >>> whether some level of compatibility can be safely ensured.
> >>>
> >>> So to sum it up, I don't believe in this approach as a general way of
> >>> describing the handling or errata.    
> >>
> >> For context, my idea attempted to put KVM, not userspace, in charge of
> >> the decision: userspace applies fixed comparison rules determined ahead
> >> of time, but KVM supplies the values compared (and hence determines the
> >> result).
> >>
> >> My worry was that otherwise we may end up with a wild-west tangle of
> >> arbitrary properties that userspace needs specific knowledge about.  
> > 
> > And this is where our understanding differs. I do not think userspace
> > has to care at all. All it has to do is to provide the saved register
> > values to the target system, and let KVM accept or refuse these
> > settings. I can't see what providing a set of predefined values back to
> > userspace gains us.
> > 
> > An unknown register on the target system fails the restore phase:
> > that's absolutely fine, as we don't want to run on a system that
> > doesn't know about the mitigation.
> > 
> > An incompatible value fails the restore as well, as KVM itself finds
> > that this is a service it cannot safely provide.
> > 
> > No userspace involvement, no QEMU upgrade required. Only the kernel
> > knows about it.  
> 
> Yes, this is what I understand as well. From experience, many times when
> we were not strict enough about some userland interface, it backfired.
> 
> The only case where such a forward-looking scheme would make sense is
> the case where the source system has a new kernel, advertising a new
> firmware workaround register, in an unknown or missing state (0 or -1).
> An older kernel on the target system might not know about this register.
> That would translate into "unknown", which is compatible with 0 or -1
> from the source. So migration would be fine, but we deny it because the
> new kernel returns -EINVAL.
> 
> But I am not sure this construct is worth implementing in the kernel. If
> people care about this case, they could implement a workaround in
> userland instead. Or just upgrade the target kernel before migration.

Upgrading the target may not be convenient. But more importantly, I
don't think we expect downgrades to be supported. This can break for an
infinity of reasons, such as the feature set implemented on the source
not being there on the target.

As you said, if userspace wants to bypass these restrictions, it can
alter the data before restoring.

> 
> >> We can tolerate a few though.  If we accumulate a significant number
> >> of errata/vulnerability properties that need to be reported to
> >> userspace, this may be worth revisiting.  If not, it doesn't matter.  
> > 
> > Andre: if you want this to make it into 5.1, the time is now.  
> 
> OK. So is v2 [1] fine then? This implements the much easier "bigger is
> better" scheme, but being 0 based instead of using a 4-bit signed encoding.
> Let me know if there is something to rework in there.
> 
> Cheers,
> Andre.
> 
> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2019-January/627739.html

Sorry, I've lost track of which is which. Please post something that is
consistent,  and addresses Steve's concerns if there is still any. Make
sure it applies on top of the current kvmarm/next, and provide
evidences that you've tested migration on the expected working and
expected failing configurations.

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm