[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 01/10] vpci: move lock

Hi Roger,

On 29/06/18 14:27, Roger Pau Monné wrote:
On Fri, Jun 29, 2018 at 11:19:57AM +0100, Wei Liu wrote:
On Wed, Jun 20, 2018 at 04:42:25PM +0200, Roger Pau Monne wrote:
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 0ec4c082a6..9d5607d5f8 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -131,11 +131,12 @@ bool vpci_process_pending(struct vcpu *v)
          if ( rc == -ERESTART )
              return true;
- spin_lock(&v->vpci.pdev->vpci->lock);
-        /* Disable memory decoding unconditionally on failure. */
-        modify_decoding(v->vpci.pdev, !rc && v->vpci.map,
-                        !rc && v->vpci.rom_only);
-        spin_unlock(&v->vpci.pdev->vpci->lock);
+        spin_lock(&v->vpci.pdev->vpci_lock);
+        if ( v->vpci.pdev->vpci )

The purpose of this check is to fix a latent bug in the original code?

Previous code didn't support removing devices, so it's more about
making it capable of supporting vpci device removal.

+            /* Disable memory decoding unconditionally on failure. */
+            modify_decoding(v->vpci.pdev, !rc && v->vpci.map,
+                            !rc && v->vpci.rom_only);
+        spin_unlock(&v->vpci.pdev->vpci_lock);
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 82607bdb9a..7d52bcf8d0 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -35,9 +35,8 @@ extern vpci_register_init_t *const __start_vpci_array[];
  extern vpci_register_init_t *const __end_vpci_array[];
  #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
-void vpci_remove_device(struct pci_dev *pdev)
+static void vpci_remove_device_locked(struct pci_dev *pdev)
-    spin_lock(&pdev->vpci->lock);


Er, yes. But keep in mind that this is going to return `true` even if
vpci_lock is locked by another CPU. Asserting lock ownership only
works correctly with recursive locks.

While I agree with your statement, the point of the ASSERT is to catch misuse, there are a fair amount of chance to have no contention on the lock (something would need to be done if it was the case anyway).

So in general, I still recommend developer to use ASSERT(spin_is_lock(...)) in any function relying on a lock taken. And who knows, maybe some day we would have a spin lock helper checking the CPU making the ASSERT more reliable.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.