[08/15] s390x: protvirt: KVM intercept changes
diff mbox series

Message ID 20191120114334.2287-9-frankja@linux.ibm.com
State New
Headers show
Series
  • s390x: Protected Virtualization support
Related show

Commit Message

Janosch Frank Nov. 20, 2019, 11:43 a.m. UTC
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(+)

Comments

Cornelia Huck Nov. 21, 2019, 2:07 p.m. UTC | #1
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:
Janosch Frank Nov. 21, 2019, 2:29 p.m. UTC | #2
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:
> 
>
Thomas Huth Nov. 21, 2019, 3:11 p.m. UTC | #3
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
Janosch Frank Nov. 28, 2019, 4:38 p.m. UTC | #4
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.
Cornelia Huck Nov. 28, 2019, 4:45 p.m. UTC | #5
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.
Janosch Frank Nov. 28, 2019, 4:54 p.m. UTC | #6
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.

Patch
diff mbox series

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: