On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
> +++ b/src/conf/domain_addr.c
> @@ -145,12 +145,18 @@ static void
> virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
> virZPCIDeviceAddressPtr addr)
> {
> - if (!zpciIds || virZPCIDeviceAddressIsEmpty(addr))
> + if (!zpciIds)
> return;
>
> - virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> + if (addr->uid_set) {
> + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> + addr->uid_set = false;
> + }
I think it would be cleaner to handle the boolean flags in the same
spot the values are taken care of, that is, in the Release{U,F}id()
functions.
> @@ -186,12 +192,16 @@ static int
> virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
> virZPCIDeviceAddressPtr addr)
> {
> - if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
> + if (addr->uid_set &&
> + virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
> return -1;
Same here...
>
> - if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
> - virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> - return -1;
> + if (addr->fid_set) {
> + if (virDomainZPCIAddressReserveFid(zpciIds->fids, addr) < 0) {
> + if (addr->uid_set)
> + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> + return -1;
> + }
> }
>
> return 0;
> @@ -202,14 +212,28 @@ static int
> virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
> virZPCIDeviceAddressPtr addr)
> {
> - if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
> - return -1;
> + bool uid_set, fid_set = false;
>
> - if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
> - virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> - return -1;
> + if (!addr->uid_set) {
> + if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
> + return -1;
> + uid_set = true;
> + }
... and here. Basicall, push all handling of boolean flags one layer
down, where the actual values are taken care of.
> @@ -234,7 +258,7 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> virPCIDeviceAddressPtr addr)
> {
> if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> - virZPCIDeviceAddress zpci = { 0 };
> + virZPCIDeviceAddress zpci = addr->zpci;
>
> if (virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, &zpci) < 0)
> return -1;
> @@ -246,6 +270,7 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> return 0;
> }
>
> +
> static int
> virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
> virPCIDeviceAddressPtr addr)
> @@ -253,10 +278,10 @@ virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
> if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> virZPCIDeviceAddressPtr zpci = &addr->zpci;
>
> - if (virZPCIDeviceAddressIsEmpty(zpci))
> - return virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
> - else
> - return virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci);
> + if (virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci) < 0)
> + return -1;
> +
> + return virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, zpci);
> }
I think the semantics for these functions need to be reconsidered.
The way they currently work, as evidenced by EnsureAddr(), is that
you either have a fully-specified address provided by the user, in
which case you want to reserve that address, or you have an empty
address because the user didn't provide ZPCI information, in which
case you allocate a new full address based on what uids and fids are
still available and use that one.
With your changes, we can now find ourselves in a hybrid situation,
where half of the ZPCI address has been provided by the user but the
remaining half is up to us... Ultimately, it might make sense to have
ReserveAddr(), ReserveNextAddr() and EnsureAddr() all call to the
same function which does something along the lines of
if (!zpci->uid_set)
AssignUid(zpci);
if (!zpci->fid_set)
AssignFid(zpci);
ReserveUid(zpci);
ReserveFid(zpci);
because that's ultimately what we're achieving anyway, only with
more obfuscation.
> +++ b/src/conf/domain_conf.c
> @@ -7471,7 +7471,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
> virTristateSwitchTypeToString(info->addr.pci.multi));
> }
>
> - if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
> + if (!virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
> virBufferAsprintf(&childBuf,
> "<zpci uid='0x%.4x' fid='0x%.8x'/>\n",
> info->addr.pci.zpci.uid,
By the time we get here, we should either have a complete ZPCI
address or no ZPCI address at all. So I would rewrite this as
if (IsIncomplete(zpci))
virReportError(VIR_ERR_INTERNAL, ...);
if (!IsPresent(zpci))
virBufferAsprintf(...);
with the first check being there just for extra safety (This Should
Never Happen™).
> +++ b/src/util/virpci.h
> @@ -43,6 +43,8 @@ typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
> struct _virZPCIDeviceAddress {
> unsigned int uid; /* exempt from syntax-check */
> unsigned int fid;
> + bool uid_set;
> + bool fid_set;
I believe we mostly use camelCase for struct members, although I
don't think we're too consistent with that :)
I wonder if it would make sense to tie the two bits of information
together more explicitly, eg.
typedef struct _virZPCIDeviceAddressID {
unsigned int value;
bool isSet;
} virZPCIDeviceAddressID;
typedef struct _virZPCIDeviceAddress {
virZPCIDeviceAddressID uid;
virZPCIDeviceAddressID fid;
} virZPCIDeviceAddress;
> +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml
> @@ -39,7 +39,7 @@
> <address domain='0x0003' bus='0x00' slot='0x00' function='0x0'/>
> </source>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'>
> - <zpci uid='0x0001' fid='0x00000001'/>
> + <zpci uid='0x0001' fid='0x00000000'/>
> </address>
> </hostdev>
> <hostdev mode='subsystem' type='pci' managed='no'>
> @@ -48,7 +48,7 @@
> <address domain='0x0004' bus='0x00' slot='0x00' function='0x0'/>
> </source>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'>
> - <zpci uid='0x0002' fid='0x00000002'/>
> + <zpci uid='0x0002' fid='0x00000001'/>
> </address>
> </hostdev>
> <hostdev mode='subsystem' type='pci' managed='no'>
> @@ -57,7 +57,7 @@
> <address domain='0x0005' bus='0x00' slot='0x00' function='0x0'/>
> </source>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'>
> - <zpci uid='0x0053' fid='0x00000000'/>
> + <zpci uid='0x0053' fid='0x00000002'/>
I'm not entirely clear on why these generated ZPCI addresses would
change. Can you explain that to me?
--
Andrea Bolognani / Red Hat / Virtualization
