On Mon, Jul 02, 2018 at 02:55:37PM +0100, Wei Liu wrote:
> On Mon, Jul 02, 2018 at 09:54:20AM +0200, Roger Pau Monné wrote:
> > On Fri, Jun 29, 2018 at 11:52:07AM +0100, Wei Liu wrote:
> > > On Wed, Jun 20, 2018 at 04:42:28PM +0200, Roger Pau Monne wrote:
> > > > So that interrupts are properly freed.
> > > >
> > > > Signed-off-by: Roger Pau Monné <roger....@citrix.com>
> > > > ---
> > > > Cc: Andrew Cooper <andrew.coop...@citrix.com>
> > > > Cc: George Dunlap <george.dun...@eu.citrix.com>
> > > > Cc: Ian Jackson <ian.jack...@eu.citrix.com>
> > > > Cc: Jan Beulich <jbeul...@suse.com>
> > > > Cc: Julien Grall <julien.gr...@arm.com>
> > > > Cc: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
> > > > Cc: Stefano Stabellini <sstabell...@kernel.org>
> > > > Cc: Tim Deegan <t...@xen.org>
> > > > Cc: Wei Liu <wei.l...@citrix.com>
> > > > ---
> > > > xen/drivers/vpci/msix.c | 43 ++++++++++++++++++++++++++++++++++++++++-
> > > > 1 file changed, 42 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> > > > index 6132f576b6..cfca1cd43a 100644
> > > > --- a/xen/drivers/vpci/msix.c
> > > > +++ b/xen/drivers/vpci/msix.c
> > > > @@ -450,7 +450,48 @@ static int init_msix(struct pci_dev *pdev)
> > > >
> > > > return 0;
> > > > }
> > > > -REGISTER_VPCI_INIT(init_msix, NULL, VPCI_PRIORITY_HIGH);
> > > > +
> > > > +static void teardown_msix(struct pci_dev *pdev)
> > > > +{
> > > > + struct vpci_msix *msix = pdev->vpci->msix;
> > > > + unsigned int i;
> > > > +
> > > > + if ( !msix )
> > > > + return;
> > > > +
> > > > + if ( msix->enabled )
> > > > + {
> > > > + /* Disable MSIX. */
> > > > + unsigned int pos = pci_find_cap_offset(pdev->seg, pdev->bus,
> > > > + PCI_SLOT(pdev->devfn),
> > > > + PCI_FUNC(pdev->devfn),
> > > > + PCI_CAP_ID_MSIX);
> > > > + uint16_t control = pci_conf_read16(pdev->seg, pdev->bus,
> > > > + PCI_SLOT(pdev->devfn),
> > > > + PCI_FUNC(pdev->devfn),
> > > > + msix_control_reg(pos));
> > > > +
> > > > + pci_conf_write16(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> > > > + PCI_FUNC(pdev->devfn), msix_control_reg(pos),
> > > > + (control & ~PCI_MSIX_FLAGS_ENABLE));
> > > > + }
> > > > +
> > > > + write_lock(&pdev->domain->arch.hvm_domain.msix_lock);
> > > > + list_del(&pdev->vpci->msix->next);
> > > > + write_unlock(&pdev->domain->arch.hvm_domain.msix_lock);
> > > > +
> > > > + for ( i = 0; i < msix->max_entries && msix->enabled; i++ )
> > >
> > > Maybe lift checking msix->enabled outside of the loop? Afaict nothing
> > > in the loop manipulates that flag.
> >
> > I've done it that way to skip one indentation level, but I can put it
> > outside if that's preferred.
>
> I would say if you really don't want to indent one more level.
>
> if ( !msix->enabled )
> return;
>
> for ( ... ) { }
>
> A possibly dumb question: why are the entries not freed as your disable
> hardware MSIX?
I'm not sure I understand the question. You cannot free specific
entries, they are part of the msix struct (you have to free the whole
struct, not specific entries).
> Can you reshuffle code to make that happen? Say, first
> take the structure of the list then disable MSI-X and free all entries.
>
> If that's not possible then I think better explanation is needed why the
> code is structured like this, because it appears there is some very
> intricate sequence required.
I can reshuffle the code so it's easier to parse, let me do that.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
