Re: [libvirt] [PATCH v2 RESEND 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/7/30 下午7:08, Andrea Bolognani 写道: On Fri, 2018-07-27 at 13:22 +0800, Yi Min Zhao wrote: 在 2018/7/24 下午10:58, Andrea Bolognani 写道: @@ -1385,7 +1403,12 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, * parent, and will have its address collected during the scan * of the parent's device type. */ -return 0; +if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || +info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) +return virDomainPCIAddressExtensionReserveAddr(addrs, addr, + info->pciAddressExtFlags); +else +return 0; This doesn't look right: the comment specifically states that the PCI address will be handled by the parent device in this case, why wouldn't the zPCI address not be handled in the same way? For this comment, we have to collect zPCI address in case that zPCI address is specified but PCI address is not. I think I shall split two checks. Original code is: if (!virDeviceInfoPCIAddressPresent(info) || ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) && (device->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE))) Separate them and only add the new code for !virDeviceInfoPCIAddressPresent(info) case. I didn't look too closely, but I think you might have to change virDeviceInfoPCIAddressPresent() itself so that it knows about PCI address extensions and behaves accordingly. Basically, with the introduction of PCI address extensions, you're making questions such as "does this device have a PCI address assigned to it?" a lot less trivial to answer, and you need to ensure this doesn't cause existing assumption to no longer hold. I have a new idea. qemuDomainCollectPCIAddress() is called in one place. So we could add a new function qemuDomainCollectPCIExtensionAddress(), and call it after qemuDomainCollectPCIAddress() is called. Then we could collect pci address and extension address separately. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 RESEND 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
On Fri, 2018-07-27 at 13:22 +0800, Yi Min Zhao wrote: > 在 2018/7/24 下午10:58, Andrea Bolognani 写道: > > > @@ -1385,7 +1403,12 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def > > > ATTRIBUTE_UNUSED, > > >* parent, and will have its address collected during the scan > > >* of the parent's device type. > > > */ > > > -return 0; > > > +if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || > > > +info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) > > > +return virDomainPCIAddressExtensionReserveAddr(addrs, addr, > > > + > > > info->pciAddressExtFlags); > > > +else > > > +return 0; > > > > This doesn't look right: the comment specifically states that the > > PCI address will be handled by the parent device in this case, > > why wouldn't the zPCI address not be handled in the same way? > > For this comment, we have to collect zPCI address in case that zPCI > address is specified > but PCI address is not. I think I shall split two checks. Original code is: > if (!virDeviceInfoPCIAddressPresent(info) || > ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) && >(device->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE))) > Separate them and only add the new code for > !virDeviceInfoPCIAddressPresent(info) case. I didn't look too closely, but I think you might have to change virDeviceInfoPCIAddressPresent() itself so that it knows about PCI address extensions and behaves accordingly. Basically, with the introduction of PCI address extensions, you're making questions such as "does this device have a PCI address assigned to it?" a lot less trivial to answer, and you need to ensure this doesn't cause existing assumption to no longer hold. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 RESEND 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/7/24 下午10:58, Andrea Bolognani 写道: @@ -1385,7 +1403,12 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, * parent, and will have its address collected during the scan * of the parent's device type. */ -return 0; +if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || +info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) +return virDomainPCIAddressExtensionReserveAddr(addrs, addr, + info->pciAddressExtFlags); +else +return 0; This doesn't look right: the comment specifically states that the PCI address will be handled by the parent device in this case, why wouldn't the zPCI address not be handled in the same way? Thanks for other comments! I cutted off them. For this comment, we have to collect zPCI address in case that zPCI address is specified but PCI address is not. I think I shall split two checks. Original code is: if (!virDeviceInfoPCIAddressPresent(info) || ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) && (device->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE))) Separate them and only add the new code for !virDeviceInfoPCIAddressPresent(info) case. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 RESEND 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
On Tue, 2018-07-10 at 16:02 +0800, Yi Min Zhao wrote:
> This patch adds new functions for reservation, assignment and release
> to handle the uid/fid. If the uid/fid is defined in the domain XML,
> they will be reserved directly in collecting phase. If any of them is
> not defined, we will find out an available value for it from zPCI
> address hashtable, and reserve it. For hotplug case, there might be or
> not zPCI definition. So allocate and reserve uid/fid for undefined
> case. Assign if needed and reserve uid/fid for defined case. If the user
> define zPCI extension address but zPCI capability doesn't exist, an
> error will be reported.
For this patch I once again didn't look too closely to the
implementation, sorry.
[...]
> +static int
> +virDomainZPCIAddressReserveId(virHashTablePtr set, unsigned int id,
> + const char *name)
One argument per line, please.
There are more instances in the patch, but I'm not going to
point them all out :)
[...]
> +static int
> +virDomainZPCIAddressAssignUid(virHashTablePtr set, virZPCIDeviceAddressPtr
> addr)
> +{
> +if (addr->uid_assigned)
> +return 0;
> +
> +addr->uid_assigned = virDomainZPCIAddressAssignId(set, >zpci_uid,
> 1,
> + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID,
> "uid");
Messed up alignment. More instances further down.
[...]
> +static void
> +virDomainZPCIAddressReleaseUid(virHashTablePtr set, virZPCIDeviceAddressPtr
> addr)
> +{
> +if (!addr->uid_assigned)
> +return;
> +
> +if (virHashRemoveEntry(set, >zpci_uid))
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Release uid %u failed"), addr->zpci_uid);
Curly braces are required here. More instances further down.
Looking at
> +static void
> +virDomainZPCIAddressReleaseFid(virHashTablePtr set, virZPCIDeviceAddressPtr
> addr)
and
> +static void
> +virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
> + virPCIDeviceAddressPtr addr)
and
> +static int
> +virDomainZPCIAddressReserveAddr(virDomainPCIAddressSetPtr addrs,
> +virPCIDeviceAddressPtr addr)
you're being awfully inconsistent about the datatypes you're passing
around...
Unless I've missed something that makes doing so impossible, please
try to make it so only the top-level datatypes (DomainPCIAddressSet
and PCIDeviceAddress) are passed around.
[...]
> +static int
> +virDomainZPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> +virZPCIDeviceAddressPtr zpci)
> +{
> +if (!zpci->uid_assigned &&
> +virDomainZPCIAddressReserveNextUid(addrs->zpciIds->uids, zpci))
> +return -1;
Messed up indentation. Also, missing curly braces.
[...]
> +int
> +virDomainPCIAddressExtensionReserveAddr(virDomainPCIAddressSetPtr addrs,
> +virPCIDeviceAddressPtr addr,
> +virDomainPCIAddressExtensionFlags
> extFlags)
> +{
> +if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> +/* Reserve uid/fid to ZPCI device which has defined uid/fid
> + * in the domain.
> + */
Messed up indentation.
[...]
> +int
> +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> +virPCIDeviceAddressPtr dev,
> +
> virDomainPCIAddressExtensionFlags extFlags)
> +{
> +if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> +/* Assign and reserve uid/fid to ZPCI device which has not defined
> uid/fid
> + * in the domain.
> + */
Messed up indentation.
[...]
> +static int
> +virDomainPCIAddressEnsureExtensionAddr(virDomainPCIAddressSetPtr addrs,
> + virDomainDeviceInfoPtr dev)
This should be virDomainPCIAddressExtensionEnsureAddr() for
consistency's sake.
> @@ -1385,7 +1403,12 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def
> ATTRIBUTE_UNUSED,
> * parent, and will have its address collected during the scan
> * of the parent's device type.
> */
> -return 0;
> +if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI ||
> +info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> +return virDomainPCIAddressExtensionReserveAddr(addrs, addr,
> +
> info->pciAddressExtFlags);
> +else
> +return 0;
This doesn't look right: the comment specifically states that the
PCI address will be handled by the parent device in this case,
why wouldn't the zPCI address not be handled in the same way?
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
