On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
> @@ -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);
> }
At the first glance this simply looks like another unjustified (in the
description) change, as you're not converting anything here but you
actually add locking (and I realize this was there before, so I'm sorry
for not pointing this out earlier). But then I wonder whether you
actually tested this, since I can't help getting the impression that
you're introducing a live-lock: The function is called from cmd_write()
and rom_write(), which in turn are called out of vpci_write(). Yet that
function already holds the lock, and the lock is not (currently)
recursive. (For the 3rd caller of the function - init_bars() - otoh
the locking looks to be entirely unnecessary.)
Then again this was present already even in Roger's original patch, so
I guess I must be missing something ...
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -138,7 +138,7 @@ static void control_write(const struct pci_dev *pdev,
> unsigned int reg,
> pci_conf_write16(pdev->sbdf, reg, val);
> }
>
> -static struct vpci_msix *msix_find(const struct domain *d, unsigned long
> addr)
> +static struct vpci_msix *msix_get(const struct domain *d, unsigned long addr)
> {
> struct vpci_msix *msix;
>
> @@ -150,15 +150,29 @@ static struct vpci_msix *msix_find(const struct domain
> *d, unsigned long addr)
> for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
> if ( bars[msix->tables[i] & PCI_MSIX_BIRMASK].enabled &&
> VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, i) )
> + {
> + spin_lock(&msix->pdev->vpci_lock);
> return msix;
> + }
I think deliberately returning with a lock held requires a respective
comment ahead of the function.
> }
>
> return NULL;
> }
>
> +static void msix_put(struct vpci_msix *msix)
> +{
> + if ( !msix )
> + return;
> +
> + spin_unlock(&msix->pdev->vpci_lock);
> +}
Maybe shorter
if ( msix )
spin_unlock(&msix->pdev->vpci_lock);
? Yet there's only one case where you may pass NULL in here, so
maybe it's better anyway to move the conditional ...
> static int msix_accept(struct vcpu *v, unsigned long addr)
> {
> - return !!msix_find(v->domain, addr);
> + struct vpci_msix *msix = msix_get(v->domain, addr);
> +
> + msix_put(msix);
> + return !!msix;
> }
... here?
> @@ -186,7 +200,7 @@ static int msix_read(struct vcpu *v, unsigned long addr,
> unsigned int len,
> unsigned long *data)
> {
> const struct domain *d = v->domain;
> - struct vpci_msix *msix = msix_find(d, addr);
> + struct vpci_msix *msix = msix_get(d, addr);
> const struct vpci_msix_entry *entry;
> unsigned int offset;
>
> @@ -196,7 +210,10 @@ static int msix_read(struct vcpu *v, unsigned long addr,
> unsigned int len,
> return X86EMUL_RETRY;
>
> if ( !access_allowed(msix->pdev, addr, len) )
> + {
> + msix_put(msix);
> return X86EMUL_OKAY;
> + }
>
> if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
> {
> @@ -222,10 +239,10 @@ static int msix_read(struct vcpu *v, unsigned long
> addr, unsigned int len,
> break;
> }
>
> + msix_put(msix);
> return X86EMUL_OKAY;
> }
>
> - spin_lock(&msix->pdev->vpci->lock);
> entry = get_entry(msix, addr);
> offset = addr & (PCI_MSIX_ENTRY_SIZE - 1);
You're increasing the locked region quite a bit here. If this is really
needed, it wants explaining. And if this is deemed acceptable as a
"side effect", it wants justifying or at least stating imo. Same for
msix_write() then, obviously. (I'm not sure Roger actually implied this
when suggesting to switch to the get/put pair.)
> @@ -327,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);
> + }
Didn't you say you would add justification of this part of the change
(and its vpci_write() counterpart) to the description?
Jan
