[Qemu-devel] Re: [PATCH v3 06/13] pcie/aer: helper functions for pcie aer capability.

[Michael S. Tsirkin]

On Fri, Sep 24, 2010 at 11:50:21AM +0900, Isaku Yamahata wrote:
> On Wed, Sep 22, 2010 at 01:50:16PM +0200, Michael S. Tsirkin wrote:
> 
> > > +    }
> > > +
> > > +    /* capability & control */
> > > +    if (ranges_overlap(addr, len, pos + PCI_ERR_CAP, 4)) {
> > > +        uint32_t err_cap = pci_get_long(dev->config + pos + PCI_ERR_CAP);
> > > +        if (!(err_cap & PCI_ERR_CAP_MHRE)) {
> > > +            pcie_aer_log_clear_all_err(&dev->exp.aer_log);
> > > +            pci_set_long(dev->w1cmask + pos + PCI_ERR_UNCOR_STATUS,
> > > +                         PCI_ERR_UNC_SUPPORTED);
> > > +        } else {
> > > +            /* When multiple header recording is enabled, only the bit 
> > > that
> > > +             * first error pointer indicates is cleared.
> > > +             * that is handled specifically.
> > > +             */
> > > +            pci_set_long(dev->w1cmask + pos + PCI_ERR_UNCOR_STATUS, 0);
> > 
> > This is wrong I think: you do not change mask on each write.
> > Do it on setup.
> 
> The register behaves quite differently depending on whether
> multiple header recording(MHR) is enabled or not.
> With MHR disabled, the register is w1c. It indicates which errors
> have occurred.
> With MHR enabled, it behaves quite complexly. It reports errors in order
> which had occurred.
> Fro details, please refer to
> 6.2.4.2. Multiple Error Handling (Advanced Error Reporting Capability).
> 

I see. No bug then. However, I think the best way to implement this is this:

- always make the bit w1c
- after config write:
  if MHR is enabled, and you see that error log is not empty and that bit is 0,
  this means that someone has written 1b.
  so pop the first error from the log, and set bit to 1 if it's not empty.

This way we only touch w1c mask on setup.

> > > +static inline void pcie_aer_errmsg(PCIDevice *dev, const PCIE_AERErrMsg 
> > > *msg)
> > > +{
> > > +    assert(pci_is_express(dev));
> > > +    assert(dev->exp.aer_errmsg);
> > > +    dev->exp.aer_errmsg(dev, msg);
> > 
> > Why do we want the indirection? Why not have users just call the function?
> 
> To handle error signaling uniformly.
> Please see 
> 6.2.5. Sequence of Device Error Signaling and Logging Operations
> and figure 6-2 and 6-3.

My question was: the only difference appears to be between bridge and
non-bridge devices: bridge has to do more stuff, but most code is
common.  So this seems to be a very roundabout way to do this.
Can't we just have a common function with an if (bridge) statement up front?
If we ever only expect 2 implementations, I think a function pointer
is overkill.


> -- 
> yamahata