[libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation

Andrea Bolognani posted 10 patches 6 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1498759443-10136-1-git-send-email-abologna@redhat.com
src/bhyve/bhyve_parse_command.c |   4 +-
src/conf/device_conf.c          | 146 ++++++++++++
src/conf/device_conf.h          |   9 +
src/conf/domain_addr.c          |   6 +-
src/conf/domain_conf.c          | 486 ++++++++++++++++++++++++----------------
src/conf/domain_conf.h          |  25 ++-
src/libvirt_private.syms        |  19 +-
src/lxc/lxc_native.c            |   2 +-
src/openvz/openvz_conf.c        |   2 +-
src/qemu/qemu_command.c         |  12 +-
src/qemu/qemu_domain.c          |  11 +-
src/qemu/qemu_domain_address.c  |  31 +--
src/qemu/qemu_hotplug.c         |   5 +-
src/qemu/qemu_parse_command.c   |  31 +--
src/vbox/vbox_common.c          |  14 +-
src/vmx/vmx.c                   |   2 +-
src/vz/vz_sdk.c                 |   6 +-
src/xen/xen_driver.c            |   2 +-
src/xenapi/xenapi_driver.c      |   2 +-
src/xenconfig/xen_common.c      |   4 +-
src/xenconfig/xen_sxpr.c        |  10 +-
src/xenconfig/xen_xl.c          |   4 +-
src/xenconfig/xen_xm.c          |   2 +-
tests/virhostdevtest.c          |   2 +-
24 files changed, 561 insertions(+), 276 deletions(-)
[libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
Posted by Andrea Bolognani 6 years, 9 months ago
This series is required to solve a known limitation in the
current incarnation of device isolation support[1], namely
the inability to isolate host devices coming from IOMMU
group 0.

The issue lies in the fact that virDomainDeviceInfo, and
all virDomain*Def that embed it, are usually allocated
through VIR_ALLOC(), which result in all their fields being
initialized to zero.

That's worked out just fine so far, because zero was a
sensible default value for all existing fields; however,
when implementing isolation groups, we add a new
virDomainDeviceInfo::isolationGroup field which we need
to be initialized to -1 instead so that it doesn't overlap
with IOMMU group 0 mentioned above.

Solving the issue involves creating twenty or so
virDomain*DefNew() functions and using them instead of
VIR_ALLOC() every time a virDomain*Def needs to be created,
which is arguably a pretty good idea regardless.

The series could probably be organized so that eg. the
patch order makes more sense, but honestly I'm pretty
tired right now and I just don't have it in me. Moreover,
the goal of the series and its implementation are both
straighforward enough that I feel reviewers will have no
problem following along.

My main concern with this series is that, while converting
from VIR_ALLOC() to virDomain*DefNew(), I might simply have
missed some instances. That would not cause any issue right
away, but once we introduce isolation groups it would lead
to suboptimal PCI addresses being allocated at best, and to
perfectly legitimate hotplug requests being denied at worst.

The fact that the test suite[2] passes gives me a lot of
confidence that I haven't overlooked anything, but if
anyone has clever ideas on how to be actually sure rather
than merely confident please do let me know.


[1] https://www.redhat.com/archives/libvir-list/2017-June/msg01018.html
[2] That is, the version of it where the default isolation
    group being initialized correctly actually matters

Andrea Bolognani (10):
  conf: Make virDomainDeviceGetInfo() private
  conf: Rename virDomainHostdevDefAlloc() to virDomainHostdevDefNew()
  conf: Clean up virDomainHostdevDefNew()
  conf: Clean up virDomain{Memory,Shmem}DefParseXML()
  conf: Move some virDomainDeviceInfo functions
  conf: Introduce virDomainDeviceInfoNew()
  conf: Clean up virDomainDeviceInfo functions
  conf, qemu: Use virDomainDeviceInfoNew()
  conf: Provide missing virDomain*DefNew() functions
  conf: Call virDomainDeviceInfoClear() in virDomain*DefNew()

 src/bhyve/bhyve_parse_command.c |   4 +-
 src/conf/device_conf.c          | 146 ++++++++++++
 src/conf/device_conf.h          |   9 +
 src/conf/domain_addr.c          |   6 +-
 src/conf/domain_conf.c          | 486 ++++++++++++++++++++++++----------------
 src/conf/domain_conf.h          |  25 ++-
 src/libvirt_private.syms        |  19 +-
 src/lxc/lxc_native.c            |   2 +-
 src/openvz/openvz_conf.c        |   2 +-
 src/qemu/qemu_command.c         |  12 +-
 src/qemu/qemu_domain.c          |  11 +-
 src/qemu/qemu_domain_address.c  |  31 +--
 src/qemu/qemu_hotplug.c         |   5 +-
 src/qemu/qemu_parse_command.c   |  31 +--
 src/vbox/vbox_common.c          |  14 +-
 src/vmx/vmx.c                   |   2 +-
 src/vz/vz_sdk.c                 |   6 +-
 src/xen/xen_driver.c            |   2 +-
 src/xenapi/xenapi_driver.c      |   2 +-
 src/xenconfig/xen_common.c      |   4 +-
 src/xenconfig/xen_sxpr.c        |  10 +-
 src/xenconfig/xen_xl.c          |   4 +-
 src/xenconfig/xen_xm.c          |   2 +-
 tests/virhostdevtest.c          |   2 +-
 24 files changed, 561 insertions(+), 276 deletions(-)

-- 
2.7.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
Posted by Andrea Bolognani 6 years, 9 months ago
On Thu, 2017-06-29 at 20:03 +0200, Andrea Bolognani wrote:
> This series is required to solve a known limitation in the
> current incarnation of device isolation support[1], namely
> the inability to isolate host devices coming from IOMMU
> group 0.
> 
> The issue lies in the fact that virDomainDeviceInfo, and
> all virDomain*Def that embed it, are usually allocated
> through VIR_ALLOC(), which result in all their fields being
> initialized to zero.
> 
> That's worked out just fine so far, because zero was a
> sensible default value for all existing fields; however,
> when implementing isolation groups, we add a new
> virDomainDeviceInfo::isolationGroup field which we need
> to be initialized to -1 instead so that it doesn't overlap
> with IOMMU group 0 mentioned above.

Or we could just, you know, do the sensible thing and
store (IOMMU group + 1) instead of (IOMMU group) in
virDomainDeviceInfo::isolationGroup and avoid the issue
altogether? I'm actually quite embarassed I didn't think
of that earlier :/

> Solving the issue involves creating twenty or so
> virDomain*DefNew() functions and using them instead of
> VIR_ALLOC() every time a virDomain*Def needs to be created,
> which is arguably a pretty good idea regardless.

We could still merge this series, though, or at least
parts of it. It improves upon some questionable code.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
Posted by Peter Krempa 6 years, 9 months ago
On Fri, Jun 30, 2017 at 10:48:36 +0200, Andrea Bolognani wrote:
> On Thu, 2017-06-29 at 20:03 +0200, Andrea Bolognani wrote:

[...]

> > That's worked out just fine so far, because zero was a
> > sensible default value for all existing fields; however,
> > when implementing isolation groups, we add a new
> > virDomainDeviceInfo::isolationGroup field which we need
> > to be initialized to -1 instead so that it doesn't overlap
> > with IOMMU group 0 mentioned above.
> 
> Or we could just, you know, do the sensible thing and
> store (IOMMU group + 1) instead of (IOMMU group) in

How is that sensible? That looks as a source of bugs in the long run.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
Posted by Peter Krempa 6 years, 9 months ago
On Fri, Jun 30, 2017 at 10:58:42 +0200, Peter Krempa wrote:
> On Fri, Jun 30, 2017 at 10:48:36 +0200, Andrea Bolognani wrote:
> > On Thu, 2017-06-29 at 20:03 +0200, Andrea Bolognani wrote:
> 
> [...]
> 
> > > That's worked out just fine so far, because zero was a
> > > sensible default value for all existing fields; however,
> > > when implementing isolation groups, we add a new
> > > virDomainDeviceInfo::isolationGroup field which we need
> > > to be initialized to -1 instead so that it doesn't overlap
> > > with IOMMU group 0 mentioned above.
> > 
> > Or we could just, you know, do the sensible thing and
> > store (IOMMU group + 1) instead of (IOMMU group) in
> 
> How is that sensible? That looks as a source of bugs in the long run.

Btw, isn't there any external detail which would indicate that the IOMMU
group is valid? In that case you could use that and read the value only
if it's supposed to be meaningful.

Or add a boolean which says that it's meaningfull if you are on the lazy
side, but storing the group with any offset is weird.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
Posted by Andrea Bolognani 6 years, 9 months ago
On Fri, 2017-06-30 at 10:58 +0200, Peter Krempa wrote:
> > Or we could just, you know, do the sensible thing and
> > store (IOMMU group + 1) instead of (IOMMU group) in
> 
> How is that sensible? That looks as a source of bugs in the long run.

Isolation groups are used to make sure any given device ends
up on the same bus as related devices and on a different bus
as unrelated devices.

They're an abstract concept, and while working on the initial
implementation it just happened to be convenient for me to
have the isolation group match the IOMMU group. There's no
specific reason that has to be the case.

We're never converting back and forth between the two, which
I agree would end up in misery at some point down the line;
we just set the isolation group once per device and then just
perform comparison between isolation groups from there on.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
Posted by Peter Krempa 6 years, 9 months ago
On Fri, Jun 30, 2017 at 12:24:33 +0200, Andrea Bolognani wrote:
> On Fri, 2017-06-30 at 10:58 +0200, Peter Krempa wrote:
> > > Or we could just, you know, do the sensible thing and
> > > store (IOMMU group + 1) instead of (IOMMU group) in
> > 
> > How is that sensible? That looks as a source of bugs in the long run.
> 
> Isolation groups are used to make sure any given device ends
> up on the same bus as related devices and on a different bus
> as unrelated devices.
> 
> They're an abstract concept, and while working on the initial
> implementation it just happened to be convenient for me to
> have the isolation group match the IOMMU group. There's no
> specific reason that has to be the case.

Fair enough. The documentation you are adding in the linked series is
vague enough to alow this meaning too:

@@ -164,6 +164,16 @@ struct _virDomainDeviceInfo {
      */
     int pciConnectFlags; /* enum virDomainPCIConnectFlags */
     char *loadparm;
+
+    /* PCI devices will only be automatically placed on a PCI bus
+     * that shares the same isolation group */
+    int isolationGroup;
+
+    /* Usually, PCI buses will take on the same isolation group
+     * as the first device that is plugged into them, but in some
+     * cases we might want to prevent that from happening by
+     * locking the isolation group */
+    bool isolationGroupLocked;
 };

> We're never converting back and forth between the two, which
> I agree would end up in misery at some point down the line;
> we just set the isolation group once per device and then just
> perform comparison between isolation groups from there on.

I'd suggest you create a helper to assign those then (be it from IOMMU
group or something else), so there's at least a single point that can be
referenced in the future and which will explain this reasoning.

Also adding a note that 0 means the device is not isolated would make
sense in the structure above.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
Posted by Andrea Bolognani 6 years, 9 months ago
On Fri, 2017-06-30 at 12:38 +0200, Peter Krempa wrote:
> > Isolation groups are used to make sure any given device ends
> > up on the same bus as related devices and on a different bus
> > as unrelated devices.
> > 
> > They're an abstract concept, and while working on the initial
> > implementation it just happened to be convenient for me to
> > have the isolation group match the IOMMU group. There's no
> > specific reason that has to be the case.
> 
> Fair enough. The documentation you are adding in the linked series is
> vague enough to alow this meaning too:
> 
> @@ -164,6 +164,16 @@ struct _virDomainDeviceInfo {
>       */
>      int pciConnectFlags; /* enum virDomainPCIConnectFlags */
>      char *loadparm;
> +
> +    /* PCI devices will only be automatically placed on a PCI bus
> +     * that shares the same isolation group */
> +    int isolationGroup;
> +
> +    /* Usually, PCI buses will take on the same isolation group
> +     * as the first device that is plugged into them, but in some
> +     * cases we might want to prevent that from happening by
> +     * locking the isolation group */
> +    bool isolationGroupLocked;
>  };

It's vague on purpose :)

All I'm describing there is the interface from the generic
PCI address allocation code's point of view: the fact that
the QEMU driver derives isolation groups from IOMMU groups
is just an implementation detail and as such should not be
mentioned at all.

> > We're never converting back and forth between the two, which
> > I agree would end up in misery at some point down the line;
> > we just set the isolation group once per device and then just
> > perform comparison between isolation groups from there on.
> 
> I'd suggest you create a helper to assign those then (be it from IOMMU
> group or something else), so there's at least a single point that can be
> referenced in the future and which will explain this reasoning.

Good idea, I'll do that!

> Also adding a note that 0 means the device is not isolated would make
> sense in the structure above.

Well, devices *always* get isolated: it's just that all
guests except for pSeries only ever have a single isolation
group ;)

-- 
Andrea Bolognani / Red Hat / Virtualization

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