Re: [PATCH 01/10] vpci: move lock
Hi, Roger, Jan!
I think the below work will help with solving issues brought up by [1].
So, after thinking a bit more I concluded that indeed we do not actually
need a per-domain vPCI lock, but instead we just want to protect pdev->vpci
which is done in this patch.
At the moment I see four entities which may run concurrently and touch vpci:
1. hypercalls PHYSDEVOP_pci_device_{add|remove} - for Dom0 only
2. hypercalls XEN_DOMCTL_{de|}assign_device - any domain
3. MMIO trap handlers vpci_{read|write}
4. vpci_process_pending
From the above #1 will update pdev->vpci and #4 may remove pdev->vpci
with vpci_remove_device on error path for Dom0. #2 and #3 seem to be
readers only. So, it is possible to improve this patch to not only take
pdev->vpci->lock out of struct vpci, but also to convert it into RW lock.
Hope this is what is needed in context of vpci and locking.
Thank you,
Oleksandr
On 20.06.18 17:42, Roger Pau Monne wrote:
> To the outside of the vpci struct. This way the lock can be used to
> check whether vpci is present, and removal can be performed while
> holding the lock, in order to make sure there are no accesses to the
> contents of the vpci struct. Previously removal could race with
> vpci_read for example, since the log was dropped prior to freeing
> pdev->vpci.
>
> Signed-off-by: Roger Pau Monné
> ---
> Cc: Andrew Cooper
> Cc: George Dunlap
> Cc: Ian Jackson
> Cc: Jan Beulich
> Cc: Julien Grall
> Cc: Konrad Rzeszutek Wilk
> Cc: Stefano Stabellini
> Cc: Tim Deegan
> Cc: Wei Liu
> ---
> tools/tests/vpci/emul.h | 5 ++--
> tools/tests/vpci/main.c | 4 +--
> xen/arch/x86/hvm/vmsi.c | 8 +++---
> xen/drivers/passthrough/pci.c | 1 +
> xen/drivers/vpci/header.c | 19 +
> xen/drivers/vpci/msi.c| 11 +---
> xen/drivers/vpci/msix.c | 8 +++---
> xen/drivers/vpci/vpci.c | 51 ++-
> xen/include/xen/pci.h | 1 +
> xen/include/xen/vpci.h| 3 +--
> 10 files changed, 70 insertions(+), 41 deletions(-)
>
> diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
> index 5d47544bf7..d344ef71c9 100644
> --- a/tools/tests/vpci/emul.h
> +++ b/tools/tests/vpci/emul.h
> @@ -44,6 +44,7 @@ struct domain {
> };
>
> struct pci_dev {
> +bool vpci_lock;
> struct vpci *vpci;
> };
>
> @@ -53,10 +54,8 @@ struct vcpu
> };
>
> extern const struct vcpu *current;
> -extern const struct pci_dev test_pdev;
> +extern struct pci_dev test_pdev;
>
> -typedef bool spinlock_t;
> -#define spin_lock_init(l) (*(l) = false)
> #define spin_lock(l) (*(l) = true)
> #define spin_unlock(l) (*(l) = false)
>
> diff --git a/tools/tests/vpci/main.c b/tools/tests/vpci/main.c
> index b9a0a6006b..26c95b08b6 100644
> --- a/tools/tests/vpci/main.c
> +++ b/tools/tests/vpci/main.c
> @@ -23,7 +23,8 @@ static struct vpci vpci;
>
> const static struct domain d;
>
> -const struct pci_dev test_pdev = {
> +struct pci_dev test_pdev = {
> +.vpci_lock = false,
> .vpci = &vpci,
> };
>
> @@ -158,7 +159,6 @@ main(int argc, char **argv)
> int rc;
>
> INIT_LIST_HEAD(&vpci.handlers);
> -spin_lock_init(&vpci.lock);
>
> VPCI_ADD_REG(vpci_read32, vpci_write32, 0, 4, r0);
> VPCI_READ_CHECK(0, 4, r0);
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index 3001d5c488..94550cb8c4 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -893,14 +893,14 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
> {
> struct pci_dev *pdev = msix->pdev;
>
> -spin_unlock(&msix->pdev->vpci->lock);
> +spin_unlock(&msix->pdev->vpci_lock);
> process_pending_softirqs();
> /* NB: we assume that pdev cannot go away for an alive domain.
> */
> -if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
> +if ( !spin_trylock(&pdev->vpci_lock) )
> return -EBUSY;
> -if ( pdev->vpci->msix != msix )
> +if ( !pdev->vpci || pdev->vpci->msix != msix )
> {
> -spin_unlock(&pdev->vpci->lock);
> +spin_unlock(&pdev->vpci_lock);
> return -EAGAIN;
> }
> }
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index c4890a4295..a5d59b83b7 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -315,6 +315,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg,
> u8 bus, u8 devfn)
> *((u8*) &pdev->devfn) = devfn;
> pdev->domain = NULL;
> INIT_LIST_HEAD(&pdev->msi_list);
> +spin_lock_init(&pdev->vpci_lock);
>
> if ( pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn),
> PCI_FUNC(devfn),
>PCI_CAP_ID_MSIX) )
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 0ec4
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);
ASSERT(spin_is_locked(&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.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 01/10] vpci: move lock
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);
>
> ASSERT(spin_is_locked(&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.
> > @@ -383,7 +391,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
> > unsigned int size)
> >
> > data = merge_result(data, tmp_data, size - data_offset,
> > data_offset);
> > }
> > -spin_unlock(&pdev->vpci->lock);
> > +spin_unlock(&pdev->vpci_lock);
>
> I think the critical section in this function and the write function can
> shrink a bit. Reading from / writing to hardware shouldn't need to be
> protected by vpci_lock.
There's no further usage of contents of the vpci struct, so I guess I
can move the unlock a little bit up. The same applies to the write
counterpart.
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 01/10] vpci: move lock
On Wed, Jun 20, 2018 at 04:42:25PM +0200, Roger Pau Monne wrote:
> To the outside of the vpci struct. This way the lock can be used to
> check whether vpci is present, and removal can be performed while
> holding the lock, in order to make sure there are no accesses to the
> contents of the vpci struct. Previously removal could race with
> vpci_read for example, since the log was dropped prior to freeing
log -> lock.
> pdev->vpci.
>
> Signed-off-by: Roger Pau Monné
[...]
> 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?
> +/* 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);
ASSERT(spin_is_locked(&pdev->vpci_lock));
> while ( !list_empty(&pdev->vpci->handlers) )
> {
> struct vpci_register *r = list_first_entry(&pdev->vpci->handlers,
> @@ -47,13 +46,20 @@ void vpci_remove_device(struct pci_dev *pdev)
> list_del(&r->node);
> xfree(r);
> }
> -spin_unlock(&pdev->vpci->lock);
> xfree(pdev->vpci->msix);
> xfree(pdev->vpci->msi);
> xfree(pdev->vpci);
> pdev->vpci = NULL;
> }
>
> +void vpci_remove_device(struct pci_dev *pdev)
> +{
> +spin_lock(&pdev->vpci_lock);
> +vpci_remove_device_locked(pdev);
> +spin_unlock(&pdev->vpci_lock);
> +}
> +
> +
Too many blank lines.
> int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
> {
> unsigned int i;
> @@ -62,12 +68,15 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
> if ( !has_vpci(pdev->domain) )
> return 0;
>
> +spin_lock(&pdev->vpci_lock);
> pdev->vpci = xzalloc(struct vpci);
> if ( !pdev->vpci )
> +{
> +spin_unlock(&pdev->vpci_lock);
> return -ENOMEM;
> +}
>
> INIT_LIST_HEAD(&pdev->vpci->handlers);
> -spin_lock_init(&pdev->vpci->lock);
>
> for ( i = 0; i < NUM_VPCI_INIT; i++ )
> {
[...]
> @@ -77,7 +86,8 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
> @@ -315,7 +318,7 @@ static uint32_t merge_result(uint32_t data, uint32_t new,
> unsigned int size,
> uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
> {
> const struct domain *d = current->domain;
> -const struct pci_dev *pdev;
> +struct pci_dev *pdev;
> const struct vpci_register *r;
> unsigned int data_offset = 0;
> uint32_t data = ~(uint32_t)0;
> @@ -331,7 +334,12 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
> unsigned int size)
> if ( !pdev )
> return vpci_read_hw(sbdf, reg, size);
>
> -spin_lock(&pdev->vpci->lock);
> +spin_lock(&pdev->vpci_lock);
> +if ( !pdev->vpci )
> +{
> +spin_unlock(&pdev->vpci_lock);
> +return vpci_read_hw(sbdf, reg, size);
> +}
>
> /* Read from the hardware or the emulated register handlers. */
> list_for_each_entry ( r, &pdev->vpci->handlers, node )
> @@ -383,7 +391,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
> unsigned int size)
>
> data = merge_result(data, tmp_data, size - data_offset, data_offset);
> }
> -spin_unlock(&pdev->vpci->lock);
> +spin_unlock(&pdev->vpci_lock);
I think the critical section in this function and the write function can
shrink a bit. Reading from / writing to hardware shouldn't need to be
protected by vpci_lock.
Wei.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 01/10] vpci: move lock
To the outside of the vpci struct. This way the lock can be used to
check whether vpci is present, and removal can be performed while
holding the lock, in order to make sure there are no accesses to the
contents of the vpci struct. Previously removal could race with
vpci_read for example, since the log was dropped prior to freeing
pdev->vpci.
Signed-off-by: Roger Pau Monné
---
Cc: Andrew Cooper
Cc: George Dunlap
Cc: Ian Jackson
Cc: Jan Beulich
Cc: Julien Grall
Cc: Konrad Rzeszutek Wilk
Cc: Stefano Stabellini
Cc: Tim Deegan
Cc: Wei Liu
---
tools/tests/vpci/emul.h | 5 ++--
tools/tests/vpci/main.c | 4 +--
xen/arch/x86/hvm/vmsi.c | 8 +++---
xen/drivers/passthrough/pci.c | 1 +
xen/drivers/vpci/header.c | 19 +
xen/drivers/vpci/msi.c| 11 +---
xen/drivers/vpci/msix.c | 8 +++---
xen/drivers/vpci/vpci.c | 51 ++-
xen/include/xen/pci.h | 1 +
xen/include/xen/vpci.h| 3 +--
10 files changed, 70 insertions(+), 41 deletions(-)
diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
index 5d47544bf7..d344ef71c9 100644
--- a/tools/tests/vpci/emul.h
+++ b/tools/tests/vpci/emul.h
@@ -44,6 +44,7 @@ struct domain {
};
struct pci_dev {
+bool vpci_lock;
struct vpci *vpci;
};
@@ -53,10 +54,8 @@ struct vcpu
};
extern const struct vcpu *current;
-extern const struct pci_dev test_pdev;
+extern struct pci_dev test_pdev;
-typedef bool spinlock_t;
-#define spin_lock_init(l) (*(l) = false)
#define spin_lock(l) (*(l) = true)
#define spin_unlock(l) (*(l) = false)
diff --git a/tools/tests/vpci/main.c b/tools/tests/vpci/main.c
index b9a0a6006b..26c95b08b6 100644
--- a/tools/tests/vpci/main.c
+++ b/tools/tests/vpci/main.c
@@ -23,7 +23,8 @@ static struct vpci vpci;
const static struct domain d;
-const struct pci_dev test_pdev = {
+struct pci_dev test_pdev = {
+.vpci_lock = false,
.vpci = &vpci,
};
@@ -158,7 +159,6 @@ main(int argc, char **argv)
int rc;
INIT_LIST_HEAD(&vpci.handlers);
-spin_lock_init(&vpci.lock);
VPCI_ADD_REG(vpci_read32, vpci_write32, 0, 4, r0);
VPCI_READ_CHECK(0, 4, r0);
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 3001d5c488..94550cb8c4 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -893,14 +893,14 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
{
struct pci_dev *pdev = msix->pdev;
-spin_unlock(&msix->pdev->vpci->lock);
+spin_unlock(&msix->pdev->vpci_lock);
process_pending_softirqs();
/* NB: we assume that pdev cannot go away for an alive domain. */
-if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
+if ( !spin_trylock(&pdev->vpci_lock) )
return -EBUSY;
-if ( pdev->vpci->msix != msix )
+if ( !pdev->vpci || pdev->vpci->msix != msix )
{
-spin_unlock(&pdev->vpci->lock);
+spin_unlock(&pdev->vpci_lock);
return -EAGAIN;
}
}
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index c4890a4295..a5d59b83b7 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -315,6 +315,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8
bus, u8 devfn)
*((u8*) &pdev->devfn) = devfn;
pdev->domain = NULL;
INIT_LIST_HEAD(&pdev->msi_list);
+spin_lock_init(&pdev->vpci_lock);
if ( pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
PCI_CAP_ID_MSIX) )
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 )
+/* 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);
rangeset_destroy(v->vpci.mem);
v->vpci.mem = NULL;
@@ -267,6 +268,12 @@ static int modify_bars(const struct pci_dev *pdev, bool
map, bool rom_only)
continue;
}
+spin_lock(&tmp->vpci_lock);
+if ( !tmp->vpci )
+{
+spin_unlock(&tmp->vpci_lock);
+continue;
+}
for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i+
