| Message ID | fef14892-f21d-e304-d9b1-7484e0ea3415@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vPCI: MSI/MSI-X related adjustments | expand |
On Mon, Dec 07, 2020 at 11:36:38AM +0100, Jan Beulich wrote: > None of the four reasons causing vpci_msix_arch_mask_entry() to get > called (there's just a single call site) are impossible or illegal prior > to an entry actually having got set up: > - the entry may remain masked (in this case, however, a prior masked -> > unmasked transition would already not have worked), > - MSI-X may not be enabled, > - the global mask bit may be set, > - the entry may not otherwise have been updated. > Hence the function asserting that the entry was previously set up was > simply wrong. Since the caller tracks the masked state (and setting up > of an entry would only be effected when that software bit is clear), > it's okay to skip both masking and unmasking requests in this case. On the original approach I just added this because I convinced myself that scenario was impossible. I think we could also do: diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c index 64dd0a929c..509cf3962c 100644 --- a/xen/drivers/vpci/msix.c +++ b/xen/drivers/vpci/msix.c @@ -357,7 +357,11 @@ static int msix_write(struct vcpu *v, unsigned long addr, unsigned int len, * so that it picks the new state. */ entry->masked = new_masked; - if ( !new_masked && msix->enabled && !msix->masked && entry->updated ) + + if ( !msix->enabled ) + break; + + if ( !new_masked && !msix->masked && entry->updated ) { /* * If MSI-X is enabled, the function mask is not active, the entry @@ -470,6 +474,7 @@ static int init_msix(struct pci_dev *pdev) for ( i = 0; i < pdev->vpci->msix->max_entries; i++) { pdev->vpci->msix->entries[i].masked = true; + pdev->vpci->msix->entries[i].updated = true; vpci_msix_arch_init_entry(&pdev->vpci->msix->entries[i]); } In order to solve the issue. As pointed out in another patch, regardless of what we end up doing with the issue at hand we might have to consider setting updated to true in init_msix in case we want to somehow support enabling an entry that has it's address and data fields set to 0. > > Fixes: d6281be9d0145 ('vpci/msix: add MSI-X handlers') > Reported-by: Manuel Bouyer <bouyer@antioche.eu.org> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Manuel, can we get confirmation that this fixes your issue? Thanks, Roger.
On 28.12.2020 19:24, Roger Pau Monné wrote: > On Mon, Dec 07, 2020 at 11:36:38AM +0100, Jan Beulich wrote: >> None of the four reasons causing vpci_msix_arch_mask_entry() to get >> called (there's just a single call site) are impossible or illegal prior >> to an entry actually having got set up: >> - the entry may remain masked (in this case, however, a prior masked -> >> unmasked transition would already not have worked), >> - MSI-X may not be enabled, >> - the global mask bit may be set, >> - the entry may not otherwise have been updated. >> Hence the function asserting that the entry was previously set up was >> simply wrong. Since the caller tracks the masked state (and setting up >> of an entry would only be effected when that software bit is clear), >> it's okay to skip both masking and unmasking requests in this case. > > On the original approach I just added this because I convinced myself > that scenario was impossible. I think we could also do: > > diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c > index 64dd0a929c..509cf3962c 100644 > --- a/xen/drivers/vpci/msix.c > +++ b/xen/drivers/vpci/msix.c > @@ -357,7 +357,11 @@ static int msix_write(struct vcpu *v, unsigned long addr, unsigned int len, > * so that it picks the new state. > */ > entry->masked = new_masked; > - if ( !new_masked && msix->enabled && !msix->masked && entry->updated ) > + > + if ( !msix->enabled ) > + break; > + > + if ( !new_masked && !msix->masked && entry->updated ) > { > /* > * If MSI-X is enabled, the function mask is not active, the entry > @@ -470,6 +474,7 @@ static int init_msix(struct pci_dev *pdev) > for ( i = 0; i < pdev->vpci->msix->max_entries; i++) > { > pdev->vpci->msix->entries[i].masked = true; > + pdev->vpci->msix->entries[i].updated = true; > vpci_msix_arch_init_entry(&pdev->vpci->msix->entries[i]); > } > > In order to solve the issue. > > As pointed out in another patch, regardless of what we end up doing > with the issue at hand we might have to consider setting updated to > true in init_msix in case we want to somehow support enabling an entry > that has it's address and data fields set to 0. Yes, but I view this as an orthogonal aspect to consider (down the road). >> Fixes: d6281be9d0145 ('vpci/msix: add MSI-X handlers') >> Reported-by: Manuel Bouyer <bouyer@antioche.eu.org> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. > Manuel, can we get confirmation that this fixes your issue? I'll give it some time before committing for him to confirm, but I guess I'd like to time out by the end of the week. Jan
On Mon, Jan 04, 2021 at 05:35:23PM +0100, Jan Beulich wrote: > Thanks. > > > Manuel, can we get confirmation that this fixes your issue? > > I'll give it some time before committing for him to confirm, > but I guess I'd like to time out by the end of the week. Yes, it works for me
--- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -840,8 +840,8 @@ void vpci_msi_arch_print(const struct vp void vpci_msix_arch_mask_entry(struct vpci_msix_entry *entry, const struct pci_dev *pdev, bool mask) { - ASSERT(entry->arch.pirq != INVALID_PIRQ); - vpci_mask_pirq(pdev->domain, entry->arch.pirq, mask); + if ( entry->arch.pirq != INVALID_PIRQ ) + vpci_mask_pirq(pdev->domain, entry->arch.pirq, mask); } int vpci_msix_arch_enable_entry(struct vpci_msix_entry *entry,
None of the four reasons causing vpci_msix_arch_mask_entry() to get called (there's just a single call site) are impossible or illegal prior to an entry actually having got set up: - the entry may remain masked (in this case, however, a prior masked -> unmasked transition would already not have worked), - MSI-X may not be enabled, - the global mask bit may be set, - the entry may not otherwise have been updated. Hence the function asserting that the entry was previously set up was simply wrong. Since the caller tracks the masked state (and setting up of an entry would only be effected when that software bit is clear), it's okay to skip both masking and unmasking requests in this case. Fixes: d6281be9d0145 ('vpci/msix: add MSI-X handlers') Reported-by: Manuel Bouyer <bouyer@antioche.eu.org> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- This is a presumed alternative to Roger's "vpci/msix: exit early if MSI-X is disabled or globally masked".