<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <div class="moz-cite-prefix">On 05/15/2015 12:23 PM, Ján Tomko wrote: </div> <blockquote cite="mid:20150515162359.GI16247@snc.brq.redhat.com" type="cite"> <pre wrap="">On Fri, May 15, 2015 at 04:43:29PM +0200, Michal Privoznik wrote: </pre> <blockquote type="cite"> <pre wrap="">From: Tony Krowiak <a class="moz-txt-link-rfc2396E" href="mailto:aekrowia@us.ibm.com"><aekrowia@us.ibm.com></a> 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 <a class="moz-txt-link-rfc2396E" href="mailto:akrowiak@linux.vnet.ibm.com"><akrowiak@linux.vnet.ibm.com></a> Signed-off-by: Daniel Hansel <a class="moz-txt-link-rfc2396E" href="mailto:daniel.hansel@linux.vnet.ibm.com"><daniel.hansel@linux.vnet.ibm.com></a> Signed-off-by: Boris Fiuczynski <a class="moz-txt-link-rfc2396E" href="mailto:fiuczy@linux.vnet.ibm.com"><fiuczy@linux.vnet.ibm.com></a> Reviewed-by: Boris Fiuczynski <a class="moz-txt-link-rfc2396E" href="mailto:fiuczy@linux.vnet.ibm.com"><fiuczy@linux.vnet.ibm.com></a> Signed-off-by: Michal Privoznik <a class="moz-txt-link-rfc2396E" href="mailto:mprivozn@redhat.com"><mprivozn@redhat.com></a> --- src/qemu/qemu_capabilities.c | 4 +++ src/qemu/qemu_capabilities.h | 2 ++ src/qemu/qemu_command.c | 73 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+) </pre> </blockquote> <pre wrap=""> 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'?</pre> </blockquote> 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. <blockquote cite="mid:20150515162359.GI16247@snc.brq.redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">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" </pre> </blockquote> <pre wrap=""> Why is this include needed?</pre> </blockquote> I believe that this is no longer needed and is a remnant of an earlier iteration that I failed to remove. My bad. <blockquote cite="mid:20150515162359.GI16247@snc.brq.redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> #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; + } </pre> </blockquote> <pre wrap=""> Can't we allow state='off' with QEMU that does not support it?</pre> </blockquote> 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. <blockquote cite="mid:20150515162359.GI16247@snc.brq.redhat.com" type="cite"> <pre wrap=""> You have an ACK from me with the include removed. Please wait for feedback from the author before pushing. Jan </pre> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">-- libvir-list mailing list <a class="moz-txt-link-abbreviated" href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a> <a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/libvir-list">https://www.redhat.com/mailman/listinfo/libvir-list</a></pre> </blockquote> </body> </html>