On 11/28/19 5:45 PM, Cornelia Huck wrote: > On Thu, 28 Nov 2019 17:38:19 +0100 > Janosch Frank 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 >>>> --- >>>> 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.