[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 4/4] vpci: move lock outside of struct vpci
Hi, Roger! On 02.02.22 11:45, Roger Pau Monné wrote: > 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://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20180717094830.54806-2-roger.pau@xxxxxxxxxx/__;!!GF_29dbcQIUBPA!ilhEn2M-44dKY8rsVO7qGmUSY22vSHwNaGJNUqhwvr-V2kdb-FDYL6f39FS8pKJm3q8HnOzyuA$ >> [lore[.]kernel[.]org] >> >> 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. Will remove > >> .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. Ok, sounds good: so, I'll implement msix_{get|put} then > >> 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. This function is going to be used outside later on, but not yet. So, I can change the name and then change it back once it is used by others. What's your preference here? > > Thanks, Roger. Thank you, Oleksandr
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |