Re: [libvirt] [PATCH v6 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/10/19 下午3:54, Andrea Bolognani 写道: On Fri, 2018-10-19 at 11:29 +0800, Yi Min Zhao wrote: 在 2018/10/18 下午11:44, Andrea Bolognani 写道: There's still the question of whether Dan is now okay with the XML structure as currently implemented; assuming that's the case, it would be great if Laine could also take a quick look at the series before it's pushed. Do you mean expect Laine to take a quick look at this series, or the next one? The next one :) I have posted v7 patches. Thanks! -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
On Fri, 2018-10-19 at 11:29 +0800, Yi Min Zhao wrote: > 在 2018/10/18 下午11:44, Andrea Bolognani 写道: > > There's still the question of whether Dan is now okay with the XML > > structure as currently implemented; assuming that's the case, it > > would be great if Laine could also take a quick look at the series > > before it's pushed. > > Do you mean expect Laine to take a quick look at this series, or the > next one? The next one :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/10/18 下午11:44, Andrea Bolognani 写道: On Thu, 2018-10-18 at 15:13 +0800, Yi Min Zhao wrote: 在 2018/10/17 下午10:30, Andrea Bolognani 写道: On Tue, 2018-10-16 at 11:28 +0800, Yi Min Zhao wrote: I think this would make things complex. If either PCI address or zPCI address exists, we have to do more checks for calling virDomainPCIAddressReserveAddr(). And there are amounts of code calling ***IsWanted() to call ***ReserveNext***(). I think keeping them separately is better. Again, I might be missing something because I haven't actually tried implementing any of this, but at least from the theoretical point of view I don't see how keeping them separate would make things simpler: if anything, it seems to me like it would make them more complicated for the calling code because now you have to worry about the PCI address extensions *in addition* to the PCI address itself. For example, during collection stage, checking both PCI address and extension address is requried, and still need to separately do some additional checks for PCI address if it is present, at last in reserving addr function we still check if PCI normal address or extension address exists to separately reserve present one. So that we have to do the check on the same condition repetively. If you don't have strong opposition, I want to send the new version ASAP. So I gave an half-hearted stab at implementing my own suggestion today, and quite unsurprisingly I have gained more sympathy for your argument in the process :) The main problem I see is that, as you noticed, we have a lot of calls to IsWanted(), IsPresent(), ReserveAddr() and ReserveNextAddr() where really we should be using EnsureAddr() pretty much all of the time and hide most of the details from the drivers, which in turn would make it easier to change them in a transparent manner. Another big problem, which I highlighted in the past, is that the current API was not designed with the idea that PCI addresses could have "parts" in mind, and so it's not nuanced enough: if I call IsEmpty() on and address where the PCI part itself has been filled in but the zPCI part hasn't, or vice versa, what should I get back? The answer is probably that, after we've made sure those functions are used as little as possible thanks to the changes outlined above, we should replace them with better named alternatives. Of course it would be entirely unfair to ask you to perform such a massive refactoring before your series can be considered for inclusion, so at this point I'm okay with merging it and cleaning up the pre-existing mess afterwards. There's still the question of whether Dan is now okay with the XML structure as currently implemented; assuming that's the case, it would be great if Laine could also take a quick look at the series before it's pushed. Do you mean expect Laine to take a quick look at this series, or the next one? -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
On 10/18/2018 05:44 PM, Andrea Bolognani wrote: > On Thu, 2018-10-18 at 15:13 +0800, Yi Min Zhao wrote: >> 在 2018/10/17 下午10:30, Andrea Bolognani 写道: >>> On Tue, 2018-10-16 at 11:28 +0800, Yi Min Zhao wrote: I think this would make things complex. If either PCI address or zPCI address exists, we have to do more checks for calling virDomainPCIAddressReserveAddr(). And there are amounts of code calling ***IsWanted() to call ***ReserveNext***(). I think keeping them separately is better. >>> >>> Again, I might be missing something because I haven't actually tried >>> implementing any of this, but at least from the theoretical point of >>> view I don't see how keeping them separate would make things simpler: >>> if anything, it seems to me like it would make them more complicated >>> for the calling code because now you have to worry about the PCI >>> address extensions *in addition* to the PCI address itself. >> >> For example, during collection stage, checking both PCI address and >> extension address >> is requried, and still need to separately do some additional checks for >> PCI address if it >> is present, at last in reserving addr function we still check if PCI >> normal address or >> extension address exists to separately reserve present one. So that we >> have to do the >> check on the same condition repetively. If you don't have strong >> opposition, I want to >> send the new version ASAP. > > So I gave an half-hearted stab at implementing my own suggestion > today, and quite unsurprisingly I have gained more sympathy for your > argument in the process :) > > The main problem I see is that, as you noticed, we have a lot of > calls to IsWanted(), IsPresent(), ReserveAddr() and ReserveNextAddr() > where really we should be using EnsureAddr() pretty much all of the > time and hide most of the details from the drivers, which in turn > would make it easier to change them in a transparent manner. > > Another big problem, which I highlighted in the past, is that the > current API was not designed with the idea that PCI addresses could > have "parts" in mind, and so it's not nuanced enough: if I call > IsEmpty() on and address where the PCI part itself has been filled > in but the zPCI part hasn't, or vice versa, what should I get back? > The answer is probably that, after we've made sure those functions > are used as little as possible thanks to the changes outlined above, > we should replace them with better named alternatives. > > Of course it would be entirely unfair to ask you to perform such > a massive refactoring before your series can be considered for > inclusion, so at this point I'm okay with merging it and cleaning > up the pre-existing mess afterwards. > > There's still the question of whether Dan is now okay with the XML > structure as currently implemented; assuming that's the case, it > would be great if Laine could also take a quick look at the series > before it's pushed. As I said before, I think the current XML is the right variant. This is exactly how QEMU implements it (have a real classic pci bus naming scheme augmented with some additional data named uid/fid). So having an zcpi name space (instead of a pci one) would be wrong. Daniel, having said this, are you ok with the current variant? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
On Thu, 2018-10-18 at 15:13 +0800, Yi Min Zhao wrote: > 在 2018/10/17 下午10:30, Andrea Bolognani 写道: > > On Tue, 2018-10-16 at 11:28 +0800, Yi Min Zhao wrote: > > > I think this would make things complex. If either PCI address or > > > zPCI address exists, we have to do more checks for calling > > > virDomainPCIAddressReserveAddr(). And there are amounts of > > > code calling ***IsWanted() to call ***ReserveNext***(). I think > > > keeping them separately is better. > > > > Again, I might be missing something because I haven't actually tried > > implementing any of this, but at least from the theoretical point of > > view I don't see how keeping them separate would make things simpler: > > if anything, it seems to me like it would make them more complicated > > for the calling code because now you have to worry about the PCI > > address extensions *in addition* to the PCI address itself. > > For example, during collection stage, checking both PCI address and > extension address > is requried, and still need to separately do some additional checks for > PCI address if it > is present, at last in reserving addr function we still check if PCI > normal address or > extension address exists to separately reserve present one. So that we > have to do the > check on the same condition repetively. If you don't have strong > opposition, I want to > send the new version ASAP. So I gave an half-hearted stab at implementing my own suggestion today, and quite unsurprisingly I have gained more sympathy for your argument in the process :) The main problem I see is that, as you noticed, we have a lot of calls to IsWanted(), IsPresent(), ReserveAddr() and ReserveNextAddr() where really we should be using EnsureAddr() pretty much all of the time and hide most of the details from the drivers, which in turn would make it easier to change them in a transparent manner. Another big problem, which I highlighted in the past, is that the current API was not designed with the idea that PCI addresses could have "parts" in mind, and so it's not nuanced enough: if I call IsEmpty() on and address where the PCI part itself has been filled in but the zPCI part hasn't, or vice versa, what should I get back? The answer is probably that, after we've made sure those functions are used as little as possible thanks to the changes outlined above, we should replace them with better named alternatives. Of course it would be entirely unfair to ask you to perform such a massive refactoring before your series can be considered for inclusion, so at this point I'm okay with merging it and cleaning up the pre-existing mess afterwards. There's still the question of whether Dan is now okay with the XML structure as currently implemented; assuming that's the case, it would be great if Laine could also take a quick look at the series before it's pushed. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/10/17 下午10:30, Andrea Bolognani 写道: On Tue, 2018-10-16 at 11:28 +0800, Yi Min Zhao wrote: 在 2018/10/11 下午10:50, Andrea Bolognani 写道: On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: # conf/device_conf.h +virDeviceInfoPCIAddressExtensionIsPresent; +virDeviceInfoPCIAddressExtensionIsWanted; virDeviceInfoPCIAddressIsPresent; virDeviceInfoPCIAddressIsWanted; virDomainDeviceAddressIsValid; @@ -119,6 +121,9 @@ virDomainCCWAddressSetFree; virDomainPCIAddressBusIsFullyReserved; virDomainPCIAddressBusSetModel; virDomainPCIAddressEnsureAddr; +virDomainPCIAddressExtensionReleaseAddr; +virDomainPCIAddressExtensionReserveAddr; +virDomainPCIAddressExtensionReserveNextAddr; I'm not quite quire we need to export these functions. With the recent changes, we've gotten to the point that we're not even passing a virZPCIDeviceAddress to them, but rather they have the very same signature as the regular virPCIDeviceAddress... So it should be possible to just make them static and only call them from the virDomainPCIAddressReserveAddr() and friends, right? Which is where I was hoping we could eventually get. Or did I miss something? I think this would make things complex. If either PCI address or zPCI address exists, we have to do more checks for calling virDomainPCIAddressReserveAddr(). And there are amounts of code calling ***IsWanted() to call ***ReserveNext***(). I think keeping them separately is better. Again, I might be missing something because I haven't actually tried implementing any of this, but at least from the theoretical point of view I don't see how keeping them separate would make things simpler: if anything, it seems to me like it would make them more complicated for the calling code because now you have to worry about the PCI address extensions *in addition* to the PCI address itself. For example, during collection stage, checking both PCI address and extension address is requried, and still need to separately do some additional checks for PCI address if it is present, at last in reserving addr function we still check if PCI normal address or extension address exists to separately reserve present one. So that we have to do the check on the same condition repetively. If you don't have strong opposition, I want to send the new version ASAP. -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
On Tue, 2018-10-16 at 11:28 +0800, Yi Min Zhao wrote: > 在 2018/10/11 下午10:50, Andrea Bolognani 写道: > > On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: > > > # conf/device_conf.h > > > +virDeviceInfoPCIAddressExtensionIsPresent; > > > +virDeviceInfoPCIAddressExtensionIsWanted; > > > virDeviceInfoPCIAddressIsPresent; > > > virDeviceInfoPCIAddressIsWanted; > > > virDomainDeviceAddressIsValid; > > > @@ -119,6 +121,9 @@ virDomainCCWAddressSetFree; > > > virDomainPCIAddressBusIsFullyReserved; > > > virDomainPCIAddressBusSetModel; > > > virDomainPCIAddressEnsureAddr; > > > +virDomainPCIAddressExtensionReleaseAddr; > > > +virDomainPCIAddressExtensionReserveAddr; > > > +virDomainPCIAddressExtensionReserveNextAddr; > > > > I'm not quite quire we need to export these functions. > > > > With the recent changes, we've gotten to the point that we're not > > even passing a virZPCIDeviceAddress to them, but rather they have > > the very same signature as the regular virPCIDeviceAddress... > > > > So it should be possible to just make them static and only call > > them from the virDomainPCIAddressReserveAddr() and friends, right? > > Which is where I was hoping we could eventually get. Or did I > > miss something? > > I think this would make things complex. If either PCI address or > zPCI address exists, we have to do more checks for calling > virDomainPCIAddressReserveAddr(). And there are amounts of > code calling ***IsWanted() to call ***ReserveNext***(). I think > keeping them separately is better. Again, I might be missing something because I haven't actually tried implementing any of this, but at least from the theoretical point of view I don't see how keeping them separate would make things simpler: if anything, it seems to me like it would make them more complicated for the calling code because now you have to worry about the PCI address extensions *in addition* to the PCI address itself. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
在 2018/10/11 下午10:50, Andrea Bolognani 写道: On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: # conf/device_conf.h +virDeviceInfoPCIAddressExtensionIsPresent; +virDeviceInfoPCIAddressExtensionIsWanted; virDeviceInfoPCIAddressIsPresent; virDeviceInfoPCIAddressIsWanted; virDomainDeviceAddressIsValid; @@ -119,6 +121,9 @@ virDomainCCWAddressSetFree; virDomainPCIAddressBusIsFullyReserved; virDomainPCIAddressBusSetModel; virDomainPCIAddressEnsureAddr; +virDomainPCIAddressExtensionReleaseAddr; +virDomainPCIAddressExtensionReserveAddr; +virDomainPCIAddressExtensionReserveNextAddr; I'm not quite quire we need to export these functions. With the recent changes, we've gotten to the point that we're not even passing a virZPCIDeviceAddress to them, but rather they have the very same signature as the regular virPCIDeviceAddress... So it should be possible to just make them static and only call them from the virDomainPCIAddressReserveAddr() and friends, right? Which is where I was hoping we could eventually get. Or did I miss something? I think this would make things complex. If either PCI address or zPCI address exists, we have to do more checks for calling virDomainPCIAddressReserveAddr(). And there are amounts of code calling ***IsWanted() to call ***ReserveNext***(). I think keeping them separately is better. Everything else looks good. -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote: > # conf/device_conf.h > +virDeviceInfoPCIAddressExtensionIsPresent; > +virDeviceInfoPCIAddressExtensionIsWanted; > virDeviceInfoPCIAddressIsPresent; > virDeviceInfoPCIAddressIsWanted; > virDomainDeviceAddressIsValid; > @@ -119,6 +121,9 @@ virDomainCCWAddressSetFree; > virDomainPCIAddressBusIsFullyReserved; > virDomainPCIAddressBusSetModel; > virDomainPCIAddressEnsureAddr; > +virDomainPCIAddressExtensionReleaseAddr; > +virDomainPCIAddressExtensionReserveAddr; > +virDomainPCIAddressExtensionReserveNextAddr; I'm not quite quire we need to export these functions. With the recent changes, we've gotten to the point that we're not even passing a virZPCIDeviceAddress to them, but rather they have the very same signature as the regular virPCIDeviceAddress... So it should be possible to just make them static and only call them from the virDomainPCIAddressReserveAddr() and friends, right? Which is where I was hoping we could eventually get. Or did I miss something? Everything else looks good. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v6 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address
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 zPCI address is
empty, 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. Whether using zPCI
address refers to extension flags in PCIDeviceAddress.
Signed-off-by: Yi Min Zhao
---
src/conf/device_conf.c | 16 +++
src/conf/device_conf.h | 3 +
src/conf/domain_addr.c | 244 +
src/conf/domain_addr.h | 12 ++
src/libvirt_private.syms | 5 +
src/qemu/qemu_domain_address.c | 59 +++-
6 files changed, 338 insertions(+), 1 deletion(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index d9b2c8f477..042c6f097b 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -28,6 +28,7 @@
#include "viruuid.h"
#include "virbuffer.h"
#include "device_conf.h"
+#include "domain_addr.h"
#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_DEVICE
@@ -235,6 +236,21 @@ virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo
*info)
}
+bool
+virDeviceInfoPCIAddressExtensionIsWanted(const virDomainDeviceInfo *info)
+{
+return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
+ virZPCIDeviceAddressIsEmpty(>addr.pci.zpci);
+}
+
+bool
+virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info)
+{
+return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
+ !virZPCIDeviceAddressIsEmpty(>addr.pci.zpci);
+}
+
+
int
virPCIDeviceAddressParseXML(xmlNodePtr node,
virPCIDeviceAddressPtr addr)
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index 1ee6905024..c93615567a 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -198,6 +198,9 @@ bool virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr
info,
bool virDeviceInfoPCIAddressIsWanted(const virDomainDeviceInfo *info);
bool virDeviceInfoPCIAddressIsPresent(const virDomainDeviceInfo *info);
+bool virDeviceInfoPCIAddressExtensionIsWanted(const virDomainDeviceInfo *info);
+bool virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo
*info);
+
int virPCIDeviceAddressParseXML(xmlNodePtr node,
virPCIDeviceAddressPtr addr);
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 7e5ce48b3f..8e033b23d0 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -33,6 +33,157 @@
VIR_LOG_INIT("conf.domain_addr");
+static int
+virDomainZPCIAddressReserveId(virHashTablePtr set,
+ unsigned int id,
+ const char *name)
+{
+if (virHashLookup(set, )) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("zPCI %s %o is already reserved"),
+ name, id);
+return -1;
+}
+
+if (virHashAddEntry(set, , (void*)1) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to reserve %s %o"),
+ name, id);
+return -1;
+}
+
+return 0;
+}
+
+
+static int
+virDomainZPCIAddressReserveUid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+return virDomainZPCIAddressReserveId(set, addr->uid, "uid");
+}
+
+
+static int
+virDomainZPCIAddressReserveFid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+return virDomainZPCIAddressReserveId(set, addr->fid, "fid");
+}
+
+
+static int
+virDomainZPCIAddressAssignId(virHashTablePtr set,
+ unsigned int *id,
+ unsigned int min,
+ unsigned int max,
+ const char *name)
+{
+while (virHashLookup(set, )) {
+if (min == max) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("There is no more free %s."),
+ name);
+return -1;
+}
+++min;
+}
+*id = min;
+
+return 0;
+}
+
+
+static int
+virDomainZPCIAddressAssignUid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+return virDomainZPCIAddressAssignId(set, >uid, 1,
+VIR_DOMAIN_DEVICE_ZPCI_MAX_UID, "uid");
+}
+
+
+static int
+virDomainZPCIAddressAssignFid(virHashTablePtr set,
+ virZPCIDeviceAddressPtr addr)
+{
+return virDomainZPCIAddressAssignId(set, >fid, 0,
+VIR_DOMAIN_DEVICE_ZPCI_MAX_FID, "fid");
+}
+
+
+static void
+virDomainZPCIAddressReleaseId(virHashTablePtr set,
+
