[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH v3 05/13] pcie: helper functions for pcie capabi
[Qemu-devel] Re: [PATCH v3 05/13] pcie: helper functions for pcie capability and extended capability.
Fri, 24 Sep 2010 11:24:50 +0900
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()?
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.
> > > > + /* 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.
[Qemu-devel] [PATCH v3 09/13] pcie upstream port: pci express switch upstream port., Isaku Yamahata, 2010/09/15
[Qemu-devel] [PATCH v3 11/13] pcie/hotplug: glue pushing attention button command. pcie_abp, Isaku Yamahata, 2010/09/15
[Qemu-devel] [PATCH v3 13/13] msix: clear not only INTA, but all INTx when MSI-X is enabled., Isaku Yamahata, 2010/09/15
[Qemu-devel] [PATCH v3 04/13] pcie: add pcie constants to pcie_regs.h, Isaku Yamahata, 2010/09/15
[Qemu-devel] [PATCH v3 06/13] pcie/aer: helper functions for pcie aer capability., Isaku Yamahata, 2010/09/15
- [Qemu-devel] [PATCH v3 02/13] pci: implement RW1C register framework., (continued)
- [Qemu-devel] [PATCH v3 02/13] pci: implement RW1C register framework., Isaku Yamahata, 2010/09/15
- [Qemu-devel] [PATCH v3 01/13] msi: implemented msi., Isaku Yamahata, 2010/09/15
- [Qemu-devel] [PATCH v3 12/13] pcie/aer: glue aer error injection into qemu monitor., Isaku Yamahata, 2010/09/15
- [Qemu-devel] [PATCH v3 10/13] pcie downstream port: pci express switch downstream port., Isaku Yamahata, 2010/09/15
- [Qemu-devel] [PATCH v3 05/13] pcie: helper functions for pcie capability and extended capability., Isaku Yamahata, 2010/09/15