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

2018-08-20 Thread 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 
---
  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



[...]

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

2018-08-20 Thread 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 
---
  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

2018-08-19 Thread 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 
---
  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 2/3] hw/pci: add teardown function for PCI resource reserve capability

2018-08-17 Thread 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 
---
  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 */

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

2018-08-16 Thread Jing Liu 
Clean up the PCI config space of resource reserve capability.

Signed-off-by: Jing Liu 
---
 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