Re: [libvirt] [PATCH v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes

On Thu, 2018-08-23 at 14:40 +0800, Yi Min Zhao wrote: > 在 2018/8/22 下午6:09, Andrea Bolognani 写道: >> On Wed, 2018-08-22 at 17:39 +0800, Yi Min Zhao wrote: >>> I tried as your idea. It makes everything complicated, especially >>> alloc/reserve/release >>> zpci address. If the user defines uid=1 and fid=0, we don't know whether >>> should >>> reserve fid. (uid=1 fid=0) is the same with (uid=1). >> You should reserve it. The user can either >> >> * not specify zPCI information at all: automatic assignment will >> be performed, no error should be reported; >> >> * specify a *full* zPCI address: manual assignment, will result >> in either that exact address being used or an error; >> >> * specify partial information: missing properties will default >> to zero, which will probably cause errors eg. if only the uid >> is specified for a bunch of devices. >> >> The last option is less predictable so it should probably never be >> used. All of the above is consistent with how the PCI part works. > And then, zpci address instance could be a member of pci address structure? > I mean not using pointer at all? Exactly. You can either just add zpci_uid and zpci_fid to the virPCIDeviceAddress struct, or have struct _virZPCIDeviceAddress { unsigned int uid; unsigned int fid; }; struct _virPCIDeviceAddress { unsigned int domain; unsigned int bus; unsigned int slot; unsigned int function; int multi; /* virTristateSwitch */ virZPCIDeviceAddress zpci; };

that is, have a separate struct for zPCI but *embed it* inside the existing one instead of allocating it separately, the same way we embed virPCIDeviceAddress itself virDomainDeviceInfo, for example. This second option looks better to me. This got me thinking that the extension flags *also* belongs to virPCIDeviceAddress, since we need them to know whether the zPCI part should be taken into account when formatting and such: you used to check whether the zpci pointer was NULL, but of course you can no longer do that once the pointer is gone; moreover, checking for a flag is more explicit so that's also an improvement. Using the flags stored into virDomainDeviceInfo would be ugly because it would make it so virPCIDeviceAddress is no longer a stand-alone object, so we should avoid that. I'm not sure you can get away with not storing the extension flags in virDomainDeviceInfo, though, because IIRC you might need to use them *before* you have figured out that you're going to assign a PCI address to the device. We might just have to store them twice, which is not great but I think preferable to introducing a reverse dependency from virPCIDeviceAddress to virDomainDeviceInfo. But I haven't really looked too closely into it, so perhaps there's a way to avoid that duplication after all :)