On 3/4/20 6:13 PM, David Hildenbrand wrote: > On 04.03.20 12:42, Janosch Frank wrote: >> Migration is not yet supported. >> >> Signed-off-by: Janosch Frank <[email protected]> >> --- >> hw/s390x/s390-virtio-ccw.c | 33 ++++++++++++++++++++++++--------- >> 1 file changed, 24 insertions(+), 9 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index dd39890f89..272531a9ee 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -43,6 +43,9 @@ >> #include "sysemu/sysemu.h" >> #include "hw/s390x/pv.h" >> #include <linux/kvm.h> >> +#include "migration/blocker.h" >> + >> +static Error *pv_mig_blocker; >> >> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) >> { >> @@ -324,19 +327,30 @@ static void s390_machine_unprotect(S390CcwMachineState >> *ms) >> { >> CPUState *t; >> >> - if (!ms->pv) >> - return; >> - s390_pv_vm_disable(); >> - CPU_FOREACH(t) { >> - S390_CPU(t)->env.pv = false; >> + if (ms->pv) { >> + s390_pv_vm_disable(); >> + CPU_FOREACH(t) { >> + S390_CPU(t)->env.pv = false; >> + } >> + ms->pv = false; >> } >> - ms->pv = false; >> + migrate_del_blocker(pv_mig_blocker); > > Is it just me or is this unnecessary code churn that wants to challenge > my reviewing capabilities? ;) Please perform that rework in the previous > patch already.
It's rather more of personal style choice to do a "if return" to not
encapsulate everything in ifs.
>
> Also, I do wonder why the blocker is always removed (IOW, how we could
> end up unprotecting, although there is nothing to unprotect).
>
> This needs a comment in the patch description.
You mean the error check for migrate_add_blocker()?
I'm still metabolizing my coffee...
>
>
>> }
>>
>> static int s390_machine_protect(S390CcwMachineState *ms)
>> {
>> + static Error *local_err;
>> CPUState *t;
>> - int rc;
>> + int rc = -1;
>> +
>> + if (!pv_mig_blocker) {
>
> I think we should instead error_free() when unprotecting ...
Ack
>
>> + error_setg(&pv_mig_blocker,
>> + "protected VMs are currently not migrateable.");
>> + }
>> + migrate_add_blocker(pv_mig_blocker, &local_err);
>> + if (local_err) {
>> + goto out_err;
>
> ... and on the error path.
To also return the rc code, I changed it to:
rc = migrate_add_blocker(pv_mig_blocker, &local_err);
if (local_err) {
error_free(pv_mig_blocker);
return rc;
}
>
>> + }
>>
>> /* Create SE VM */
>> rc = s390_pv_vm_enable();
>> @@ -440,11 +454,12 @@ static void s390_machine_reset(MachineState *machine)
>>
>> if (s390_machine_protect(ms)) {
>> s390_machine_inject_pv_error(cs);
>
> ew, injecting an exception during a reset. (I know this belongs into the
> previous patch)
Yeah, not too proud about this one, but doing it at diag308 time is not
possible, since we need to reset the devices before attempting to protect.
>
>> - s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>> - return;
>> + goto pv_err;
>
> I have no idea why this hunk is in this patch. What am I missing?
The error needs to be communicated to the guest, so we need to resume
CPU operation instead of doing a load which we would do on success.
>
>> }
>>
>> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>> +pv_err:
>> + s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>> break;
>> default:
>> g_assert_not_reached();
>>
>
>
signature.asc
Description: OpenPGP digital signature
