Re: [Qemu-devel] [PATCH v3 03/14] migration.json: add AMD SEV specific migration parameters

[Singh, Brijesh]

On 8/8/19 5:48 AM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (address@hidden) wrote:
>> On 8/7/19 6:06 AM, Dr. David Alan Gilbert wrote:
>>> * Singh, Brijesh (address@hidden) wrote:
>>>> AMD SEV migration flow requires that target machine's public Diffie-Hellman
>>>> key (PDH) and certificate chain must be passed before initiating the guest
>>>> migration. User can use QMP 'migrate-set-parameters' to pass the 
>>>> certificate
>>>> chain. The certificate chain will be used while creating the outgoing
>>>> encryption context.
>>>>
>>>> Signed-off-by: Brijesh Singh <address@hidden>
>>>> ---
>>>>
>>>> I was able to pass the certificate chain through the HMP but somehow
>>>> QMP socket interface is not working for me. If anyone has any tips on
>>>> what I am missing in the patch then please let me know. In meantime,
>>>> I will also continue my investigation on why its not working for me.
>>> It looks OK to me; what's the qmp you're trying and what's the failure
>>> error?
> Before I forget, you've not updated hmp_info_migrate_parameters in
> hmp-cmds.c, e.g.:
>
>              MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
>              params->has_tls_authz ? params->tls_authz : "");
> +        monitor_printf(mon, "%s:'%s'\n",
> +            MigrationParameter_str(MIGRATION_PARAMETER_SEV_PDH),
> +            params->has_sev_pdh ? params->sev_pdh : "");
>      }


thanks, i will address it in next rev.


>> I am not seeing any error. I am using the below command through qmp-shell.
>>
>> (qmp) migrate-set-paramaters sev-pdh="...." sev-plat-cert="...."
>> sev-amd-cert="..."
>>
>>
>> The command does not return any error. I added some debugs in
>> migrate_params_test_apply() and qmp_migrate_set_parameters() to see the
>> valye of params->has_sev_pdh and its always zero. The functions are
>> getting called when I issue the migrate-set-parameters qmp but the
>> values are not passed hence the memory_encryption->setup() never gets
>> called.
> Driving QMP by hand I'm seeing it apparently be stored in the
> parameters:
>
> Escape character is '^]'.
> {"QMP": {"version": {"qemu": {"micro": 94, "minor": 0, "major": 4}, 
> "package": "v4.1.0-rc4-16-ge6e1f28afd-dirty"}, "capabilities": ["oob"]}}
> { "execute": "qmp_capabilities" }
> {"return": {}}
> { "execute": "migrate-set-parameters" , "arguments": { "sev-pdh": "foo" } }
> {"return": {}}
>
> then from HMP with the patch above:
> (qemu) info migrate_parameters 
> ....
> sev-pdh:'foo'


Hmm, I have been using scripts to automate all these certificating
passing. let me closely look into it and see if its script bug.


