[Qemu-devel] [PATCH v2 0/3] hw/pci: PCI resource reserve capability

[Qemu-devel] [PATCH v2 0/3] hw/pci: PCI resource reserve capability

[Jing Liu]

This patch serial is about PCI resource reserve capability.

First patch refactors the resource reserve fields in GenPCIERoorPort
structure out to another new structure, called "PCIResReserve". Modify
the parameter list of pci_bridge_qemu_reserve_cap_init().

Then we add the teardown function called pci_bridge_qemu_reserve_cap_uninit().

Last we enable the resource reserve capability for legacy PCI bridge
so that firmware can reserve additional resources for the bridge.

Change Log:
v2 -> v1
* add refactoring patch
* add teardown function
* some other fixes

Jing Liu (3):
  hw/pci: factor PCI reserve resources to a separate structure
  hw/pci: add teardown function for PCI resource reserve capability
  hw/pci: add PCI resource reserve capability to legacy PCI bridge

 hw/pci-bridge/gen_pcie_root_port.c | 32 +++++++++++++-------------
 hw/pci-bridge/pci_bridge_dev.c     | 25 ++++++++++++++++++++
 hw/pci/pci_bridge.c                | 47 +++++++++++++++++++++-----------------
 include/hw/pci/pci_bridge.h        | 18 +++++++++++----
 4 files changed, 80 insertions(+), 42 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/3] hw/pci: factor PCI reserve resources to a separate structure

[Jing Liu]

Factor "bus_reserve", "io_reserve", "mem_reserve", "pref32_reserve"
and "pref64_reserve" fields of the "GenPCIERootPort" structure out
to "PCIResReserve" structure, so that other PCI bridges can
reuse it to add resource reserve capability.

Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
---
 hw/pci-bridge/gen_pcie_root_port.c | 32 ++++++++++++++++----------------
 hw/pci/pci_bridge.c                | 38 +++++++++++++++++---------------------
 include/hw/pci/pci_bridge.h        | 17 ++++++++++++-----
 3 files changed, 45 insertions(+), 42 deletions(-)

diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
index d117e20..babce3c 100644
--- a/hw/pci-bridge/gen_pcie_root_port.c
+++ b/hw/pci-bridge/gen_pcie_root_port.c
@@ -29,12 +29,7 @@ typedef struct GenPCIERootPort {
 
     bool migrate_msix;
 
-    /* additional resources to reserve on firmware init */
-    uint32_t bus_reserve;
-    uint64_t io_reserve;
-    uint64_t mem_reserve;
-    uint64_t pref32_reserve;
-    uint64_t pref64_reserve;
+    PCIResReserve res_reserve;
 } GenPCIERootPort;
 
 static uint8_t gen_rp_aer_vector(const PCIDevice *d)
@@ -82,16 +77,15 @@ static void gen_rp_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    int rc = pci_bridge_qemu_reserve_cap_init(d, 0, grp->bus_reserve,
-            grp->io_reserve, grp->mem_reserve, grp->pref32_reserve,
-            grp->pref64_reserve, errp);
+    int rc = pci_bridge_qemu_reserve_cap_init(d, 0,
+                                              grp->res_reserve, errp);
 
     if (rc < 0) {
         rpc->parent_class.exit(d);
         return;
     }
 
-    if (!grp->io_reserve) {
+    if (!grp->res_reserve.io_reserve) {
         pci_word_test_and_clear_mask(d->wmask + PCI_COMMAND,
                                      PCI_COMMAND_IO);
         d->wmask[PCI_IO_BASE] = 0;
@@ -117,12 +111,18 @@ static const VMStateDescription vmstate_rp_dev = {
 };
 
 static Property gen_rp_props[] = {
-    DEFINE_PROP_BOOL("x-migrate-msix", GenPCIERootPort, migrate_msix, true),
-    DEFINE_PROP_UINT32("bus-reserve", GenPCIERootPort, bus_reserve, -1),
-    DEFINE_PROP_SIZE("io-reserve", GenPCIERootPort, io_reserve, -1),
-    DEFINE_PROP_SIZE("mem-reserve", GenPCIERootPort, mem_reserve, -1),
-    DEFINE_PROP_SIZE("pref32-reserve", GenPCIERootPort, pref32_reserve, -1),
-    DEFINE_PROP_SIZE("pref64-reserve", GenPCIERootPort, pref64_reserve, -1),
+    DEFINE_PROP_BOOL("x-migrate-msix", GenPCIERootPort,
+                     migrate_msix, true),
+    DEFINE_PROP_UINT32("bus-reserve", GenPCIERootPort,
+                       res_reserve.bus_reserve, -1),
+    DEFINE_PROP_SIZE("io-reserve", GenPCIERootPort,
+                     res_reserve.io_reserve, -1),
+    DEFINE_PROP_SIZE("mem-reserve", GenPCIERootPort,
+                     res_reserve.mem_reserve, -1),
+    DEFINE_PROP_SIZE("pref32-reserve", GenPCIERootPort,
+                     res_reserve.pref32_reserve, -1),
+    DEFINE_PROP_SIZE("pref64-reserve", GenPCIERootPort,
+                     res_reserve.pref64_reserve, -1),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 40a39f5..15b055e 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -411,38 +411,34 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
 
 
 int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
-                                     uint32_t bus_reserve, uint64_t io_reserve,
-                                     uint64_t mem_non_pref_reserve,
-                                     uint64_t mem_pref_32_reserve,
-                                     uint64_t mem_pref_64_reserve,
-                                     Error **errp)
+                                     PCIResReserve res, Error **errp)
 {
-    if (mem_pref_32_reserve != (uint64_t)-1 &&
-        mem_pref_64_reserve != (uint64_t)-1) {
+    if (res.pref32_reserve != (uint64_t)-1 &&
+        res.pref64_reserve != (uint64_t)-1) {
         error_setg(errp,
                    "PCI resource reserve cap: PREF32 and PREF64 conflict");
         return -EINVAL;
     }
 
-    if (mem_non_pref_reserve != (uint64_t)-1 &&
-        mem_non_pref_reserve >= (1ULL << 32)) {
+    if (res.mem_reserve != (uint64_t)-1 &&
+        res.mem_reserve >= (1ULL << 32)) {
         error_setg(errp,
                    "PCI resource reserve cap: mem-reserve must be less than 4G");
         return -EINVAL;
     }
 
-    if (mem_pref_32_reserve != (uint64_t)-1 &&
-        mem_pref_32_reserve >= (1ULL << 32)) {
+    if (res.pref32_reserve != (uint64_t)-1 &&
+        res.pref32_reserve >= (1ULL << 32)) {
         error_setg(errp,
                    "PCI resource reserve cap: pref32-reserve  must be less than 4G");
         return -EINVAL;
     }
 
-    if (bus_reserve == (uint32_t)-1 &&
-        io_reserve == (uint64_t)-1 &&
-        mem_non_pref_reserve == (uint64_t)-1 &&
-        mem_pref_32_reserve == (uint64_t)-1 &&
-        mem_pref_64_reserve == (uint64_t)-1) {
+    if (res.bus_reserve == (uint32_t)-1 &&
+        res.io_reserve == (uint64_t)-1 &&
+        res.mem_reserve == (uint64_t)-1 &&
+        res.pref32_reserve == (uint64_t)-1 &&
+        res.pref64_reserve == (uint64_t)-1) {
         return 0;
     }
 
@@ -450,11 +446,11 @@ int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
     PCIBridgeQemuCap cap = {
             .len = cap_len,
             .type = REDHAT_PCI_CAP_RESOURCE_RESERVE,
-            .bus_res = bus_reserve,
-            .io = io_reserve,
-            .mem = mem_non_pref_reserve,
-            .mem_pref_32 = mem_pref_32_reserve,
-            .mem_pref_64 = mem_pref_64_reserve
+            .bus_res = res.bus_reserve,
+            .io = res.io_reserve,
+            .mem = res.mem_reserve,
+            .mem_pref_32 = res.pref32_reserve,
+            .mem_pref_64 = res.pref64_reserve
     };
 
     int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index 0347da5..6186a32 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -133,11 +133,18 @@ typedef struct PCIBridgeQemuCap {
 
 #define REDHAT_PCI_CAP_RESOURCE_RESERVE 1
 
+/*
+ * additional resources to reserve on firmware init
+ */
+typedef struct PCIResReserve {
+    uint32_t bus_reserve;
+    uint64_t io_reserve;
+    uint64_t mem_reserve;
+    uint64_t pref32_reserve;
+    uint64_t pref64_reserve;
+} PCIResReserve;
+
 int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
-                              uint32_t bus_reserve, uint64_t io_reserve,
-                              uint64_t mem_non_pref_reserve,
-                              uint64_t mem_pref_32_reserve,
-                              uint64_t mem_pref_64_reserve,
-                              Error **errp);
+                               PCIResReserve res_reserve, Error **errp);
 
 #endif /* QEMU_PCI_BRIDGE_H */
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/3] hw/pci: add teardown function for PCI resource reserve capability

[Jing Liu]

Clean up the PCI config space of resource reserve capability.

Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
---
 hw/pci/pci_bridge.c         | 9 +++++++++
 include/hw/pci/pci_bridge.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 15b055e..dbcee90 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -465,6 +465,15 @@ int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
     return 0;
 }
 
+void pci_bridge_qemu_reserve_cap_uninit(PCIDevice *dev)
+{
+    uint8_t pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
+
+    pci_del_capability(dev, PCI_CAP_ID_VNDR, sizeof(PCIBridgeQemuCap));
+    memset(dev->config + pos + PCI_CAP_FLAGS, 0,
+           sizeof(PCIBridgeQemuCap) - PCI_CAP_FLAGS);
+}
+
 static const TypeInfo pci_bridge_type_info = {
     .name = TYPE_PCI_BRIDGE,
     .parent = TYPE_PCI_DEVICE,
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index 6186a32..b1e25ad 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -147,4 +147,5 @@ typedef struct PCIResReserve {
 int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
                                PCIResReserve res_reserve, Error **errp);
 
+void pci_bridge_qemu_reserve_cap_uninit(PCIDevice *dev);
 #endif /* QEMU_PCI_BRIDGE_H */
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 3/3] hw/pci: add PCI resource reserve capability to legacy PCI bridge

[Jing Liu]

Add hint to firmware (e.g. SeaBIOS) to reserve addtional
BUS/IO/MEM/PREF resource for legacy pci-pci bridge.

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

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index b2d861d..874e618 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -46,6 +46,8 @@ struct PCIBridgeDev {
     uint32_t flags;
 
     OnOffAuto msi;
+
+    PCIResReserve res_reserve;
 };
 typedef struct PCIBridgeDev PCIBridgeDev;
 
@@ -95,6 +97,12 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
         error_free(local_err);
     }
 
+    err = pci_bridge_qemu_reserve_cap_init(dev, 0,
+                                         bridge_dev->res_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 +111,10 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
     }
     return;
 
+cap_error:
+    if (msi_present(dev)) {
+        msi_uninit(dev);
+    }
 msi_error:
     slotid_cap_cleanup(dev);
 slotid_error:
@@ -116,6 +128,8 @@ shpc_error:
 static void pci_bridge_dev_exitfn(PCIDevice *dev)
 {
     PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
+
+    pci_bridge_qemu_reserve_cap_uninit(dev);
     if (msi_present(dev)) {
         msi_uninit(dev);
     }
@@ -162,6 +176,17 @@ 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_UINT32("bus-reserve", PCIBridgeDev,
+                       res_reserve.bus_reserve, -1),
+    DEFINE_PROP_SIZE("io-reserve", PCIBridgeDev,
+                     res_reserve.io_reserve, -1),
+    DEFINE_PROP_SIZE("mem-reserve", PCIBridgeDev,
+                     res_reserve.mem_reserve, -1),
+    DEFINE_PROP_SIZE("pref32-reserve", PCIBridgeDev,
+                     res_reserve.pref32_reserve, -1),
+    DEFINE_PROP_SIZE("pref64-reserve", PCIBridgeDev,
+                     res_reserve.pref64_reserve, -1),
+
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 0/3] hw/pci: PCI resource reserve capability

[Laszlo Ersek]

Hi,

On 08/16/18 11:28, Jing Liu wrote:
> This patch serial is about PCI resource reserve capability.
> 
> First patch refactors the resource reserve fields in GenPCIERoorPort
> structure out to another new structure, called "PCIResReserve". Modify
> the parameter list of pci_bridge_qemu_reserve_cap_init().
> 
> Then we add the teardown function called pci_bridge_qemu_reserve_cap_uninit().
> 
> Last we enable the resource reserve capability for legacy PCI bridge
> so that firmware can reserve additional resources for the bridge.
> 
> Change Log:
> v2 -> v1
> * add refactoring patch
> * add teardown function
> * some other fixes
> 
> Jing Liu (3):
>   hw/pci: factor PCI reserve resources to a separate structure
>   hw/pci: add teardown function for PCI resource reserve capability
>   hw/pci: add PCI resource reserve capability to legacy PCI bridge
> 
>  hw/pci-bridge/gen_pcie_root_port.c | 32 +++++++++++++-------------
>  hw/pci-bridge/pci_bridge_dev.c     | 25 ++++++++++++++++++++
>  hw/pci/pci_bridge.c                | 47 +++++++++++++++++++++-----------------
>  include/hw/pci/pci_bridge.h        | 18 +++++++++++----
>  4 files changed, 80 insertions(+), 42 deletions(-)
> 

just some meta comments for now:

- I've added Marcel; please keep him CC'd on this set (and the SeaBIOS
counterpart, [SeaBIOS] [PATCH v2 0/3] pci: resource reserve capability
found)

- my task queue has blown up and I'm unsure when I'll get to reviewing
this set. The same applies to the SeaBIOS counterpart.

This is just to say that you should feel free to go ahead and work with
the (sub)maintainers; I'll try to get back to this as time allows, but
don't wait for me.

If further versions are necessary, I'd appreciate being CC'd on those,
just so I know what to look at when I find the time again.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v2 0/3] hw/pci: PCI resource reserve capability

[Liu, Jing2]

Hi Laszlo,
Thanks very much for your reminder.
Looking forward to comments from all!

Thanks,
Jing

On 8/17/2018 12:17 AM, Laszlo Ersek wrote:
> Hi,
> 
> On 08/16/18 11:28, Jing Liu wrote:
>> This patch serial is about PCI resource reserve capability.
>>
>> First patch refactors the resource reserve fields in GenPCIERoorPort
>> structure out to another new structure, called "PCIResReserve". Modify
>> the parameter list of pci_bridge_qemu_reserve_cap_init().
>>
>> Then we add the teardown function called pci_bridge_qemu_reserve_cap_uninit().
>>
>> Last we enable the resource reserve capability for legacy PCI bridge
>> so that firmware can reserve additional resources for the bridge.
>>
>> Change Log:
>> v2 -> v1
>> * add refactoring patch
>> * add teardown function
>> * some other fixes
>>
>> Jing Liu (3):
>>    hw/pci: factor PCI reserve resources to a separate structure
>>    hw/pci: add teardown function for PCI resource reserve capability
>>    hw/pci: add PCI resource reserve capability to legacy PCI bridge
>>
>>   hw/pci-bridge/gen_pcie_root_port.c | 32 +++++++++++++-------------
>>   hw/pci-bridge/pci_bridge_dev.c     | 25 ++++++++++++++++++++
>>   hw/pci/pci_bridge.c                | 47 +++++++++++++++++++++-----------------
>>   include/hw/pci/pci_bridge.h        | 18 +++++++++++----
>>   4 files changed, 80 insertions(+), 42 deletions(-)
>>
> 
> just some meta comments for now:
> 
> - I've added Marcel; please keep him CC'd on this set (and the SeaBIOS
> counterpart, [SeaBIOS] [PATCH v2 0/3] pci: resource reserve capability
> found)
> 
> - my task queue has blown up and I'm unsure when I'll get to reviewing
> this set. The same applies to the SeaBIOS counterpart.
> 
> This is just to say that you should feel free to go ahead and work with
> the (sub)maintainers; I'll try to get back to this as time allows, but
> don't wait for me.
> 
> If further versions are necessary, I'd appreciate being CC'd on those,
> just so I know what to look at when I find the time again.
> 
> Thanks!
> Laszlo
> 

Re: [Qemu-devel] [PATCH v2 1/3] hw/pci: factor PCI reserve resources to a separate structure

[Marcel Apfelbaum]

Hi Jing,

On 08/16/2018 12:28 PM, Jing Liu wrote:
> Factor "bus_reserve", "io_reserve", "mem_reserve", "pref32_reserve"
> and "pref64_reserve" fields of the "GenPCIERootPort" structure out
> to "PCIResReserve" structure, so that other PCI bridges can
> reuse it to add resource reserve capability.
>
> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
> ---
>   hw/pci-bridge/gen_pcie_root_port.c | 32 ++++++++++++++++----------------
>   hw/pci/pci_bridge.c                | 38 +++++++++++++++++---------------------
>   include/hw/pci/pci_bridge.h        | 17 ++++++++++++-----
>   3 files changed, 45 insertions(+), 42 deletions(-)
>
> diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
> index d117e20..babce3c 100644
> --- a/hw/pci-bridge/gen_pcie_root_port.c
> +++ b/hw/pci-bridge/gen_pcie_root_port.c
> @@ -29,12 +29,7 @@ typedef struct GenPCIERootPort {
>   
>       bool migrate_msix;
>   
> -    /* additional resources to reserve on firmware init */
> -    uint32_t bus_reserve;
> -    uint64_t io_reserve;
> -    uint64_t mem_reserve;
> -    uint64_t pref32_reserve;
> -    uint64_t pref64_reserve;
> +    PCIResReserve res_reserve;
>   } GenPCIERootPort;
>   
>   static uint8_t gen_rp_aer_vector(const PCIDevice *d)
> @@ -82,16 +77,15 @@ static void gen_rp_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    int rc = pci_bridge_qemu_reserve_cap_init(d, 0, grp->bus_reserve,
> -            grp->io_reserve, grp->mem_reserve, grp->pref32_reserve,
> -            grp->pref64_reserve, errp);
> +    int rc = pci_bridge_qemu_reserve_cap_init(d, 0,
> +                                              grp->res_reserve, errp);
>   
>       if (rc < 0) {
>           rpc->parent_class.exit(d);
>           return;
>       }
>   
> -    if (!grp->io_reserve) {
> +    if (!grp->res_reserve.io_reserve) {
>           pci_word_test_and_clear_mask(d->wmask + PCI_COMMAND,
>                                        PCI_COMMAND_IO);
>           d->wmask[PCI_IO_BASE] = 0;
> @@ -117,12 +111,18 @@ static const VMStateDescription vmstate_rp_dev = {
>   };
>   
>   static Property gen_rp_props[] = {
> -    DEFINE_PROP_BOOL("x-migrate-msix", GenPCIERootPort, migrate_msix, true),
> -    DEFINE_PROP_UINT32("bus-reserve", GenPCIERootPort, bus_reserve, -1),
> -    DEFINE_PROP_SIZE("io-reserve", GenPCIERootPort, io_reserve, -1),
> -    DEFINE_PROP_SIZE("mem-reserve", GenPCIERootPort, mem_reserve, -1),
> -    DEFINE_PROP_SIZE("pref32-reserve", GenPCIERootPort, pref32_reserve, -1),
> -    DEFINE_PROP_SIZE("pref64-reserve", GenPCIERootPort, pref64_reserve, -1),
> +    DEFINE_PROP_BOOL("x-migrate-msix", GenPCIERootPort,
> +                     migrate_msix, true),
> +    DEFINE_PROP_UINT32("bus-reserve", GenPCIERootPort,
> +                       res_reserve.bus_reserve, -1),
> +    DEFINE_PROP_SIZE("io-reserve", GenPCIERootPort,
> +                     res_reserve.io_reserve, -1),
> +    DEFINE_PROP_SIZE("mem-reserve", GenPCIERootPort,
> +                     res_reserve.mem_reserve, -1),
> +    DEFINE_PROP_SIZE("pref32-reserve", GenPCIERootPort,
> +                     res_reserve.pref32_reserve, -1),
> +    DEFINE_PROP_SIZE("pref64-reserve", GenPCIERootPort,
> +                     res_reserve.pref64_reserve, -1),
>       DEFINE_PROP_END_OF_LIST()
>   };
>   
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 40a39f5..15b055e 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -411,38 +411,34 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
>   
>   
>   int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
> -                                     uint32_t bus_reserve, uint64_t io_reserve,
> -                                     uint64_t mem_non_pref_reserve,
> -                                     uint64_t mem_pref_32_reserve,
> -                                     uint64_t mem_pref_64_reserve,
> -                                     Error **errp)
> +                                     PCIResReserve res, Error **errp)
>   {
> -    if (mem_pref_32_reserve != (uint64_t)-1 &&
> -        mem_pref_64_reserve != (uint64_t)-1) {
> +    if (res.pref32_reserve != (uint64_t)-1 &&
> +        res.pref64_reserve != (uint64_t)-1) {
>           error_setg(errp,
>                      "PCI resource reserve cap: PREF32 and PREF64 conflict");
>           return -EINVAL;
>       }
>   
> -    if (mem_non_pref_reserve != (uint64_t)-1 &&
> -        mem_non_pref_reserve >= (1ULL << 32)) {
> +    if (res.mem_reserve != (uint64_t)-1 &&
> +        res.mem_reserve >= (1ULL << 32)) {
>           error_setg(errp,
>                      "PCI resource reserve cap: mem-reserve must be less than 4G");
>           return -EINVAL;
>       }
>   
> -    if (mem_pref_32_reserve != (uint64_t)-1 &&
> -        mem_pref_32_reserve >= (1ULL << 32)) {
> +    if (res.pref32_reserve != (uint64_t)-1 &&
> +        res.pref32_reserve >= (1ULL << 32)) {
>           error_setg(errp,
>                      "PCI resource reserve cap: pref32-reserve  must be less than 4G");
>           return -EINVAL;
>       }
>   
> -    if (bus_reserve == (uint32_t)-1 &&
> -        io_reserve == (uint64_t)-1 &&
> -        mem_non_pref_reserve == (uint64_t)-1 &&
> -        mem_pref_32_reserve == (uint64_t)-1 &&
> -        mem_pref_64_reserve == (uint64_t)-1) {
> +    if (res.bus_reserve == (uint32_t)-1 &&
> +        res.io_reserve == (uint64_t)-1 &&
> +        res.mem_reserve == (uint64_t)-1 &&
> +        res.pref32_reserve == (uint64_t)-1 &&
> +        res.pref64_reserve == (uint64_t)-1) {
>           return 0;
>       }
>   
> @@ -450,11 +446,11 @@ int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
>       PCIBridgeQemuCap cap = {
>               .len = cap_len,
>               .type = REDHAT_PCI_CAP_RESOURCE_RESERVE,
> -            .bus_res = bus_reserve,
> -            .io = io_reserve,
> -            .mem = mem_non_pref_reserve,
> -            .mem_pref_32 = mem_pref_32_reserve,
> -            .mem_pref_64 = mem_pref_64_reserve
> +            .bus_res = res.bus_reserve,
> +            .io = res.io_reserve,
> +            .mem = res.mem_reserve,
> +            .mem_pref_32 = res.pref32_reserve,
> +            .mem_pref_64 = res.pref64_reserve
>       };
>   
>       int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index 0347da5..6186a32 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -133,11 +133,18 @@ typedef struct PCIBridgeQemuCap {
>   
>   #define REDHAT_PCI_CAP_RESOURCE_RESERVE 1
>   
> +/*
> + * additional resources to reserve on firmware init
> + */
> +typedef struct PCIResReserve {
> +    uint32_t bus_reserve;
> +    uint64_t io_reserve;
> +    uint64_t mem_reserve;

The patch looks good to me, I noticed you renamed
'mem_no_pref_reserve' to 'mem reserve'.
I remember we had a lot of discussions about the  naming, so they would
be clear and consistent with the firmware counterpart.

Please add a least a comment in the PCIResReserve.
Also, since you encapsulated the fields into a new struct,
you could remove the  "_reserve" suffix so we
remain with clear "bus", "io", "mem" ...

Thanks,
Marcel

> +    uint64_t pref32_reserve;
> +    uint64_t pref64_reserve;
> +} PCIResReserve;
> +
>   int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
> -                              uint32_t bus_reserve, uint64_t io_reserve,
> -                              uint64_t mem_non_pref_reserve,
> -                              uint64_t mem_pref_32_reserve,
> -                              uint64_t mem_pref_64_reserve,
> -                              Error **errp);
> +                               PCIResReserve res_reserve, Error **errp);
>   
>   #endif /* QEMU_PCI_BRIDGE_H */

Re: [Qemu-devel] [PATCH v2 2/3] hw/pci: add teardown function for PCI resource reserve capability

[Marcel Apfelbaum]

Hi Jing,

On 08/16/2018 12:28 PM, Jing Liu wrote:
> Clean up the PCI config space of resource reserve capability.
>
> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
> ---
>   hw/pci/pci_bridge.c         | 9 +++++++++
>   include/hw/pci/pci_bridge.h | 1 +
>   2 files changed, 10 insertions(+)
>
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 15b055e..dbcee90 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -465,6 +465,15 @@ int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
>       return 0;
>   }
>   
> +void pci_bridge_qemu_reserve_cap_uninit(PCIDevice *dev)
> +{
> +    uint8_t pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> +
> +    pci_del_capability(dev, PCI_CAP_ID_VNDR, sizeof(PCIBridgeQemuCap));

I think that you only need to call pci_del_capability,

> +    memset(dev->config + pos + PCI_CAP_FLAGS, 0,
> +           sizeof(PCIBridgeQemuCap) - PCI_CAP_FLAGS);
> +}

... no need for the above line. The reason is pci_del_capability
will "unlink" the capability, and even if the data remains in
the configuration space array, it will not be used.

Do you agree? If yes, just call pci_del_capability and you don't need
this patch.


Thanks,
Marcel

> +
>   static const TypeInfo pci_bridge_type_info = {
>       .name = TYPE_PCI_BRIDGE,
>       .parent = TYPE_PCI_DEVICE,
> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index 6186a32..b1e25ad 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -147,4 +147,5 @@ typedef struct PCIResReserve {
>   int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
>                                  PCIResReserve res_reserve, Error **errp);
>   
> +void pci_bridge_qemu_reserve_cap_uninit(PCIDevice *dev);
>   #endif /* QEMU_PCI_BRIDGE_H */

Re: [Qemu-devel] [PATCH v2 3/3] hw/pci: add PCI resource reserve capability to legacy PCI bridge

[Marcel Apfelbaum]

Hi Jing,

On 08/16/2018 12:28 PM, Jing Liu wrote:
> Add hint to firmware (e.g. SeaBIOS) to reserve addtional
> BUS/IO/MEM/PREF resource for legacy pci-pci bridge.
>
> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
> ---
>   hw/pci-bridge/pci_bridge_dev.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
>
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index b2d861d..874e618 100644Hi
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -46,6 +46,8 @@ struct PCIBridgeDev {
>       uint32_t flags;
>   
>       OnOffAuto msi;
> +
> +    PCIResReserve res_reserve;
>   };
>   typedef struct PCIBridgeDev PCIBridgeDev;
>   
> @@ -95,6 +97,12 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>           error_free(local_err);
>       }
>   
> +    err = pci_bridge_qemu_reserve_cap_init(dev, 0,
> +                                         bridge_dev->res_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 +111,10 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>       }
>       return;
>   
> +cap_error:
> +    if (msi_present(dev)) {
> +        msi_uninit(dev);

You were right. Since msi_uninit checks msi_present
inside, you don;t need this check, sorry for misleading you.

> +    }
>   msi_error:
>       slotid_cap_cleanup(dev);
>   slotid_error:
> @@ -116,6 +128,8 @@ shpc_error:
>   static void pci_bridge_dev_exitfn(PCIDevice *dev)
>   {
>       PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
> +
> +    pci_bridge_qemu_reserve_cap_uninit(dev);

Here you should have  only the "delete_cap" function.

>       if (msi_present(dev)) {
>           msi_uninit(dev);
>       }
> @@ -162,6 +176,17 @@ 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_UINT32("bus-reserve", PCIBridgeDev,
> +                       res_reserve.bus_reserve, -1),
> +    DEFINE_PROP_SIZE("io-reserve", PCIBridgeDev,
> +                     res_reserve.io_reserve, -1),
> +    DEFINE_PROP_SIZE("mem-reserve", PCIBridgeDev,
> +                     res_reserve.mem_reserve, -1),
> +    DEFINE_PROP_SIZE("pref32-reserve", PCIBridgeDev,
> +                     res_reserve.pref32_reserve, -1),
> +    DEFINE_PROP_SIZE("pref64-reserve", PCIBridgeDev,
> +                     res_reserve.pref64_reserve, -1),
> +
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   

Thanks,
Marcel

Re: [Qemu-devel] [PATCH v2 0/3] hw/pci: PCI resource reserve capability

[Marcel Apfelbaum]

Hi Jing,

On 08/16/2018 12:28 PM, Jing Liu wrote:
> This patch serial is about PCI resource reserve capability.
>
> First patch refactors the resource reserve fields in GenPCIERoorPort
> structure out to another new structure, called "PCIResReserve". Modify
> the parameter list of pci_bridge_qemu_reserve_cap_init().
>
> Then we add the teardown function called pci_bridge_qemu_reserve_cap_uninit().
>
> Last we enable the resource reserve capability for legacy PCI bridge
> so that firmware can reserve additional resources for the bridge.

The series looks good to me, please see some minor comments
in the patches.

Can you please point me to the SeaBIOS / OVMF counterpart?

Thanks,
Marcel


> Change Log:
> v2 -> v1
> * add refactoring patch
> * add teardown function
> * some other fixes
>
> Jing Liu (3):
>    hw/pci: factor PCI reserve resources to a separate structure
>    hw/pci: add teardown function for PCI resource reserve capability
>    hw/pci: add PCI resource reserve capability to legacy PCI bridge
>
>   hw/pci-bridge/gen_pcie_root_port.c | 32 +++++++++++++-------------
>   hw/pci-bridge/pci_bridge_dev.c     | 25 ++++++++++++++++++++
>   hw/pci/pci_bridge.c                | 47 +++++++++++++++++++++-----------------
>   include/hw/pci/pci_bridge.h        | 18 +++++++++++----
>   4 files changed, 80 insertions(+), 42 deletions(-)
>

Re: [Qemu-devel] [PATCH v2 0/3] hw/pci: PCI resource reserve capability

[Liu, Jing2]

Hi Marcel,

On 8/18/2018 12:18 AM, Marcel Apfelbaum wrote:
> Hi Jing,
> 
> On 08/16/2018 12:28 PM, Jing Liu wrote:
>> This patch serial is about PCI resource reserve capability.
>>
>> First patch refactors the resource reserve fields in GenPCIERoorPort
>> structure out to another new structure, called "PCIResReserve". Modify
>> the parameter list of pci_bridge_qemu_reserve_cap_init().
>>
>> Then we add the teardown function called 
>> pci_bridge_qemu_reserve_cap_uninit().
>>
>> Last we enable the resource reserve capability for legacy PCI bridge
>> so that firmware can reserve additional resources for the bridge.
> 
> The series looks good to me, please see some minor comments
> in the patches.
> 
Thanks very much for your reviewing. I will improve the
codes and send new version then.

> Can you please point me to the SeaBIOS / OVMF counterpart?
> 
Sure. SeaBIOS patch serial is here:
https://patchew.org/Seabios/1534386737-8131-1-git-send-email-jing2.liu@linux.intel.com/

Thanks,
Jing

> Thanks,
> Marcel
> 
> 
>> Change Log:
>> v2 -> v1
>> * add refactoring patch
>> * add teardown function
>> * some other fixes
>>
>> Jing Liu (3):
>>    hw/pci: factor PCI reserve resources to a separate structure
>>    hw/pci: add teardown function for PCI resource reserve capability
>>    hw/pci: add PCI resource reserve capability to legacy PCI bridge
>>
>>   hw/pci-bridge/gen_pcie_root_port.c | 32 +++++++++++++-------------
>>   hw/pci-bridge/pci_bridge_dev.c     | 25 ++++++++++++++++++++
>>   hw/pci/pci_bridge.c                | 47 
>> +++++++++++++++++++++-----------------
>>   include/hw/pci/pci_bridge.h        | 18 +++++++++++----
>>   4 files changed, 80 insertions(+), 42 deletions(-)
>>
> 

Re: [Qemu-devel] [PATCH v2 2/3] hw/pci: add teardown function for PCI resource reserve capability

[Liu, Jing2]

Hi Marcel,

On 8/18/2018 12:10 AM, Marcel Apfelbaum wrote:
> Hi Jing,
> 
> On 08/16/2018 12:28 PM, Jing Liu wrote:
>> Clean up the PCI config space of resource reserve capability.
>>
>> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
>> ---
>>   hw/pci/pci_bridge.c         | 9 +++++++++
>>   include/hw/pci/pci_bridge.h | 1 +
>>   2 files changed, 10 insertions(+)
>>
>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>> index 15b055e..dbcee90 100644
>> --- a/hw/pci/pci_bridge.c
>> +++ b/hw/pci/pci_bridge.c
>> @@ -465,6 +465,15 @@ int pci_bridge_qemu_reserve_cap_init(PCIDevice 
>> *dev, int cap_offset,
>>       return 0;
>>   }
>> +void pci_bridge_qemu_reserve_cap_uninit(PCIDevice *dev)
>> +{
>> +    uint8_t pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
>> +
>> +    pci_del_capability(dev, PCI_CAP_ID_VNDR, sizeof(PCIBridgeQemuCap));
> 
> I think that you only need to call pci_del_capability,
> 
>> +    memset(dev->config + pos + PCI_CAP_FLAGS, 0,
>> +           sizeof(PCIBridgeQemuCap) - PCI_CAP_FLAGS);
>> +}
> 
> ... no need for the above line. The reason is pci_del_capability
> will "unlink" the capability, and even if the data remains in
> the configuration space array, it will not be used.
> 
I think I got it: pci_del_capability "unlink" by set the tag
pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
so that pdev->config will not be used, right?

> Do you agree? If yes, just call pci_del_capability and you don't need
> this patch.
> 
Yup, I agree with you. And let me remove this patch in next version.

Thanks,
Jing

> 
> Thanks,
> Marcel
> 

[...]

Re: [Qemu-devel] [PATCH v2 1/3] hw/pci: factor PCI reserve resources to a separate structure

[Liu, Jing2]

Hi Marcel,

On 8/17/2018 11:49 PM, Marcel Apfelbaum wrote:
> Hi Jing,
> 
[...]
>> +/*
>> + * additional resources to reserve on firmware init
>> + */
>> +typedef struct PCIResReserve {
>> +    uint32_t bus_reserve;
>> +    uint64_t io_reserve;
>> +    uint64_t mem_reserve;
> 
> The patch looks good to me, I noticed you renamed
> 'mem_no_pref_reserve' to 'mem reserve'.
> I remember we had a lot of discussions about the  naming, so they would
> be clear and consistent with the firmware counterpart.
> 
OK, will change 'mem_no_pref_reserve' to 'mem_no_pref' and also for
others.
> Please add a least a comment in the PCIResReserve.
Will add a comment to the structure definition, and where it's called.

> Also, since you encapsulated the fields into a new struct,
> you could remove the  "_reserve" suffix so we
> remain with clear "bus", "io", "mem" ...
> 
Got it.

Thanks,
Jing

> Thanks,
> Marcel
> 
>> +    uint64_t pref32_reserve;
>> +    uint64_t pref64_reserve;
>> +} PCIResReserve;
>> +
>>   int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
>> -                              uint32_t bus_reserve, uint64_t io_reserve,
>> -                              uint64_t mem_non_pref_reserve,
>> -                              uint64_t mem_pref_32_reserve,
>> -                              uint64_t mem_pref_64_reserve,
>> -                              Error **errp);
>> +                               PCIResReserve res_reserve, Error **errp);
>>   #endif /* QEMU_PCI_BRIDGE_H */
> 

Re: [Qemu-devel] [PATCH v2 2/3] hw/pci: add teardown function for PCI resource reserve capability

[Marcel Apfelbaum]

Hi Jing,

On 08/20/2018 05:58 AM, Liu, Jing2 wrote:
> Hi Marcel,
>
> On 8/18/2018 12:10 AM, Marcel Apfelbaum wrote:
>> Hi Jing,
>>
>> On 08/16/2018 12:28 PM, Jing Liu wrote:
>>> Clean up the PCI config space of resource reserve capability.
>>>
>>> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
>>> ---
>>>   hw/pci/pci_bridge.c         | 9 +++++++++
>>>   include/hw/pci/pci_bridge.h | 1 +
>>>   2 files changed, 10 insertions(+)
>>>
>>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>>> index 15b055e..dbcee90 100644
>>> --- a/hw/pci/pci_bridge.c
>>> +++ b/hw/pci/pci_bridge.c
>>> @@ -465,6 +465,15 @@ int pci_bridge_qemu_reserve_cap_init(PCIDevice 
>>> *dev, int cap_offset,
>>>       return 0;
>>>   }
>>> +void pci_bridge_qemu_reserve_cap_uninit(PCIDevice *dev)
>>> +{
>>> +    uint8_t pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
>>> +
>>> +    pci_del_capability(dev, PCI_CAP_ID_VNDR, 
>>> sizeof(PCIBridgeQemuCap));
>>
>> I think that you only need to call pci_del_capability,
>>
>>> +    memset(dev->config + pos + PCI_CAP_FLAGS, 0,
>>> +           sizeof(PCIBridgeQemuCap) - PCI_CAP_FLAGS);
>>> +}
>>
>> ... no need for the above line. The reason is pci_del_capability
>> will "unlink" the capability, and even if the data remains in
>> the configuration space array, it will not be used.
>>
> I think I got it: pci_del_capability "unlink" by set the tag
> pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
> so that pdev->config will not be used, right?

If is the latest capability in the list, yes.
Otherwise it will simply link 'prev' with 'next' using config array offsets.

Thanks,
Marcel

>
>> Do you agree? If yes, just call pci_del_capability and you don't need
>> this patch.
>>
> Yup, I agree with you. And let me remove this patch in next version.
>
> Thanks,
> Jing
>
>>
>> Thanks,
>> Marcel
>>
>
> [...]

Re: [Qemu-devel] [PATCH v2 2/3] hw/pci: add teardown function for PCI resource reserve capability

[Liu, Jing2]

Hi Marcel,

On 8/20/2018 9:38 PM, Marcel Apfelbaum wrote:
> Hi Jing,
> 
> On 08/20/2018 05:58 AM, Liu, Jing2 wrote:
>> Hi Marcel,
>>
>> On 8/18/2018 12:10 AM, Marcel Apfelbaum wrote:
>>> Hi Jing,
>>>
>>> On 08/16/2018 12:28 PM, Jing Liu wrote:
>>>> Clean up the PCI config space of resource reserve capability.
>>>>
>>>> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
>>>> ---
>>>>   hw/pci/pci_bridge.c         | 9 +++++++++
>>>>   include/hw/pci/pci_bridge.h | 1 +
>>>>   2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>>>> index 15b055e..dbcee90 100644
>>>> --- a/hw/pci/pci_bridge.c
>>>> +++ b/hw/pci/pci_bridge.c
>>>> @@ -465,6 +465,15 @@ int pci_bridge_qemu_reserve_cap_init(PCIDevice 
>>>> *dev, int cap_offset,
>>>>       return 0;
>>>>   }
>>>> +void pci_bridge_qemu_reserve_cap_uninit(PCIDevice *dev)
>>>> +{
>>>> +    uint8_t pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
>>>> +
>>>> +    pci_del_capability(dev, PCI_CAP_ID_VNDR, 
>>>> sizeof(PCIBridgeQemuCap));
>>>
>>> I think that you only need to call pci_del_capability,
>>>
>>>> +    memset(dev->config + pos + PCI_CAP_FLAGS, 0,
>>>> +           sizeof(PCIBridgeQemuCap) - PCI_CAP_FLAGS);
>>>> +}
>>>
>>> ... no need for the above line. The reason is pci_del_capability
>>> will "unlink" the capability, and even if the data remains in
>>> the configuration space array, it will not be used.
>>>
>> I think I got it: pci_del_capability "unlink" by set the tag
>> pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
>> so that pdev->config will not be used, right?
> 
> If is the latest capability in the list, yes.
> Otherwise it will simply link 'prev' with 'next' using config array 
> offsets.
I got it! Thanks very much for the details!

Jing

> 
> Thanks,
> Marcel
> 
>>
>>> Do you agree? If yes, just call pci_del_capability and you don't need
>>> this patch.
>>>
>> Yup, I agree with you. And let me remove this patch in next version.
>>
>> Thanks,
>> Jing
>>
>>>
>>> Thanks,
>>> Marcel
>>>
>>
>> [...]
>