Re: [PATCH v2 20/29] domain_conf: format pnv-phb3-root-port empty addr
On 2/21/22 10:36, Ján Tomko wrote: On a Tuesday in 2022, Daniel Henrique Barboza wrote: Hiding the empty (:00:0.0) PCI address in the case of devices that will connect to slot 0x0 can be counterintuitive to the user, which will see something like this: Even if the user deliberately adds the root-port element: We end up removing the element after saving the domain file. This happens because virPCIDeviceAddressIsEmpty() can't distinguish between a zeroed address that was set by the user versus an address that wasn't filled at all. I'm afraid this is incomplete. virPCIDeviceAddressIsEmpty is for example used to figure out whether the device needs an address assigned too. So we'll need an extra field to distinguish between empty and zero addresses, which will probably need adjustment in a lot of callers :( Hmmm I don't think this is a hill I want to die on in this series. This patch is already adding a "VIR_DOMAIN_DEF_FORMAT_EMPTY_PCI_ADDR" flag to force the address formatting regardless of virPCIDeviceAddressIsEmpty(). I believe that I can rename it to "VIR_DOMAIN_DEF_FORMAT_PCI_ADDR", making it clear that the idea is to force the formatting at all times for the device, regardless of any assumptions about the address being empty, or not assigned, or any other cases where virPCIDeviceAddressIsEmpty() might apply. Thanks, Daniel Jano Given that all root-ports of PowerNV domains will connect to slot 0 of the PHB, if the PHB controller has index = 0 this scenario will occur every time. This patch aims to alleaviate this behavior by adding a new virDomainDefFormatFlags that will allow an empty address to be formatted in the XML. This flag is then used only when formatting PowerNV root ports. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 25 - src/conf/domain_conf.h | 2 ++ 2 files changed, 26 insertions(+), 1 deletion(-)
Re: [PATCH v2 20/29] domain_conf: format pnv-phb3-root-port empty addr
On a Tuesday in 2022, Daniel Henrique Barboza wrote: Hiding the empty (:00:0.0) PCI address in the case of devices that will connect to slot 0x0 can be counterintuitive to the user, which will see something like this: Even if the user deliberately adds the root-port element: We end up removing the element after saving the domain file. This happens because virPCIDeviceAddressIsEmpty() can't distinguish between a zeroed address that was set by the user versus an address that wasn't filled at all. I'm afraid this is incomplete. virPCIDeviceAddressIsEmpty is for example used to figure out whether the device needs an address assigned too. So we'll need an extra field to distinguish between empty and zero addresses, which will probably need adjustment in a lot of callers :( Jano Given that all root-ports of PowerNV domains will connect to slot 0 of the PHB, if the PHB controller has index = 0 this scenario will occur every time. This patch aims to alleaviate this behavior by adding a new virDomainDefFormatFlags that will allow an empty address to be formatted in the XML. This flag is then used only when formatting PowerNV root ports. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 25 - src/conf/domain_conf.h | 2 ++ 2 files changed, 26 insertions(+), 1 deletion(-) signature.asc Description: PGP signature
[PATCH v2 20/29] domain_conf: format pnv-phb3-root-port empty addr
Hiding the empty (:00:0.0) PCI address in the case of devices that
will connect to slot 0x0 can be counterintuitive to the user, which will
see something like this:
Even if the user deliberately adds the root-port element:
We end up removing the element after saving the domain file.
This happens because virPCIDeviceAddressIsEmpty() can't distinguish
between a zeroed address that was set by the user versus an address that
wasn't filled at all.
Given that all root-ports of PowerNV domains will connect to slot 0 of
the PHB, if the PHB controller has index = 0 this scenario will occur
every time. This patch aims to alleaviate this behavior by adding a new
virDomainDefFormatFlags that will allow an empty address to be formatted
in the XML. This flag is then used only when formatting PowerNV root
ports.
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 25 -
src/conf/domain_conf.h | 2 ++
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 13d5eb5b9d..026d801682 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2463,6 +2463,25 @@ virDomainControllerIsPowerNVPHB(const
virDomainControllerDef *cont)
}
+static bool
+virDomainControllerIsPowerNVRootPort(const virDomainControllerDef *cont)
+{
+virDomainControllerPCIModelName name;
+
+if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI ||
+cont->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT) {
+return false;
+}
+
+name = cont->opts.pciopts.modelName;
+
+if (name != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PNV_PHB3_ROOT_PORT)
+return false;
+
+return true;
+}
+
+
virDomainFSDef *
virDomainFSDefNew(virDomainXMLOption *xmlopt)
{
@@ -6490,7 +6509,8 @@ virDomainDeviceInfoFormat(virBuffer *buf,
switch ((virDomainDeviceAddressType) info->type) {
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
-if (!virPCIDeviceAddressIsEmpty(>addr.pci)) {
+if (!virPCIDeviceAddressIsEmpty(>addr.pci) ||
+flags & VIR_DOMAIN_DEF_FORMAT_EMPTY_PCI_ADDR) {
virBufferAsprintf(, " domain='0x%04x' bus='0x%02x' "
"slot='0x%02x' function='0x%d'",
info->addr.pci.domain,
@@ -23958,6 +23978,9 @@ virDomainControllerDefFormat(virBuffer *buf,
}
}
+if (virDomainControllerIsPowerNVRootPort(def))
+flags |= VIR_DOMAIN_DEF_FORMAT_EMPTY_PCI_ADDR;
+
virDomainControllerDriverFormat(, def);
virDomainDeviceInfoFormat(, >info, flags);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index adeafa83e7..dd3d942a35 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3502,6 +3502,8 @@ typedef enum {
VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM = 1 << 6,
VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT = 1 << 7,
VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST= 1 << 8,
+/* format empty PCI addr (:00:0.0) */
+VIR_DOMAIN_DEF_FORMAT_EMPTY_PCI_ADDR = 1 << 9,
} virDomainDefFormatFlags;
/* Use these flags to skip specific domain ABI consistency checks done
--
2.34.1