>>>>  void qmp_migrate_set_parameters(MigrateSetParameters *params, Error 
>>>> **errp)
>>>> @@ -1410,6 +1443,27 @@ void 
>>>> qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>>>>          params->tls_hostname->type = QTYPE_QSTRING;
>>>>          params->tls_hostname->u.s = strdup("");
>>>>      }
>>>> +    /* TODO Rewrite "" to null instead */
>>>> +    if (params->has_sev_pdh
>>>> +        && params->sev_pdh->type == QTYPE_QNULL) {
>>>> +        qobject_unref(params->sev_pdh->u.n);
>>>> +        params->sev_pdh->type = QTYPE_QSTRING;
>>>> +        params->sev_pdh->u.s = strdup("");
>>>> +    }
>>>> +    /* TODO Rewrite "" to null instead */
>>>> +    if (params->has_sev_plat_cert
>>>> +        && params->sev_plat_cert->type == QTYPE_QNULL) {
>>>> +        qobject_unref(params->sev_plat_cert->u.n);
>>>> +        params->sev_plat_cert->type = QTYPE_QSTRING;
>>>> +        params->sev_plat_cert->u.s = strdup("");
>>>> +    }
>>>> +    /* TODO Rewrite "" to null instead */
>>>> +    if (params->has_sev_amd_cert
>>>> +        && params->sev_amd_cert->type == QTYPE_QNULL) {
>>>> +        qobject_unref(params->sev_amd_cert->u.n);
>>>> +        params->sev_amd_cert->type = QTYPE_QSTRING;
>>>> +        params->sev_amd_cert->u.s = strdup("");
>>>> +    }
>>>>  
>>>>      migrate_params_test_apply(params, &tmp);
>>>>  
>>>> @@ -3466,6 +3520,9 @@ static void migration_instance_finalize(Object *obj)
>>>>      qemu_mutex_destroy(&ms->qemu_file_lock);
>>>>      g_free(params->tls_hostname);
>>>>      g_free(params->tls_creds);
>>>> +    g_free(params->sev_pdh);
>>>> +    g_free(params->sev_plat_cert);
>>>> +    g_free(params->sev_amd_cert);
>>>>      qemu_sem_destroy(&ms->rate_limit_sem);
>>>>      qemu_sem_destroy(&ms->pause_sem);
>>>>      qemu_sem_destroy(&ms->postcopy_pause_sem);
>>>> @@ -3507,6 +3564,10 @@ static void migration_instance_init(Object *obj)
>>>>      params->has_announce_rounds = true;
>>>>      params->has_announce_step = true;
>>>>  
>>>> +    params->sev_pdh = g_strdup("");
>>>> +    params->sev_plat_cert = g_strdup("");
>>>> +    params->sev_amd_cert = g_strdup("");
>>>> +
>>>>      qemu_sem_init(&ms->postcopy_pause_sem, 0);
>>>>      qemu_sem_init(&ms->postcopy_pause_rp_sem, 0);
>>>>      qemu_sem_init(&ms->rp_state.rp_sem, 0);
>>>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>>> index 5ca3ebe942..354219f27a 100644
>>>> --- a/monitor/hmp-cmds.c
>>>> +++ b/monitor/hmp-cmds.c
>>>> @@ -1872,6 +1872,24 @@ void hmp_migrate_set_parameter(Monitor *mon, const 
>>>> QDict *qdict)
>>>>          p->has_announce_step = true;
>>>>          visit_type_size(v, param, &p->announce_step, &err);
>>>>          break;
>>>> +    case MIGRATION_PARAMETER_SEV_PDH:
>>>> +        p->has_sev_pdh = true;
>>>> +        p->sev_pdh = g_new0(StrOrNull, 1);
>>>> +        p->sev_pdh->type = QTYPE_QSTRING;
>>>> +        visit_type_str(v, param, &p->sev_pdh->u.s, &err);
>>>> +        break;
>>>> +    case MIGRATION_PARAMETER_SEV_PLAT_CERT:
>>>> +        p->has_sev_plat_cert = true;
>>>> +        p->sev_plat_cert = g_new0(StrOrNull, 1);
>>>> +        p->sev_plat_cert->type = QTYPE_QSTRING;
>>>> +        visit_type_str(v, param, &p->sev_plat_cert->u.s, &err);
>>>> +        break;
>>>> +    case MIGRATION_PARAMETER_SEV_AMD_CERT:
>>>> +        p->has_sev_amd_cert = true;
>>>> +        p->sev_amd_cert = g_new0(StrOrNull, 1);
>>>> +        p->sev_amd_cert->type = QTYPE_QSTRING;
>>>> +        visit_type_str(v, param, &p->sev_amd_cert->u.s, &err);
>>>> +        break;
>>>>      default:
>>>>          assert(0);
>>>>      }
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index 9cfbaf8c6c..bb07995d2c 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -580,6 +580,15 @@
>>>>  # @max-cpu-throttle: maximum cpu throttle percentage.
>>>>  #                    Defaults to 99. (Since 3.1)
>>>>  #
>>>> +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
>>>> +#           (Since 4.2)
>>>> +#
>>>> +# @sev-plat-cert: The target host platform certificate chain encoded in 
>>>> base64
>>>> +#                 (Since 4.2)
>>>> +#
>>>> +# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded 
>>>> in
>>>> +#                base64 (Since 4.2)
>>>> +#
>>>>  # Since: 2.4
>>>>  ##
>>>>  { 'enum': 'MigrationParameter',
>>>> @@ -592,7 +601,7 @@
>>>>             'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
>>>>             'multifd-channels',
>>>>             'xbzrle-cache-size', 'max-postcopy-bandwidth',
>>>> -           'max-cpu-throttle' ] }
>>>> +           'max-cpu-throttle', 'sev-pdh', 'sev-plat-cert', 'sev-amd-cert' 
>>>> ] }
>>>>  
>>>>  ##
>>>>  # @MigrateSetParameters:
>>>> @@ -682,6 +691,15 @@
>>>>  # @max-cpu-throttle: maximum cpu throttle percentage.
>>>>  #                    The default value is 99. (Since 3.1)
>>>>  #
>>>> +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
>>>> +#           (Since 4.2)
>>>> +#
>>>> +# @sev-plat-cert: The target host platform certificate chain encoded in 
>>>> base64
>>>> +#                 (Since 4.2)
>>>> +#
>>>> +# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded 
>>>> in
>>>> +#                base64 (Since 4.2)
>>>> +#
>>>>  # Since: 2.4
>>>>  ##
>>>>  # TODO either fuse back into MigrationParameters, or make
>>>> @@ -707,7 +725,10 @@
>>>>              '*multifd-channels': 'int',
>>>>              '*xbzrle-cache-size': 'size',
>>>>              '*max-postcopy-bandwidth': 'size',
>>>> -      '*max-cpu-throttle': 'int' } }
>>>> +            '*max-cpu-throttle': 'int',
>>>> +            '*sev-pdh':'StrOrNull',
>>>> +            '*sev-plat-cert': 'StrOrNull',
>>>> +            '*sev-amd-cert' : 'StrOrNull' } }
>>>>  
>>>>  ##
>>>>  # @migrate-set-parameters:
>>>> @@ -817,6 +838,15 @@
>>>>  #                    Defaults to 99.
>>>>  #                     (Since 3.1)
>>>>  #
>>>> +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
>>>> +#           (Since 4.2)
>>>> +#
>>>> +# @sev-plat-cert: The target host platform certificate chain encoded in 
>>>> base64
>>>> +#                 (Since 4.2)
>>>> +#
>>>> +# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded 
>>>> in
>>>> +#                base64 (Since 4.2)
>>>> +#
>>>>  # Since: 2.4
>>>>  ##
>>>>  { 'struct': 'MigrationParameters',
>>>> @@ -839,8 +869,11 @@
>>>>              '*block-incremental': 'bool' ,
>>>>              '*multifd-channels': 'uint8',
>>>>              '*xbzrle-cache-size': 'size',
>>>> -      '*max-postcopy-bandwidth': 'size',
>>>> -            '*max-cpu-throttle':'uint8'} }
>>>> +            '*max-postcopy-bandwidth': 'size',
>>>> +            '*max-cpu-throttle':'uint8',
>>>> +            '*sev-pdh':'str',
>>>> +            '*sev-plat-cert': 'str',
>>>> +            '*sev-amd-cert' : 'str'} }
>>>>  
>>>>  ##
>>>>  # @query-migrate-parameters:
>>>> -- 
>>>> 2.17.1
>>>>
>>> --
>>> Dr. David Alan Gilbert / address@hidden / Manchester, UK
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK