[Qemu-devel] Re: [PATCH v3 05/13] pcie: helper functions for pcie capability and extended capability.

[Michael S. Tsirkin]

On Fri, Sep 24, 2010 at 11:24:50AM +0900, Isaku Yamahata wrote:
> On Sun, Sep 19, 2010 at 01:45:33PM +0200, Michael S. Tsirkin wrote:
> > On Sun, Sep 19, 2010 at 01:56:23PM +0900, Isaku Yamahata wrote:
> > > On Wed, Sep 15, 2010 at 02:43:10PM +0200, Michael S. Tsirkin wrote:
> > > > > +/***************************************************************************
> > > > > + * pci express capability helper functions
> > > > > + */
> > > > > +void pcie_notify(PCIDevice *dev, uint16_t vector, bool trigger, int 
> > > > > level)
> > > > 
> > > > Why is this not static? It makes sense for internal stuff possibly,
> > > > but I think functions will need to know what to do: they can't
> > > > treat msi/msix/irq identically anyway.
> > > 
> > > The aer code which I split out uses it.
> > 
> > Move it there?
> 
> _Both_ pcie.c and pcie_aer.c use it.
> 
> 
> > > > > +        assert(offset == PCI_CONFIG_SPACE_SIZE);
> > > > > +        pci_set_long(dev->config + offset,
> > > > > +                     PCI_EXT_CAP(0, 0, PCI_EXT_CAP_NEXT(header)));
> > > > > +    }
> > > > > +
> > > > > +    /* Make those registers read-only reserved zero */
> > > > 
> > > > So you make them readonly in both add and delete?
> > > > delete should revert add: let's put the
> > > > masks back the way they were: writeable.
> > > 
> > > In fact zeroing in add is redundant, but I added it following msix code.
> > 
> > It is not redundand there as registers are writeable by default:
> >     memset(dev->wmask + PCI_CONFIG_HEADER_SIZE, 0xff,
> >            config_size - PCI_CONFIG_HEADER_SIZE);
> > 
> > 
> > > 
> > > The usage model is
> > > - At first the registers are unused, so should be read only zero.
> > 
> > You can't know they are unused: in PCI spec everything
> > outside capability list is vendor specific so it
> > is writeable to let drivers do their thing.
> 
> So why PCIDevice::wmask is zero when initialized by do_pci_register_device()?

I think it isn't: only the header is 0: we have in pci_init_wmask
    memset(dev->wmask + PCI_CONFIG_HEADER_SIZE, 0xff,
           config_size - PCI_CONFIG_HEADER_SIZE);
what am I missing?

> Anyway pcie_capability_del() is only called via PCIDeviceInfo::exit,
> so I don't see any reason why manipulating the capability linked
> list just before destroying PCIDevice.
> Let's eliminate pcie_capability_del() at all.

OK.

> > > > > +    /* events not listed aren't supported */
> > > > > +};
> > > > > +
> > > > > +typedef void (*pcie_flr_fn)(PCIDevice *dev);
> > > > 
> > > > Is flr special?  Can't we use the generic reset handlers?
> > > > If not why?
> > > 
> > > Reset(cold reset/warm reset) in generic sense corresponds to
> > > conventional reset in express sense which corresponds to PCI RST#.
> > > On the other hand FLR is different from the conventional one.
> > > 
> > > Cited from the spec
> > > 
> > > 6.6. PCI Express Reset - Rules
> > > 6.6.1. Conventional Reset
> > >  Conventional Reset includes all reset mechanisms other than Function
> > >  Level Reset.
> > > 6.6.2. Function-Level Reset (FLR)
> > > 
> > > Most devices would implement FLR as just calling something like
> > > qdev_reset. But the spec differentiates FLR from conventional reset,
> > > so the generic pcie layer should do.
> > 
> > I am not sure I agree. If most devices don't care, or
> > behave almost identically, it's ok to just call qdev_reset:
> > devices can find out that FLR is in progress by looking
> > at the config space (bit will be set there) - and we can
> > add a helper pcie_flr_in_progress() to test it.
> > 
> > This way most devices will work out of box.
> > Only if behaviour is mostly different would it make sense
> > to have a completely separate reset path.
> > What happens with devices you implemented?
> 
> With either approach, we can implement FLR.
> The above approach will eventually result in passing passing parameter,
> somthing like reset_kind, to callbacks. The argument tells what kind of
> reset is triggered. cold reset, warm reset and many bus/device specific
> resets. In fact it was my first approach.
> But when we discussed on qdev reset() with Anthony, he claimed that handling
> reset type should be handled by introducing other APIs because
> it would just bloat qdev reset callback function with big switch.
> So I introduced new flr callback.

So I think this addresses this in a different way: we don't need a
parameter as devices can check state to see what reset is in progress.
Most of them do not need to bother, so we expect that there will not be any
giant switch statements.

But - you are writing the code after all so you tell us whether the reset code
is mostly same or mostly different: if it's same we should probably
reuse existing callbacks, to avoid boilerplate code
in devices, if it is different maybe add a new one.

Anthony, makes sense?

> -- 
> yamahata