[Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Jing Liu]

Add hint to firmware (e.g. SeaBIOS) to reserve addtional
IO/MEM/PREF spaces for legacy pci-pci bridge, to enable
some pci devices hotplugging whose IO/MEM/PREF spaces
requests are larger than the ones in pci-pci bridge set
by firmware.

Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
---
 hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index b2d861d..8e9afbd 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -46,6 +46,12 @@ struct PCIBridgeDev {
     uint32_t flags;
 
     OnOffAuto msi;
+
+    /* additional resources to reserve on firmware init */
+    uint64_t io_reserve;
+    uint64_t mem_reserve;
+    uint64_t pref32_reserve;
+    uint64_t pref64_reserve;
 };
 typedef struct PCIBridgeDev PCIBridgeDev;
 
@@ -95,6 +101,13 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
         error_free(local_err);
     }
 
+    err = pci_bridge_qemu_reserve_cap_init(dev, 0, 0,
+          bridge_dev->io_reserve, bridge_dev->mem_reserve,
+          bridge_dev->pref32_reserve, bridge_dev->pref64_reserve, errp);
+    if (err) {
+        goto cap_error;
+    }
+
     if (shpc_present(dev)) {
         /* TODO: spec recommends using 64 bit prefetcheable BAR.
          * Check whether that works well. */
@@ -103,6 +116,8 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
     }
     return;
 
+cap_error:
+    msi_uninit(dev);
 msi_error:
     slotid_cap_cleanup(dev);
 slotid_error:
@@ -162,6 +177,11 @@ static Property pci_bridge_dev_properties[] = {
                             ON_OFF_AUTO_AUTO),
     DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
                     PCI_BRIDGE_DEV_F_SHPC_REQ, true),
+    DEFINE_PROP_SIZE("io-reserve", PCIBridgeDev, io_reserve, -1),
+    DEFINE_PROP_SIZE("mem-reserve", PCIBridgeDev, mem_reserve, -1),
+    DEFINE_PROP_SIZE("pref32-reserve", PCIBridgeDev, pref32_reserve, -1),
+    DEFINE_PROP_SIZE("pref64-reserve", PCIBridgeDev, pref64_reserve, -1),
+
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Laszlo Ersek]

On 08/07/18 09:04, Jing Liu wrote:
> Add hint to firmware (e.g. SeaBIOS) to reserve addtional
> IO/MEM/PREF spaces for legacy pci-pci bridge, to enable
> some pci devices hotplugging whose IO/MEM/PREF spaces
> requests are larger than the ones in pci-pci bridge set
> by firmware.
> 
> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
> ---
>  hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index b2d861d..8e9afbd 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -46,6 +46,12 @@ struct PCIBridgeDev {
>      uint32_t flags;
>  
>      OnOffAuto msi;
> +
> +    /* additional resources to reserve on firmware init */
> +    uint64_t io_reserve;
> +    uint64_t mem_reserve;
> +    uint64_t pref32_reserve;
> +    uint64_t pref64_reserve;
>  };
>  typedef struct PCIBridgeDev PCIBridgeDev;

(1) Please evaluate the following idea:

- Factor the "bus_reserve", "io_reserve", "mem_reserve",
"pref32_reserve" and "pref64_reserve" fiels of the "GenPCIERootPort"
structure out to another structure.

- I think this new structure type should be defined in
"include/hw/pci/pci_bridge.h", just before the declaration of the
pci_bridge_qemu_reserve_cap_init() function.

- Note that the PCIBridgeQemuCap structure should *not* be modified
(i.e. it should not be rebased to the new sub-structure) -- the fields
are not identical!

- The GenPCIERootPort structure should embed this new sub-structure; the
field name could be "res_reserve".

- The parameter list of pci_bridge_qemu_reserve_cap_init() should be
updated to take a pointer to a constant sub-structure.

- gen_rp_realize() can simply pass &grp->res_reserve.

- gen_rp_props should be updated accordingly. The property names should
stay the same, but they should reference fields of the "res_reserve" field.

(2) Then, in a separate patch, you can add another "res_reserve" field
to "PCIBridgeDev". (As a consequence, "bus_reserve" will also be added,
which I think is the right thing to do.)

(3) There is a class that is derived from TYPE_PCI_BRIDGE_DEV:
TYPE_PCI_BRIDGE_SEAT_DEV. Is it correct to add the new properties /
fields to the latter as well?

