Re: [Xen-devel] [PATCH 06/10] vpci/header: add teardown cleanup

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jul 02, 2018 at 03:30:19PM +0100, Wei Liu wrote:
> On Mon, Jul 02, 2018 at 10:04:30AM +0200, Roger Pau Monné wrote:
> > On Fri, Jun 29, 2018 at 12:15:34PM +0100, Wei Liu wrote:
> > > On Wed, Jun 20, 2018 at 04:42:30PM +0200, Roger Pau Monne wrote:
> > > > In order to unmap the BARs
> > > > 
> > > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > > ---
> > > > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > > > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > > > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > > > Cc: Julien Grall <julien.grall@xxxxxxx>
> > > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > > > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > Cc: Tim Deegan <tim@xxxxxxx>
> > > > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > > ---
> > > >  xen/drivers/vpci/header.c | 30 +++++++++++++++++++++++-------
> > > >  1 file changed, 23 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> > > > index 4363270a55..686e04e35a 100644
> > > > --- a/xen/drivers/vpci/header.c
> > > > +++ b/xen/drivers/vpci/header.c
> > > > @@ -131,12 +131,15 @@ bool vpci_process_pending(struct vcpu *v)
> > > >          if ( rc == -ERESTART )
> > > >              return true;
> > > >  
> > > > -        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.map,
> > > > -                            !rc && v->vpci.rom_only);
> > > > -        spin_unlock(&v->vpci.pdev->vpci_lock);
> > > > +        if ( v->vpci.pdev )
> > > > +        {
> > > > +            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.map,
> > > > +                                !rc && v->vpci.rom_only);
> > > > +            spin_unlock(&v->vpci.pdev->vpci_lock);
> > > > +        }
> > > >  
> > > >          rangeset_destroy(v->vpci.mem);
> > > >          v->vpci.mem = NULL;
> > > > @@ -560,7 +563,20 @@ static int init_bars(struct pci_dev *pdev)
> > > >  
> > > >      return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, true, false) 
> > > > : 0;
> > > >  }
> > > > -REGISTER_VPCI_INIT(init_bars, NULL, VPCI_PRIORITY_MIDDLE);
> > > > +
> > > > +static void teardown_bars(struct pci_dev *pdev)
> > > > +{
> > > > +    uint16_t cmd = pci_conf_read16(pdev->seg, pdev->bus, 
> > > > PCI_SLOT(pdev->devfn),
> > > > +                                   PCI_FUNC(pdev->devfn), PCI_COMMAND);
> > > > +
> > > > +    if ( cmd & PCI_COMMAND_MEMORY )
> > > > +    {
> > > > +        /* Unmap all BARs from guest p2m. */
> > > > +        modify_bars(pdev, false, false);
> > > 
> > > So modify_bars will eventually call defer_map in most cases (which I
> > > believe are the ones your care about here).
> > > 
> > > But then the following line sets vpci.pdev to NULL, which means the
> > > check in vpci_process_pending is false and modify_decoding is skipped.
> > > If that what you want?
> > 
> > Yes, after the call to teardown_bars the pdev can be removed, so the
> > deferred part of the unmap shouldn't rely on pdev being available.
> > That means that the memory decoding bit is not switched off. I would
> > expect that the device will get reset into a proper state (or in the
> > SR-IOV just disappear).
> > 
> 
> Which reset are you referring to? I suppose you mean VF FLR?

Whatever reset procedure is used is ATM out of the scope of this
patch. For example in the VF case the header teardown code will be
used when SR-IOV is disabled, and thus the VFs just disappear. If
SR-IOV is enabled afterwards the state of the VFs is clean.

For other devices (or when not disabling SR-IOV), then yes, some form
of reset (either FLR, PM or secondary bus reset) should be performed
before assigning the device again to a guest.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel