3:14 p.m.
Hi John,
Thanks for very details review feedbacks.
On 04/02/2018 01:26 PM, John Ferlan wrote:
On 04/02/2018 10:18 AM, Brijesh Singh wrote:
> The launch-security element can be used to define the security
> model to use when launching a domain. Currently we support 'sev'.
>
> When 'sev' is used, the VM will be launched with AMD SEV feature enabled.
> SEV feature supports running encrypted VM under the control of KVM.
> Encrypted VMs have their pages (code and data) secured such that only the
> guest itself has access to the unencrypted version. Each encrypted VM is
> associated with a unique encryption key; if its data is accessed to a
> different entity using a different key the encrypted guests data will be
> incorrectly decrypted, leading to unintelligible data.
>
> Reviewed-by: "Daniel P. Berrangé" <berrange(a)redhat.com>
> Signed-off-by: Brijesh Singh <brijesh.singh(a)amd.com>
> ---
> docs/formatdomain.html.in | 120 ++++++++++++++++++++++++++++++++++++++++++
> docs/schemas/domaincommon.rng | 39 ++++++++++++++
> src/conf/domain_conf.c | 110 ++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 26 +++++++++
> 4 files changed, 295 insertions(+)
>
Again - I see the R-by from Daniel, but I have more comments for the
patches...
Also since this patch is where we introduce the XML, the xml2xml changes
that are in the last patch should move into here so that we keep
everything together.
Noted, I will merge the hunks from last patch in this patch.
We're probably getting beyond the point of me doing the movement,
so a
v6 will be needed.
Yes, I can see us doing v6 and will roll all other review feedbacks.
> diff --git a/docs/formatdomain.html.in
b/docs/formatdomain.html.in
> index 82e7d7c..2a6bed7 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -8200,6 +8200,126 @@ qemu-kvm -net nic,model=? /dev/null
>
> <p>Note: DEA/TDEA is synonymous with DES/TDES.</p>
>
> + <h3><a id="sev">Secure Encrypted Virtualization
(SEV)</a></h3>
> +
> + <p>
> + The contents of the <code><launch-security
type='sev'></code> element
> + is used to provide the guest owners input used for creating an encrypted
> + VM using the AMD SEV feature.
> +
> + SEV is an extension to the AMD-V architecture which supports running
> + encrypted virtual machine (VMs) under the control of KVM. Encrypted
> + VMs have their pages (code and data) secured such that only the guest
> + itself has access to the unencrypted version. Each encrypted VM is
> + associated with a unique encryption key; if its data is accessed to a
> + different entity using a different key the encrypted guests data will
> + be incorrectly decrypted, leading to unintelligible data.
> +
> + For more information see various input parameters and its format see SEV API
spec
s/see/see the/
> + <a
href="https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specificat...
https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf </a>
> + <span class="since">Since 4.2.0</span>
I'll be 4.3.0 now that 4.2.0 is released.
Noted.
> + </p>
> + <pre>
> +<domain>
> + ...
> + <launch-security type='sev'>
> + <policy> 0 </policy>
> + <cbitpos> 47 </cbitpos>
> + <reduced-phys-bits> 5 </reduced-phys-bits>
> + <session> ... </session>
> + <dh-cert> ... </dh>
> + </sev>
> + ...
> +</domain>
> +</pre>
Your example should provide some more realistic values.
Sure, I will add some realistic value.
> +
> + <p>
> + A least <code>cbitpos</code> and
<code>reduced-phys-bits</code> must be
> + nested within the <code>launch-security</code> element.
> + </p>
The above won't be necessary as long as you note required below, e.g.
Ah make sense. thanks
> + <dl>
> + <dt><code>cbitpos</code></dt>
> + <dd>The <code>cbitpos</code> element provides the C-bit (aka
encryption bit)
s/The/The required/
Noted.
> + location in guest page table entry. The value of
<code>cbitpos</code> is
> + hypervisor dependent and can be obtained through the
<code>sev</code> element
> + from domaincapabilities.
s/from domaincapabilities/from the domain capabilities/
Noted.
> + </dd>
> + <dt><code>reduced-phys-bits</code></dt>
> + <dd>The <code>reduced-phys-bits</code> element provides the
physical
s/The/The required/
Noted.
> + address bit reducation. Similar to <code>cbitpos</code> the value
of <code>
s/reducation/reduction/
Noted.
> + reduced-phys-bit</code> is hypervisor dependent and
can be obtained
> + through the <code>sev</code> element from domaincapabilities.
s/from domaincapabilities/from the domain capabilities/
Noted.
> + </dd>
> + <dt><code>policy</code></dt>
> + <dd>The optional <code>policy</code> element provides the
guest policy
> + which must be maintained by the SEV firmware. This policy is enforced by
> + the firmware and restricts what configuration and operational commands
> + can be performed on this guest by the hypervisor. The guest policy
> + provided during guest launch is bound to the guest and cannot be changed
> + throughout the lifetime of the guest. The policy is also transmitted
> + during snapshot and migration flows and enforced on the destination platform.
> +
There's some long lines above and below - while not a policy - for those
using 80 character wide displays, we try to keep lines in the 70-75 char
range.
OK, while I updating the file to address your other review feedbacks,
will wrap characters to fit in 80 char range.
> + The guest policy is a 4-byte structure with the fields
shown in Table:
How about 4 unsigned byte
I was trying to copy wording from SEV spec so that if anyone is trying
to look at spec and then come to see our libvirt html then it matches.
> +
> + <table class="top_table">
> + <tr>
> + <th> Bit(s) </th>
> + <th> Description </th>
> + </tr>
> + <tr>
> + <td> 0 </td>
> + <td> Debugging of the guest is disallowed when set </td>
> + </tr>
> + <tr>
> + <td> 1 </td>
> + <td> Sharing keys with other guests is disallowed when set
</td>
> + </tr>
> + <tr>
> + <td> 2 </td>
> + <td> SEV-ES is required when set</td>
> + </tr>
> + <tr>
> + <td> 3 </td>
> + <td> Sending the guest to another platform is disallowed when
set</td>
> + </tr>
> + <tr>
> + <td> 4 </td>
> + <td> The guest must not be transmitted to another platform that is
> + not in the domain when set. </td>
> + </tr>
> + <tr>
> + <td> 5 </td>
> + <td> The guest must not be transmitted to another platform that is
> + not SEV capable when set. </td>
> + </tr>
> + <tr>
> + <td> 15:6 </td>
> + <td> reserved </td>
> + </tr>
> + <tr>
> + <td> 16:32 </td>
> + <td> The guest must not be transmitted to another platform with a
> + lower firmware version. </td>
I don't see this in the qemu policies:
#define SEV_POLICY_NODBG 0x1
#define SEV_POLICY_NOKS 0x2
#define SEV_POLICY_ES 0x4
#define SEV_POLICY_NOSEND 0x8
#define SEV_POLICY_DOMAIN 0x10
#define SEV_POLICY_SEV 0x20
Sois there just one bit for the lower firmware thing or is this a word
field with some value expected?
It's actually 8 bit value which will be packed with the firmware major
and minor numbers. The policy value is blob to us from the Guest owner.
libvirt/qemu should simply pass as-is (any policy changes done by
libvirt or qemu user will be caught by the SEV firmware). Since policy
values can't be modified by the hypervisor hence I was not too keen on
documenting it. But in previous patch the feedback was to document it
hence I tried to copy/paste the value from spec.
> + </tr>
> + </table>
> + Default value is 0x1
So this is the QEMU default currently; however, that could change in the
future. I think we could just not say what the default is. It'll make
some changes a bit later on easier too.
It's libvirt default (in cases where guest owner did not provide policy
value). We also pass a valid policy value while invoking qemu from the
libvirt. So we don't depend on QEMU default changing in future.
> +
> + </dd>
> + <dt><code>dh-cert</code></dt>
> + <dd>The optional <code>dh-cert</code> element provides the
guest owners public
s/owners/owners base64 encoded/
Noted.
> + Diffie-Hellman (DH) key. The key is used to negotiate a
master secret
> + key between the SEV firmware and guest owner. This master secret key is
> + then used to establish a trusted channel between SEV firmware and guest
s/SEV/the SEV/
Noted.
> + owner. The value must be encoded in base64.
and the last sentence wouldn't be needed any more...
Noted.
> + </dd>
> + <dt><code>session</code></dt>
> + <dd>The optional <code>session</code> element provides the
guest owners
> + session blob defined in SEV API spec. The value must be encoded in base64.
s/session/base64 encoded session/
s/in/in the/
and the last sentence wouldn't be needed any more...
Noted.
> +
> + See SEV spec LAUNCH_START section for session blob format.
s/for/for the/
> + </dd>
> + </dl>
> +
> <h2><a id="examples">Example
configs</a></h2>
>
> <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index a72c919..6a0e129 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -77,6 +77,9 @@
> <optional>
> <ref name='keywrap'/>
> </optional>
> + <optional>
> + <ref name='launch-security'/>> +
</optional>> </interleave>
> </element>
> </define>
> @@ -436,6 +439,42 @@
> </element>
> </define>
>
> + <define name="launch-security">
> + <element name="launch-security">
> + <attribute name="type">
> + <value>sev</value>
> + </attribute>
> + <interleave>
> + <element name="cbitpos">
> + <data type='unsignedInt'/>
> + </element>
> + <element name="reduced-phys-bits">
> + <data type='unsignedInt'/>
> + </element>
> + <optional>
> + <element name="policy">
> + <ref name='hexuint'/>
> + </element>
> + </optional>
Hopefully hexuint will suffice over time... On the other hand, this
patch uses virXPathULongHex in order to parse.
IIRC, I was not able to find anything other than hexuint in
basictypes.rng and also was not able to a function like
virXPathUIntHex(..). If you can point me to the function which can be
used to get the hexuint then I am good with it. Also, I am open to
adding such function if it does not exist and anyone else sees the need
for it.
> + <optional>
> + <element name="handle">
> + <ref name='unsignedInt'/>
> + </element>
> + </optional>
> + <optional>
> + <element name="dh-cert">
> + <data type="string"/>
> + </element>
> + </optional>
> + <optional>
> + <element name="session">
> + <data type="string"/>
> + </element>
> + </optional>
> + </interleave>
> + </element>
> + </define>
> +
Similar to patch 2 questions... Should this be
"launchSecurity"
"reducedPhysBits"
"DHCert"
Changing has fairly major implications, but I'd rather not have it baked
in and then have someone throw a complaint and have to redo things.
Since many people are off today, let's give them at least until tomorrow
to provide feedback before spinning a v6...
Yes agree, lets see what everyone else thinks about naming before I go
off and change the series.
> <!--
> Enable or disable perf events for the domain. For each
> of the events the following rules apply:
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ae7c0d9..4acf45c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -929,6 +929,10 @@ VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST,
> "ivshmem-plain",
> "ivshmem-doorbell")
>
> +VIR_ENUM_IMPL(virDomainLaunchSecurity, VIR_DOMAIN_LAUNCH_SECURITY_LAST,
> + "",
> + "sev")
> +
> static virClassPtr virDomainObjClass;
> static virClassPtr virDomainXMLOptionClass;
> static void virDomainObjDispose(void *obj);
> @@ -2897,6 +2901,14 @@ virDomainCachetuneDefFree(virDomainCachetuneDefPtr cachetune)
> VIR_FREE(cachetune);
> }
>
again 2 blank lines between functions
Noted.
> +static void
> +virDomainSevDefFree(virDomainSevDefPtr def)
> +{
Rather than making callers know...
OK.
if (!def)
return;
> + VIR_FREE(def->dh_cert);
> + VIR_FREE(def->session);
> +
> + VIR_FREE(def);
> +}
>
> void virDomainDefFree(virDomainDefPtr def)
> {
> @@ -3079,6 +3091,9 @@ void virDomainDefFree(virDomainDefPtr def)
> if (def->namespaceData && def->ns.free)
> (def->ns.free)(def->namespaceData);
>
> + if (def->sev)
Meaning this one won't be necessary
OK
> + virDomainSevDefFree(def->sev);
> +
> xmlFreeNode(def->metadata);
>
> VIR_FREE(def);
> @@ -15537,6 +15552,70 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
> return ret;
> }
>
2 blank lines
Noted.
> +static virDomainSevDefPtr
> +virDomainSevDefParseXML(xmlNodePtr sevNode, xmlXPathContextPtr ctxt)
Two lines:
virDomainSevDefParseXML(xmlNodePtr sevNode,
xmlXPathContextPtr ctxt)
> +{
> + char *tmp = NULL, *type = NULL;
Prefer 2 lines here too:
Noted.
char *tmp = NULL;
char *type = NULL;
> + xmlNodePtr save = ctxt->node;
> + virDomainSevDefPtr def;
> + unsigned long policy;
probably should initialize = 0;
> +
> + ctxt->node = sevNode;
> +
> + if (VIR_ALLOC(def) < 0)
> + return NULL;
> +
> + if (!(type = virXMLPropString(sevNode, "type"))) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing launch-security type"));
> + goto error;
> + }
> +
Assuming 'sev' for domain_conf isn't "normal" - that is it's
not future
proof...
> + if (virDomainLaunchSecurityTypeFromString(type) !=
> + VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
This produces a libvirt failed for some reason type error message...
I will add error message.
> + goto error;
> + }
So, as ugly as this will look, using switch/case logic:
Works with me, thanks
def->sectype = virDomainLaunchSecurityTypeFromString(type);
switch((virDomainLaunchSecurity) def->sectype) {
case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
break;
case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
default:
virReportError(VIR_ERR_XML_ERROR,
_("unsupported launch-security type '%s'"),
type);
goto error;
}
NB: I've *added* ->sectype to @def [see below]
> +
> + if (virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos) <
0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s", _("failed to get
cbitpos"));
s/get/get launch-security/
> + goto error;
> + }
> +
> + if (virXPathUInt("string(./reduced-phys-bits)", ctxt,
> + &def->reduced_phys_bits) < 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("failed to get reduced-phys-bits"));
s/get/get launch-security/
> + goto error;
> + }
> +
> + if (virXPathULongHex("string(./policy)", ctxt, &policy) <
0)> + policy = 0x1;
Hmmm... This one is optional which makes things a bit interesting. How
do you know it was provided? Today the default could be 0x1, but
sometime in the future if you decide upon a different default, then
things get really complicated. Or for some other chip and/or hypervisor
the default won't be 1.
Firmware does not have default policy, a caller must provide the policy
value. In our cases, we are saying if caller does not provide a policy
in xml then we default to 0x1.
Also virXPathULongHex returns -1 or -2 w/ different meanings - if
it's
not there, You get a -1 when not provided and -2 when you have invalid
value in field, which should be an error.
ah thats good information, I was not aware of -2 thing.
Finally, ULongHex returns 64 bits and your field is 32 bits leading
to
possible overflow
Right, this is where I was struggling because there is no such function
as virXPathUIntHex(...) which ca be used to get uint32_t. Please let me
know your thoughts on how do you want me to handle this situation.
So, either you have to have a "policy_provided" boolean of
some sort or
I think preferably leave it as 0 (zero) indicating not provided and then
when generating a command line check against 0 and provide the
hypervisor dependent value on the command line *OR* just don't provided
it an let the hypervisor choose it's default value because the value
wasn't provided (that's a later patch though)
Also see [1] below...
So my suggestion is:
if (virXPathULongHex("string(./policy)", ctxt, &policy) == -2 ||
policy > UINT_MAX) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("invalid launch-security policy value"));
goto error;
}
def->policy = policy;
If -1 is returned, the def->policy = 0; otherwise, it's set to something.
> +
> + def->policy = policy;
> +
> + if ((tmp = virXPathString("string(./dh-cert)", ctxt))) {
> + if (VIR_STRDUP(def->dh_cert, tmp) < 0)
> + goto error;
> +
> + VIR_FREE(tmp);
> + }
> +
> + if ((tmp = virXPathString("string(./session)", ctxt))) {
> + if (VIR_STRDUP(def->session, tmp) < 0)
> + goto error;
> +
> + VIR_FREE(tmp);
> + }
> +
> + ctxt->node = save;
> + return def;
> +
> + error:
> + VIR_FREE(tmp);
> + virDomainSevDefFree(def);
> + ctxt->node = save;
> + return NULL;
> +}
2 blank lines
>
> static virDomainMemoryDefPtr
> virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
> @@ -20210,6 +20289,13 @@ virDomainDefParseXML(xmlDocPtr xml,
> ctxt->node = node;
> VIR_FREE(nodes);
>
> + /* Check for SEV feature */
> + if ((node = virXPathNode("./launch-security", ctxt)) != NULL) {
> + def->sev = virDomainSevDefParseXML(node, ctxt);
> + if (!def->sev)
> + goto error;
> + }
> +
> /* analysis of memory devices */
> if ((n = virXPathNodeSet("./devices/memory", ctxt, &nodes)) <
0)
> goto error;
> @@ -26087,6 +26173,27 @@ virDomainKeyWrapDefFormat(virBufferPtr buf,
virDomainKeyWrapDefPtr keywrap)
> }
>
2 blank lines
> static void
> +virDomainSevDefFormat(virBufferPtr buf, virDomainSevDefPtr sev)
Two lines e.g.
virDomainSevDefFormat(virBufferPtr buf,
virDomainSevDefPtr sev)
> +{
if (!sev)
return;
> + virBufferAddLit(buf, "<launch-security type='sev'>\n");
Since we shouldn't be assuming 'sev' at this point
virBufferAsprintf(buf, "<launch-security type='%s'>\n",
virDomainLaunchSecurityTypeToString(def->sectype));
> + virBufferAdjustIndent(buf, 2);
> +
> + virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n",
sev->cbitpos);
> + virBufferAsprintf(buf,
"<reduced-phys-bits>%d</reduced-phys-bits>\n",
> + sev->reduced_phys_bits);
> + virBufferAsprintf(buf, "<policy>%d</policy>\n",
sev->policy);
This has been defined as a "0x" number, but you're printing in decimal
format. Let's print in 0x value.
[1] Also if you leave this a 0 (zero), then you can compare sev->policy
> 0 before printing, e.g.:
if (sev->policy)
virBufferAsprintf(buf, "<policy>0x%x</policy>\n",
sev->policy);
you may also want to do a "0x%04x"
> + if (sev->dh_cert)
> + virBufferAsprintf(buf, "<dh_cert>%s</dh_cert>\n",
sev->dh_cert);
Use virBufferEscapeString
> +
> + if (sev->session)
> + virBufferAsprintf(buf, "<session>%s</session>\n",
sev->session);
Use virBufferEscapeString
> +
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</launch-security>\n");
> +}
> +
> +
> +static void
> virDomainPerfDefFormat(virBufferPtr buf, virDomainPerfDefPtr perf)
> {
> size_t i;
> @@ -27258,6 +27365,9 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> if (def->keywrap)
> virDomainKeyWrapDefFormat(buf, def->keywrap);
>
> + if (def->sev)
> + virDomainSevDefFormat(buf, def->sev);
> +
See above, no need for the if (def->sev)
> virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "</domain>\n");
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 61379e5..60f9dd2 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -142,6 +142,9 @@ typedef virDomainPanicDef *virDomainPanicDefPtr;
> typedef struct _virDomainMemoryDef virDomainMemoryDef;
> typedef virDomainMemoryDef *virDomainMemoryDefPtr;
>
> +typedef struct _virDomainSevDef virDomainSevDef;
> +typedef virDomainSevDef *virDomainSevDefPtr;
> +
> /* forward declarations virDomainChrSourceDef, required by
> * virDomainNetDef
> */
> @@ -2290,6 +2293,25 @@ struct _virDomainKeyWrapDef {
> };
>
> typedef enum {
> + VIR_DOMAIN_LAUNCH_SECURITY_NONE,
> + VIR_DOMAIN_LAUNCH_SECURITY_SEV,
> +
> + VIR_DOMAIN_LAUNCH_SECURITY_LAST,
> +} virDomainLaunchSecurity;
> +
> +typedef struct _virDomainSevDef virDomainSevDef;
> +typedef virDomainSevDef *virDomainSevDefPtr;
> +
> +struct _virDomainSevDef {
int sectype; /* enum virDomainLaunchSecurity */
John
> + char *dh_cert;
> + char *session;
> + unsigned int policy;
> + unsigned int cbitpos;
> + unsigned int reduced_phys_bits;
> +};
> +
> +
> +typedef enum {
> VIR_DOMAIN_IOMMU_MODEL_INTEL,
>
> VIR_DOMAIN_IOMMU_MODEL_LAST
> @@ -2454,6 +2476,9 @@ struct _virDomainDef {
>
> virDomainKeyWrapDefPtr keywrap;
>
> + /* SEV-specific domain */
> + virDomainSevDefPtr sev;
> +
> /* Application-specific custom metadata */
> xmlNodePtr metadata;
>
> @@ -3345,6 +3370,7 @@ VIR_ENUM_DECL(virDomainMemorySource)
> VIR_ENUM_DECL(virDomainMemoryAllocation)
> VIR_ENUM_DECL(virDomainIOMMUModel)
> VIR_ENUM_DECL(virDomainShmemModel)
> +VIR_ENUM_DECL(virDomainLaunchSecurity)
> /* from libvirt.h */
> VIR_ENUM_DECL(virDomainState)
> VIR_ENUM_DECL(virDomainNostateReason)
>