>  
> @@ -95,6 +101,13 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>          error_free(local_err);
>      }
>  
> +    err = pci_bridge_qemu_reserve_cap_init(dev, 0, 0,
> +          bridge_dev->io_reserve, bridge_dev->mem_reserve,
> +          bridge_dev->pref32_reserve, bridge_dev->pref64_reserve, errp);
> +    if (err) {
> +        goto cap_error;
> +    }
> +
>      if (shpc_present(dev)) {
>          /* TODO: spec recommends using 64 bit prefetcheable BAR.
>           * Check whether that works well. */
> @@ -103,6 +116,8 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>      }
>      return;
>  
> +cap_error:
> +    msi_uninit(dev);

(4) This error handler doesn't look entirely correct; we can reach it
without having initialized MSI. (MSI setup is conditional; and even if
we attempt it, it is permitted to fail with "msi=auto".) Therefore,
msi_uninit() should be wrapped into "if (msi_present())", same as you
see in pci_bridge_dev_exitfn().

>  msi_error:
>      slotid_cap_cleanup(dev);
>  slotid_error:
> @@ -162,6 +177,11 @@ static Property pci_bridge_dev_properties[] = {
>                              ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
>                      PCI_BRIDGE_DEV_F_SHPC_REQ, true),
> +    DEFINE_PROP_SIZE("io-reserve", PCIBridgeDev, io_reserve, -1),
> +    DEFINE_PROP_SIZE("mem-reserve", PCIBridgeDev, mem_reserve, -1),
> +    DEFINE_PROP_SIZE("pref32-reserve", PCIBridgeDev, pref32_reserve, -1),
> +    DEFINE_PROP_SIZE("pref64-reserve", PCIBridgeDev, pref64_reserve, -1),
> +
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> 

(5) Similarly to (2), this property list should cover "bus-reserve" too.
If we are exposing the same capability in PCI config space, then I think
we should let the user control all fields of it as well.

(6) Please clarify the capability ID in the subject line of the patch.
For example:

  hw/pci: support resource reservation capability on "pci-bridge"

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Laszlo Ersek]

On 08/07/18 14:19, Laszlo Ersek wrote:
> On 08/07/18 09:04, Jing Liu wrote:
>> Add hint to firmware (e.g. SeaBIOS) to reserve addtional
>> IO/MEM/PREF spaces for legacy pci-pci bridge, to enable
>> some pci devices hotplugging whose IO/MEM/PREF spaces
>> requests are larger than the ones in pci-pci bridge set
>> by firmware.
>>
>> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
>> ---
>>  hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)

>> +cap_error:
>> +    msi_uninit(dev);
> 
> (4) This error handler doesn't look entirely correct; we can reach it
> without having initialized MSI. (MSI setup is conditional; and even if
> we attempt it, it is permitted to fail with "msi=auto".) Therefore,
> msi_uninit() should be wrapped into "if (msi_present())", same as you
> see in pci_bridge_dev_exitfn().
> 
>>  msi_error:
>>      slotid_cap_cleanup(dev);
>>  slotid_error:

I tried to understand the error handling a bit better. I'm confused.

First, under the label "shpc_error", we call pci_bridge_exitfn(), which
seems to clean up everything (checking individually for each thing to
clean up). Given this, I wonder why we introduced the "slotid_error" and
"msi_error" labels at all. Cascading teardown on the error path, and
invkoing a function that checks everything individually and then tears
it all down, are usually mutually exclusive.

Second, msi_uninit() and shpc_cleanup() are internally inconsistent
between each other. The former removes the respective capability from
config space -- with pci_del_capability() --, but the latter only has a
comment, "/* TODO: cleanup config space changes? */". The same comment
is present in the slotid_cap_cleanup() function. Given this
inconsistency, I don't know what to suggest for the new capability's
teardown, in pci_bridge_dev_exitfn()  -- should we just ignore it (as
suggested by this patch)?

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Marcel Apfelbaum]

Hi,

On 08/07/2018 03:19 PM, Laszlo Ersek wrote:
> On 08/07/18 09:04, Jing Liu wrote:
>> Add hint to firmware (e.g. SeaBIOS) to reserve addtional
>> IO/MEM/PREF spaces for legacy pci-pci bridge, to enable
>> some pci devices hotplugging whose IO/MEM/PREF spaces
>> requests are larger than the ones in pci-pci bridge set
>> by firmware.
>>
>> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
>> ---
>>   hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>> index b2d861d..8e9afbd 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>> @@ -46,6 +46,12 @@ struct PCIBridgeDev {
>>       uint32_t flags;
>>   
>>       OnOffAuto msi;
>> +
>> +    /* additional resources to reserve on firmware init */
>> +    uint64_t io_reserve;
>> +    uint64_t mem_reserve;
>> +    uint64_t pref32_reserve;
>> +    uint64_t pref64_reserve;
>>   };
>>   typedef struct PCIBridgeDev PCIBridgeDev;
> (1) Please evaluate the following idea:
>
> - Factor the "bus_reserve", "io_reserve", "mem_reserve",
> "pref32_reserve" and "pref64_reserve" fiels of the "GenPCIERootPort"
> structure out to another structure.
>
> - I think this new structure type should be defined in
> "include/hw/pci/pci_bridge.h", just before the declaration of the
> pci_bridge_qemu_reserve_cap_init() function.

+1

> - Note that the PCIBridgeQemuCap structure should *not* be modified
> (i.e. it should not be rebased to the new sub-structure) -- the fields
> are not identical!
>
> - The GenPCIERootPort structure should embed this new sub-structure; the
> field name could be "res_reserve".
>
> - The parameter list of pci_bridge_qemu_reserve_cap_init() should be
> updated to take a pointer to a constant sub-structure.
>
> - gen_rp_realize() can simply pass &grp->res_reserve.
>
> - gen_rp_props should be updated accordingly. The property names should
> stay the same, but they should reference fields of the "res_reserve" field.
>
> (2) Then, in a separate patch, you can add another "res_reserve" field
> to "PCIBridgeDev". (As a consequence, "bus_reserve" will also be added,
> which I think is the right thing to do.)

It looks like a good implementation idea to me.

> (3) There is a class that is derived from TYPE_PCI_BRIDGE_DEV:
> TYPE_PCI_BRIDGE_SEAT_DEV. Is it correct to add the new properties /
> fields to the latter as well?

I see no problem with that, as long as the defaults will not create the 
vendor capability.

Thanks,
Marcel

>>   
>> @@ -95,6 +101,13 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>>           error_free(local_err);
>>       }
>>   
>> +    err = pci_bridge_qemu_reserve_cap_init(dev, 0, 0,
>> +          bridge_dev->io_reserve, bridge_dev->mem_reserve,
>> +          bridge_dev->pref32_reserve, bridge_dev->pref64_reserve, errp);
>> +    if (err) {
>> +        goto cap_error;
>> +    }
>> +
>>       if (shpc_present(dev)) {
>>           /* TODO: spec recommends using 64 bit prefetcheable BAR.
>>            * Check whether that works well. */
>> @@ -103,6 +116,8 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>>       }
>>       return;
>>   
>> +cap_error:
>> +    msi_uninit(dev);
> (4) This error handler doesn't look entirely correct; we can reach it
> without having initialized MSI. (MSI setup is conditional; and even if
> we attempt it, it is permitted to fail with "msi=auto".) Therefore,
> msi_uninit() should be wrapped into "if (msi_present())", same as you
> see in pci_bridge_dev_exitfn().
>
>>   msi_error:
>>       slotid_cap_cleanup(dev);
>>   slotid_error:
>> @@ -162,6 +177,11 @@ static Property pci_bridge_dev_properties[] = {
>>                               ON_OFF_AUTO_AUTO),
>>       DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
>>                       PCI_BRIDGE_DEV_F_SHPC_REQ, true),
>> +    DEFINE_PROP_SIZE("io-reserve", PCIBridgeDev, io_reserve, -1),
>> +    DEFINE_PROP_SIZE("mem-reserve", PCIBridgeDev, mem_reserve, -1),
>> +    DEFINE_PROP_SIZE("pref32-reserve", PCIBridgeDev, pref32_reserve, -1),
>> +    DEFINE_PROP_SIZE("pref64-reserve", PCIBridgeDev, pref64_reserve, -1),
>> +
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>>
> (5) Similarly to (2), this property list should cover "bus-reserve" too.
> If we are exposing the same capability in PCI config space, then I think
> we should let the user control all fields of it as well.
>
> (6) Please clarify the capability ID in the subject line of the patch.
> For example:
>
>    hw/pci: support resource reservation capability on "pci-bridge"
>
> Thanks
> Laszlo

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Marcel Apfelbaum]

Hi Laszlo,

On 08/07/2018 06:59 PM, Laszlo Ersek wrote:
> On 08/07/18 14:19, Laszlo Ersek wrote:
>> On 08/07/18 09:04, Jing Liu wrote:
>>> Add hint to firmware (e.g. SeaBIOS) to reserve addtional
>>> IO/MEM/PREF spaces for legacy pci-pci bridge, to enable
>>> some pci devices hotplugging whose IO/MEM/PREF spaces
>>> requests are larger than the ones in pci-pci bridge set
>>> by firmware.
>>>
>>> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
>>> ---
>>>   hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++++++
>>>   1 file changed, 20 insertions(+)
>>> +cap_error:
>>> +    msi_uninit(dev);
>> (4) This error handler doesn't look entirely correct; we can reach it
>> without having initialized MSI. (MSI setup is conditional; and even if
>> we attempt it, it is permitted to fail with "msi=auto".) Therefore,
>> msi_uninit() should be wrapped into "if (msi_present())", same as you
>> see in pci_bridge_dev_exitfn().

You are right.  msi_present should be checked.

>>
>>>   msi_error:
>>>       slotid_cap_cleanup(dev);
>>>   slotid_error:
> I tried to understand the error handling a bit better. I'm confused.
>
> First, under the label "shpc_error", we call pci_bridge_exitfn(), which
> seems to clean up everything (checking individually for each thing to
> clean up). Given this, I wonder why we introduced the "slotid_error" and
> "msi_error" labels at all. Cascading teardown on the error path, and
> invkoing a function that checks everything individually and then tears
> it all down, are usually mutually exclusive.

I think is possible you miss-interpreted pci_bridge_dev_exitfn
with pci_bridge_exitfn. The first one is the "catch all", the second
one that is used in the error path is for the bridge specific cleanup.

> Second, msi_uninit() and shpc_cleanup() are internally inconsistent
> between each other. The former removes the respective capability from
> config space -- with pci_del_capability() --,

Right

>   but the latter only has a
> comment, "/* TODO: cleanup config space changes? */".

But also disables the QEMU_PCI_CAP_SLOTID (but no code checks it anyway)
I agree it should call pci_del_capability to delete the SHPC capability,
maybe is some "debt" from early development stages.

>   The same comment
> is present in the slotid_cap_cleanup() function. Given this
> inconsistency,

Here we also miss a call to pci_del_capability.

> I don't know what to suggest for the new capability's
> teardown, in pci_bridge_dev_exitfn()  -- should we just ignore it (as
> suggested by this patch)?

No, we should remove it properly.

I think it is not considered a "big" issue since adding/removing PCI 
capabilities
is not an operation that deals with resources, we only edit an array  
(the config space)
that will not be used anyway if the device init sequence failed.

That does not mean the code should not be clean.

Thanks,
Marcel

>
> Thanks
> Laszlo
>

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Laszlo Ersek]

On 08/12/18 09:11, Marcel Apfelbaum wrote:
> On 08/07/2018 06:59 PM, Laszlo Ersek wrote:

>> First, under the label "shpc_error", we call pci_bridge_exitfn(), which
>> seems to clean up everything (checking individually for each thing to
>> clean up). Given this, I wonder why we introduced the "slotid_error" and
>> "msi_error" labels at all. Cascading teardown on the error path, and
>> invkoing a function that checks everything individually and then tears
>> it all down, are usually mutually exclusive.
> 
> I think is possible you miss-interpreted pci_bridge_dev_exitfn
> with pci_bridge_exitfn. The first one is the "catch all", the second
> one that is used in the error path is for the bridge specific cleanup.

Ah, you are correct, I totally mistook the call to pci_bridge_exitfn()
for pci_bridge_dev_exitfn(). I do see the difference now, so it's clear
that the error path in pci_bridge_dev_realize() is of the "cascading"
kind, and not of the "call a catch-all function" kind.

That gives this patch a clear pattern to follow.

Thank you!
Laszlo

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Liu, Jing2]

Hi Laszlo,
Sorry for late reply. I missed these mails because of wrong filter.
And thanks very much for comments. My reply as belows.

On 8/7/2018 8:19 PM, Laszlo Ersek wrote:
> On 08/07/18 09:04, Jing Liu wrote:
[...]
>> @@ -46,6 +46,12 @@ struct PCIBridgeDev {
>>       uint32_t flags;
>>   
>>       OnOffAuto msi;
>> +
>> +    /* additional resources to reserve on firmware init */
>> +    uint64_t io_reserve;
>> +    uint64_t mem_reserve;
>> +    uint64_t pref32_reserve;
>> +    uint64_t pref64_reserve;
>>   };
>>   typedef struct PCIBridgeDev PCIBridgeDev;
> 
> (1) Please evaluate the following idea:
> 
This is really good and I'm only not sure if DEFINE_PROP_ like,
DEFINE_PROP_UINT32("bus-reserve", GenPCIERootPort,
res_reserve.bus_reserve, -1), is a right/good way?
I mean the third parameter "_f".

[...]
> 
> - gen_rp_props should be updated accordingly. The property names should
> stay the same, but they should reference fields of the "res_reserve" field.
> 
> (2) Then, in a separate patch, you can add another "res_reserve" field
> to "PCIBridgeDev". (As a consequence, "bus_reserve" will also be added,
> which I think is the right thing to do.)
> 
I consider whether we need limit the bus_reserve of pci-pci bridge. For 
old codes, we could add "unlimited" numbers of devices (but less than 
than max) onto pci-pci bridge.

> (3) There is a class that is derived from TYPE_PCI_BRIDGE_DEV:
> TYPE_PCI_BRIDGE_SEAT_DEV. Is it correct to add the new properties /
> fields to the latter as well?
> 
Umm, I looked into the codes that doesn't have hotplug property?
So do we need add resource reserve for it?

>>   
>> @@ -95,6 +101,13 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>>           error_free(local_err);
>>       }
>>   
>> +    err = pci_bridge_qemu_reserve_cap_init(dev, 0, 0,
>> +          bridge_dev->io_reserve, bridge_dev->mem_reserve,
>> +          bridge_dev->pref32_reserve, bridge_dev->pref64_reserve, errp);
>> +    if (err) {
>> +        goto cap_error;
>> +    }
>> +
>>       if (shpc_present(dev)) {
>>           /* TODO: spec recommends using 64 bit prefetcheable BAR.
>>            * Check whether that works well. */
>> @@ -103,6 +116,8 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>>       }
>>       return;
>>   
>> +cap_error:
>> +    msi_uninit(dev);
> 
> (4) This error handler doesn't look entirely correct; we can reach it
> without having initialized MSI. (MSI setup is conditional; and even if
> we attempt it, it is permitted to fail with "msi=auto".) Therefore,
> msi_uninit() should be wrapped into "if (msi_present())", same as you
> see in pci_bridge_dev_exitfn().

sure, can add a pre-check. But I don't understand why we need that,
msi_uninit() will check msi_present()?

> 
>>   msi_error:
>>       slotid_cap_cleanup(dev);
>>   slotid_error:
>> @@ -162,6 +177,11 @@ static Property pci_bridge_dev_properties[] = {
>>                               ON_OFF_AUTO_AUTO),
>>       DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
>>                       PCI_BRIDGE_DEV_F_SHPC_REQ, true),
>> +    DEFINE_PROP_SIZE("io-reserve", PCIBridgeDev, io_reserve, -1),
>> +    DEFINE_PROP_SIZE("mem-reserve", PCIBridgeDev, mem_reserve, -1),
>> +    DEFINE_PROP_SIZE("pref32-reserve", PCIBridgeDev, pref32_reserve, -1),
>> +    DEFINE_PROP_SIZE("pref64-reserve", PCIBridgeDev, pref64_reserve, -1),
>> +
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>>
> 
> (5) Similarly to (2), this property list should cover "bus-reserve" too.
> If we are exposing the same capability in PCI config space, then I think
> we should let the user control all fields of it as well.
> 
> (6) Please clarify the capability ID in the subject line of the patch.
OK, will add the details.


Thanks very much.
Jing

> For example:
> 
>    hw/pci: support resource reservation capability on "pci-bridge"
> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Liu, Jing2]

Hi Marcel,

On 8/12/2018 3:11 PM, Marcel Apfelbaum wrote:
> Hi Laszlo,
> 
[...]
>>>>   hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++++++
>>>>   1 file changed, 20 insertions(+)
>>>> +cap_error:
>>>> +    msi_uninit(dev);
>>> (4) This error handler doesn't look entirely correct; we can reach it
>>> without having initialized MSI. (MSI setup is conditional; and even if
>>> we attempt it, it is permitted to fail with "msi=auto".) Therefore,
>>> msi_uninit() should be wrapped into "if (msi_present())", same as you
>>> see in pci_bridge_dev_exitfn().
> 
> You are right.  msi_present should be checked.

I looked at the codes calling this function. It need be added to be strong.
But could I ask why we need check twice? msi_unint() help check again.
> 
>>>
>>>>   msi_error:
>>>>       slotid_cap_cleanup(dev);
>>>>   slotid_error:
>> I tried to understand the error handling a bit better. I'm confused.
>>
[...]
>> Second, msi_uninit() and shpc_cleanup() are internally inconsistent
>> between each other. The former removes the respective capability from
>> config space -- with pci_del_capability() --,
> 
> Right
> 
>>   but the latter only has a
>> comment, "/* TODO: cleanup config space changes? */".
> 
> But also disables the QEMU_PCI_CAP_SLOTID (but no code checks it anyway)
> I agree it should call pci_del_capability to delete the SHPC capability,
> maybe is some "debt" from early development stages.
> 
>>   The same comment
>> is present in the slotid_cap_cleanup() function. Given this
>> inconsistency,
> 
> Here we also miss a call to pci_del_capability.
> 
>> I don't know what to suggest for the new capability's
>> teardown, in pci_bridge_dev_exitfn()  
Aha, yes, the teardown needs to be added here.
Will add that in next version.

-- should we just ignore it (as
>> suggested by this patch)?
> 
> No, we should remove it properly.
> 
> I think it is not considered a "big" issue since adding/removing PCI 
> capabilities
> is not an operation that deals with resources, we only edit an array 
> (the config space)
> that will not be used anyway if the device init sequence failed.
> 
> That does not mean the code should not be clean.
Do we need set up another serial patches (separated from this
one) to add pci_del_capability() for slotid_cap_cleanup() and 
shpc_cleanup()?

Thanks,
Jing

[...]

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Liu, Jing2]

On 8/12/2018 3:11 PM, Marcel Apfelbaum wrote:
[...]

>> I don't know what to suggest for the new capability's
>> teardown, in pci_bridge_dev_exitfn()  -- should we just ignore it (as
>> suggested by this patch)?
> 
> No, we should remove it properly.
> 
> I think it is not considered a "big" issue since adding/removing PCI 
> capabilities
> is not an operation that deals with resources, we only edit an array 
> (the config space)
> that will not be used anyway if the device init sequence failed.
> 
> That does not mean the code should not be clean.
I just search some PCI_CAP_ID_* to see what they do for exitfn/uninit.
It's weird that some more capabilities aren't being deleted, like 
pci_ich9_uninit().

Thanks,
Jing

> 
> Thanks,
> Marcel
> 
>>
>> Thanks
>> Laszlo
>>
> 

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Laszlo Ersek]

On 08/14/18 10:39, Liu, Jing2 wrote:
> Hi Laszlo,
> Sorry for late reply. I missed these mails because of wrong filter.
> And thanks very much for comments. My reply as belows.
> 
> On 8/7/2018 8:19 PM, Laszlo Ersek wrote:
>> On 08/07/18 09:04, Jing Liu wrote:
> [...]
>>> @@ -46,6 +46,12 @@ struct PCIBridgeDev {
>>>       uint32_t flags;
>>>         OnOffAuto msi;
>>> +
>>> +    /* additional resources to reserve on firmware init */
>>> +    uint64_t io_reserve;
>>> +    uint64_t mem_reserve;
>>> +    uint64_t pref32_reserve;
>>> +    uint64_t pref64_reserve;
>>>   };
>>>   typedef struct PCIBridgeDev PCIBridgeDev;
>>
>> (1) Please evaluate the following idea:
>>
> This is really good and I'm only not sure if DEFINE_PROP_ like,
> DEFINE_PROP_UINT32("bus-reserve", GenPCIERootPort,
> res_reserve.bus_reserve, -1), is a right/good way?
> I mean the third parameter "_f".

