Hiding the empty (0000: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:

    <controller type='pci' index='0' model='pcie-root'/>
    <controller type='pci' index='1' model='pcie-root-port'>
      <model name='pnv-phb3-root-port'/>
      <target chassis='1' port='0x8'/>
    </controller>

Even if the user deliberately adds the root-port <address> element:

    <address type='pci' domain='0x0000' bus='0x00' slot='0x00' function='0x0'/>

We end up removing the <address> 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 <danielhb413@gmail.com>
---
 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(&info->addr.pci)) {
+        if (!virPCIDeviceAddressIsEmpty(&info->addr.pci) ||
+            flags & VIR_DOMAIN_DEF_FORMAT_EMPTY_PCI_ADDR) {
             virBufferAsprintf(&attrBuf, " 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(&childBuf, def);
 
     virDomainDeviceInfoFormat(&childBuf, &def->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 (0000: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

On a Tuesday in 2022, Daniel Henrique Barboza wrote:
>Hiding the empty (0000: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:
>
>    <controller type='pci' index='0' model='pcie-root'/>
>    <controller type='pci' index='1' model='pcie-root-port'>
>      <model name='pnv-phb3-root-port'/>
>      <target chassis='1' port='0x8'/>
>    </controller>
>
>Even if the user deliberately adds the root-port <address> element:
>
>    <address type='pci' domain='0x0000' bus='0x00' slot='0x00' function='0x0'/>
>
>We end up removing the <address> 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 <danielhb413@gmail.com>
>---
> src/conf/domain_conf.c | 25 ++++++++++++++++++++++++-
> src/conf/domain_conf.h |  2 ++
> 2 files changed, 26 insertions(+), 1 deletion(-)
>

On 2/21/22 10:36, Ján Tomko wrote:
> On a Tuesday in 2022, Daniel Henrique Barboza wrote:
>> Hiding the empty (0000: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:
>>
>>    <controller type='pci' index='0' model='pcie-root'/>
>>    <controller type='pci' index='1' model='pcie-root-port'>
>>      <model name='pnv-phb3-root-port'/>
>>      <target chassis='1' port='0x8'/>
>>    </controller>
>>
>> Even if the user deliberately adds the root-port <address> element:
>>
>>    <address type='pci' domain='0x0000' bus='0x00' slot='0x00' function='0x0'/>
>>
>> We end up removing the <address> 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 <addr> 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 <danielhb413@gmail.com>
>> ---
>> src/conf/domain_conf.c | 25 ++++++++++++++++++++++++-
>> src/conf/domain_conf.h |  2 ++
>> 2 files changed, 26 insertions(+), 1 deletion(-)
>>