On 04/02/2018 12:33 PM, John Ferlan wrote:
On 04/02/2018 10:18 AM, Brijesh Singh wrote:
Extend hypervisor capabilities to include sev feature. When available,
hypervisor supports launching an encrypted VM on AMD platform. The
sev feature tag provides additional details like platform diffie-hellman
Diffie-Hellman
right?
Yes, I will use camel case in next patch.
key and certificate chain which can be used by the guest owner to
establish a cryptographic session with the SEV firmware to negotiate
keys used for attestation or to provide secret during launch.
Reviewed-by: "Daniel P. Berrangé" <berrange@xxxxxxxxxx>
Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
---
docs/formatdomaincaps.html.in | 40 ++++++++++++++++++++++++++++++++++++++++
docs/schemas/domaincaps.rng | 20 ++++++++++++++++++++
src/conf/domain_capabilities.c | 20 ++++++++++++++++++++
src/conf/domain_capabilities.h | 1 +
src/qemu/qemu_capabilities.c | 2 ++
5 files changed, 83 insertions(+)
I see this has Daniel's R-by, but I have a few notes and questions...
diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in
index 6bfcaf6..f383141 100644
--- a/docs/formatdomaincaps.html.in
+++ b/docs/formatdomaincaps.html.in
@@ -417,6 +417,12 @@
<value>3</value>
</enum>
</gic>
+ <sev>
+ <pdh> </pdh>
+ <cert-chain> </cert-chain>
+ <cbitpos> </cbitpos>
+ <reduced-phys-bits> </reduced-phys-bits>
+ </sev>
The example output should have some sort of example output and not an
empty space.
Noted, I will fill in some random values.
</features>
</domainCapabilities>
</pre>
@@ -441,5 +447,39 @@
<code>gic</code> element.</dd>
</dl>
+ <h4><a id="elementsSEV">SEV capabilities</a></h4>
+
+ <p>AMD Secure Encrypted Virtualization (SEV) capabilities are exposed under
+ the <code>sev</code> element.
+ SEV is an extension to the AMD-V architecture which supports running
+ virtual machines (VMs) under the control of a hypervisor. When supported,
+ guest owner can create a VM whose memory contents will be transparently
+ encrypted with a key unique to that VM.
I think it would be cleaner to add a </p> to after VM and then a new <p>
on the next line
Noted.
+
+ For more details on SEV feature see:
+ <a href="https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf";>
+ SEV API spec</a> and <a href="http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf";>
+ SEV White Paper</a>
+
+ </p>
+
+ <dl>
+ <dt><code>pdh</code></dt>
+ <dd>Platform diffie-hellman key, which can be exported to remote entities
Again, I think this should be Diffie-Hellman ?
And it's the public key right - so "A base64 encoded platform
Diffie-Hellman public key which..."
Noted.
+ which wish to establish a secure transport context with the SEV platform
"which wish" reads strange - how about "that desire"
+ in order to transmit data securely. The key is encoded in base64</dd>
Add "A base64 encoded" up front makes the last sentence duplicitous.
+ <dt><code>cert-chain</code></dt>
+ <dd> Platform certificate chain -- which includes platform endorsement key
+ (PEK), owners certificate authory (OCA) and chip endorsement key (CEK).
+ The certificate chain is encoded in base64.</dd>
A base64 encoded platform certificate chain that includes the platform
endorsement key (PEK), owners certificate authority (OCD), and chip
endorsement key (CEK).
Noted, makes it much cleaner. thanks
+ <dt><code>cbitpos</code></dt>
+ <dd>When memory encryption is enabled, one of the physical address bit
s/bit/bits
Noted.
+ (aka the C-bit) is utilized to mark if a memory page is protected. The
+ C-bit position is Hypervisor dependent.</dd>
+ <dt><code>reduced-phys-bits</code></dt>
+ <dd>When memory encryption is enabled, we loose certain bits in physical
+ address space. The number of bits we loose is hypervisor dependent.</dd>
+ </dl>
+
s/loose/lose
Noted
</body>
</html>
diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng
index 3905318..53b33eb 100644
--- a/docs/schemas/domaincaps.rng
+++ b/docs/schemas/domaincaps.rng
@@ -173,6 +173,9 @@
<element name='features'>
<interleave>
<ref name='gic'/>
+ <optional>
+ <ref name='sev'/>
+ </optional>
</interleave>
</element>
</define>
@@ -184,6 +187,23 @@
</element>
</define>
+ <define name='sev'>
+ <element name='sev'>
+ <element name='pdh'>
+ <data type='string'/>
+ </element>
+ <element name='cert-chain'>
+ <data type='string'/>
+ </element>
+ <element name='cbitpos'>
+ <data type='unsignedInt'/>
+ </element>
+ <element name='reduced-phys-bits'>
+ <data type='unsignedInt'/>
+ </element>
+ </element>
+ </define>
+
I want to make sure of recent preferences which aren't documented, but
sometimes draw attention of certain reviewers...
Do we want dashes or camelCase for new names? e.g. "cert-chain" or
"certChain"? And likewise for reduced-phys-bits or reducedPhysBits?
I have been trying keep the same naming convension as sev-guest object
name (which uses dashes). I am flexible, if libvirt community prefers
camelCase over the dashes then I am okay with it.
<define name='value'>
<zeroOrMore>
<element name='value'>
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index f7d9be5..082065f 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -549,6 +549,25 @@ virDomainCapsFeatureGICFormat(virBufferPtr buf,
FORMAT_EPILOGUE(gic);
}
+static void
+virDomainCapsFeatureSEVFormat(virBufferPtr buf,
+ virSEVCapabilityPtr const sev)
+{
+ if (!sev)
+ return;
+
+ virBufferAddLit(buf, "<sev supported='yes'>\n");
+ 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, "<pdh>%s</pdh>\n", sev->pdh);
+ virBufferAsprintf(buf, "<cert-chain>%s</cert-chain>\n",
+ sev->cert_chain);
When formatting strings, use virBufferEscapeString
Yes, I know they're base64 encoded, but not taking any chances...
Depending on whether there's an opinion w/r/t camelCase or not, I can
either fix up the nits or wait for a v6 with adjustments to the user
facing values.
Noted.
-Brijesh
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