Yes, I think you can refer to members within structure fields like that.

> 
> [...]
>>
>> - gen_rp_props should be updated accordingly. The property names should
>> stay the same, but they should reference fields of the "res_reserve"
>> field.
>>
>> (2) Then, in a separate patch, you can add another "res_reserve" field
>> to "PCIBridgeDev". (As a consequence, "bus_reserve" will also be added,
>> which I think is the right thing to do.)
>>
> I consider whether we need limit the bus_reserve of pci-pci bridge. For
> old codes, we could add "unlimited" numbers of devices (but less than
> than max) onto pci-pci bridge.

In theory the bus_reserve hint makes sense too. There likely isn't a
great use case for it in practice, but that's not reason enough IMO to
diverge in the implementation (the factored-out structure and the
properties). It's certainly not wrong to offer the property, IMO.


> 
>> (3) There is a class that is derived from TYPE_PCI_BRIDGE_DEV:
>> TYPE_PCI_BRIDGE_SEAT_DEV. Is it correct to add the new properties /
>> fields to the latter as well?
>>
> Umm, I looked into the codes that doesn't have hotplug property?
> So do we need add resource reserve for it?

No clue. I don't know anything about TYPE_PCI_BRIDGE_SEAT_DEV; I'll have
to defer to Marcel and others on that.

> 
>>>   @@ -95,6 +101,13 @@ static void pci_bridge_dev_realize(PCIDevice
>>> *dev, Error **errp)
>>>           error_free(local_err);
>>>       }
>>>   +    err = pci_bridge_qemu_reserve_cap_init(dev, 0, 0,
>>> +          bridge_dev->io_reserve, bridge_dev->mem_reserve,
>>> +          bridge_dev->pref32_reserve, bridge_dev->pref64_reserve,
>>> errp);
>>> +    if (err) {
>>> +        goto cap_error;
>>> +    }
>>> +
>>>       if (shpc_present(dev)) {
>>>           /* TODO: spec recommends using 64 bit prefetcheable BAR.
>>>            * Check whether that works well. */
>>> @@ -103,6 +116,8 @@ static void pci_bridge_dev_realize(PCIDevice
>>> *dev, Error **errp)
>>>       }
>>>       return;
>>>   +cap_error:
>>> +    msi_uninit(dev);
>>
>> (4) This error handler doesn't look entirely correct; we can reach it
>> without having initialized MSI. (MSI setup is conditional; and even if
>> we attempt it, it is permitted to fail with "msi=auto".) Therefore,
>> msi_uninit() should be wrapped into "if (msi_present())", same as you
>> see in pci_bridge_dev_exitfn().
> 
> sure, can add a pre-check. But I don't understand why we need that,
> msi_uninit() will check msi_present()?

Thanks for pointing that out.

So, I grepped the source code for msi_uninit(). It seems that only one
location gates it with msi_present() -- in pci_bridge_dev_exitfn() --,
while 13 other locations just call it unconditionally.

I don't know what the consistent approach is here. I'll have to defer to
the PCI maintainers; their taste will decide. Technically your code
looks correct, then.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Liu, Jing2]

On 8/14/2018 9:27 PM, Laszlo Ersek wrote:
[...]
>>>>    +cap_error:
>>>> +    msi_uninit(dev);
>>>
>>> (4) This error handler doesn't look entirely correct; we can reach it
>>> without having initialized MSI. (MSI setup is conditional; and even if
>>> we attempt it, it is permitted to fail with "msi=auto".) Therefore,
>>> msi_uninit() should be wrapped into "if (msi_present())", same as you
>>> see in pci_bridge_dev_exitfn().
>>
>> sure, can add a pre-check. But I don't understand why we need that,
>> msi_uninit() will check msi_present()?
> 
> Thanks for pointing that out.
> 
> So, I grepped the source code for msi_uninit(). It seems that only one
> location gates it with msi_present() -- in pci_bridge_dev_exitfn() --,
> while 13 other locations just call it unconditionally.
> 
> I don't know what the consistent approach is here. I'll have to defer to
> the PCI maintainers; their taste will decide. Technically your code
> looks correct, then.
Yes, I also grep that. I guess if we directly use msi_uninit, this is 
because msi_init is non-conditional too.
Anyway, I added check in my version two. BTW, other comments are 
included in v2.

Thanks,
Jing

> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Marcel Apfelbaum]

On 08/14/2018 12:15 PM, Liu, Jing2 wrote:
> Hi Marcel,
>
> On 8/12/2018 3:11 PM, Marcel Apfelbaum wrote:
>> Hi Laszlo,
>>
> [...]
>>>>>   hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++++++
>>>>>   1 file changed, 20 insertions(+)
>>>>> +cap_error:
>>>>> +    msi_uninit(dev);
>>>> (4) This error handler doesn't look entirely correct; we can reach it
>>>> without having initialized MSI. (MSI setup is conditional; and even if
>>>> we attempt it, it is permitted to fail with "msi=auto".) Therefore,
>>>> msi_uninit() should be wrapped into "if (msi_present())", same as you
>>>> see in pci_bridge_dev_exitfn().
>>
>> You are right.  msi_present should be checked.
>
> I looked at the codes calling this function. It need be added to be 
> strong.
> But could I ask why we need check twice? msi_unint() help check again.

You are right, no need to check again. Sorry for misleading you.

>>
>>>>
>>>>>   msi_error:
>>>>>       slotid_cap_cleanup(dev);
>>>>>   slotid_error:
>>> I tried to understand the error handling a bit better. I'm confused.
>>>
> [...]
>>> Second, msi_uninit() and shpc_cleanup() are internally inconsistent
>>> between each other. The former removes the respective capability from
>>> config space -- with pci_del_capability() --,
>>
>> Right
>>
>>>   but the latter only has a
>>> comment, "/* TODO: cleanup config space changes? */".
>>
>> But also disables the QEMU_PCI_CAP_SLOTID (but no code checks it anyway)
>> I agree it should call pci_del_capability to delete the SHPC capability,
>> maybe is some "debt" from early development stages.
>>
>>>   The same comment
>>> is present in the slotid_cap_cleanup() function. Given this
>>> inconsistency,
>>
>> Here we also miss a call to pci_del_capability.
>>
>>> I don't know what to suggest for the new capability's
>>> teardown, in pci_bridge_dev_exitfn() 
> Aha, yes, the teardown needs to be added here.
> Will add that in next version.
>
> -- should we just ignore it (as
>>> suggested by this patch)?
>>
>> No, we should remove it properly.
>>
>> I think it is not considered a "big" issue since adding/removing PCI 
>> capabilities
>> is not an operation that deals with resources, we only edit an array 
>> (the config space)
>> that will not be used anyway if the device init sequence failed.
>>
>> That does not mean the code should not be clean.
> Do we need set up another serial patches (separated from this
> one) to add pci_del_capability() for slotid_cap_cleanup() and 
> shpc_cleanup()?
>

It would be nice, but as you pointed out, it doesn't have to be part
of this series.

Thanks,
Marcel

> Thanks,
> Jing
>
> [...]

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Marcel Apfelbaum]

On 08/14/2018 12:32 PM, Liu, Jing2 wrote:
>
>
> On 8/12/2018 3:11 PM, Marcel Apfelbaum wrote:
> [...]
>
>>> I don't know what to suggest for the new capability's
>>> teardown, in pci_bridge_dev_exitfn()  -- should we just ignore it (as
>>> suggested by this patch)?
>>
>> No, we should remove it properly.
>>
>> I think it is not considered a "big" issue since adding/removing PCI 
>> capabilities
>> is not an operation that deals with resources, we only edit an array 
>> (the config space)
>> that will not be used anyway if the device init sequence failed.
>>
>> That does not mean the code should not be clean.
> I just search some PCI_CAP_ID_* to see what they do for exitfn/uninit.
> It's weird that some more capabilities aren't being deleted, like 
> pci_ich9_uninit().
>

I agree it does not look good. As I pointed out  above,
if the capability does not require extra resources, the array representing
the configuration space for the device gets "deleted" anyway when we
dispose of the device.

Thanks,
Marcel

> Thanks,
> Jing
>
>>
>> Thanks,
>> Marcel
>>
>>>
>>> Thanks
>>> Laszlo
>>>
>>