|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH v2 01/11] 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 lock was dropped prior to freeing
pdev->vpci.
Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
---
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Tim Deegan <tim@xxxxxxx>
Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
---
Changes since v1:
- Assert that vpci_lock is locked in vpci_remove_device_locked.
- Remove double newline.
- Shrink critical section in vpci_{read/write}.
---
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 | 54 +++++++++++++++++++++++------------
xen/include/xen/pci.h | 1 +
xen/include/xen/vpci.h | 3 +-
10 files changed, 72 insertions(+), 42 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++ )
{
const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
@@ -285,12 +292,14 @@ static int modify_bars(const struct pci_dev *pdev, bool
map, bool rom_only)
rc = rangeset_remove_range(mem, start, end);
if ( rc )
{
+ spin_unlock(&tmp->vpci_lock);
printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: %d\n",
start, end, rc);
rangeset_destroy(mem);
return rc;
}
}
+ spin_unlock(&tmp->vpci_lock);
}
ASSERT(dev);
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index 8f15ad7bf2..108e871d1c 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -275,7 +275,7 @@ void vpci_dump_msi(void)
rcu_read_lock(&domlist_read_lock);
for_each_domain ( d )
{
- const struct pci_dev *pdev;
+ struct pci_dev *pdev;
if ( !has_vpci(d) )
continue;
@@ -287,8 +287,13 @@ void vpci_dump_msi(void)
const struct vpci_msi *msi;
const struct vpci_msix *msix;
- if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
+ if ( !spin_trylock(&pdev->vpci_lock) )
continue;
+ if ( !pdev->vpci )
+ {
+ spin_unlock(&pdev->vpci_lock);
+ continue;
+ }
msi = pdev->vpci->msi;
if ( msi && msi->enabled )
@@ -330,7 +335,7 @@ void vpci_dump_msi(void)
}
}
- spin_unlock(&pdev->vpci->lock);
+ spin_unlock(&pdev->vpci_lock);
process_pending_softirqs();
}
}
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index bcf63256f6..e28096329d 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -236,7 +236,7 @@ static int msix_read(struct vcpu *v, unsigned long addr,
unsigned int len,
return X86EMUL_OKAY;
}
- spin_lock(&msix->pdev->vpci->lock);
+ spin_lock(&msix->pdev->vpci_lock);
entry = get_entry(msix, addr);
offset = addr & (PCI_MSIX_ENTRY_SIZE - 1);
@@ -265,7 +265,7 @@ static int msix_read(struct vcpu *v, unsigned long addr,
unsigned int len,
ASSERT_UNREACHABLE();
break;
}
- spin_unlock(&msix->pdev->vpci->lock);
+ spin_unlock(&msix->pdev->vpci_lock);
return X86EMUL_OKAY;
}
@@ -308,7 +308,7 @@ static int msix_write(struct vcpu *v, unsigned long addr,
unsigned int len,
return X86EMUL_OKAY;
}
- spin_lock(&msix->pdev->vpci->lock);
+ spin_lock(&msix->pdev->vpci_lock);
entry = get_entry(msix, addr);
offset = addr & (PCI_MSIX_ENTRY_SIZE - 1);
@@ -384,7 +384,7 @@ static int msix_write(struct vcpu *v, unsigned long addr,
unsigned int len,
ASSERT_UNREACHABLE();
break;
}
- spin_unlock(&msix->pdev->vpci->lock);
+ spin_unlock(&msix->pdev->vpci_lock);
return X86EMUL_OKAY;
}
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 82607bdb9a..0e6ff09c9a 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -35,9 +35,10 @@ 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 +48,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);
+ if ( pdev->vpci )
+ vpci_remove_device_locked(pdev);
+ spin_unlock(&pdev->vpci_lock);
+}
+
int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
{
unsigned int i;
@@ -62,12 +70,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 +88,8 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
}
if ( rc )
- vpci_remove_device(pdev);
+ vpci_remove_device_locked(pdev);
+ spin_unlock(&pdev->vpci_lock);
return rc;
}
@@ -148,8 +160,6 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t
*read_handler,
r->offset = offset;
r->private = data;
- spin_lock(&vpci->lock);
-
/* The list of handlers must be kept sorted at all times. */
list_for_each ( prev, &vpci->handlers )
{
@@ -161,14 +171,12 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t
*read_handler,
break;
if ( cmp == 0 )
{
- spin_unlock(&vpci->lock);
xfree(r);
return -EEXIST;
}
}
list_add_tail(&r->node, prev);
- spin_unlock(&vpci->lock);
return 0;
}
@@ -179,7 +187,6 @@ int vpci_remove_register(struct vpci *vpci, unsigned int
offset,
const struct vpci_register r = { .offset = offset, .size = size };
struct vpci_register *rm;
- spin_lock(&vpci->lock);
list_for_each_entry ( rm, &vpci->handlers, node )
{
int cmp = vpci_register_cmp(&r, rm);
@@ -191,14 +198,12 @@ int vpci_remove_register(struct vpci *vpci, unsigned int
offset,
if ( !cmp && rm->offset == offset && rm->size == size )
{
list_del(&rm->node);
- spin_unlock(&vpci->lock);
xfree(rm);
return 0;
}
if ( cmp <= 0 )
break;
}
- spin_unlock(&vpci->lock);
return -ENOENT;
}
@@ -315,7 +320,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 +336,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 )
@@ -374,6 +384,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
unsigned int size)
break;
ASSERT(data_offset < size);
}
+ spin_unlock(&pdev->vpci_lock);
if ( data_offset < size )
{
@@ -383,7 +394,6 @@ 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);
return data & (0xffffffff >> (32 - 8 * size));
}
@@ -418,7 +428,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned
int size,
uint32_t data)
{
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;
@@ -439,7 +449,14 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg,
unsigned int size,
return;
}
- spin_lock(&pdev->vpci->lock);
+ spin_lock(&pdev->vpci_lock);
+ if ( !pdev->vpci )
+ {
+ spin_unlock(&pdev->vpci_lock);
+ vpci_write_hw(sbdf, reg, size, data);
+ return;
+ }
+
/* Write the value to the hardware or emulated registers. */
list_for_each_entry ( r, &pdev->vpci->handlers, node )
@@ -474,13 +491,12 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg,
unsigned int size,
break;
ASSERT(data_offset < size);
}
+ spin_unlock(&pdev->vpci_lock);
if ( data_offset < size )
/* Tailing gap, write the remaining. */
vpci_write_hw(sbdf, reg + data_offset, size - data_offset,
data >> (data_offset * 8));
-
- spin_unlock(&pdev->vpci->lock);
}
/*
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 4cfa774615..e554c14b2f 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -114,6 +114,7 @@ struct pci_dev {
u64 vf_rlen[6];
/* Data for vPCI. */
+ spinlock_t vpci_lock;
struct vpci *vpci;
};
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index af2b8580ee..98556d31ed 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -29,7 +29,7 @@ int __must_check vpci_add_handlers(struct pci_dev *dev);
/* Remove all handlers and free vpci related structures. */
void vpci_remove_device(struct pci_dev *pdev);
-/* Add/remove a register handler. */
+/* Add/remove a register handler. Must be called holding the vpci_lock. */
int __must_check vpci_add_register(struct vpci *vpci,
vpci_read_t *read_handler,
vpci_write_t *write_handler,
@@ -58,7 +58,6 @@ bool __must_check vpci_process_pending(struct vcpu *v);
struct vpci {
/* List of vPCI handlers for a device. */
struct list_head handlers;
- spinlock_t lock;
#ifdef __XEN__
/* Hide the rest of the vpci struct from the user-space test harness. */
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |