Re: [libvirt] [GSoC] Design ideas for implementing cleanup attribute

[Michal Privoznik]

On 05/28/2018 09:34 AM, Sukrit Bhatnagar wrote:
> 
> This is what new macros will look like:
> 
> # define _VIR_TYPE_PTR(type) type##Ptr
> 
> # define _VIR_ATTR_AUTOFREE_PTR(type)  __attribute__((cleanup(type##Free)))
> # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close)))
> # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
> 
> # define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) 
> _VIR_TYPE_PTR(type)
> 
> 
> The problem is that our vir*Free functions take on vir*Ptr as the
> parameter and not
> vir*Ptr * (pointer to it).
> 
> For example, instead of:
> void virArpTableFree(virArpTablePtr table);
> 
> we would need:
> void virArpTableFree(virArpTablePtr *table);
> 
> if we declare something like:
> VIR_AUTOFREE_PTR(virArpTable) table = NULL;
> 
> 
> Also, I tried to add a new function:
> void virArpTablePtrFree(virArpTablePtr *table)
> {
> size_t i;
> 
> if (!*table)
> return;
> 
> for (i = 0; i < (*table)->n; i++) {
> VIR_FREE((*table)->t[i].ipaddr);
> VIR_FREE((*table)->t[i].mac);
> }
> VIR_FREE((*table)->t);
> VIR_FREE((*table));
> VIR_FREE(table);

As Erik pointed out, this last VIR_FREE(table) looks fishy. However, do
you have patch that I can apply and reproduce?

Michal

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

Re: [libvirt] [PATCH 00/22] New CPU related APIs

[Chris Venteicher]

Quoting Jiri Denemark (2018-05-28 09:19:51)
> On Wed, May 16, 2018 at 10:39:19 +0200, Jiri Denemark wrote:
> > The current virConnectCompareCPU and virConnectBaselineCPU APIs are not
> > very useful because they ignore what a hypervisor can do on the current
> > host. This series adds two new APIs which are designed to work with
> > capabilities of a specific hypervisor to provide usable results.
> > 
> > The third CPU related API virConnectGetCPUModelNames is pretty useless
> > too, but no new API with similar functionality is needed because domain
> > capabilities XML already contains the relevant data.
> 
> I made the suggested changes and pushed the series. Thanks for the
> reviews.
> 
> Jirka

Hi Jirka,

FYI I reviewed your patches too and everthing I found that was not already 
identified was nitpick stuff but I do have a something I am wondering about...

The new hypervisor specific compare and baseline commands seem to depend on 
qemuCaps being pre-populated with model data that is specific to a hypervisor 
instance.

How do we make sure the qemuCaps are pre-populated with cpu model data for any 
arbitrary hypervisor (with a different exec path, arch, etc) that we can issue 
the hypervisor compare or baseline commands against?

Is it a case of executing a virsh command to populate qemuCaps for a 
non-default 
hypervisor prior to issuing the hypervisor compare or baseline commands?

Sorry if a complete noob question or I missed the answer in previous mails.

Chris

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

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

[libvirt] [PATCH 2/2] docs: bhyve: document guest CPU topology feature

[Roman Bogorodskiy]

Signed-off-by: Roman Bogorodskiy 
---
 docs/drvbhyve.html.in | 16 
 docs/news.xml |  9 +
 2 files changed, 25 insertions(+)

diff --git a/docs/drvbhyve.html.in b/docs/drvbhyve.html.in
index 5b5513d3df..78a291c6bb 100644
--- a/docs/drvbhyve.html.in
+++ b/docs/drvbhyve.html.in
@@ -444,6 +444,22 @@ be wired and cannot be swapped out as follows:
 /memoryBacking
 ...
 /domain
+
+
+CPU topology
+
+Since 4.4.0, it's possible to specify guest CPU 
topology, if bhyve
+supports that. Support for specifying guest CPU topology was added to bhyve in
+http://svnweb.freebsd.org/changeset/base/332298;>r332298 for 
-CURRENT.
+Example:
+
+domain type="bhyve"
+...
+cpu
+  topology sockets='1' cores='2' threads='1'/
+/cpu
+...
+/domain
 
 
   
diff --git a/docs/news.xml b/docs/news.xml
index c45850f625..318bca5de1 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -94,6 +94,15 @@
   both new APIs consider capabilities of a specific hypervisor.
 
   
+  
+
+  bhyve: Support specifying guest CPU topology
+
+
+  Bhyve's guest CPU topology could be specified using the
+  cputopology ..//cpu element.
+
+  
 
 
   
-- 
2.17.0

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

[libvirt] [PATCH 0/2] bhyve: add CPU topology support

[Roman Bogorodskiy]

This series adds support for specifying CPU topology for bhyve guests.
This also would need domxml-from-native support that I plan to add
separately.

I've updated docs to mention 4.4.0, but more probably it'll go to 4.5.0
as the freeze is close and I'm not sure I'll be able to turn around
before it.

Roman Bogorodskiy (2):
  bhyve: add CPU topology support
  docs: bhyve: document guest CPU topology feature

 docs/drvbhyve.html.in | 16 
 docs/news.xml |  9 +++
 src/bhyve/bhyve_capabilities.c|  7 +++--
 src/bhyve/bhyve_capabilities.h|  1 +
 src/bhyve/bhyve_command.c | 17 +++-
 .../bhyvexml2argv-cputopology.args|  9 +++
 .../bhyvexml2argv-cputopology.ldargs  |  3 +++
 .../bhyvexml2argv-cputopology.xml | 26 +++
 tests/bhyvexml2argvtest.c |  7 -
 9 files changed, 91 insertions(+), 4 deletions(-)
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.ldargs
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.xml

-- 
2.17.0

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

[libvirt] [PATCH 1/2] bhyve: add CPU topology support

[Roman Bogorodskiy]

Recently, bhyve started supporting specifying guest CPU topology.
It looks this way:

  bhyve -c cpus=C,sockets=S,cores=C,threads=T ...

The old behaviour with bhyve -c C, where C is a number of vCPUs, is
still supported.

So if we have CPU topology in the domain XML, use the new syntax,
otherwise keeps the old behaviour.

Signed-off-by: Roman Bogorodskiy 
---
 src/bhyve/bhyve_capabilities.c|  7 +++--
 src/bhyve/bhyve_capabilities.h|  1 +
 src/bhyve/bhyve_command.c | 17 +++-
 .../bhyvexml2argv-cputopology.args|  9 +++
 .../bhyvexml2argv-cputopology.ldargs  |  3 +++
 .../bhyvexml2argv-cputopology.xml | 26 +++
 tests/bhyvexml2argvtest.c |  7 -
 7 files changed, 66 insertions(+), 4 deletions(-)
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.ldargs
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.xml

diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
index e13085b1d5..a3229cea75 100644
--- a/src/bhyve/bhyve_capabilities.c
+++ b/src/bhyve/bhyve_capabilities.c
@@ -227,7 +227,7 @@ bhyveProbeCapsDeviceHelper(unsigned int *caps,
 }
 
 static int
-bhyveProbeCapsRTC_UTC(unsigned int *caps, char *binary)
+bhyveProbeCapsFromHelp(unsigned int *caps, char *binary)
 {
 char *help;
 virCommandPtr cmd = NULL;
@@ -244,6 +244,9 @@ bhyveProbeCapsRTC_UTC(unsigned int *caps, char *binary)
 if (strstr(help, "-u:") != NULL)
 *caps |= BHYVE_CAP_RTC_UTC;
 
+if (strstr(help, "sockets=n][,cores=n][,threads=n") != NULL)
+*caps |= BHYVE_CAP_CPUTOPOLOGY;
+
  out:
 VIR_FREE(help);
 virCommandFree(cmd);
@@ -314,7 +317,7 @@ virBhyveProbeCaps(unsigned int *caps)
 if (binary == NULL)
 goto out;
 
-if ((ret = bhyveProbeCapsRTC_UTC(caps, binary)))
+if ((ret = bhyveProbeCapsFromHelp(caps, binary)))
 goto out;
 
 if ((ret = bhyveProbeCapsAHCI32Slot(caps, binary)))
diff --git a/src/bhyve/bhyve_capabilities.h b/src/bhyve/bhyve_capabilities.h
index 0e310e6eda..873bc9c12d 100644
--- a/src/bhyve/bhyve_capabilities.h
+++ b/src/bhyve/bhyve_capabilities.h
@@ -49,6 +49,7 @@ typedef enum {
 BHYVE_CAP_LPC_BOOTROM = 1 << 3,
 BHYVE_CAP_FBUF = 1 << 4,
 BHYVE_CAP_XHCI = 1 << 5,
+BHYVE_CAP_CPUTOPOLOGY = 1 << 6,
 } virBhyveCapsFlags;
 
 int virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps);
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index e3f7ded7db..b319518520 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -467,7 +467,22 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
 
 /* CPUs */
 virCommandAddArg(cmd, "-c");
-virCommandAddArgFormat(cmd, "%d", virDomainDefGetVcpus(def));
+if (def->cpu && def->cpu->sockets) {
+if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_CPUTOPOLOGY) != 0) {
+virCommandAddArgFormat(cmd, 
"cpus=%d,sockets=%d,cores=%d,threads=%d",
+   virDomainDefGetVcpus(def),
+   def->cpu->sockets,
+   def->cpu->cores,
+   def->cpu->threads);
+} else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Installed bhyve binary does not support "
+ "defining CPU topology"));
+goto error;
+}
+} else {
+virCommandAddArgFormat(cmd, "%d", virDomainDefGetVcpus(def));
+}
 
 /* Memory */
 virCommandAddArg(cmd, "-m");
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args 
b/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args
new file mode 100644
index 00..2d175a4178
--- /dev/null
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args
@@ -0,0 +1,9 @@
+/usr/sbin/bhyve \
+-c cpus=2,sockets=1,cores=2,threads=1 \
+-m 214 \
+-u \
+-H \
+-P \
+-s 0:0,hostbridge \
+-s 2:0,ahci,hd:/tmp/freebsd.img \
+-s 3:0,virtio-net,faketapdev,mac=52:54:00:b9:94:02 bhyve
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.ldargs 
b/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.ldargs
new file mode 100644
index 00..32538b558e
--- /dev/null
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.ldargs
@@ -0,0 +1,3 @@
+/usr/sbin/bhyveload \
+-m 214 \
+-d /tmp/freebsd.img bhyve
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.xml 
b/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.xml
new file mode 100644
index 00..83c7d423c4
--- /dev/null
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.xml
@@ -0,0 +1,26 @@
+
+  bhyve
+  df3be7e7-a104-11e3-aeb0-50e5492bd3dc
+  219136
+  2
+  
+
+  
+  
+hvm
+  
+  
+
+  
+  
+  
+  
+
+
+  
+  
+  
+  
+
+  

Re: [libvirt] [GSoC] Design ideas for implementing cleanup attribute

[Sukrit Bhatnagar]

On 28 May 2018 at 13:54, Pavel Hrdina  wrote:
> On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
>> On 25 May 2018 at 16:20, Pavel Hrdina  wrote:
>> > On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
>> >> On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
>> >> > On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
>> >> > > However, I realize it might not be possible to register free
>> >> > > functions for a native type without having to introduce something
>> >> > > like
>> >> > >
>> >> > >   typedef char * virString;
>> >> > >
>> >> > > thus causing massive churn. How does GLib deal with that?
>> >> >
>> >> > If you would look into GLib documentation you would see that this
>> >> > design basically copies the one in GLib:
>> >>
>> >> Sorry, I should have looked up the documentation and implementation
>> >> before asking silly questions. Guess the morning coffee hadn't quite
>> >> kicked in yet :/
>> >>
>> >> > GLiblibvirt
>> >> >
>> >> > g_autofree  VIR_AUTOFREE
>> >> > g_autoptr   VIR_AUTOPTR
>> >> > g_auto  VIR_AUTOCLEAR
>> >>
>> >> For what it's worth, I think VIR_AUTOCLEAR is a much better name
>> >> than g_auto :)
>> >>
>> >> > In GLib you are using them like this:
>> >> >
>> >> > g_autofree char *string = NULL;
>> >> > g_autoptr(virDomain) dom = NULL;
>> >> > g_auto(virDomain) dom = { 0 };
>> >> >
>> >> > So yes it would require to introduce a lot of typedefs for basic types
>> >> > and that is not worth it.
>> >>
>> >> I'm not sure we would need so many typedefs, but there would
>> >> certainly be a lot of churn involved.
>> >>
>> >> Personally, I'm not so sure it wouldn't be worth the effort,
>> >> but it's definitely something that we can experiment with it at
>> >> a later time instead of holding up what's already a pretty
>> >> significant improvement.
>> >>
>> >> > In libvirt we would have:
>> >> >
>> >> > VIR_AUTOFREE char *string = NULL;
>> >> > VIR_AUTOPTR(virDomainPtr) dom = NULL;
>> >> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
>> >> >
>> >> > If you notice the difference, in libvirt we can use virDomainPtr
>> >> > directly because we have these typedefs, in GLib macro
>> >> > G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
>> >>
>> >> While I'm not a fan of our *Ptr typedefs in general, I guess this
>> >> time I'm glad we have them because VIR_AUTOPTR() doesn't hide the
>> >> fact that what you're declaring is a pointer; that is, the macro
>> >> argument is also exactly the type of the variable.
>> >
>> > So let's make a summary of how it could look like:
>> >
>> > VIR_AUTOFREE(char *) string = NULL;
>> > VIR_AUTOPTR(virDomainPtr) vm = NULL;
>> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
>> >
>> > VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree);
>> > VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
>>
>> Do we define new functions for freeing/clearing, because that is what
>> VIR_DEFINE_AUTOFREE_FUNC seems to do.
>>
>>
>> This is what new macros will look like:
>>
>> # define _VIR_TYPE_PTR(type) type##Ptr
>>
>> # define _VIR_ATTR_AUTOFREE_PTR(type)  __attribute__((cleanup(type##Free)))
>> # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close)))
>> # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
>>
>> # define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) 
>> _VIR_TYPE_PTR(type)
>
> NACK to this, please look few lines above how the macros should be named
> and which macros we would like to have implemented.
>
> There is no need to have the _VIR* helper macros and you need to
> implement the VIR_DEFINE_* macros.
>
>> The problem is that our vir*Free functions take on vir*Ptr as the
>> parameter and not
>> vir*Ptr * (pointer to it).
>
> The problem is only with your current implementation, the original
> design should not have this issue.
>
> The part of VIR_DEFINE_* macros is definition of new wrapper function
> for the cleanup function which means that our existing free functions
> are not used directly.
>
> GLib version of the define macro:
>
> #define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) \
> typedef TypeName *_GLIB_AUTOPTR_TYPENAME(TypeName); \
> G_GNUC_BEGIN_IGNORE_DEPRECATIONS \
> static inline void _GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) \
> { \
> if (*_ptr) \
> (func) (*_ptr); \
> } \
> G_GNUC_END_IGNORE_DEPRECATIONS
>
> Obviously we don't need the "typedef" line and the DEPRECATIONS macros.


So, using the discussed design, I have added the following lines:

# define VIR_AUTOPTR_FUNC_NAME(type) type##Free
# define VIR_AUTOCLEAR_FUNC_NAME(type) type##Clear

# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
static inline void VIR_AUTOPTR_FUNC_NAME(type) (type *_ptr) \
{ \
if (*_ptr) \
(func) (*_ptr); \
} \

# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
static inline 

Re: [libvirt] plan for next release

[Daniel Veillard]

On Mon, May 28, 2018 at 04:48:08PM +0200, Erik Skultety wrote:
> On Mon, May 28, 2018 at 10:15:22AM +0200, Erik Skultety wrote:
> > On Mon, May 28, 2018 at 10:01:06AM +0200, Daniel Veillard wrote:
> > >   Seems the end of the month is coming soon !
> > > I suggest to enter freeze tomorrow morningm with plan for an RC2 on 
> > > Thursday
> > > and a final release on the week-end,
> > >
> > >   Hope this works for everybody,
> > >
> > >  thanks !
> >
> > Hi,
> > would Wednesday and RC2 on Friday work too? I'm asking because there's the 
> > AMD
> > SEV patches (I'm reviewing it at the moment)  which introduce a new API and 
> > we
> > really should get this into this release, the patches look good so far, so I
> > don't expect more than a v7 with mostly nitpick being fixed...
> >
> > Thanks,
> > Erik
> 
> I'm sorry, but I have to retract my optimism on making this into 4.4.0, 
> because
> I suggested addition of one more API to get the SEV platform certificates as
> these can be up to 8K of base64 string which I agree with Dan shouldn't be
> exposed in domain capabilities XML. Therefore, you can proceed with your
> original plan on freezing tomorrow.

  Ok, thanks !

Daniel

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] plan for next release

[Erik Skultety]

On Mon, May 28, 2018 at 10:15:22AM +0200, Erik Skultety wrote:
> On Mon, May 28, 2018 at 10:01:06AM +0200, Daniel Veillard wrote:
> >   Seems the end of the month is coming soon !
> > I suggest to enter freeze tomorrow morningm with plan for an RC2 on Thursday
> > and a final release on the week-end,
> >
> >   Hope this works for everybody,
> >
> >  thanks !
>
> Hi,
> would Wednesday and RC2 on Friday work too? I'm asking because there's the AMD
> SEV patches (I'm reviewing it at the moment)  which introduce a new API and we
> really should get this into this release, the patches look good so far, so I
> don't expect more than a v7 with mostly nitpick being fixed...
>
> Thanks,
> Erik

I'm sorry, but I have to retract my optimism on making this into 4.4.0, because
I suggested addition of one more API to get the SEV platform certificates as
these can be up to 8K of base64 string which I agree with Dan shouldn't be
exposed in domain capabilities XML. Therefore, you can proceed with your
original plan on freezing tomorrow.

Erik

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

Re: [libvirt] [PATCH] qemu: Don't build cache= cmd line for scsi-block

[Peter Krempa]

On Mon, May 28, 2018 at 16:31:47 +0200, Michal Privoznik wrote:
> Trying to set any cache for  makes no sense.
> Such disk translates into -device scsi-block on the command line
> and the device lacks any cache setting because it's merely a
> middle man between qemu and real SCSI device.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c | 5 +
>  1 file changed, 5 insertions(+)

Broken by commit 327430fcfcf34c7b27d9d378df19032fd5d0e451 which was
released in 4.3.0.


> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 7408f6bc70..c7ff074e29 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1824,6 +1824,11 @@ qemuBuildDriveDevCacheStr(virDomainDiskDefPtr disk,
>  if (disk->cachemode == VIR_DOMAIN_DISK_CACHE_DEFAULT)
>  return 0;
>  
> +/* VIR_DOMAIN_DISK_DEVICE_LUN translates into 'scsi-block'
> + * where any caching setting makes no sense. */
> +if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN)
> +return 0;

ACK

> +
>  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DISK_WRITE_CACHE))
>  return 0;
>  
> -- 
> 2.16.1
> 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v6 6/9] libvirt: add new public API to get launch security info

[Erik Skultety]

On Mon, May 28, 2018 at 04:36:54PM +0200, Erik Skultety wrote:
> On Wed, May 23, 2018 at 04:18:31PM -0500, Brijesh Singh wrote:
> > The API can be used outside the libvirt to get the launch security
> > information. When SEV is enabled, the API can be used to get the
> > measurement of the launch process.
> >
> > Signed-off-by: Brijesh Singh 
> > ---

Putting Dan Berrange on CC to comment, forgot to do that in my original reply.

Erik

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

Re: [libvirt] [PATCH v6 8/9] qemu: Add support to launch security info

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:33PM -0500, Brijesh Singh wrote:
> This patch implements the internal driver API for launch event into
> qemu driver. When SEV is enabled, execute 'query-sev-launch-measurement'
> to get the measurement of memory encrypted through launch sequence.
>
> Signed-off-by: Brijesh Singh 
> ---

Apart from what I'm writing below, naming comments from patch 6 apply here
too...

>  src/qemu/qemu_driver.c   | 68 
> 
>  src/qemu/qemu_monitor.c  |  8 ++
>  src/qemu/qemu_monitor.h  |  3 ++
>  src/qemu/qemu_monitor_json.c | 42 +++
>  src/qemu/qemu_monitor_json.h |  2 ++
>  5 files changed, 123 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 3a328e5d4679..6569dea32fce 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -21194,6 +21194,73 @@ qemuDomainSetLifecycleAction(virDomainPtr dom,
>  }
>
>
> +static int
> +qemuDomainGetSevMeasurement(virQEMUDriverPtr driver,
> +virDomainObjPtr vm,
> +virTypedParameterPtr *params,
> +int *nparams,
> +unsigned int flags)
> +{
> +int ret = -1;
> +char *tmp;
> +int maxpar = 0;
> +
> +virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
> +
> +if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
> +return -1;
> +
> +if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> +goto endjob;
> +
> +tmp = qemuMonitorGetSevMeasurement(QEMU_DOMAIN_PRIVATE(vm)->mon);
> +if (tmp == NULL)
> +goto endjob;
> +
> +if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +goto endjob;
> +
> +if (virTypedParamsAddString(params, nparams, ,
> +VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT,
> +tmp) < 0)
> +goto endjob;
> +
> +ret = 0;
> +
> + endjob:

You're going to leak @tmp here, since virTypedParamsAddString makes its own
copy of tmp.

> +qemuDomainObjEndJob(driver, vm);
> +return ret;
> +}
> +
> +
> +static int
> +qemuDomainGetLaunchSecurityInfo(virDomainPtr domain,
> +virTypedParameterPtr *params,
> +int *nparams,
> +unsigned int flags)
> +{
> +virQEMUDriverPtr driver = domain->conn->privateData;
> +virDomainObjPtr vm;
> +int ret = -1;
> +
> +if (!(vm = qemuDomObjFromDomain(domain)))
> +goto cleanup;
> +
> +if (virDomainGetLaunchSecurityInfoEnsureACL(domain->conn, vm->def) < 0)
> +goto cleanup;
> +
> +if (vm->def->sev) {
> +if (qemuDomainGetSevMeasurement(driver, vm, params, nparams, flags) 
> < 0)
> +goto cleanup;
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +virDomainObjEndAPI();
> +return ret;
> +}
> +
>  static virHypervisorDriver qemuHypervisorDriver = {
>  .name = QEMU_DRIVER_NAME,
>  .connectURIProbe = qemuConnectURIProbe,
> @@ -21414,6 +21481,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
>  .domainSetVcpu = qemuDomainSetVcpu, /* 3.1.0 */
>  .domainSetBlockThreshold = qemuDomainSetBlockThreshold, /* 3.2.0 */
>  .domainSetLifecycleAction = qemuDomainSetLifecycleAction, /* 3.9.0 */
> +.domainGetLaunchSecurityInfo = qemuDomainGetLaunchSecurityInfo, /* 4.2.0 
> */
>  };
>
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 3b034930408c..977cbe5a41f8 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -4226,3 +4226,11 @@ qemuMonitorBlockdevDel(qemuMonitorPtr mon,
>
>  return qemuMonitorJSONBlockdevDel(mon, nodename);
>  }
> +
> +char *
> +qemuMonitorGetSevMeasurement(qemuMonitorPtr mon)
> +{
> +QEMU_CHECK_MONITOR_NULL(mon);
> +
> +return qemuMonitorJSONGetSevMeasurement(mon);
> +}
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index b1b7ef09c929..8a64ae5f3d96 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1137,4 +1137,7 @@ int qemuMonitorBlockdevAdd(qemuMonitorPtr mon,
>  int qemuMonitorBlockdevDel(qemuMonitorPtr mon,
> const char *nodename);
>
> +char *
> +qemuMonitorGetSevMeasurement(qemuMonitorPtr mon);
> +
>  #endif /* QEMU_MONITOR_H */
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 24d3a2ff412f..041f595ca1e4 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -8024,3 +8024,45 @@ qemuMonitorJSONBlockdevDel(qemuMonitorPtr mon,
>  virJSONValueFree(reply);
>  return ret;
>  }
> +
> +/**
> + * The function is used to retrieve the measurement of SEV guest.

s/of SEV/of a SEV

> + * The measurement is signature of the memory contents that was encrypted
> + * through the SEV launch flow.
> + *
> + * A example jason output:

Re: [libvirt] [PATCH v6 7/9] remote: implement the remote protocol for launch security

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:32PM -0500, Brijesh Singh wrote:
> Add remote support for launch security info.

Add support for GetLaunchSecurityInfo to remote driver.

My naming comments from patch 6 apply here too.

>
> Signed-off-by: Brijesh Singh 

Functionally though,
Reviewed-by: Erik Skultety 

PS: I still want to wait for additional comment.

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

Re: [libvirt] [PATCH v6 6/9] libvirt: add new public API to get launch security info

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:31PM -0500, Brijesh Singh wrote:
> The API can be used outside the libvirt to get the launch security
> information. When SEV is enabled, the API can be used to get the
> measurement of the launch process.
>
> Signed-off-by: Brijesh Singh 
> ---
>  include/libvirt/libvirt-domain.h | 17 ++
>  src/driver-hypervisor.h  |  7 ++
>  src/libvirt-domain.c | 48 
> 
>  src/libvirt_public.syms  |  5 +
>  4 files changed, 77 insertions(+)
>
> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index d7cbd187969d..f252d18da72f 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -4764,4 +4764,21 @@ int virDomainSetLifecycleAction(virDomainPtr domain,
>  unsigned int action,
>  unsigned int flags);
>
> +/**
> + * Launch Security API
> + */
> +
> +/**
> + * VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT:
> + *
> + * Macro represents the launch measurement of the SEV guest,
> + * as VIR_TYPED_PARAM_STRING.
> + */
> +#define VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT "sev-measurement"

fails make syntax-check because of indentation, should be "# define ..."

...

> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 95df3a0dbc7b..5ccae5da8883 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -785,4 +785,9 @@ LIBVIRT_4.1.0 {
>  virStoragePoolLookupByTargetPath;
>  } LIBVIRT_3.9.0;
>
> +LIBVIRT_4.4.0 {
> +global:
> +virDomainGetLaunchSecurityInfo;
> +} LIBVIRT_4.1.0;

Will most probably become 4.5.0 :(

Technically, I don't have any notes related to the functional changes,
therefore I'd give you my RB, however, I still find the naming confusing and I
can't think of something better. What if one day we'll actually be able to/need
to modify the configuration for some reason, we should reserve a name like this
for future modifications of launch-security data of the guest. Next, you're
preparing for adding support for some kind of setter in the virsh command, any
idea of what the setter data might be? Because I can imagine that you'd still
want to perform a measurement, but want to send additional arguments, to the
remote side's firmware to change the behaviour of the measurement and you can't
do this with a simple flag, you also need typed params for that which means
wou'd end up with something like:

int
virDomainLaunchSecurityMeasure(virDomainPtr domain,
   virTypedParamsPtr send_params,
   unsigned int nsend_params,
   virTypedParamsPtr *recv_params,
   unsigned int nrecv_params);

And you can both send additional arguments as well as receive arguments
according to the remote implementation, it's IMHO safer, but it's extremely
ugly at the same time and we're hitting the 'one function does all' kind of
scenario which we also shouldn't do. At the same time though, we could say that
any arguments that you might need to alter the result of the measurement should
be available prior to launching the guest in its XML config file, that way,
you'll never need anything apart from the recv_params,recv_nparams and the name
you're suggesting can stay, since only by using flags you can tell libvirt
daemon whether you want to work with the info in the config or take a
measurement or something else.

This was just me thinking out loud and would like to get some input from you
and/or other reviewer because even though I'm okay with the code, I don't want
to make a decision we can't take back just yet.

Erik

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

[libvirt] [PATCH] qemu: Don't build cache= cmd line for scsi-block

[Michal Privoznik]

Trying to set any cache for  makes no sense.
Such disk translates into -device scsi-block on the command line
and the device lacks any cache setting because it's merely a
middle man between qemu and real SCSI device.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7408f6bc70..c7ff074e29 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1824,6 +1824,11 @@ qemuBuildDriveDevCacheStr(virDomainDiskDefPtr disk,
 if (disk->cachemode == VIR_DOMAIN_DISK_CACHE_DEFAULT)
 return 0;
 
+/* VIR_DOMAIN_DISK_DEVICE_LUN translates into 'scsi-block'
+ * where any caching setting makes no sense. */
+if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN)
+return 0;
+
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DISK_WRITE_CACHE))
 return 0;
 
-- 
2.16.1

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

Re: [libvirt] [PATCH 00/22] New CPU related APIs

[Jiri Denemark]

On Wed, May 16, 2018 at 10:39:19 +0200, Jiri Denemark wrote:
> The current virConnectCompareCPU and virConnectBaselineCPU APIs are not
> very useful because they ignore what a hypervisor can do on the current
> host. This series adds two new APIs which are designed to work with
> capabilities of a specific hypervisor to provide usable results.
> 
> The third CPU related API virConnectGetCPUModelNames is pretty useless
> too, but no new API with similar functionality is needed because domain
> capabilities XML already contains the relevant data.

I made the suggested changes and pushed the series. Thanks for the
reviews.

Jirka

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

Re: [libvirt] [PATCH v6 9/9] virsh: implement new command for launch security

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:34PM -0500, Brijesh Singh wrote:
> Add new 'launch-security' command, the command can be used to get or set
> the launch security information when booting encrypted VMs.
>
> Signed-off-by: Brijesh Singh 
> ---
>  tools/virsh-domain.c | 81 
> 
>  tools/virsh.pod  |  5 
>  2 files changed, 86 insertions(+)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index cfbbf5a7bc39..27bb702c8bb7 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -13870,6 +13870,81 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd)
>  return ret >= 0;
>  }
>
> +/*
> + * "launch-security" command

Sigh... feature/command/API naming is soo tricky in libvirt, even though we
always try to be careful, we'll often hit the issues with commands being named
wrongly or just confusing, this is one of those cases, because the XML element
is called launch-security this really does make the impression the command
makes changes to the launch-security configuration for the guest, which is not
the case here. I think that if we ever find ourselves in that position that we
need to change the configuration of launch-security for a guest, then we need
to reserve this name for that, but now, launch-security-measure sounds more
descriptive of what it does right now to me, see my question about the setter
below, so that we can find a more suitable name for it.

This of course means the underlying APIs would need to change too, see my
comments to patches 6-8.

Any ideas for the naming guys?

> + */
> +static const vshCmdInfo info_launch_security[] = {
> +{.name = "help",
> +.data = N_("Get or set launch-security information")

So since we don't have the setter yet, it should only report as a getter,
because we're not introducing the setter switch as of now, once we do, we can
change the commentary as well...

> +},
> +{.name = "desc",
> +.data = N_("Get or set the current launch-security information for "
> +   "a guest domain.\n"

Here too...

> +   "To get the launch-security information use following"
> +   "command: \n\n"
> +  "virsh # launch-security ")

^these 3 lines are unnecessary because that's contained in the SYNOPSIS
already.

> +},
> +{.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_launch_security[] = {
> +VIRSH_COMMON_OPT_DOMAIN_FULL(0),
> +VIRSH_COMMON_OPT_DOMAIN_CONFIG,
> +VIRSH_COMMON_OPT_DOMAIN_LIVE,
> +VIRSH_COMMON_OPT_DOMAIN_CURRENT,
> +{.name = NULL}
> +};
> +
> +static void
> +virshPrintLaunchSecurityInfo(vshControl *ctl, virTypedParameterPtr params,
> + int nparams)
> +{
> +size_t i;
> +
> +for (i = 0; i < nparams; i++) {
> +if (params[i].type == VIR_TYPED_PARAM_STRING)
> +vshPrintExtra(ctl, "%-15s: %s\n", params[i].field, 
> params[i].value.s);
> +}
> +}

No need for ^this Print helper really...

> +
> +static bool
> +cmdLaunchSecurity(vshControl *ctl, const vshCmd *cmd)
> +{
> +virDomainPtr dom;
> +int nparams = 0;
> +virTypedParameterPtr params = NULL;
> +bool ret = false;
> +unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
> +bool current = vshCommandOptBool(cmd, "current");
> +bool config = vshCommandOptBool(cmd, "config");
> +bool live = vshCommandOptBool(cmd, "live");
> +
> +VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
> +VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
> +
> +if (config)
> +flags |= VIR_DOMAIN_AFFECT_CONFIG;
> +if (live)
> +flags |= VIR_DOMAIN_AFFECT_LIVE;
> +

So what should the setter then do, conceptually is enough for me as an answer?
I'm asking because I don't see a point in using VIR_DOMAIN_AFFECT_X at all
because those are for commands that do perform changes to the VM configuration,
either live or offline, getting a measurement doesn't touch the configuration
at all.

Erik

> +if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> +return false;
> +
> +if (virDomainGetLaunchSecurityInfo(dom, , , flags) != 0) {
> +vshError(ctl, "%s", _("Unable to get launch security info"));
> +goto cleanup;
> +}
> +
> +virshPrintLaunchSecurityInfo(ctl, params, nparams);
> +
> +ret = true;
> + cleanup:
> +virTypedParamsFree(params, nparams);
> +virshDomainFree(dom);
> +return ret;
> +}
> +
> +
>  const vshCmdDef domManagementCmds[] = {
>  {.name = "attach-device",
>   .handler = cmdAttachDevice,
> @@ -14485,5 +14560,11 @@ const vshCmdDef domManagementCmds[] = {
>   .info = info_domblkthreshold,
>   .flags = 0
>  },
> +{.name = "launch-security-info",
> + .handler = cmdLaunchSecurity,
> + .opts = opts_launch_security,
> + .info = info_launch_security,
> + .flags = 0
> +},
>  {.name = NULL}
>  };
> diff --git 

Re: [libvirt] [PATCH 2/4] conf: domain: Invoke post-parse callbacks after parsing private XML parts

[Ján Tomko]

On Mon, May 28, 2018 at 11:36:45AM +0200, Peter Krempa wrote:

When parsing status XML the post-parse callbacks can't access any
private data present in the status XML as the private bits were parsed
after invoking post-parse callbacks.

Move the invocation so that everything is parsed first.

Signed-off-by: Peter Krempa 
---
src/conf/domain_conf.c | 37 +
1 file changed, 25 insertions(+), 12 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/4] qemu: domain: Pass 'qemuCaps' to post parse callbacks when parsing status XML

[Ján Tomko]

On Mon, May 28, 2018 at 11:36:47AM +0200, Peter Krempa wrote:

When status XML was parsed the post-parse callbacks could not access
qemu caps and potentially upgrade the definition according to the
present caps. Implement the callback to pass it in.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_domain.c | 10 ++
1 file changed, 10 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] conf: domain: Allow passing in 'parseOpaque' for post-parse of status XML

[Ján Tomko]

On Mon, May 28, 2018 at 11:36:46AM +0200, Peter Krempa wrote:

The status XML parser function virDomainObjParseXML could not pass in
parseOpaque into the post parse callbacks. Add a callback which will
allow hypervisor drivers to fill it from the 'virDomainObj' data.

Signed-off-by: Peter Krempa 
---
src/conf/domain_conf.c | 6 +-
src/conf/domain_conf.h | 5 +
2 files changed, 10 insertions(+), 1 deletion(-)


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/4] conf: domain: Remove code accessing 'bootHash' from the post-parse infrestructure

[Ján Tomko]

On Mon, May 28, 2018 at 11:36:44AM +0200, Peter Krempa wrote:

There is only one block accessing it. Removing it is necessary so that
post parse callbacks can later be invoked after the hypervisor private
data callback so that e.g. qemuCaps are properly filled for the
postparse callbacks.

As the function signature of virDomainDefPostParseInternal does not
differ from virDomainDefPostParse now, the wrapper can be dropped.

Signed-off-by: Peter Krempa 
---
src/conf/domain_conf.c | 62 +++---
1 file changed, 24 insertions(+), 38 deletions(-)




@@ -20502,9 +20474,23 @@ virDomainDefParseXML(xmlDocPtr xml,
(def->ns.parse)(xml, root, ctxt, >namespaceData) < 0)
goto error;

+/* Fill in default boot device if none was present */
+if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
+if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("per-device boot elements cannot be used"
+ " together with os/boot elements"));
+goto error;
+}
+
+if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) {
+def->os.nBootDevs = 1;
+def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;
+}
+}
+


I don't like moving this back into ParseXML.

Can we remove it completely instead?
https://www.redhat.com/archives/libvir-list/2018-May/msg02041.html
cover.1527515594.git.jto...@redhat.com

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/4] conf: introduce virDomainDefCheckBootOrder

[Ján Tomko]

Move the check for boot elements into a separate function
and remove its dependency on the parser-supplied bootHash table.

Reconstructing the hash table from the domain definition
effectively duplicates the check for duplicate boot order
values, also present in virDomainDeviceBootParseXML.

Signed-off-by: Ján Tomko 
---
 src/conf/domain_conf.c   | 89 +++-
 tests/qemuargv2xmldata/nomachine-aarch64.xml |  1 +
 tests/qemuargv2xmldata/nomachine-ppc64.xml   |  1 +
 tests/qemuargv2xmldata/nomachine-x86_64.xml  |  1 +
 tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml  |  1 +
 5 files changed, 78 insertions(+), 15 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d6ac47c629..f087a3680f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4939,10 +4939,79 @@ virDomainDefPostParseCPU(virDomainDefPtr def)
 }
 
 
+static int
+virDomainDefCollectBootOrder(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *data)
+{
+virHashTablePtr bootHash = data;
+char *order = NULL;
+int ret = -1;
+
+if (info->bootIndex == 0)
+return 0;
+
+if (virAsprintf(, "%u", info->bootIndex) < 0)
+goto cleanup;
+
+if (virHashLookup(bootHash, order)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("boot order '%s' used for more than one device"),
+   order);
+goto cleanup;
+}
+
+if (virHashAddEntry(bootHash, order, (void *) 1) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(order);
+return ret;
+}
+
+
+static int
+virDomainDefCheckBootOrder(virDomainDefPtr def)
+{
+virHashTablePtr bootHash = NULL;
+int ret = -1;
+
+if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
+return 0;
+
+if (!(bootHash = virHashCreate(5, NULL)))
+goto cleanup;
+
+if (virDomainDeviceInfoIterate(def, virDomainDefCollectBootOrder, 
bootHash) < 0)
+goto cleanup;
+
+if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("per-device boot elements cannot be used"
+ " together with os/boot elements"));
+goto cleanup;
+}
+
+if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) {
+def->os.nBootDevs = 1;
+def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;
+}
+
+ret = 0;
+
+ cleanup:
+virHashFree(bootHash);
+return ret;
+}
+
+
 static int
 virDomainDefPostParseCommon(virDomainDefPtr def,
 struct virDomainDefPostParseDeviceIteratorData 
*data,
-virHashTablePtr bootHash)
+virHashTablePtr bootHash ATTRIBUTE_UNUSED)
 {
 size_t i;
 
@@ -4953,20 +5022,6 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
 return -1;
 }
 
-if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && bootHash) {
-if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("per-device boot elements cannot be used"
- " together with os/boot elements"));
-return -1;
-}
-
-if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) {
-def->os.nBootDevs = 1;
-def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;
-}
-}
-
 if (virDomainVcpuDefPostParse(def) < 0)
 return -1;
 
@@ -4979,6 +5034,10 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
 if (virDomainDefRejectDuplicatePanics(def) < 0)
 return -1;
 
+if (!(data->xmlopt->config.features & 
VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER) &&
+virDomainDefCheckBootOrder(def) < 0)
+return -1;
+
 if (virDomainDefPostParseTimer(def) < 0)
 return -1;
 
diff --git a/tests/qemuargv2xmldata/nomachine-aarch64.xml 
b/tests/qemuargv2xmldata/nomachine-aarch64.xml
index eb8f9db803..9492423389 100644
--- a/tests/qemuargv2xmldata/nomachine-aarch64.xml
+++ b/tests/qemuargv2xmldata/nomachine-aarch64.xml
@@ -6,6 +6,7 @@
   1
   
 hvm
+
   
   
 
diff --git a/tests/qemuargv2xmldata/nomachine-ppc64.xml 
b/tests/qemuargv2xmldata/nomachine-ppc64.xml
index 439f9e9ac6..1f15a950e3 100644
--- a/tests/qemuargv2xmldata/nomachine-ppc64.xml
+++ b/tests/qemuargv2xmldata/nomachine-ppc64.xml
@@ -6,6 +6,7 @@
   1
   
 hvm
+
   
   
   destroy
diff --git a/tests/qemuargv2xmldata/nomachine-x86_64.xml 
b/tests/qemuargv2xmldata/nomachine-x86_64.xml
index 71a36f0833..33cde4c55a 100644
--- a/tests/qemuargv2xmldata/nomachine-x86_64.xml
+++ b/tests/qemuargv2xmldata/nomachine-x86_64.xml
@@ -6,6 +6,7 @@
   1
   
 hvm
+
   
   
 
diff --git 

[libvirt] [PATCH 0/4] Remove 'bootHash'

[Ján Tomko]

An alternative to Peter's:
[PATCH 1/4] conf: domain: Remove code accessing 'bootHash' from the post-parse 
infrestructure
<8b4627fedd199117af1ab46f5562e3838679357e.1527500017.git.pkre...@redhat.com>

Ján Tomko (3):
  vmx: add VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER
  conf: introduce virDomainDefCheckBootOrder
  conf: remove 'bootHash' completely

Peter Krempa (1):
  conf: remove 'bootHash' from the post-parse infrastructure

 src/conf/domain_conf.c   | 209 +++
 src/conf/domain_conf.h   |   1 +
 src/vmx/vmx.c|   3 +-
 tests/qemuargv2xmldata/nomachine-aarch64.xml |   1 +
 tests/qemuargv2xmldata/nomachine-ppc64.xml   |   1 +
 tests/qemuargv2xmldata/nomachine-x86_64.xml  |   1 +
 tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml  |   1 +
 7 files changed, 120 insertions(+), 97 deletions(-)

-- 
2.16.1

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

[libvirt] [PATCH 1/4] vmx: add VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER

[Ján Tomko]

Further patches will introduce validation and a default setting
of def->os.bootDevs in postParse.

Introduce a feature flag to opt out of this and set it in the vmx
driver.

Signed-off-by: Ján Tomko 
---
 src/conf/domain_conf.h | 1 +
 src/vmx/vmx.c  | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b7e52a1e03..e10206b358 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2557,6 +2557,7 @@ typedef enum {
 VIR_DOMAIN_DEF_FEATURE_NAME_SLASH = (1 << 3),
 VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS = (1 << 4),
 VIR_DOMAIN_DEF_FEATURE_USER_ALIAS = (1 << 5),
+VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER = (1 << 6),
 } virDomainDefFeatures;
 
 
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index df6a58a474..bdc27b15b0 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -550,7 +550,8 @@ static virDomainDefParserConfig virVMXDomainDefParserConfig 
= {
 .devicesPostParseCallback = virVMXDomainDevicesDefPostParse,
 .domainPostParseCallback = virVMXDomainDefPostParse,
 .features = (VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI |
- VIR_DOMAIN_DEF_FEATURE_NAME_SLASH),
+ VIR_DOMAIN_DEF_FEATURE_NAME_SLASH |
+ VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER),
 };
 
 struct virVMXDomainDefNamespaceData {
-- 
2.16.1

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

[libvirt] [PATCH 3/4] conf: remove 'bootHash' from the post-parse infrastructure

[Ján Tomko]

From: Peter Krempa 

As the function signature of virDomainDefPostParseInternal does not
differ from virDomainDefPostParse now, the wrapper can be dropped.

Signed-off-by: Peter Krempa 
Signed-off-by: Ján Tomko 
---
 src/conf/domain_conf.c | 33 +
 1 file changed, 9 insertions(+), 24 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f087a3680f..6076424bc6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5010,8 +5010,7 @@ virDomainDefCheckBootOrder(virDomainDefPtr def)
 
 static int
 virDomainDefPostParseCommon(virDomainDefPtr def,
-struct virDomainDefPostParseDeviceIteratorData 
*data,
-virHashTablePtr bootHash ATTRIBUTE_UNUSED)
+struct virDomainDefPostParseDeviceIteratorData 
*data)
 {
 size_t i;
 
@@ -5118,13 +5117,12 @@ virDomainDefPostParseCheckFailure(virDomainDefPtr def,
 }
 
 
-static int
-virDomainDefPostParseInternal(virDomainDefPtr def,
-  virCapsPtr caps,
-  unsigned int parseFlags,
-  virDomainXMLOptionPtr xmlopt,
-  void *parseOpaque,
-  virHashTablePtr bootHash)
+int
+virDomainDefPostParse(virDomainDefPtr def,
+  virCapsPtr caps,
+  unsigned int parseFlags,
+  virDomainXMLOptionPtr xmlopt,
+  void *parseOpaque)
 {
 int ret = -1;
 bool localParseOpaque = false;
@@ -5180,7 +5178,7 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
 if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0)
 goto cleanup;
 
-if ((ret = virDomainDefPostParseCommon(def, , bootHash)) < 0)
+if ((ret = virDomainDefPostParseCommon(def, )) < 0)
 goto cleanup;
 
 if (xmlopt->config.assignAddressesCallback) {
@@ -5207,18 +5205,6 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
 }
 
 
-int
-virDomainDefPostParse(virDomainDefPtr def,
-  virCapsPtr caps,
-  unsigned int parseFlags,
-  virDomainXMLOptionPtr xmlopt,
-  void *parseOpaque)
-{
-return virDomainDefPostParseInternal(def, caps, parseFlags, xmlopt,
- parseOpaque, NULL);
-}
-
-
 /**
  * virDomainDiskAddressDiskBusCompatibility:
  * @bus: disk bus type
@@ -20562,8 +20548,7 @@ virDomainDefParseXML(xmlDocPtr xml,
 goto error;
 
 /* callback to fill driver specific domain aspects */
-if (virDomainDefPostParseInternal(def, caps, flags, xmlopt, parseOpaque,
-  bootHash) < 0)
+if (virDomainDefPostParse(def, caps, flags, xmlopt, parseOpaque) < 0)
 goto error;
 
 /* valdiate configuration */
-- 
2.16.1

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

[libvirt] [PATCH 4/4] conf: remove 'bootHash' completely

[Ján Tomko]

Its only use is now to check for duplicate boot order values,
which is now also done in virDomainDefPostParseCommon.

Remove it completely.

Signed-off-by: Ján Tomko 
---
 src/conf/domain_conf.c | 89 ++
 1 file changed, 31 insertions(+), 58 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6076424bc6..c150b7d7d4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6587,8 +6587,7 @@ virDomainDeviceUSBMasterParseXML(xmlNodePtr node,
 
 static int
 virDomainDeviceBootParseXML(xmlNodePtr node,
-virDomainDeviceInfoPtr info,
-virHashTablePtr bootHash)
+virDomainDeviceInfoPtr info)
 {
 char *order;
 char *loadparm = NULL;
@@ -6608,18 +6607,6 @@ virDomainDeviceBootParseXML(xmlNodePtr node,
 goto cleanup;
 }
 
-if (bootHash) {
-if (virHashLookup(bootHash, order)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("boot order '%s' used for more than one device"),
-   order);
-goto cleanup;
-}
-
-if (virHashAddEntry(bootHash, order, (void *) 1) < 0)
-goto cleanup;
-}
-
 loadparm = virXMLPropString(node, "loadparm");
 if (loadparm) {
 if (virStringToUpper(>loadparm, loadparm) != 1) {
@@ -6817,7 +6804,6 @@ virDomainDeviceAliasIsUserAlias(const char *aliasStr)
 static int
 virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED,
 xmlNodePtr node,
-virHashTablePtr bootHash,
 virDomainDeviceInfoPtr info,
 unsigned int flags)
 {
@@ -6877,7 +6863,7 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt 
ATTRIBUTE_UNUSED,
 }
 
 if (boot) {
-if (virDomainDeviceBootParseXML(boot, info, bootHash))
+if (virDomainDeviceBootParseXML(boot, info))
 goto cleanup;
 }
 
@@ -9402,7 +9388,6 @@ static virDomainDiskDefPtr
 virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
  xmlNodePtr node,
  xmlXPathContextPtr ctxt,
- virHashTablePtr bootHash,
  virSecurityLabelDefPtr* vmSeclabels,
  int nvmSeclabels,
  unsigned int flags)
@@ -9769,7 +9754,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 }
 def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
 } else {
-if (virDomainDeviceInfoParseXML(xmlopt, node, bootHash, >info,
+if (virDomainDeviceInfoParseXML(xmlopt, node, >info,
 flags | 
VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT) < 0)
 goto error;
 }
@@ -10260,7 +10245,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr 
xmlopt,
 if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
 def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) {
 VIR_DEBUG("Ignoring device address for none model usb controller");
-} else if (virDomainDeviceInfoParseXML(xmlopt, node, NULL,
+} else if (virDomainDeviceInfoParseXML(xmlopt, node,
>info, flags) < 0) {
 goto error;
 }
@@ -10648,7 +10633,7 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
 def->dst = target;
 target = NULL;
 
-if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, >info, flags) < 0)
+if (virDomainDeviceInfoParseXML(xmlopt, node, >info, flags) < 0)
 goto error;
 
  cleanup:
@@ -10938,7 +10923,6 @@ static virDomainNetDefPtr
 virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
 xmlNodePtr node,
 xmlXPathContextPtr ctxt,
-virHashTablePtr bootHash,
 char *prefix,
 unsigned int flags)
 {
@@ -11226,7 +11210,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
 }
 def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
 } else {
-if (virDomainDeviceInfoParseXML(xmlopt, node, bootHash, >info,
+if (virDomainDeviceInfoParseXML(xmlopt, node, >info,
 flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT
 | VIR_DOMAIN_DEF_PARSE_ALLOW_ROM) < 0)
 goto error;
@@ -12525,7 +12509,7 @@ virDomainChrDefParseXML(virDomainXMLOptionPtr xmlopt,
 if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
 def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
 VIR_DEBUG("Ignoring device address for gustfwd channel");
-} else if (virDomainDeviceInfoParseXML(xmlopt, node, NULL,
+} else if (virDomainDeviceInfoParseXML(xmlopt, node,
>info, 

Re: [libvirt] [PATCH 11/22] qemu: Implement virConnectCompareHypervisorCPU

[Jiri Denemark]

On Fri, May 25, 2018 at 13:50:19 -0400, Collin Walling wrote:
> Sorry for the delay. I've been experiencing issues with the mail server :(
> 
> On 05/16/2018 04:39 AM, Jiri Denemark wrote:
> > Signed-off-by: Jiri Denemark 
> > ---
> >  src/qemu/qemu_driver.c | 60 ++
> >  1 file changed, 60 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 6c086b9ef8..4b48afdad1 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -13100,6 +13100,65 @@ qemuConnectCompareCPU(virConnectPtr conn,
> >  }
> >  
> >  
> > +static int
> > +qemuConnectCompareHypervisorCPU(virConnectPtr conn,
> > +const char *emulator,
> > +const char *archStr,
> > +const char *machine,
> > +const char *virttypeStr,
> > +const char *xmlCPU,
> > +unsigned int flags)
> > +{
> > +int ret = VIR_CPU_COMPARE_ERROR;
> > +virQEMUDriverPtr driver = conn->privateData;
> > +virQEMUCapsPtr qemuCaps = NULL;
> > +bool failIncompatible;
> > +virCPUDefPtr hvCPU;
> > +virArch arch;
> > +virDomainVirtType virttype;
> > +
> > +virCheckFlags(VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE,
> > +  VIR_CPU_COMPARE_ERROR);
> > +
> > +if (virConnectCompareHypervisorCPUEnsureACL(conn) < 0)
> > +goto cleanup;
> > +
> > +failIncompatible = !!(flags & 
> > VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE);
> > +
> > +qemuCaps = virQEMUCapsCacheLookupDefault(driver->qemuCapsCache,
> > + emulator,
> > + archStr,
> > + virttypeStr,
> > + machine,
> > + , , NULL);
> > +if (!qemuCaps)
> > +goto cleanup;
> > +
> > +hvCPU = virQEMUCapsGetHostModel(qemuCaps, virttype,
> > +VIR_QEMU_CAPS_HOST_CPU_REPORTED);
> 
> nit: add a blank line here
> 
> > +if (!hvCPU || hvCPU->fallback != VIR_CPU_FALLBACK_FORBID) {
> > +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > +   _("QEMU '%s' does not support reporting CPU model 
> > for "
> > + "virttype '%s'"),
> > +   virQEMUCapsGetBinary(qemuCaps),
> > +   virDomainVirtTypeToString(virttype));
> > +goto cleanup;
> > +}
> > +
> > +if (ARCH_IS_X86(arch)) {
> > +ret = virCPUCompareXML(arch, hvCPU, xmlCPU, failIncompatible);
> > +} else {
> > +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > +   _("comparing hypervisor CPUs is not supported for "
> > + "arch %s"), virArchToString(arch));
> 
> At first glance, this message makes me think that this function is for 
> "comparing two hypervisor CPUs".
> Perhaps the message should say "comparing with the hypervisor CPU is not 
> supported for arch %s" instead? 
> I think that makes it more clear that the other CPU in question is not 
> (necessarily) a hypervisor CPU.

Yeah, that's definitely better.

Jirka

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

Re: [libvirt] [PATCH 10/22] virsh: Introduce new hypervisor-cpu-compare command

[Jiri Denemark]

On Fri, May 25, 2018 at 15:22:43 -0400, Collin Walling wrote:
> >>> +switch (result) {
> >>> +case VIR_CPU_COMPARE_INCOMPATIBLE:
> >>> +vshPrint(ctl,
> >>> + _("CPU described in %s is incompatible with the CPU 
> >>> provided "
> >>> +   "by hypervisor on the host\n"),
> >>> + from);
> >>
> >> How much information regarding a CPU definition does libvirt consider when 
> >> comparing CPU's
> >> for x86 (and for other archs, if you happen to know)? On s390, we only 
> >> take the cpu model 
> >> and features into consideration.
> >>
> >> If the archs other than s390 will only use the cpu model and features as 
> >> well -- or if this 
> >> API should explicitly work only with CPU models -- then perhaps it is more 
> >> accurate for these 
> >> messages to state "CPU model" instead of "CPU"? This change would also 
> >> have to be propagated 
> >> in to the documentation, replacing "CPU definition" with "CPU model".
> > 
> > It doesn't really matter what libvirt currently checks for which
> > architecture. The API takes a CPU definition XML and libvirt will use
> > anything it needs from that.
> > 
> 
> I had to bat this around in my head a bit. Truthfully, I think trying to 
> expand on
> why we got the result might be a little much.

That's (partially) the reason behind --error option. It will cause the
API to return an error (rather than VIR_CPU_COMPARE_INCOMPATIBLE) and
you can check the error messages for details about the incompatibility.
For example, the x86 CPU driver will list all missing features.

> Perhaps I should have more faith in the user to understand what is
> taken into consideration when CPUs are compared :)

The user should not really need to know what is used for comparison.
They would just have a CPU definition they want to use for a guest and
this API can be used to check whether such CPU can be run on a given
host.

For example, oVirt has a set of CPU definitions from which a user can
select and calls the compare API to check which hosts can run which CPU
so that they can properly schedule where to run individual VMs.

Jirka

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

Re: [libvirt] [PATCH v6 5/9] qemu: add support to launch SEV guest

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:30PM -0500, Brijesh Singh wrote:
> QEMU >= 2.12 provides 'sev-guest' object which is used to launch encrypted
>

Unnecessary new line here...

> VMs on AMD platform using SEV feature. The various inputs required to
> launch SEV guest is provided through the  tag. A typical
> SEV guest launch command line looks like this:
>
> # $QEMU ...\
>   -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 ...\
>   -machine memory-encryption=sev0 \
>
> Signed-off-by: Brijesh Singh 
> ---

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v6 4/9] qemu/cgroup: add /dev/sev in shared devices list

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:29PM -0500, Brijesh Singh wrote:
> QEMU uses /dev/sev device while creating the SEV guest, lets add /dev/sev
> in the list of devices allowed to be accessed by the QEMU.
>
> Signed-off-by: Brijesh Singh <>
> ---

I think this might be swapped with the previous patch and thus become patch 3
so that the changes are in a more logical order
config->qemu->remote->libvirt.

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v6 3/9] conf: introduce launch-security element in domain

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:28PM -0500, 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.
>
> Signed-off-by: Brijesh Singh 
> ---
>  docs/formatdomain.html.in  | 115 ++
>  docs/schemas/domaincommon.rng  |  39 ++
>  src/conf/domain_conf.c | 133 
> +
>  src/conf/domain_conf.h |  27 +
>  tests/genericxml2xmlindata/launch-security-sev.xml |  24 
>  tests/genericxml2xmltest.c |   2 +
>  6 files changed, 340 insertions(+)
>  create mode 100644 tests/genericxml2xmlindata/launch-security-sev.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 665d0f25293e..cab08ea52003 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -8310,6 +8310,121 @@ qemu-kvm -net nic,model=? /dev/null
>
>  Note: DEA/TDEA is synonymous with DES/TDES.
>
> +Secure Encrypted Virtualization (SEV)
> +
> +
> +   The contents of the launch-security type='sev' 
> 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 
> the SEV API spec
> +href="https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf;> 
> https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf 
> +   Since 4.4.0
> +   
> +
> +domain
> +  ...
> +  launch-security type='sev'
> +policy 0x0001 /policy
> +cbitpos 47 /cbitpos
> +reduced-phys-bits 1 /reduced-phys-bits
> +session AAACCCDD=FFFCCCDSDS /session
> +dh-cert RBBBSDDD=FDDCCCDDDG /dh
> +  /sev
> +  ...
> +/domain
> +
> +
> +
> +  cbitpos
> +  The required cbitpos element provides the C-bit (aka 
> encryption bit)
> +  location in guest page table entry. The value of cbitpos 
> is
> +  hypervisor dependent and can be obtained through the sev 
> element
> +  from the domain capabilities.

>From the cover letter it seemed like this is always the same as the value
reported in the domain capabilities and therefore I wrote what I wrote, but is
it always the same value as the one reported in capabilities as it's
hypervisor-dependent or can it in fact be configurable? Because if it can
change, then of course it makes sense to let user config this for a guest and
this is okay.

> +  
> +  reduced-phys-bits
> +  The required reduced-phys-bits element provides the 
> physical
> +  address bit reducation. Similar to cbitpos the value of 
> 
> +  reduced-phys-bit is hypervisor dependent and can be obtained
> +  through the sev element from the domain capabilities.
> +  

Same here...

> +
> +   15:6 

Just wondering, you write 16:32, shouldn't ^this be 6:15 then instead of 15:6?

> +   reserved 
> +
> +
> +   16:32 
> +   The guest must not be transmitted to another platform with a
> +   lower firmware version. 
> +
> +  
> +

...

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f16e157397d4..69b6c84b9540 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -77,6 +77,9 @@
>  
>
>  
> +
> +  
> +
>
>  
>
> @@ -436,6 +439,42 @@
>  
>
>
> +  
> +
> +  
> +sev
> +  
> +  
> +
> +  
> +
> +
> +  
> +
> +
> +  
> +
> +  
> +

You made 'policy' required per v5 comments, so the RNG schema needs to be
updated as well.

> +
> +   

Re: [libvirt] [PATCH v6 2/9] qemu: introduce SEV feature in hypervisor capabilities

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:27PM -0500, 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
> (PDH) 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.
>
> Signed-off-by: Brijesh Singh 
> ---

...and this one should be IMHO named
conf: Expose SEV in domain capabilities

>  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(+)
>
> diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in
> index b68ae4b4f1f3..f37b059ba6b1 100644
> --- a/docs/formatdomaincaps.html.in
> +++ b/docs/formatdomaincaps.html.in
> @@ -434,6 +434,12 @@
>/enum
>  /gic
>  vmcoreinfo supported='yes'/
> +sev
> +  pdhUWxKSlNrVlRTRk5KVGtkSVFVMUU=/pdh
> +  
> cert-chainVVd4S1NsTnJWbFJUUms1S1ZHdGtTVkZWTVVVPQ==/cert-chain
> +  cbitpos47/cbitpos
> +  reduced-phys-bits1/reduced-phys-bits
> +/sev
>/features
>  /domainCapabilities
>  
> @@ -462,5 +468,39 @@
>
>  Reports whether the vmcoreinfo feature can be enabled
>
> +SEV capabilities
> +
> +AMD Secure Encrypted Virtualization (SEV) capabilities are exposed 
> under
> +the sev 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.
> +
> +
> +For more details on SEV feature see:
> +   href="https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf;>
> +SEV API spec and  href="http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf;>
> +SEV White Paper
> +
> +
> +
> +  pdh
> +  A base64 encoded platform Diffie-Hellman public key which can be
> +  exported to remote entities that desire to establish a secure transport
> +  context with the SEV platform in order to transmit data securely.
> +  cert-chain
> +   A base64 encoded platform certificate chain that includes the 
> platform
> +  endorsement key (PEK), owners certificate authority (OCD), and chip
> +  endorsement key (CEK).
> +  cbitpos
> +  When memory encryption is enabled, one of the physical address bits
> +  (aka the C-bit) is utilized to mark if a memory page is protected. The
> +  C-bit position is Hypervisor dependent.
> +  reduced-phys-bits
> +  When memory encryption is enabled, we lose certain bits in physical
> +  address space. The number of bits we lose is hypervisor dependent.
> +
> +
>
>  
> diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng
> index 5913d711a3fe..26265645b82a 100644
> --- a/docs/schemas/domaincaps.rng
> +++ b/docs/schemas/domaincaps.rng
> @@ -184,6 +184,9 @@
>
>  
>  
> +
> +

needs 1 more level of indent...

> +
>
>  
>
> @@ -201,6 +204,23 @@
>  
>
>
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +
> +  
> +
>
>  
>
> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
> index 6e2ab0a28796..3b767c45cbb3 100644
> --- a/src/conf/domain_capabilities.c
> +++ b/src/conf/domain_capabilities.c
> @@ -542,6 +542,25 @@ virDomainCapsFeatureGICFormat(virBufferPtr buf,
>  FORMAT_EPILOGUE(gic);
>  }
>
> +static void
> +virDomainCapsFeatureSEVFormat(virBufferPtr buf,
> +  virSEVCapabilityPtr const sev)
> +{
> +if (!sev)
> +return;
> +
> +virBufferAddLit(buf, "\n");
> +virBufferAdjustIndent(buf, 2);
> +virBufferAsprintf(buf, "%d\n", sev->cbitpos);
> +virBufferAsprintf(buf, "%d\n",
> +  sev->reduced_phys_bits);
> +virBufferEscapeString(buf, "%s\n", sev->pdh);
> +virBufferEscapeString(buf, "%s\n",
> +  sev->cert_chain);

As I said, I have to agree with Dan here that reporting the 8k string in
capabilities is a good idea, so we can store it, we just wouldn't format it
^here...

> +virBufferAdjustIndent(buf, -2);
> +virBufferAddLit(buf, "\n");
> +}
> +
>
>  char *
>  virDomainCapsFormat(virDomainCapsPtr const caps)
> @@ -585,6 +604,7 @@ virDomainCapsFormat(virDomainCapsPtr const caps)
>

Re: [libvirt] [PATCH v6 0/9] x86: Secure Encrypted Virtualization (AMD)

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:25PM -0500, Brijesh Singh wrote:
> This patch series provides support for launching an encrypted guest using
> AMD's new Secure Encrypted  Virtualization (SEV) feature.
>
> SEV is an extension to the AMD-V architecture which supports running
> multiple VMs under the control of a hypervisor. When enabled, SEV feature
> allows the memory contents of a virtual machine (VM) to be transparently
> encrypted with a key unique to the guest VM.
>
> At very high level the flow looks this:
>
> 1. mgmt tool calls virConnectGetDomainCapabilities. This returns an XML 
> document
> that includes the following
>
> 
> ...
>   
>  
>  
>  
>  
> o

It's sad that ^this didn't get more attention since Dan suggested a separate
API to get the certificates, because I myself don't feel like having 2k/8k
base64 strings reported in domain capabilities is a good idea, it should only
report that the capability is supported with the given qemu and that the bit
stuff which is hypervisor specific. For the certificates,  which are not QEMU
specific, rather those are platform dependent and static, we should introduce a
virNodeGetSEVInfo (or something like that - I'm not good with names, but I
think we struggle with this in general) getter for that which would work on top
of virTypedParams so that it's extensible. Moreover, it would also be okay IMHO
to feed the return data of this new API not only with the certs but also with
the bit-related stuff, even though that's already obtained through
capabilities.


>
> If  is provided then we indicate that hypervisor is capable of launching
> SEV guest.
>
> 2. (optional) mgmt tool can provide the PDH and Cert-chain to guest owner in 
> case
> if guest owner wish to establish a secure connection with SEV firmware to
> negotiate a key used for validating the measurement.
>
> 3. mgmt tool requests to start a guest calling virCreateXML(), passing 
> VIR_DOMAIN_START_PAUSED.
> The xml would include
>
> 
>  /* the value is same as what is obtained via 
> virConnectGetDomainCapabilities()
>  /* the value is same as what is 
> obtained via virConnectGetDomainCapabilities()
>..  /* guest owners diffie-hellman key */ (optional)
>.. /* guest owners session blob */ (optional)
>.. /* guest policy */ (optional)
> 

Now that I'm looking at the design, why do we need to report ,
 in the domain XML if that is platform specific, it's static
and we already report it withing capabilities? If it can't be used for a
per-guest configuration, i.e. it can change, I don't think we should expose the
same information on multiple places.

Still, does anyone have another idea for an improvement?

Erik

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

Re: [libvirt] [PATCHv2 3/3] xen_common: convert to typesafe virConf accessors

[Fabiano Fidêncio]

On Mon, May 28, 2018 at 9:16 AM, Ján Tomko  wrote:
> On Sun, May 27, 2018 at 02:08:58PM +0200, Fabiano Fidêncio wrote:
>>
>> On Sun, May 27, 2018 at 1:17 PM, Ján Tomko  wrote:
>>>
>>> On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote:


 From: Fabiano Fidêncio 

 There are still some places using virConfGetValue() and then checking
 the specific type of the pointers and so on.

 Those place are not going to be changed as:
 - Directly using virConfGetValue*() would trigger virReportError() on
 their current code
>>>
>>>
>>>
>>> Is that a problem in xenParseCPUFeatures?
>>
>>
>> It would, at least, generate one more log, which would be misleading
>> whoever ends up debugging some issue on that codepath later on.
>>
>
> I don't see it.
> xenConfigGetULong already reports an error when the "maxvcpus" value is
> malformed.

What xenConfigGetULong does is:

val = virConfGetValue()
if (val->type == _ULLONG) {
   ...
} else if (val->type == _STRING) {
   virStrToLong_ul(...) ...
   and in case of failure, it reports an error;
} else {
   an error is reported
}

If we simply try to do something similar with virConfGetValue ... we'd
end up with:

if (virConfGetValueULLong() < 0) {
   /* here, in case virConfGetValueULLong fails, an error is already
reported ...
   and we don't want it to happen, as the config may come as a string */
if (virConfGetString() ... {
}
}


So, summing up, the main difference is that checking by ULLONG in the
current approach doesn't generate any error/failure. While checking
for ULLONG wih virConfGetValueULLong() would trigger the error from
inside the function.

One thing that could be done ... expand virConfGetValueULLong() to
also support receiving its value as VIR_CONF_STRING. I've talked to
Michal about that and he explicitly mentioned that this may not be the
way to go and then I've decided to leave the code as it is.

Is it clear? Did I miss something?

Best Regards,
-- 
Fabiano Fidêncio

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

[libvirt] [PATCH 3/4] conf: domain: Allow passing in 'parseOpaque' for post-parse of status XML

[Peter Krempa]

The status XML parser function virDomainObjParseXML could not pass in
parseOpaque into the post parse callbacks. Add a callback which will
allow hypervisor drivers to fill it from the 'virDomainObj' data.

Signed-off-by: Peter Krempa 
---
 src/conf/domain_conf.c | 6 +-
 src/conf/domain_conf.h | 5 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 43ae5ad96a..cc38c441dc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -20518,6 +20518,7 @@ virDomainObjParseXML(xmlDocPtr xml,
 int n;
 int state;
 int reason = 0;
+void *parseOpaque = NULL;

 if (!(obj = virDomainObjNew(xmlopt)))
 return NULL;
@@ -20589,8 +20590,11 @@ virDomainObjParseXML(xmlDocPtr xml,
 xmlopt->privateData.parse(ctxt, obj, >config) < 0)
 goto error;

+if (xmlopt->privateData.getParseOpaque)
+parseOpaque = xmlopt->privateData.getParseOpaque(obj);
+
 /* callback to fill driver specific domain aspects */
-if (virDomainDefPostParse(obj->def, caps, flags, xmlopt, NULL) < 0)
+if (virDomainDefPostParse(obj->def, caps, flags, xmlopt, parseOpaque) < 0)
 goto error;

 /* valdiate configuration */
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b7e52a1e03..651dcc445f 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2662,6 +2662,8 @@ typedef int 
(*virDomainXMLPrivateDataParseFunc)(xmlXPathContextPtr,
 virDomainObjPtr,
 virDomainDefParserConfigPtr);

+typedef void *(*virDomainXMLPrivateDataGetParseOpaqueFunc)(virDomainObjPtr vm);
+
 typedef int 
(*virDomainXMLPrivateDataStorageSourceParseFunc)(xmlXPathContextPtr ctxt,
  
virStorageSourcePtr src);
 typedef int 
(*virDomainXMLPrivateDataStorageSourceFormatFunc)(virStorageSourcePtr src,
@@ -2680,6 +2682,9 @@ struct _virDomainXMLPrivateDataCallbacks {
 virDomainXMLPrivateDataNewFuncchrSourceNew;
 virDomainXMLPrivateDataFormatFunc format;
 virDomainXMLPrivateDataParseFunc  parse;
+/* following function shall return a pointer which will be used as the
+ * 'parseOpaque' argument for virDomainDefPostParse */
+virDomainXMLPrivateDataGetParseOpaqueFunc getParseOpaque;
 virDomainXMLPrivateDataStorageSourceParseFunc storageParse;
 virDomainXMLPrivateDataStorageSourceFormatFunc storageFormat;
 };
-- 
2.16.2

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

[libvirt] [PATCH 2/4] conf: domain: Invoke post-parse callbacks after parsing private XML parts

[Peter Krempa]

When parsing status XML the post-parse callbacks can't access any
private data present in the status XML as the private bits were parsed
after invoking post-parse callbacks.

Move the invocation so that everything is parsed first.

Signed-off-by: Peter Krempa 
---
 src/conf/domain_conf.c | 37 +
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index eead28f5fb..43ae5ad96a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18746,7 +18746,6 @@ virDomainDefParseXML(xmlDocPtr xml,
  xmlXPathContextPtr ctxt,
  virCapsPtr caps,
  virDomainXMLOptionPtr xmlopt,
- void *parseOpaque,
  unsigned int flags)
 {
 xmlNodePtr *nodes = NULL, node = NULL;
@@ -20489,14 +20488,6 @@ virDomainDefParseXML(xmlDocPtr xml,
 }
 }

-/* callback to fill driver specific domain aspects */
-if (virDomainDefPostParse(def, caps, flags, xmlopt, parseOpaque) < 0)
-goto error;
-
-/* valdiate configuration */
-if (virDomainDefValidate(def, caps, flags, xmlopt) < 0)
-goto error;
-
 virHashFree(bootHash);

 return def;
@@ -20539,7 +20530,7 @@ virDomainObjParseXML(xmlDocPtr xml,

 oldnode = ctxt->node;
 ctxt->node = config;
-obj->def = virDomainDefParseXML(xml, config, ctxt, caps, xmlopt, NULL, 
flags);
+obj->def = virDomainDefParseXML(xml, config, ctxt, caps, xmlopt, flags);
 ctxt->node = oldnode;
 if (!obj->def)
 goto error;
@@ -20598,6 +20589,14 @@ virDomainObjParseXML(xmlDocPtr xml,
 xmlopt->privateData.parse(ctxt, obj, >config) < 0)
 goto error;

+/* callback to fill driver specific domain aspects */
+if (virDomainDefPostParse(obj->def, caps, flags, xmlopt, NULL) < 0)
+goto error;
+
+/* valdiate configuration */
+if (virDomainDefValidate(obj->def, caps, flags, xmlopt) < 0)
+goto error;
+
 return obj;

  error:
@@ -20660,6 +20659,7 @@ virDomainDefParseNode(xmlDocPtr xml,
 {
 xmlXPathContextPtr ctxt = NULL;
 virDomainDefPtr def = NULL;
+virDomainDefPtr ret = NULL;

 if (!virXMLNodeNameEqual(root, "domain")) {
 virReportError(VIR_ERR_XML_ERROR,
@@ -20676,11 +20676,24 @@ virDomainDefParseNode(xmlDocPtr xml,
 }

 ctxt->node = root;
-def = virDomainDefParseXML(xml, root, ctxt, caps, xmlopt, parseOpaque, 
flags);
+
+if (!(def = virDomainDefParseXML(xml, root, ctxt, caps, xmlopt, flags)))
+goto cleanup;
+
+/* callback to fill driver specific domain aspects */
+if (virDomainDefPostParse(def, caps, flags, xmlopt, parseOpaque) < 0)
+goto cleanup;
+
+/* valdiate configuration */
+if (virDomainDefValidate(def, caps, flags, xmlopt) < 0)
+goto cleanup;
+
+VIR_STEAL_PTR(ret, def);

  cleanup:
+virDomainDefFree(def);
 xmlXPathFreeContext(ctxt);
-return def;
+return ret;
 }


-- 
2.16.2

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

[libvirt] [PATCH 1/4] conf: domain: Remove code accessing 'bootHash' from the post-parse infrestructure

[Peter Krempa]

There is only one block accessing it. Removing it is necessary so that
post parse callbacks can later be invoked after the hypervisor private
data callback so that e.g. qemuCaps are properly filled for the
postparse callbacks.

As the function signature of virDomainDefPostParseInternal does not
differ from virDomainDefPostParse now, the wrapper can be dropped.

Signed-off-by: Peter Krempa 
---
 src/conf/domain_conf.c | 62 +++---
 1 file changed, 24 insertions(+), 38 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d6ac47c629..eead28f5fb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4941,8 +4941,7 @@ virDomainDefPostParseCPU(virDomainDefPtr def)

 static int
 virDomainDefPostParseCommon(virDomainDefPtr def,
-struct virDomainDefPostParseDeviceIteratorData 
*data,
-virHashTablePtr bootHash)
+struct virDomainDefPostParseDeviceIteratorData 
*data)
 {
 size_t i;

@@ -4953,20 +4952,6 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
 return -1;
 }

-if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && bootHash) {
-if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("per-device boot elements cannot be used"
- " together with os/boot elements"));
-return -1;
-}
-
-if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) {
-def->os.nBootDevs = 1;
-def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;
-}
-}
-
 if (virDomainVcpuDefPostParse(def) < 0)
 return -1;

@@ -5059,13 +5044,12 @@ virDomainDefPostParseCheckFailure(virDomainDefPtr def,
 }


-static int
-virDomainDefPostParseInternal(virDomainDefPtr def,
-  virCapsPtr caps,
-  unsigned int parseFlags,
-  virDomainXMLOptionPtr xmlopt,
-  void *parseOpaque,
-  virHashTablePtr bootHash)
+int
+virDomainDefPostParse(virDomainDefPtr def,
+  virCapsPtr caps,
+  unsigned int parseFlags,
+  virDomainXMLOptionPtr xmlopt,
+  void *parseOpaque)
 {
 int ret = -1;
 bool localParseOpaque = false;
@@ -5121,7 +5105,7 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
 if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0)
 goto cleanup;

-if ((ret = virDomainDefPostParseCommon(def, , bootHash)) < 0)
+if ((ret = virDomainDefPostParseCommon(def, )) < 0)
 goto cleanup;

 if (xmlopt->config.assignAddressesCallback) {
@@ -5148,18 +5132,6 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
 }


-int
-virDomainDefPostParse(virDomainDefPtr def,
-  virCapsPtr caps,
-  unsigned int parseFlags,
-  virDomainXMLOptionPtr xmlopt,
-  void *parseOpaque)
-{
-return virDomainDefPostParseInternal(def, caps, parseFlags, xmlopt,
- parseOpaque, NULL);
-}
-
-
 /**
  * virDomainDiskAddressDiskBusCompatibility:
  * @bus: disk bus type
@@ -20502,9 +20474,23 @@ virDomainDefParseXML(xmlDocPtr xml,
 (def->ns.parse)(xml, root, ctxt, >namespaceData) < 0)
 goto error;

+/* Fill in default boot device if none was present */
+if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
+if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("per-device boot elements cannot be used"
+ " together with os/boot elements"));
+goto error;
+}
+
+if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) {
+def->os.nBootDevs = 1;
+def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;
+}
+}
+
 /* callback to fill driver specific domain aspects */
-if (virDomainDefPostParseInternal(def, caps, flags, xmlopt, parseOpaque,
-  bootHash) < 0)
+if (virDomainDefPostParse(def, caps, flags, xmlopt, parseOpaque) < 0)
 goto error;

 /* valdiate configuration */
-- 
2.16.2

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

[libvirt] [PATCH 0/4] conf: Invoke post-parse callbacks after parsing private XML

[Peter Krempa]

This will allow the qemu post-parse callbacks to access 'qemuCaps' which
is stored in the status XML when we restore the status of VMs.

It will become useful in cases when we need to decide whether storage
authentication/encryption secret aliases will need to be regenerated.

Peter Krempa (4):
  conf: domain: Remove code accessing 'bootHash' from the post-parse
infrestructure
  conf: domain: Invoke post-parse callbacks after parsing private XML
parts
  conf: domain: Allow passing in 'parseOpaque' for post-parse of status
XML
  qemu: domain: Pass 'qemuCaps' to post parse callbacks when parsing
status XML

 src/conf/domain_conf.c | 97 ++
 src/conf/domain_conf.h |  5 +++
 src/qemu/qemu_domain.c | 10 ++
 3 files changed, 65 insertions(+), 47 deletions(-)

-- 
2.16.2

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

[libvirt] [PATCH 4/4] qemu: domain: Pass 'qemuCaps' to post parse callbacks when parsing status XML

[Peter Krempa]

When status XML was parsed the post-parse callbacks could not access
qemu caps and potentially upgrade the definition according to the
present caps. Implement the callback to pass it in.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_domain.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 47910acb83..0b99698046 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2801,6 +2801,15 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
 }


+static void *
+qemuDomainObjPrivateXMLGetParseOpaque(virDomainObjPtr vm)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+
+return priv->qemuCaps;
+}
+
+
 virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = {
 .alloc = qemuDomainObjPrivateAlloc,
 .free = qemuDomainObjPrivateFree,
@@ -2809,6 +2818,7 @@ virDomainXMLPrivateDataCallbacks 
virQEMUDriverPrivateDataCallbacks = {
 .chrSourceNew = qemuDomainChrSourcePrivateNew,
 .parse = qemuDomainObjPrivateXMLParse,
 .format = qemuDomainObjPrivateXMLFormat,
+.getParseOpaque = qemuDomainObjPrivateXMLGetParseOpaque,
 .storageParse = qemuStorageSourcePrivateDataParse,
 .storageFormat = qemuStorageSourcePrivateDataFormat,
 };
-- 
2.16.2

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

Re: [libvirt] [PATCH 02/13] qemu: Introduce zPCI capability

[Xiao Feng Ren]

On 5/25/2018 6:58 PM, Pino Toscano wrote:

On Thursday, 24 May 2018 14:24:27 CEST Xiao Feng Ren wrote:

From: Yi Min Zhao 

Let's introduce zPCI capability.

Signed-off-by: Yi Min Zhao 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Stefan Zimmermann 
Reviewed-by: Bjoern Walk 
---
  src/qemu/qemu_capabilities.c | 2 ++
  src/qemu/qemu_capabilities.h | 1 +
  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 +
  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 +
  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 +
  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml  | 1 +
  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml  | 1 +
  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml  | 1 +
  8 files changed, 9 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 8a63db5f4f..e6fd7ee468 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -489,6 +489,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"screendump_device",
"hda-output",
"blockdev-del",
+ "zpci",

Wrong indentation here.


Ok, will revise.

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

Re: [libvirt] [PATCH 07/13] qemu: Add hotpluging support for PCI devices on S390 guests

[Xiao Feng Ren]

On 5/25/2018 6:22 PM, Bjoern Walk wrote:

Cornelia Huck  [2018-05-24, 06:25PM +0200]:

On Thu, 24 May 2018 14:24:32 +0200
Xiao Feng Ren  wrote:


From: Yi Min Zhao 

This commit adds hotplug support for PCI devices on S390 guests.
There's no need to implement hot unplug for zPCI as QEMU implements
an unplug callback which will unplug both PCI and zPCI device in a
cascaded way.
Currently, the following PCI devices are supported:
   virtio-blk-pci
   virtio-net-pci
   virtio-rng-pci
   virtio-input-host-pci
   virtio-keyboard-pci
   virtio-mouse-pci
   virtio-tablet-pci
   vfio-pci
   Shmem device
   SCSIVhost device

Hm, how did you arrive at this list? Is it 'anything that uses msi-x'?

I guess it's just any device that libvirt actually supports hotplug for,
with a few exceptions (cf. patch 3 and 6). Not familiar wuth msi-x.


The list should be the devices that support MSI-X and have realized the 
hotplug in qemu,
we support the hotplug for them in the libvirt.  So the list will be 
updated to:


  virtio-blk-pci
  virtio-net-pci
  virtio-input-host-pci
  virtio-keyboard-pci
  virtio-mouse-pci
  virtio-tablet-pci
  vfio-pci
  SCSIVhost device

As the model of Shmem is not support in qemu, the rng device doesn't 
support MSI-X.  So remove them.



In addition, the bug found need to be fixed for the hotplug of tablet, 
in the virDomainDeviceIsUSB()

if ((t == VIR_DOMAIN_DEVICE_DISK &&
 dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) ||
    (t == VIR_DOMAIN_DEVICE_INPUT &&
 dev->data.input->type == VIR_DOMAIN_INPUT_BUS_USB) ||  
-->dev->data.input->bus == VIR_DOMAIN_INPUT_BUS_USB

    (t == VIR_DOMAIN_DEVICE_HOSTDEV &&
I will send the fix if this bug is agreed.

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

Re: [libvirt] [GSoC] Design ideas for implementing cleanup attribute

[Pavel Hrdina]

On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
> On 25 May 2018 at 16:20, Pavel Hrdina  wrote:
> > On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
> >> On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
> >> > On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
> >> > > However, I realize it might not be possible to register free
> >> > > functions for a native type without having to introduce something
> >> > > like
> >> > >
> >> > >   typedef char * virString;
> >> > >
> >> > > thus causing massive churn. How does GLib deal with that?
> >> >
> >> > If you would look into GLib documentation you would see that this
> >> > design basically copies the one in GLib:
> >>
> >> Sorry, I should have looked up the documentation and implementation
> >> before asking silly questions. Guess the morning coffee hadn't quite
> >> kicked in yet :/
> >>
> >> > GLiblibvirt
> >> >
> >> > g_autofree  VIR_AUTOFREE
> >> > g_autoptr   VIR_AUTOPTR
> >> > g_auto  VIR_AUTOCLEAR
> >>
> >> For what it's worth, I think VIR_AUTOCLEAR is a much better name
> >> than g_auto :)
> >>
> >> > In GLib you are using them like this:
> >> >
> >> > g_autofree char *string = NULL;
> >> > g_autoptr(virDomain) dom = NULL;
> >> > g_auto(virDomain) dom = { 0 };
> >> >
> >> > So yes it would require to introduce a lot of typedefs for basic types
> >> > and that is not worth it.
> >>
> >> I'm not sure we would need so many typedefs, but there would
> >> certainly be a lot of churn involved.
> >>
> >> Personally, I'm not so sure it wouldn't be worth the effort,
> >> but it's definitely something that we can experiment with it at
> >> a later time instead of holding up what's already a pretty
> >> significant improvement.
> >>
> >> > In libvirt we would have:
> >> >
> >> > VIR_AUTOFREE char *string = NULL;
> >> > VIR_AUTOPTR(virDomainPtr) dom = NULL;
> >> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
> >> >
> >> > If you notice the difference, in libvirt we can use virDomainPtr
> >> > directly because we have these typedefs, in GLib macro
> >> > G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
> >>
> >> While I'm not a fan of our *Ptr typedefs in general, I guess this
> >> time I'm glad we have them because VIR_AUTOPTR() doesn't hide the
> >> fact that what you're declaring is a pointer; that is, the macro
> >> argument is also exactly the type of the variable.
> >
> > So let's make a summary of how it could look like:
> >
> > VIR_AUTOFREE(char *) string = NULL;
> > VIR_AUTOPTR(virDomainPtr) vm = NULL;
> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
> >
> > VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree);
> > VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
> 
> Do we define new functions for freeing/clearing, because that is what
> VIR_DEFINE_AUTOFREE_FUNC seems to do.
> 
> 
> This is what new macros will look like:
> 
> # define _VIR_TYPE_PTR(type) type##Ptr
> 
> # define _VIR_ATTR_AUTOFREE_PTR(type)  __attribute__((cleanup(type##Free)))
> # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close)))
> # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
> 
> # define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) 
> _VIR_TYPE_PTR(type)

NACK to this, please look few lines above how the macros should be named
and which macros we would like to have implemented.

There is no need to have the _VIR* helper macros and you need to
implement the VIR_DEFINE_* macros.

> The problem is that our vir*Free functions take on vir*Ptr as the
> parameter and not
> vir*Ptr * (pointer to it).

The problem is only with your current implementation, the original
design should not have this issue.

The part of VIR_DEFINE_* macros is definition of new wrapper function
for the cleanup function which means that our existing free functions
are not used directly.

GLib version of the define macro:

#define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) \
typedef TypeName *_GLIB_AUTOPTR_TYPENAME(TypeName); \
G_GNUC_BEGIN_IGNORE_DEPRECATIONS \
static inline void _GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) \
{ \
if (*_ptr) \
(func) (*_ptr); \
} \
G_GNUC_END_IGNORE_DEPRECATIONS

