From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Oleksandr Andrushchenko <andr2000@gmail.com>
Cc: <xen-devel@lists.xenproject.org>, <julien@xen.org>,
<sstabellini@kernel.org>, <oleksandr_tyshchenko@epam.com>,
<volodymyr_babchuk@epam.com>, <Artem_Mygaiev@epam.com>,
<jbeulich@suse.com>, <andrew.cooper3@citrix.com>,
<george.dunlap@citrix.com>, <paul@xen.org>,
<bertrand.marquis@arm.com>, <rahul.singh@arm.com>,
Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Subject: Re: [PATCH 4/4] vpci: move lock outside of struct vpci
Date: Wed, 2 Feb 2022 10:45:01 +0100 [thread overview]
Message-ID: <YfpSnTrh6dcbrNNX@Air-de-Roger> (raw)
In-Reply-To: <20220201162508.417008-5-andr2000@gmail.com>
On Tue, Feb 01, 2022 at 06:25:08PM +0200, Oleksandr Andrushchenko wrote:
> From: Roger Pau Monne <roger.pau@citrix.com>
>
> 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@citrix.com>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> ---
> 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@citrix.com/
>
> 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.
next prev parent reply other threads:[~2022-02-02 9:45 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-01 16:25 [PATCH 0/4] PCI devices passthrough pre-req patches Oleksandr Andrushchenko
2022-02-01 16:25 ` [PATCH 1/4] rangeset: add RANGESETF_no_print flag Oleksandr Andrushchenko
2022-02-01 16:25 ` [PATCH 2/4] rangeset: add rangeset_reset helper function Oleksandr Andrushchenko
2022-02-01 17:05 ` Julien Grall
2022-02-01 17:14 ` Oleksandr Andrushchenko
2022-02-01 17:33 ` Julien Grall
2022-02-01 17:39 ` Oleksandr Andrushchenko
2022-02-01 16:25 ` [PATCH 3/4] vpci: shrink critical section in vpci_{read/write} Oleksandr Andrushchenko
2022-02-02 8:44 ` Roger Pau Monné
2022-02-02 9:05 ` Jan Beulich
2022-02-02 9:38 ` Oleksandr Andrushchenko
2022-02-02 9:45 ` Jan Beulich
2022-02-02 9:49 ` Oleksandr Andrushchenko
2022-02-02 9:49 ` Roger Pau Monné
2022-02-01 16:25 ` [PATCH 4/4] vpci: move lock outside of struct vpci Oleksandr Andrushchenko
2022-02-02 9:22 ` Jan Beulich
2022-02-02 11:03 ` Oleksandr Andrushchenko
2022-02-02 11:14 ` Jan Beulich
2022-02-02 11:26 ` Oleksandr Andrushchenko
2022-02-02 9:45 ` Roger Pau Monné [this message]
2022-02-02 10:15 ` Oleksandr Andrushchenko
2022-02-02 8:48 ` [PATCH 0/4] PCI devices passthrough pre-req patches Jan Beulich
2022-02-02 9:03 ` Oleksandr Andrushchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YfpSnTrh6dcbrNNX@Air-de-Roger \
--to=roger.pau@citrix.com \
--cc=Artem_Mygaiev@epam.com \
--cc=andr2000@gmail.com \
--cc=andrew.cooper3@citrix.com \
--cc=bertrand.marquis@arm.com \
--cc=george.dunlap@citrix.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=oleksandr_andrushchenko@epam.com \
--cc=oleksandr_tyshchenko@epam.com \
--cc=paul@xen.org \
--cc=rahul.singh@arm.com \
--cc=sstabellini@kernel.org \
--cc=volodymyr_babchuk@epam.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.