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