Obviously we don't need the "typedef" line and the DEPRECATIONS macros.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] plan for next release

[Erik Skultety]

On Mon, May 28, 2018 at 10:01:06AM +0200, Daniel Veillard wrote:
>   Seems the end of the month is coming soon !
> I suggest to enter freeze tomorrow morningm with plan for an RC2 on Thursday
> and a final release on the week-end,
>
>   Hope this works for everybody,
>
>  thanks !

Hi,
would Wednesday and RC2 on Friday work too? I'm asking because there's the AMD
SEV patches (I'm reviewing it at the moment)  which introduce a new API and we
really should get this into this release, the patches look good so far, so I
don't expect more than a v7 with mostly nitpick being fixed...

Thanks,
Erik

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

Re: [libvirt] [GSoC] Design ideas for implementing cleanup attribute

[Erik Skultety]

On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
> On 25 May 2018 at 16:20, Pavel Hrdina  wrote:
> > On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
> >> On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
> >> > On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
> >> > > However, I realize it might not be possible to register free
> >> > > functions for a native type without having to introduce something
> >> > > like
> >> > >
> >> > >   typedef char * virString;
> >> > >
> >> > > thus causing massive churn. How does GLib deal with that?
> >> >
> >> > If you would look into GLib documentation you would see that this
> >> > design basically copies the one in GLib:
> >>
> >> Sorry, I should have looked up the documentation and implementation
> >> before asking silly questions. Guess the morning coffee hadn't quite
> >> kicked in yet :/
> >>
> >> > GLiblibvirt
> >> >
> >> > g_autofree  VIR_AUTOFREE
> >> > g_autoptr   VIR_AUTOPTR
> >> > g_auto  VIR_AUTOCLEAR
> >>
> >> For what it's worth, I think VIR_AUTOCLEAR is a much better name
> >> than g_auto :)
> >>
> >> > In GLib you are using them like this:
> >> >
> >> > g_autofree char *string = NULL;
> >> > g_autoptr(virDomain) dom = NULL;
> >> > g_auto(virDomain) dom = { 0 };
> >> >
> >> > So yes it would require to introduce a lot of typedefs for basic types
> >> > and that is not worth it.
> >>
> >> I'm not sure we would need so many typedefs, but there would
> >> certainly be a lot of churn involved.
> >>
> >> Personally, I'm not so sure it wouldn't be worth the effort,
> >> but it's definitely something that we can experiment with it at
> >> a later time instead of holding up what's already a pretty
> >> significant improvement.
> >>
> >> > In libvirt we would have:
> >> >
> >> > VIR_AUTOFREE char *string = NULL;
> >> > VIR_AUTOPTR(virDomainPtr) dom = NULL;
> >> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
> >> >
> >> > If you notice the difference, in libvirt we can use virDomainPtr
> >> > directly because we have these typedefs, in GLib macro
> >> > G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
> >>
> >> While I'm not a fan of our *Ptr typedefs in general, I guess this
> >> time I'm glad we have them because VIR_AUTOPTR() doesn't hide the
> >> fact that what you're declaring is a pointer; that is, the macro
> >> argument is also exactly the type of the variable.
> >
> > So let's make a summary of how it could look like:
> >
> > VIR_AUTOFREE(char *) string = NULL;
> > VIR_AUTOPTR(virDomainPtr) vm = NULL;
> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
> >
> > VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree);
> > VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
>
> Do we define new functions for freeing/clearing, because that is what
> VIR_DEFINE_AUTOFREE_FUNC seems to do.
>
>
> This is what new macros will look like:
>
> # define _VIR_TYPE_PTR(type) type##Ptr
>
> # define _VIR_ATTR_AUTOFREE_PTR(type)  __attribute__((cleanup(type##Free)))
> # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close)))
> # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
>
> # define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) 
> _VIR_TYPE_PTR(type)
>
>
> The problem is that our vir*Free functions take on vir*Ptr as the
> parameter and not
> vir*Ptr * (pointer to it).
>
> For example, instead of:
> void virArpTableFree(virArpTablePtr table);
>
> we would need:
> void virArpTableFree(virArpTablePtr *table);


