Re: [PATCH v3 1/2] KVM: s390: Don't indicate suppression on dirtying, failing memop

[Christian Borntraeger]

Am 12.05.22 um 17:50 schrieb David Hildenbrand:
> On 12.05.22 15:51, Christian Borntraeger wrote:
> > Am 12.05.22 um 15:22 schrieb David Hildenbrand:
> > > On 12.05.22 15:10, Janis Schoetterl-Glausch wrote:
> > > > If user space uses a memop to emulate an instruction and that
> > > > memop fails, the execution of the instruction ends.
> > > > Instruction execution can end in different ways, one of which is
> > > > suppression, which requires that the instruction execute like a no-op.
> > > > A writing memop that spans multiple pages and fails due to key
> > > > protection may have modified guest memory, as a result, the likely
> > > > correct ending is termination. Therefore, do not indicate a
> > > > suppressing instruction ending in this case.
> > >
> > > I think that is possibly problematic handling.
> > >
> > > In TCG we stumbled in similar issues in the past for MVC when crossing
> > > page boundaries. Failing after modifying the first page already
> > > seriously broke some user space, because the guest would retry the
> > > instruction after fixing up the fault reason on the second page: if
> > > source and destination operands overlap, you'll be in trouble because
> > > the input parameters already changed.
> > >
> > > For this reason, in TCG we make sure that all accesses are valid before
> > > starting modifications.
> > >
> > > See target/s390x/tcg/mem_helper.c:do_helper_mvc with access_prepare()
> > > and friends as an example.
> > >
> > > Now, I don't know how to tackle that for KVM, I just wanted to raise
> > > awareness that injecting an interrupt after modifying page content is
> > > possible dodgy and dangerous.
> >
> > this is really special and only for key protection crossing pages.
> > Its been done since the 70ies in that way on z/VM. The architecture
> > is and was always written in a way to allow termination for this
> > case for hypervisors.
>
> Just so I understand correctly: all instructions that a hypervisor with
> hardware virtualization is supposed to emulate are "written in a way to
> allow termination", correct? That makes things a lot easier.

Only for key protection. Key protection can always be terminating no matter
what the instruction says. This is historical baggage - key protection was
resulting in abends - killing the process. So it does not matter if we
provide the extra info as in enhanced suppression on protection as nobody
is making use of that (apart from debuggers maybe).