| Message ID | 20191120114334.2287-9-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: Protected Virtualization support | expand |
On Wed, 20 Nov 2019 06:43:27 -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, 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 <frankja@linux.ibm.com> > --- > 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 _NOTIF ? > > #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); You don't seem to do anything with this yet? > > 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); Doesn't handle_instruction() want to know whether it got a request for emulation vs a notification? > break; > case ICPT_PROGRAM:
On 11/21/19 3:07 PM, Cornelia Huck wrote: > On Wed, 20 Nov 2019 06:43:27 -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, 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 <frankja@linux.ibm.com> >> --- >> 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 > > _NOTIF ? Yeah, forgot about that > >> >> #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); > > You don't seem to do anything with this yet? No, I'm still a bit in the dark about how we want to tie protvirt into qemu. > >> >> 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); > > Doesn't handle_instruction() want to know whether it got a request for > emulation vs a notification? Currently not, the sclp patch looks at the vcpu run icptcode to figure out what's going on. > >> break; >> case ICPT_PROGRAM: > >
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 <frankja@linux.ibm.com> > --- > 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
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 <frankja@linux.ibm.com> >> --- >> 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.
On Thu, 28 Nov 2019 17:38:19 +0100 Janosch Frank <frankja@linux.ibm.com> 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 <frankja@linux.ibm.com> > >> --- > >> 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.
On 11/28/19 5:45 PM, Cornelia Huck wrote: > On Thu, 28 Nov 2019 17:38:19 +0100 > Janosch Frank <frankja@linux.ibm.com> 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 <frankja@linux.ibm.com> >>>> --- >>>> 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 In my latest answer to Thomas I stated that we could move to a separate pv instruction handler. I just had another look and rediscovered, that it would mean a lot more changes. I would need to duplicate the ipa/b parsing and for diagnose even the base+disp parsing. So yes, I'd like to treat the 104 like the 4 intercept... > 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"). We can remove the notification from QEMU, as far is I know, we moved the instruction that used this path to a 104 code. > > 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. > Yes I'll ask if we'll get a new code for instructions that are only valid in PV mode; currently there are none.
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); break; case ICPT_PROGRAM:
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 <frankja@linux.ibm.com> --- target/s390x/kvm.c | 6 ++++++ 1 file changed, 6 insertions(+)