On 3/9/20 2:41 PM, David Hildenbrand wrote: > On 09.03.20 12:21, Janosch Frank wrote: >> Migration is not yet supported. >> >> Signed-off-by: Janosch Frank <[email protected]> >> --- >> hw/s390x/s390-virtio-ccw.c | 26 ++++++++++++++++++++++++-- >> 1 file changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index f718cfc591..4bb38704ff 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) >> { >> @@ -331,16 +334,33 @@ static void s390_machine_unprotect(S390CcwMachineState >> *ms) >> } >> ms->pv = false; >> } >> + migrate_del_blocker(pv_mig_blocker); >> + error_free(pv_mig_blocker); >> + pv_mig_blocker = NULL; > > Maybe use error_free_or_abort(&pv_mig_blocker);
I dislike hiding setting the ptr to NULL with that function.
The assert for NULL in error_setg has caused me a lot of headache initially.
But if that's how it's done I'll fix it.
>
>> }
>>
>> static int s390_machine_protect(S390CcwMachineState *ms)
>> {
>> + static Error *local_err;
>> CPUState *t;
>> int rc;
>>
>> + error_setg(&pv_mig_blocker,
>> + "protected VMs are currently not migrateable.");
>> + rc = migrate_add_blocker(pv_mig_blocker, &local_err);
>> + if (local_err) {
>> + error_report_err(local_err);
>> + error_free(pv_mig_blocker);
>> + pv_mig_blocker = NULL;
>
> Dito.
>
>> + return rc;
>> + }
>> +
>> /* Create SE VM */
>> rc = s390_pv_vm_enable();
>> if (rc) {
>> + error_report_err(local_err);
>> + error_free(pv_mig_blocker);
>> + pv_mig_blocker = NULL;
>
> Dito.
>
>> return rc;
>> }
>>
>> @@ -470,11 +490,13 @@ static void s390_machine_reset(MachineState *machine)
>>
>> if (s390_machine_protect(ms)) {
>> s390_machine_inject_pv_error(cs);
>> - s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>> - return;
>> + goto pv_err;
>> }
>>
>> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>> +pv_err:
>> + /* Continue after the diag308 so the guest knows something went
>> wrong. */
>> + s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>> break;
>
> Why does this change *not* belong into path #2? I think I asked this
> already.
Yes, I'll move it
signature.asc
Description: OpenPGP digital signature
