[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 4/4] vpci: move lock outside of struct vpci
On Tue, Feb 01, 2022 at 06:25:08PM +0200, Oleksandr Andrushchenko wrote: > From: Roger Pau Monne <roger.pau@xxxxxxxxxx> > > 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> > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx> > --- > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > Cc: Jan Beulich <jbeulich@xxxxxxxx> > Cc: Julien Grall <julien@xxxxxxx> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx> > --- > New in v5 of this series: this is an updated version of the patch published at > https://lore.kernel.org/xen-devel/20180717094830.54806-2-roger.pau@xxxxxxxxxx/ > > Changes since v5: > - do not split code into vpci_remove_device_handlers_locked yet > - move INIT_LIST_HEAD outside the locked region (Jan) > - stripped out locking optimizations for vpci_{read|write} into a > dedicated patch > Changes since v2: > - fixed pdev->vpci = xzalloc(struct vpci); under spin_lock (Jan) > 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 | 21 ++++++++---- > xen/drivers/vpci/msi.c | 11 ++++-- > xen/drivers/vpci/msix.c | 8 ++--- > xen/drivers/vpci/vpci.c | 63 ++++++++++++++++++++++------------- > xen/include/xen/pci.h | 1 + > xen/include/xen/vpci.h | 3 +- > 10 files changed, 78 insertions(+), 47 deletions(-) > > diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h > index 2e1d3057c9d8..d018fb5eef21 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 b9a0a6006bb9..26c95b08b6b1 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, Nit: vpci_lock will already be initialized to false by default, so this is redundant. > .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 13e2a190b439..1f7a37f78264 100644 > --- a/xen/arch/x86/hvm/vmsi.c > +++ b/xen/arch/x86/hvm/vmsi.c > @@ -910,14 +910,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 1fad80362f0e..af648c6a19b5 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -328,6 +328,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, > u8 bus, u8 devfn) > *((u8*) &pdev->bus) = bus; > *((u8*) &pdev->devfn) = devfn; > pdev->domain = NULL; > + spin_lock_init(&pdev->vpci_lock); > > arch_pci_init_pdev(pdev); > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index 40ff79c33f8f..bd23c0274d48 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -142,12 +142,13 @@ 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.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd, > - !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.cmd & ~PCI_COMMAND_MEMORY : > v->vpci.cmd, > + !rc && v->vpci.rom_only); > + spin_unlock(&v->vpci.pdev->vpci_lock); > > rangeset_destroy(v->vpci.mem); > v->vpci.mem = NULL; > @@ -285,6 +286,12 @@ static int modify_bars(const struct pci_dev *pdev, > uint16_t cmd, 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]; > @@ -303,12 +310,14 @@ static int modify_bars(const struct pci_dev *pdev, > uint16_t cmd, 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 5757a7aed20f..e3ce46869dad 100644 > --- a/xen/drivers/vpci/msi.c > +++ b/xen/drivers/vpci/msi.c > @@ -270,7 +270,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; > @@ -282,8 +282,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 ) > @@ -323,7 +328,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 846f1b8d7038..5310cc3ff520 100644 > --- a/xen/drivers/vpci/msix.c > +++ b/xen/drivers/vpci/msix.c > @@ -225,7 +225,7 @@ static int msix_read(struct vcpu *v, unsigned long addr, > unsigned int len, I think you also need to add locking to msix_find, otherwise it will dereference pdev->vpci without holding the vpci_lock. It might be a better approach to rename msix_find to msix_get and return the vpci_msix struct with the vpci_lock taken, so we can assert it's not going to disappear under our feet. Then you will also need to add a msix_put function that releases the lock. > 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); > > @@ -254,7 +254,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; > } > @@ -297,7 +297,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); > > @@ -370,7 +370,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 fb0947179b79..c015a4d77540 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -35,12 +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) Nit: since it's a static function you can drop the vpci_ prefix here. Thanks, Roger.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |