Re: [PATCH 08/15] s390x: protvirt: KVM intercept changes

[Cornelia Huck]

On Thu, 28 Nov 2019 17:38:19 +0100
Janosch Frank <address@hidden> wrote:

> On 11/21/19 4:11 PM, Thomas Huth wrote:
> > On 20/11/2019 12.43, Janosch Frank 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, but the 108 is a notification, that something
> >> happened and the hypervisor might need to adjust its tracking data to
> >> that fact. An example for that is the set prefix notification
> >> interception, where KVM only reads the new prefix, but does not update
> >> the prefix in the state description.
> >>
> >> Signed-off-by: Janosch Frank <address@hidden>
> >> ---
> >>  target/s390x/kvm.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >> index 418154ccfe..58251c0229 100644
> >> --- a/target/s390x/kvm.c
> >> +++ b/target/s390x/kvm.c
> >> @@ -115,6 +115,8 @@
> >>  #define ICPT_CPU_STOP                   0x28
> >>  #define ICPT_OPEREXC                    0x2c
> >>  #define ICPT_IO                         0x40
> >> +#define ICPT_PV_INSTR                   0x68
> >> +#define ICPT_PV_INSTR_NOT               0x6c
> >>  
> >>  #define NR_LOCAL_IRQS 32
> >>  /*
> >> @@ -151,6 +153,7 @@ static int cap_s390_irq;
> >>  static int cap_ri;
> >>  static int cap_gs;
> >>  static int cap_hpage_1m;
> >> +static int cap_protvirt;
> >>  
> >>  static int active_cmma;
> >>  
> >> @@ -336,6 +339,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >>      cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
> >>      cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
> >>      cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
> >> +    cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
> >>  
> >>      if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
> >>          || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
> >> @@ -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_NOT:
> >>              r = handle_instruction(cpu, run);  
> > 
> > Even if this works by default, my gut feeling tells me that it would be
> > safer and cleaner to have a separate handler for this...
> > Otherwise we might get surprising results if future machine generations
> > intercept/notify for more or different instructions, I guess?
> > 
> > However, it's just a gut feeling ... I really don't have much experience
> > with this PV stuff yet ... what do the others here think?
> > 
> >  Thomas  
> 
> 
> Adding a handle_instruction_pv doesn't hurt me too much.
> The default case can then do an error_report() and exit(1);
> 
> PV was designed in a way that we can re-use as much code as possible, so
> I tried using the normal instruction handlers and only change as little
> as possible in the instructions themselves.

I think we could argue that handling 4 and 104 in the same function
makes sense; but the 108 notification should really be separate, I
think. From what I've seen, the expectation of what the hypervisor
needs to do is just something else in this case ("hey, I did something;
just to let you know").

Is the set of instructions you get a 104 for always supposed to be a
subset of the instructions you get a 4 for? I'd expect it to be so.

pgpB7oo6s5ke2.pgp
Description: OpenPGP digital signature