Re: [PATCH v3 05/22] target/i386/monitor: Return QMP error when SEV is disabled in build

[Philippe Mathieu-Daudé]

On 10/4/21 10:11, Paolo Bonzini wrote:
> On 02/10/21 14:53, Philippe Mathieu-Daudé wrote:
>> If the management layer tries to inject a secret, it gets an empty
>> response in case the binary built without SEV:
>>
>>    { "execute": "sev-inject-launch-secret",
>>      "arguments": { "packet-header": "mypkt", "secret": "mypass",
>> "gpa": 4294959104 }
>>    }
>>    {
>>        "return": {
>>        }
>>    }
>>
>> Make it clearer by returning an error, mentioning the feature is
>> disabled:
>>
>>    { "execute": "sev-inject-launch-secret",
>>      "arguments": { "packet-header": "mypkt", "secret": "mypass",
>> "gpa": 4294959104 }
>>    }
>>    {
>>        "error": {
>>            "class": "GenericError",
>>            "desc": "this feature or command is not currently supported"
>>        }
>>    }
>>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   target/i386/monitor.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
>> index 196c1c9e77f..a9f85acd473 100644
>> --- a/target/i386/monitor.c
>> +++ b/target/i386/monitor.c
>> @@ -28,6 +28,7 @@
>>   #include "monitor/hmp-target.h"
>>   #include "monitor/hmp.h"
>>   #include "qapi/qmp/qdict.h"
>> +#include "qapi/qmp/qerror.h"
>>   #include "sysemu/kvm.h"
>>   #include "sysemu/sev.h"
>>   #include "qapi/error.h"
>> @@ -743,6 +744,10 @@ void qmp_sev_inject_launch_secret(const char
>> *packet_hdr,
>>                                     bool has_gpa, uint64_t gpa,
>>                                     Error **errp)
>>   {
>> +    if (!sev_enabled()) {
>> +        error_setg(errp, QERR_UNSUPPORTED);
>> +        return;
>> +    }
>>       if (!has_gpa) {
>>           uint8_t *data;
>>           struct sev_secret_area *area;
>>
> 
> This should be done in the sev_inject_launch_secret stub instead, I
> think.  Or if you do it here, you can remove the "if (!sev_guest)"
> conditional in the non-stub version.

This part is not related to SEV builtin; what we want to avoid here
is management layer to try to inject secret while the guest hasn't
been started with SEV (IOW 'no memory encryption requested for KVM).

Maybe this error message is more explicit?

  error_setg(errp, "Guest is not using memory encryption");

Or:

  error_setg(errp, "Guest is not using SEV");