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

[Yi Min Zhao]

在 2018/9/13 下午9:58, 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.
Got it.
>
>>> [...]
>>>> @@ -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?
I did a simple test. It worked. Do you prefer this way?
>
>> 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.
Right. That's why I finally gave up it.
>
>>> 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 :)
Yes.
>
>
> [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.

-- 
Yi Min