Re: [PATCH v2 06/13] s390x: protvirt: KVM intercept changes

[Cornelia Huck <cohuck@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3731 bytes --]

On Fri, 6 Dec 2019 09:45:41 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 12/6/19 9:29 AM, Cornelia Huck wrote:
> > On Fri, 6 Dec 2019 08:44:52 +0100
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> On 12/5/19 6:46 PM, Cornelia Huck wrote:  
> >>> On Thu, 5 Dec 2019 18:34:32 +0100
> >>> Janosch Frank <frankja@linux.ibm.com> wrote:
> >>>     
> >>>> On 12/5/19 6:15 PM, Cornelia Huck wrote:    
> >>>>> On Fri, 29 Nov 2019 04:48:02 -0500
> >>>>> Janosch Frank <frankja@linux.ibm.com> wrote:
> >>>>>       
> >>>>>> Secure guests no longer intercept with code 4 for an instruction
> >>>>>> interception. Instead they have codes 104 and 108 for secure
> >>>>>> instruction interception and secure instruction notification
> >>>>>> respectively.
> >>>>>>
> >>>>>> The 104 mirrors the 4 interception.
> >>>>>>
> >>>>>> The 108 is a notification interception to let KVM and QEMU know that
> >>>>>> something changed and we need to update tracking information or
> >>>>>> perform specific tasks. It's currently taken for the following
> >>>>>> instructions:
> >>>>>>
> >>>>>> * stpx (To inform about the changed prefix location)
> >>>>>> * sclp (On incorrect SCCB values, so we can inject a IRQ)
> >>>>>> * sigp (All but "stop and store status")
> >>>>>> * diag308 (Subcodes 0/1)
> >>>>>>
> >>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >>>>>> ---
> >>>>>>  target/s390x/kvm.c | 6 ++++++
> >>>>>>  1 file changed, 6 insertions(+)
> >>>>>>    
> >>>     
> >>>>>> @@ -1664,6 +1668,8 @@ static int handle_intercept(S390CPU *cpu)
> >>>>>>              (long)cs->kvm_run->psw_addr);
> >>>>>>      switch (icpt_code) {
> >>>>>>          case ICPT_INSTRUCTION:
> >>>>>> +        case ICPT_PV_INSTR:
> >>>>>> +        case ICPT_PV_INSTR_NOTIFICATION:
> >>>>>>              r = handle_instruction(cpu, run);      
> >>>>>
> >>>>> I'm still a bit uneasy about going through the same path for both 104
> >>>>> and 108. How does the handler figure out whether it should emulate an
> >>>>> instruction, or just process a notification? Is it guaranteed that a
> >>>>> given instruction is always showing up as either a 104 or a 108, so
> >>>>> that the handler can check the pv state?      
> >>>>
> >>>> diag 308 subcode 0/1 are 108, but all other subcodes are defined as a
> >>>> 104 (if they are an exit at all)...    
> >>>
> >>> I think that's a reason to really split 108 from 4/104, or at least add
> >>> an parameter...    
> >>
> >> And still call the diag 308 handler or have separate handlers?  
> > 
> > I'd probably split it into a "normal" one and one for pv special
> > handling... does that make sense?
> >   
> IMHO: not really
> We still need to do ipa/ipb parsing for both paths, which will result in
> code duplication. Looking at diag308 subcode 4, we would have a code 4
> one which just does the device resets and reboots and one which does all
> that, plus the teardown of the protected guest.
> 
> I tried to inline as much as possible to have as little changes as
> possible. Notable exception is sclp, which has more checks than
> emulation code...

Fair enough.

But taking a step back: What's the purpose of the new exits, then?
IIUC, we have the following cases:

- code 4: normal guest, nothing special
- code 104: protected guest, emulate the instruction
- code 108: protected guest, notification for the instruction

The backend code can figure out what to do simply by checking whether
the guest is protected or not (as whatever needs to be done is simply
determined by that anyway).

Are we overlooking something? Or is the information contained in the
different exits simply redundant?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]