[libvirt] [PATCH v5 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address

[Yi Min Zhao]

在 2018/9/17 下午8:05, Andrea Bolognani 写道:
> On Mon, 2018-09-17 at 13:43 +0800, Yi Min Zhao wrote:
>> 在 2018/9/11 下午9:59, Andrea Bolognani 写道:
>>>> +static void
>>>> +virDomainZPCIAddressReleaseUid(virHashTablePtr set,
>>>> +                               virZPCIDeviceAddressPtr addr)
>>>> +{
>>>> +    if (virHashRemoveEntry(set, &addr->uid) < 0) {
>>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> +                       _("Release uid %u failed"), addr->uid);
>>>> +    }
>>> You should have a generic virDomainZPCIAddressReleaseId() function
>>> that you call from ReleaseUid() and ReleaseFid(), just like you
>>> have for reserving them.
>> Actually there's the function ***ReleaseId() like you said. Don't you
>> see it?
> No such function exists; there is a virDomainZPCIAddressReleaseIds()
> but that doesn't do what I had in mind, which is along the lines of
>
>    static void
>    virDomainZPCIAddressReleaseId(virHashTablePtr set,
>                                  unsigned int *id,
>                                  const char *name)
>    {
>        if (virHashRemoveEntry(set, id) < 0) {
>            virReportError(VIR_ERR_INTERNAL_ERROR,
>                           _("Release %s %u failed"), name, *id);
>        }
>    
>        *id = 0;
>    }
>    
>    static void
>    virDomainZPCIAddressReleaseUid(virHashTablePtr set,
>                                   virZPCIDeviceAddressPtr addr)
>    {
>        virDomainZPCIAddressReleaseId(set, &addr->uid, "uid");
>    }
>    
>    static void
>    virDomainZPCIAddressReleaseFid(virHashTablePtr set,
>                                   virZPCIDeviceAddressPtr addr)
>    {
>        virDomainZPCIAddressReleaseId(set, &addr->fid, "fid");
>    }
Got it.
>
>>>> +int
>>>> +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
>>>> +                                            virPCIDeviceAddressPtr dev,
>>>> +                                            virDomainPCIAddressExtensionFlags extFlags)
>>>> +{
>>>> +    if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
>>>> +        virZPCIDeviceAddressIsEmpty(&dev->zpci)) {
>>> You shouldn't need the second check: just go ahead and reserve the
>>> next address regardless of what's currently stored in the device
>>> info, no?
>> I think it's hard to do it as what you said. We process assigned zpci
>> addresses
>> firstly. And then reserve next address for empty zpci addresses. If we don't
>> check this, we might reserve an address for a reserved one.
> Shouldn't the EnsureAddr() function take care of avoiding that?
> It will call ReserveAddr() or ReserveNextAddr() based on whether
> or not an address has already been provided by the user.
>
IIUC, EnsureAddr() is handling hotplug case, and ReserveNextAddr() is also
called in startup stage. After reserve defined addresses, RerserveNextAddr()
is called to allocate addresses. Here, we should check if it's reserved.
You could see virDomainDeviceInfoIterate(def, 
qemuDomainAssignPCIAddressExtension, addrs)
in qemuDomainAssignPCIAddresses().

-- 
Yi Min