[libvirt] [PATCH v3 09/18] qemu: set/use info->pciConnectFlags during qemuDomainAssignDevicePCISlots

Wed Oct 5 13:04:51 UTC 2016

On Tue, 2016-10-04 at 11:50 -0400, Laine Stump wrote:
> > All these differences between the code you're removing and
> > the code that's supposed to replace it make me wonder whether
> > I'm missing something. Feel free to point out my mistake
> > right about... Now! :)
> 
> Yeah, you're missing the fact that these current flag settings are 
> incorrect, decided on when I had less information/knowledge than I do 
> now.  Fixing them doesn't create any changes in behavior, but it does 
> set us up to make behavior changes (in the right direction) in upcoming 
> patches.
> 
> > On a related note, reviewing this would be *much* easier if
> > you could start off qemuDomainDeviceConnectFlagsInternal()
> > as (mostly) a straight copy of this chunk of code, and then
> > stray from it after switching over, instead of starting with
> > a completely different implementation.
> 
> I might have to create an extra patch so that this function isn't called 
> until it is "fixed", but I see your point.

Let me elaborate a bit.

After reading your explanation above for the specific cases
I pointed out, I see why you did change the flags in the
way you did. But, until you replied to my questions, the
information was just not there.

This series is basically doing two things at once:

  * refactoring the code so that connect flags for PCI
    devices is centralized

  * updating connect flags to either fix earlier mistakes
    or enable better device placement (more PCIe)

We definitely want both, and the latter is of course much
easier to achieve after the former has been completed.

However, they really are two separate goals and mixing
them makes the reviewer's life harder, which is why I'm
strongly against it :)

My recommendation would be to have either two separate
series (it's okay for a series to depend on another one),
or a single series with clearly defined "part one" and
"part two".

The refactoring should not change the behavior at all,
just move around bits until all connect flags related
decisions happen in a single place; after that's out of
the way, changes in connect flags, even the ones that are
fixing existing bugs, should go in separate commits that
explain why the change is being performed. The last few
comments in this series are great examples of the approach
I'm describing.

The result will be a much easier time for the reviewer
and a much more useful Git history to refer to a few years
down the line, when we will inevitably have to reassess
some of the choices made today. It should also be noted
that the commit messages for the series have been overall
excellent - we just need more! ;)

> > I also believe the original code is easier to follow, because
> > it looks more like a direct mapping from model to flags.
> > Ideally we would just list all devices and models, grouped by
> > purpose, and explicitly assign flags to them, in a similar
> > way we get flags for PCI controllers.
> 
> But the PCI controllers each have a different flag (because they're all 
> slightly different) so an entry for each one is needed anyhow. For the 
> rest of the devices, most of them are plain "PCI + hotpluggable", with a 
> relatively small number of exceptions. If we explicitly put in an entry 
> for every model of every device, we could end up in a situation of 
> losing the interesting "trees" (exceptions) in the "forest" (all the 
> nearly identical settings to the default).
> 
> > And shouldn't the compiler give us at least a warning when
> > we're not considering all the possible values of an enum as
> > part of a switch()? That would be super-neat!
> 
> It's because many of these variables aren't defined as enums in the 
> device object, but as int. I suppose this could be taken care of by 
> typecasting to the enum in the switches. Again, it seems like a lot of 
> extra lines when the majority of emulated devices are still legacy PCI 
> though. Let me think about it...

I see what you mean, however by using the fallthrough
property of the switch statement we should be able to group
a whole bunch of devices and avoid excessive redundancy,
like in the very code you're removing with this commit:

  case VIR_DOMAIN_CONTROLLER_TYPE_USB:
      /* allow UHCI and EHCI controllers to be manually placed on
       * the PCIe bus (but don't put them there automatically)
       */
      switch (device->data.controller->model) {
      case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
      case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
      case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
      case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
      case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
      case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
          flags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
          break;
      case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
          /* should this be PCIE-only? Or do we need to allow PCI
           * for backward compatibility?
           */
          flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE
                   | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE);
          break;
      case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
      case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
      case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
          /* Allow these for PCI only */
          break;
      }
  }
  break;

Is this more verbose than the code you're replacing it
with? Sure. But breaking down all the models this way
allows us to clearly see what kind of device can go in
which controller with a single glance, which I consider
a big win.

Especially if we can then use casting to convince the
compiler to alert us if we ever add a new model and
forget to update this function, which is prefereable to
it silently getting the default connect flags.

The more deliberate we have to be when making changes,
the least likely it is that something will slip through
the cracks.

> > [...]
> > > @@ -1325,27 +1264,31 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
> > >             * in hostdevs list anyway, so handle them with other hostdevs
> > >             * instead of here.
> > >             */
> > > -        if ((def->nets[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) ||
> > > -            !virDeviceInfoPCIAddressWanted(&def->nets[i]->info)) {
> > > +        virDomainNetDefPtr net = def->nets[i];
> > > +
> > > +        if ((net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) ||
> > > +            !virDeviceInfoPCIAddressWanted(&net->info)) {
> > >                continue;
> > >            }
> > > -        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->nets[i]->info,
> > > -                                                flags) < 0)
> > > +
> > > +        if (qemuDomainPCIAddressReserveNextSlot(addrs, &net->info) < 0)
> > 
> > The change from def->nets[i] to net is a good one, but it
> > should have been done in a separate commit earlier in the
> > series. In fact, patch 2/18 is basically the same change
> > in a different part of libvirt :)
> 
> Well, that patch changed def->controllers[i] to cont, not def->nets[i] :-P
> 
> There were a few places this happened (going over the usage threshold of 
> def->blahs[i] so that it became cleaner to have a temp variable). The 
> ones with controllers[i] were just so excessive that I went back and 
> pulled them out into a separate patch. When a few more came along later, 
> they didn't seem as significant, and I was more burned out on the whole 
> "create more patches to make the set look more intimidating to a 
> reviewer while simultaneously creating merge conflicts when I rebase to 
> add the new patch" thing.
> 
> If that makes it easier for you, I can pull out the rest of these on the 
> next round though.

Yes, I'd very much like to have these in a separate patch,
early in the refactoring series / first part of the series.

And at least as far as I'm concerned, a series consisting
in a big number of small, self-contained patches is *less*
rather than more intimidating :)

> > [...]
> > > @@ -1412,7 +1355,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
> > >    
> > >                if (foundAddr) {
> > >                    /* Reserve this function on the slot we found */
> > > -                if (virDomainPCIAddressReserveAddr(addrs, &addr, flags,
> > > +                if (virDomainPCIAddressReserveAddr(addrs, &addr,
> > > +                                                   def->controllers[i]->info.pciConnectFlags,
> > 
> > Long line is long.
> 
> I'm no longer able to figure out when you seriously want me to fix 
> these, and when you're just existentially complaining about the state of 
> the universe :-P

Can't it be both? :)

For a more serious answer, see [1].

[1] https://www.redhat.com/archives/libvir-list/2016-October/msg00157.html
-- 
Andrea Bolognani / Red Hat / Virtualization