Hmm, so gcc claims the cleanup attribute to take a function which is required
to take a pointer to the type which it already is, it's just typedef'd, I
haven't tried this myself but do you get a compiler error if you try it with
the existing function, i.e. the one which doesn't mention the explicit '*' to
signal a pointer type? If so and we can't use the our typedef'd Ptr type
directly, then we'll need to switch all the free callback signatures to a plain
virSomeType *arg rather than trying to pass a double pointer which is just
unnecessary and ugly.

>
> if we declare something like:
> VIR_AUTOFREE_PTR(virArpTable) table = NULL;
>
>
> Also, I tried to add a new function:
> void virArpTablePtrFree(virArpTablePtr *table)
> {
> size_t i;
>
> if (!*table)
> return;
>
> for (i = 0; i < (*table)->n; i++) {
> VIR_FREE((*table)->t[i].ipaddr);
> VIR_FREE((*table)->t[i].mac);
> }
> VIR_FREE((*table)->t);
> VIR_FREE((*table));
> VIR_FREE(table);

My honest guess would be ^this you're trying to free the double pointer itself
which you didn't and you're also not supposed to allocate.

 Erik

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

[libvirt] plan for next release

[Daniel Veillard]

  Seems the end of the month is coming soon !
I suggest to enter freeze tomorrow morningm with plan for an RC2 on Thursday
and a final release on the week-end,

  Hope this works for everybody,

 thanks !

Daniel

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [GSoC] Design ideas for implementing cleanup attribute

[Sukrit Bhatnagar]

On 25 May 2018 at 16:20, Pavel Hrdina  wrote:
> On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
>> On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
>> > On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
>> > > However, I realize it might not be possible to register free
>> > > functions for a native type without having to introduce something
>> > > like
>> > >
>> > >   typedef char * virString;
>> > >
>> > > thus causing massive churn. How does GLib deal with that?
>> >
>> > If you would look into GLib documentation you would see that this
>> > design basically copies the one in GLib:
>>
>> Sorry, I should have looked up the documentation and implementation
>> before asking silly questions. Guess the morning coffee hadn't quite
>> kicked in yet :/
>>
>> > GLiblibvirt
>> >
>> > g_autofree  VIR_AUTOFREE
>> > g_autoptr   VIR_AUTOPTR
>> > g_auto  VIR_AUTOCLEAR
>>
>> For what it's worth, I think VIR_AUTOCLEAR is a much better name
>> than g_auto :)
>>
>> > In GLib you are using them like this:
>> >
>> > g_autofree char *string = NULL;
>> > g_autoptr(virDomain) dom = NULL;
>> > g_auto(virDomain) dom = { 0 };
>> >
>> > So yes it would require to introduce a lot of typedefs for basic types
>> > and that is not worth it.
>>
>> I'm not sure we would need so many typedefs, but there would
>> certainly be a lot of churn involved.
>>
>> Personally, I'm not so sure it wouldn't be worth the effort,
>> but it's definitely something that we can experiment with it at
>> a later time instead of holding up what's already a pretty
>> significant improvement.
>>
>> > In libvirt we would have:
>> >
>> > VIR_AUTOFREE char *string = NULL;
>> > VIR_AUTOPTR(virDomainPtr) dom = NULL;
>> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
>> >
>> > If you notice the difference, in libvirt we can use virDomainPtr
>> > directly because we have these typedefs, in GLib macro
>> > G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
>>
>> While I'm not a fan of our *Ptr typedefs in general, I guess this
>> time I'm glad we have them because VIR_AUTOPTR() doesn't hide the
>> fact that what you're declaring is a pointer; that is, the macro
>> argument is also exactly the type of the variable.
>
> So let's make a summary of how it could look like:
>
> VIR_AUTOFREE(char *) string = NULL;
> VIR_AUTOPTR(virDomainPtr) vm = NULL;
> VIR_AUTOCLEAR(virDomain) dom = { 0 };
>
> VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree);
> VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);

Do we define new functions for freeing/clearing, because that is what
VIR_DEFINE_AUTOFREE_FUNC seems to do.


This is what new macros will look like:

# define _VIR_TYPE_PTR(type) type##Ptr

# define _VIR_ATTR_AUTOFREE_PTR(type)  __attribute__((cleanup(type##Free)))
# define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close)))
# define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))

# define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) _VIR_TYPE_PTR(type)


The problem is that our vir*Free functions take on vir*Ptr as the
parameter and not
vir*Ptr * (pointer to it).

For example, instead of:
void virArpTableFree(virArpTablePtr table);

we would need:
void virArpTableFree(virArpTablePtr *table);

if we declare something like:
VIR_AUTOFREE_PTR(virArpTable) table = NULL;


Also, I tried to add a new function:
void virArpTablePtrFree(virArpTablePtr *table)
{
size_t i;

if (!*table)
return;

for (i = 0; i < (*table)->n; i++) {
VIR_FREE((*table)->t[i].ipaddr);
VIR_FREE((*table)->t[i].mac);
}
VIR_FREE((*table)->t);
VIR_FREE((*table));
VIR_FREE(table);
}

but I am getting the errors:
*** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-virbuftest':
free(): invalid pointer: 0x7ffc7be60d48 ***
...
*** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-commandtest':
free(): invalid pointer: 0x7fff727583fc ***
...

I am not quite sure how to debug this. Am I missing something basic?

>
> Note: don't take the types and function names as something that actually
> exists and be used like that, it's just an example to show how it would
> work :).
>
> Pavel

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

Re: [libvirt] [PATCH 1/5] storage: adding a skeleton for libvirt volume events.

[Peter Krempa]

On Sun, May 27, 2018 at 16:25:35 -0300, Julio Faracco wrote:
> This commit adds some basic structures to support events for volumes as
> libvirt does with pools, networks, domains, secrets, etc. This commit
> add only lifecycle event to be included at create and delete actions.
> 
> Signed-off-by: Julio Faracco 
> ---
>  include/libvirt/libvirt-storage.h |  86 
>  src/libvirt-storage.c | 125 ++
>  src/libvirt_public.syms   |   6 ++
>  3 files changed, 217 insertions(+)
> 
> diff --git a/include/libvirt/libvirt-storage.h 
> b/include/libvirt/libvirt-storage.h
> index 413d9f6c4c..291742e5ef 100644
> --- a/include/libvirt/libvirt-storage.h
> +++ b/include/libvirt/libvirt-storage.h

[...]

> +/**
> + * virStorageVolEventLifecycleType:
> + *
> + * a virStorageVolEventLifecycleType is emitted during storage vol
> + * lifecycle events
> + */
> +typedef enum {
> +VIR_STORAGE_VOL_EVENT_CREATED = 0,
> +VIR_STORAGE_VOL_EVENT_DELETED = 1,

How do you plan to handle the case of a storage pool refresh for the
filesystem (directory?) pool. Currently if you refresh it libvirt
removes all volume metadata and re-detects everything. If a image file
was manually deleted from the directory it will vanish after the
refresh. This means that if you want to properly emit these you'll need
to add a diffing mechanism so that you can regenerate all the events.

> +
> +# ifdef VIR_ENUM_SENTINELS
> +VIR_STORAGE_VOL_EVENT_LAST
> +# endif
> +} virStorageVolEventLifecycleType;
> +
> +/**
> + * virConnectStorageVolEventLifecycleCallback:
> + * @conn: connection object
> + * @vol: vol on which the event occurred
> + * @event: The specific virStorageVolEventLifeCycleType which occurred
> + * @detail: contains some details on the reason of the event.
> + * @opaque: application specified data
> + *
> + * This callback is called when a vol lifecycle action is performed, like 
> start
> + * or stop.

There are no such actions for a volume.

> + *
> + * The callback signature to use when registering for an event of type
> + * VIR_STORAGE_VOL_EVENT_ID_LIFECYCLE with
> + * virConnectStorageVolEventRegisterAny()
> + */
> +typedef void (*virConnectStorageVolEventLifecycleCallback)(virConnectPtr 
> conn,
> +   virStorageVolPtr 
> vol,
> +   int event,
> +   int detail,
> +   void *opaque);
> +
>  #endif /* __VIR_LIBVIRT_STORAGE_H__ */


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v6 1/9] qemu: provide support to query the SEV capability

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:26PM -0500, Brijesh Singh wrote:
> QEMU version >= 2.12 provides support for launching an encrypted VMs on
> AMD x86 platform using Secure Encrypted Virtualization (SEV) feature.
> This patch adds support to query the SEV capability from the qemu.
>
> Signed-off-by: Brijesh Singh 

