Re: [PATCH v2 3/4] libvirt: qemu: enable/disable protected key management ops

[Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx>]

On 05/15/2015 12:23 PM, Ján Tomko wrote:

On Fri, May 15, 2015 at 04:43:29PM +0200, Michal Privoznik wrote:

From: Tony Krowiak <aekrowia@xxxxxxxxxx>

Introduces two new -machine option parameters to the QEMU command to
enable/disable the CPACF protected key management operations for a guest:

    aes-key-wrap='on|off'
    dea-key-wrap='on|off'

The QEMU code maps the corresponding domain configuration elements to the
QEMU -machine option parameters to create the QEMU command:

    <cipher name='aes' state='on'>   --> aes-key-wrap=on
    <cipher name='aes' state='off'>  --> aes-key-wrap=off
    <cipher name='dea' state='on'>   --> dea-key-wrap=on
    <cipher name='dea' state='off'>  --> dea-key-wrap=off

Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Daniel Hansel <daniel.hansel@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
 src/qemu/qemu_capabilities.c |  4 +++
 src/qemu/qemu_capabilities.h |  2 ++
 src/qemu/qemu_command.c      | 73 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)

So the difference to v1 is that they are no longer turned on by default
if QEMU supports it. (I hope I did not miss anything else; it would
have been helpful if you listed the important changes)

I agree that this should not be done on XML parsing as was done in v1.
Would it make sense to treat the missing option (STATE_ABSENT) as
'turn it on if qemu supports it'?

Some background:
My original design was similar to Michal's in that if key wrapping was not
configured for the guest in the domain XML, then the machine options would not
be inserted into the QEMU command line. Our internal reviewers commented that
there should be default values for libvirt that match the QEMU defaults, so
I did exactly as you suggested here, inserting default values into the QEMU
command line on STATE_ABSENT. Our internal reviewers then pointed out that the
dumpxml command would return a configuration that did not match that of the running
guest, so I added the XML post parsing piece to set default values into virDomainDef if
QEMU supports the key wrapping machine options. In any case, I'm not married to
any of these ideas, so you have my ACK pending Jan's suggestions.

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2939f8d..98fc5f8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -38,6 +38,7 @@
 #include "virnetdevbridge.h"
 #include "virstring.h"
 #include "virtime.h"
+#include "virutil.h"

Why is this include needed?

I believe that this is no longer needed and is a remnant of an earlier iteration that I failed to
remove. My bad.

 #include "viruuid.h"
 #include "c-ctype.h"
 #include "domain_nwfilter.h"
@@ -7286,6 +7287,39 @@ qemuBuildObsoleteAccelArg(virCommandPtr cmd,
     return 0;
 }
 
+static bool
+qemuAppendKeyWrapMachineParm(virBuffer *buf, virQEMUCapsPtr qemuCaps,
+                             int flag, const char *pname, int pstate)
+{
+    if (pstate != VIR_TRISTATE_SWITCH_ABSENT) {
+        if (!virQEMUCapsGet(qemuCaps, flag)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("%s is not available with this QEMU binary"), pname);
+            return false;
+        }

Can't we allow state='off' with QEMU that does not support it?

We can certainly bypass the appending of the machine option if state='off', but if I am
not mistaken, sending a machine option to QEMU that it does not support will cause
QEMU to throw an error. I think it is wisest to inform the user of a configuration
error here.

You have an ACK from me with the include removed. Please wait for
feedback from the author before pushing.

Jan

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list