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

[Andrea Bolognani]

On Thu, 2018-09-13 at 17:58 +0800, Yi Min Zhao wrote:
> 在 2018/9/11 下午8:05, Andrea Bolognani 写道:
> > On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
> > [...]
> > > +bool
> > > +virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
> > > +{
> > > +    return !(addr->uid || addr->fid);
> > > +}
> > 
> > This function belongs in virpci, besides the struct definition and
> > the very much related virPCIDeviceAddressIsEmpty().
> 
> I'm not very clear with your comment. Do you mean I
> should move it to other place?

Yeah, the function and its definition should be in src/util/virpci.c
and src/util/virpci.h respectively.

I realize now that virPCIDeviceAddressIsValid() and
virPCIDeviceAddressIsEmpty() are *not* in util/virpci, though I
swear that I posted patches moving them over... My bad, I'll do
that right away.

Sorry for the confusion.

> > [...]
> > > @@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> > >       if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true))
> > >           goto cleanup;
> > >   
> > > +    if (virZPCIDeviceAddressParseXML(node, addr) < 0)
> > > +        goto cleanup;
> > 
> > I'm not super convinced about this.
> > 
> > On one hand, it makes it so callers can't possibly forget to parse
> > the zPCI part, and that's literally embedded in the PCI part now;
> > on the other hand, we have functions to verify separately whether
> > the PCI and zPCI parts are empty, which is pretty weird.
> 
> It's weird indeed. But if we merge zPCI part check into PCI code. We might
> have to merge other code. But PCI address has extension only on S390
> at least now.

That's not necessarily a problem, at least in principle.

See all the code dealing with "isolation groups", for example:
while the concept is only ever really used for pSeries guests, it's
implemented in a way that's designed to stay out of the way in all
other cases, and the result is that you won't have to worry about
it except when needed.

The zPCI code should ideally behave similarly.

> > I guess we can leave as it is for now, but the semantics are muddy
> > enough that I can pretty much guarantee someone will have to clean
> > them up at some point down the line.
> 
> OK.
> > 
> > [...]
> > > @@ -1044,6 +1044,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> > >                                          dev->isolationGroup, function) < 0)
> > >           return -1;
> > >   
> > > +    if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
> > > +        addr.zpci = dev->addr.pci.zpci;
> > 
> > I'm confused by this part. Shouldn't this either request a new set
> > of zPCI ids, the same way it's allocating a new PCI address right
> > above, or do nothing at all because zPCI address allocation is
> > handled by later calling virDomainZPCIAddressReserveNextAddr()?
> 
> Here, we want to store the defined zPCI address which has been reserved.
> If we don't do this, we might lose zPCI address and do reservation again
> but the reserved one are still existing in zPCI set.

I can't picture the exact scenario right now but I assume something
like that can happen if you have

  <address type='pci' uid='0x0001' fid='0x00000001'/>

in which case the zPCI part has already been provided by the user
but we still need to allocate the PCI part ourselves.

Speaking of which, I wonder if something like

  <address type='pci'>
    <zpci uid='0x0001' fid='0x00000001'/>
  </address>

would be a more appropriate syntax: not only it clearly shows that
the uid and fid attributes are connected to zPCI, but if we ever
introduce additional PCI address extensions they could be similarly
be represented as childs of <address> instead of adding even more
attributes to the existing element.

Either way, this kind of ad-hoc messing with the zPCI part seems
to me like clear evidence of the fact that we need to step back and
rethink the way the various pieces of the puzzle fit together.

>From the high level point of view, code outside of conf/ should,
for the most part, not need to concern itself with zPCI at all: it
would eg. ask for a PCI address to be allocated, and if the device
in question can be a zPCI device then a zPCI extension address will
be allocated for it as part of the same function call; the only
part of qemu/ that should care about the zPCI address is the one
generating the relevant command line arguments.

Can you try and see whether this kind of API would work?

> For this problem, I once thought about adding extension address into
> DeviceInfo next to PCI address embraced in a struct. Then here is
> not a problem.

That could almost work, seeing how virDomainDeviceInfoFormat()
doesn't use virPCIDeviceAddressFormat() to format the PCI address[1],
but at least in theory you should be able to take a
virPCIDeviceAddress and turn it into a string without having to peek
into other objects such as the parent virDomainDeviceInfo, so that
approach wouldn't be very clean at all.

> > This is basically another manifestation of the issue I mentioned
> > above: we don't seem to have a well-defined and strictly adhered
> > to idea of how the PCI part and zPCI part should relate to each
> > other, so they're either considered separate entities or part of
> > the same entity depending on which APIs you're looking at.
> 
> I think the above idea I thought about before is like what you said?

Do you mean the idea of moving the zPCI part out of the PCI address?
If so, I've already replied above; if not, please be more specific :)


[1] Which is arguably an issue that should be resolved as well...
    There are a few places where virPCIDeviceAddressFormat() *is*
    used, and those won't ever format the zPCI stuff.
-- 
Andrea Bolognani / Red Hat / Virtualization