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

[Janosch Frank]

On 11/28/19 5:45 PM, Cornelia Huck wrote:
> 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

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.

signature.asc
Description: OpenPGP digital signature