minor nit (#bikesheding): this patch should be IMHO named the way the second 
one is:
qemu: Introduce SEV to hypervisor capabilities

Erik

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

Re: [libvirt] [PATCH v6 1/9] qemu: provide support to query the SEV capability

[Erik Skultety]

On Wed, May 23, 2018 at 04:18:26PM -0500, Brijesh Singh wrote:
> QEMU version >= 2.12 provides support for launching an encrypted VMs on
> AMD x86 platform using Secure Encrypted Virtualization (SEV) feature.
> This patch adds support to query the SEV capability from the qemu.
>
> Signed-off-by: Brijesh Singh 
> ---
>  src/conf/domain_capabilities.h | 13 
>  src/qemu/qemu_capabilities.c   | 47 ++
>  src/qemu/qemu_capabilities.h   |  4 ++
>  src/qemu/qemu_capspriv.h   |  4 ++
>  src/qemu/qemu_monitor.c|  9 +++
>  src/qemu/qemu_monitor.h|  3 +
>  src/qemu/qemu_monitor_json.c   | 74 
> ++
>  src/qemu/qemu_monitor_json.h   |  3 +
>  .../caps_2.12.0.x86_64.replies | 10 +++
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |  3 +-
>  10 files changed, 169 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
> index 9b852e8649bf..c1093234ceb8 100644
> --- a/src/conf/domain_capabilities.h
> +++ b/src/conf/domain_capabilities.h
> @@ -137,6 +137,19 @@ struct _virDomainCapsCPU {
>  virDomainCapsCPUModelsPtr custom;
>  };
>
> +/*
> + * SEV capabilities
> + */
> +typedef struct _virSEVCapability virSEVCapability;
> +typedef virSEVCapability *virSEVCapabilityPtr;
> +struct _virSEVCapability {
> +char *pdh;
> +char *cert_chain;
> +unsigned int cbitpos;
> +unsigned int reduced_phys_bits;
> +};
> +
> +
>  struct _virDomainCaps {
>  virObjectLockable parent;
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 8a63db5f4f33..49b74f7e12c1 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -489,6 +489,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>"screendump_device",
>"hda-output",
>"blockdev-del",
> +  "sev-guest",
>  );
>
>
> @@ -555,6 +556,8 @@ struct _virQEMUCaps {
>  size_t ngicCapabilities;
>  virGICCapability *gicCapabilities;
>
> +virSEVCapability *sevCapabilities;
> +
>  virQEMUCapsHostCPUData kvmCPU;
>  virQEMUCapsHostCPUData tcgCPU;
>  };
> @@ -1121,6 +1124,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] 
> = {
>  { "virtual-css-bridge", QEMU_CAPS_CCW },
>  { "vfio-ccw", QEMU_CAPS_DEVICE_VFIO_CCW },
>  { "hda-output", QEMU_CAPS_HDA_OUTPUT },
> +{ "sev-guest", QEMU_CAPS_SEV_GUEST },
>  };
>
>  static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = 
> {
> @@ -2050,6 +2054,28 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps,
>  }
>
>
> +void
> +virQEMUSevCapabilitiesFree(virSEVCapability *cap)

Since virSEVCapability will be added to virDomainCaps too, you need to move
^this into domain_capabilities.c so it will become virSEVCapabilityFree, I've
got a further comment regarding this in patch 2 as well.

NOTE: notice the SEV in the function name, we should stay consistent in naming
and since SEV is the name of the feature...


> +{
> +if (!cap)
> +return;
> +
> +VIR_FREE(cap->pdh);
> +VIR_FREE(cap->cert_chain);
> +VIR_FREE(cap);
> +}
> +
> +
> +void
> +virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps,
> +  virSEVCapability *capabilities)
> +{
> +virQEMUSevCapabilitiesFree(qemuCaps->sevCapabilities);

virSEVCapabilityFree(qemuCaps->sevCapabilities)

> +
> +qemuCaps->sevCapabilities = capabilities;
> +}
> +
> +
>  static int
>  virQEMUCapsProbeQMPCommands(virQEMUCapsPtr qemuCaps,
>  qemuMonitorPtr mon)
> @@ -2580,6 +2606,21 @@ virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr 
> qemuCaps,
>  }
>
>
> +static int
> +virQEMUCapsProbeQMPSEVCapabilities(virQEMUCapsPtr qemuCaps,
> +   qemuMonitorPtr mon)
> +{
> +virSEVCapability *caps = NULL;
> +
> +if (qemuMonitorGetSEVCapabilities(mon, ) < 0)
> +return -1;
> +
> +virQEMUCapsSetSEVCapabilities(qemuCaps, caps);
> +
> +return 0;
> +}
> +
> +
>  bool
>  virQEMUCapsCPUFilterFeatures(const char *name,
>   void *opaque)
> @@ -3965,6 +4006,12 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
>  virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW);
>  }
>
> +/* Probe for SEV capabilities */
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
> +if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
> +virQEMUCapsClear(qemuCaps, QEMU_CAPS_SEV_GUEST);
> +}
> +
>  ret = 0;
>   cleanup:
>  return ret;
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 3e120e64c0b4..8b7eef4359b7 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -473,6 

Re: [libvirt] [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Markus Armbruster]

Eduardo Habkost  writes:

> On Fri, May 25, 2018 at 08:30:59AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost  writes:
>> > On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote:
> [...]
>> >> >> Worse, a machine type property that is static for all machine types now
>> >> >> could conceivably become dynamic when we add a machine type
>> >> >> configuration knob.
>> >> >> 
>> >> >
>> >> > This isn't the first time a machine capability that seems static
>> >> > actually depends on other configuration arguments.  We will
>> >> > probably need to address this eventually.
>> >> 
>> >> Then the best time to address it is now, provided we can :)
>> >
>> > I'm not sure this is the best time.  If libvirt only needs the
>> > runtime value and don't need any info at query-machines time, I
>> > think support for this on query-machines will be left unused and
>> > they will only use the query-current-machine value.
>> >
>> > Just giving libvirt the runtime data it wants
>> > (query-current-machine) seems way better than requiring libvirt
>> > to interpret a set of rules and independently calculate something
>> > QEMU already knows.
>> 
>> I wouldn't mind adding a query-current-machine to report dynamic machine
>> capabilities if that helps QMP clients.  query-machines could continue
>> to report static machine capabilities then.
>> 
>> However, we do need a plan on how to distribute machine capabilities
>> between query-machines and query-current-machine, in particular how to
>> handle changing staticness.
>
> Handling dynamic data that becomes static is easy: we can keep it
> on both.

The duplication is less than elegant, but backward-compatibility
generally is.

>> wakeup-suspend-support is static for most machine types, but dynamic for
>> some.  Where should it go?
>
> I think it obviously should go on query-current-machine.  Maybe
> it can be added to query-machines in the future if it's deemed
> useful.
>
>> It needs to go into query-current-machine when its dynamic with the
>> current machine.  It may go there just to keep things regular even if
>> its static with the current machine.
>> 
>> Does it go into query-machines, too?  If not, clients lose the ability
>> to examine all machines efficiently.  Even if this isn't an issue for
>> wakeup-suspend-support: are we sure this can't be an issue for any
>> future capabilities?
>
> I don't think this will be an issue for wakup-suspend-support,
> but this is already an issue for existing capabilities.  See
> below[1].
>
>
>> 
>> If it goes into query-machines, what should its value be for the machine
>> types where it's dynamic?  Should it be absent, perhaps, letting clients
>> know they have to consult query-current-machine to find the value?
>> 
>> What if a capability previously thought static becomes dynamic?  We can
>> add it to query-current-machine just fine, but removing it from
>> query-machines would be a compatibility break.  Making it optional,
>> too.  Should all new members of MachineInfo be optional, just in case?
>> 
>
> The above are questions we must ponder if we are considering
> extending query-machines.  We have been avoiding them for a few
> years, by simply not extending query-machines very often and
> letting libvirt hardcode machine-type info.  :(
>
>
>> These are design questions we need to ponder *now*.  Picking a solution
>> that satisfies current needs while ignoring future needs has bitten us
>> in the posterior time and again.  We're not going to successfully
>> predict *all* future needs, but not even trying should be easy to beat.
>> 
>> That's what I meant by "the best time to address it is now".
>> 
>
> I agree.  I just think there are countless other use cases that
> require extending query-machines today.  I'd prefer to use one of
> them as a starting point for this design exercise, instead of
> wakeup-suspend-support.

Analyzing all the use cases we know makes sense.

> [1] Doing a:
>   $ git grep 'STR.*machine, "'
> on libvirt source is enough to find some code demonstrating where
> query-machines is already lacking today:
>
> src/libxl/libxl_capabilities.c:583:if (STREQ(machine, "xenpv"))
> src/libxl/libxl_capabilities.c:729:if (STREQ(domCaps->machine, "xenfv"))
> src/libxl/libxl_driver.c:6379:if (STRNEQ(machine, "xenpv") && 
> STRNEQ(machine, "xenfv")) {
> src/qemu/qemu_capabilities.c:2435:STREQ(def->os.machine, 
> "ppce500"))
> src/qemu/qemu_capabilities.c:2439:STREQ(def->os.machine, "prep"))
> src/qemu/qemu_capabilities.c:2443:STREQ(def->os.machine, 
> "bamboo"))
> src/qemu/qemu_capabilities.c:2446:if (STREQ(def->os.machine, 
> "mpc8544ds"))
> src/qemu/qemu_capabilities.c:5376:STREQ(def->os.machine, "isapc");
> src/qemu/qemu_command.c:3829:if (STRPREFIX(def->os.machine, 
> "s390-virtio") &&
> src/qemu/qemu_domain.c:2798:if (STREQ(def->os.machine, "isapc")) {
> 

Re: [libvirt] [PATCHv2 3/3] xen_common: convert to typesafe virConf accessors

[Ján Tomko]

On Sun, May 27, 2018 at 02:08:58PM +0200, Fabiano Fidêncio wrote:

On Sun, May 27, 2018 at 1:17 PM, Ján Tomko  wrote:

On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote:


From: Fabiano Fidêncio 

There are still some places using virConfGetValue() and then checking
the specific type of the pointers and so on.

Those place are not going to be changed as:
- Directly using virConfGetValue*() would trigger virReportError() on
their current code



Is that a problem in xenParseCPUFeatures?


It would, at least, generate one more log, which would be misleading
whoever ends up debugging some issue on that codepath later on.



I don't see it.
xenConfigGetULong already reports an error when the "maxvcpus" value is
malformed.

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v3] external (pull) backup API

[Peter Krempa]

On Fri, May 25, 2018 at 10:26:12 -0500, Eric Blake wrote:
> On 05/17/2018 05:43 PM, Eric Blake wrote:
> > Here's my updated counterproposal for a backup API.
> > 
> > In comparison to v2 posted by Nikolay:
> > https://www.redhat.com/archives/libvir-list/2018-April/msg00115.html
> > - changed terminology a bit: Nikolay's "BlockSnapshot" is now called a
> > "Checkpoint", and "BlockExportStart/Stop" is now "BackupBegin/End"
> > - flesh out more API descriptions
> > - better documentation of proposed XML, for both checkpoints and backup
> > 
> > Barring any major issues turned up during review, I've already starting
> > to code this into libvirt with a goal of getting an implementation ready
> > for review this month.
> > 
> 
> > // Many additional functions copying heavily from virDomainSnapshot*:
> > 
> > virDomainCheckpointList(virDomainPtr domain,
> >      virDomainCheckpointPtr **checkpoints,
> >      unsigned int flags);
> > 
> 
> > 
> > int
> > virDomainCheckpointListChildren(virDomainCheckpointPtr checkpoint,
> >      virDomainCheckpointPtr **children,
> >      unsigned int flags);
> > 
> > Notably, none of the older racy list functions, like
> > virDomainSnapshotNum, virDomainSnapshotNumChildren, or
> > virDomainSnapshotListChildrenNames; also, for now, there is no revert
> > support like virDomainSnapshotRevert.
> 
> I'm finding it easier to understand if I name these:
> 
> virDomainListCheckpoints() (find checkpoints relative to a domain)
> virDomainCheckpointListChildren() (find children relative to a checkpoint)

If you are going to name them "checkpoints" here we should first
rename "snapshots with memory" in our docs since we refer to them as
checkpoints. We refer to disk-only snapshots as snapshots and wanted to
emphasize the difference.

> 
> The counterpart Snapshot API used virDomainListAllSnapshots(); the term
> 'All' was present because it was added after the initial racy
> virDomainSnapshotNum(), but as we are avoiding the racy API here we can skip
> it from the beginning.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 08/13] conf: Introduce parser, formatter for uid and fid

[Bjoern Walk]

Pino Toscano  [2018-05-25, 01:05PM +0200]:
> On Thursday, 24 May 2018 14:24:33 CEST Xiao Feng Ren wrote:
> > diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
> > index 1a18cd31b1..8050a3ebc4 100644
> > --- a/docs/schemas/basictypes.rng
> > +++ b/docs/schemas/basictypes.rng
> > @@ -111,6 +111,34 @@
> >
> >  
> >
> > +  
> > +
> > +  
> > +
> > +  
> > +(0x)?[0-9a-fA-F]{1,4}
> > +  
> > +  
> > +1
> > +65535
> > +  
> > +
> > +  
> 
> This seems to be the "uint16" type already.

uint16 has '0' inclusive which we do not technically allow, although we
couldn't come up with a nice enough regex to also exlude it there.

> > +
> > +
> > +  
> > +
> > +  
> > +(0x)?[0-9a-fA-F]{1,8}
> > +  
> > +  
> > +0
> > +4294967295
> > +  
> > +
> > +  
> 
> This could be a new "uint32" type, changing the 0x prefix as
> non-optional (otherwise the value "10" can be both valid as decimal
> and hexadeciaml).

Here it would be fine.

> > @@ -57,6 +125,8 @@ void
> >  virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
> >  {
> >  VIR_FREE(info->alias);
> > +if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
> > +VIR_FREE(info->addr.pci.zpci);
> 
> VIR_FREE should be safe to use on a NULL pointer, so just call it
> directly without checking the type.

But if the union info->addr is not of type PCI then the pointer could
hold arbitrary value, right? Which is not the case currently, because
the zpci field is far enough in the structure to be always 0 set, but we
shouldn't rely on this IMHO.

-- 
IBM Systems
Linux on Z & Virtualization Development

IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819

Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list