[v3,06/13] pcie/aer: helper functions for pcie aer capability.

This patch implements helper functions for pcie aer capability
which will be used later.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
Changes v2 -> v3:
- split out from pcie.[ch] to pcie_aer.[ch] to make the files sorter.
- embeded PCIExpressDevice into PCIDevice.
- CodingStyle fix
---
 Makefile.objs |    2 +-
 hw/pci.h      |    7 +
 hw/pcie.h     |    6 +
 hw/pcie_aer.c |  796 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pcie_aer.h |  105 ++++++++
 qemu-common.h |    3 +
 6 files changed, 918 insertions(+), 1 deletions(-)
 create mode 100644 hw/pcie_aer.c
 create mode 100644 hw/pcie_aer.h

On Wed, Sep 15, 2010 at 02:38:19PM +0900, Isaku Yamahata wrote:
> This patch implements helper functions for pcie aer capability
> which will be used later.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> Changes v2 -> v3:
> - split out from pcie.[ch] to pcie_aer.[ch] to make the files sorter.
> - embeded PCIExpressDevice into PCIDevice.
> - CodingStyle fix
> ---
>  Makefile.objs |    2 +-
>  hw/pci.h      |    7 +
>  hw/pcie.h     |    6 +
>  hw/pcie_aer.c |  796 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pcie_aer.h |  105 ++++++++
>  qemu-common.h |    3 +
>  6 files changed, 918 insertions(+), 1 deletions(-)
>  create mode 100644 hw/pcie_aer.c
>  create mode 100644 hw/pcie_aer.h
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index eeb5134..68bcc48 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -186,7 +186,7 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
>  # PCI watchdog devices
>  hw-obj-y += wdt_i6300esb.o
>  
> -hw-obj-y += pcie.o
> +hw-obj-y += pcie.o pcie_aer.o
>  hw-obj-y += msix.o msi.o
>  
>  # PCI network cards
> diff --git a/hw/pci.h b/hw/pci.h
> index 19e85f5..73bf901 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -401,6 +401,13 @@ static inline uint8_t pci_pcie_cap(const PCIDevice *d)
>      return pci_is_express(d) ? d->exp.exp_cap : 0;
>  }
>  
> +/* AER */
> +static inline uint16_t pcie_aer_cap(const PCIDevice *d)
> +{
> +    assert(pci_is_express(d));
> +    return d->exp.aer_cap;
> +}
> +

So looking at how this is used, I think this is
a wrong API. We should have higher level APIs: pcie_get_uncor_err
or something like that. this is what devices really need, right?


>  /* These are not pci specific. Should move into a separate header.
>   * Only pci.c uses them, so keep them here for now.
>   */
> diff --git a/hw/pcie.h b/hw/pcie.h
> index 37713dc..febcbc2 100644
> --- a/hw/pcie.h
> +++ b/hw/pcie.h
> @@ -23,6 +23,7 @@
>  
>  #include "hw.h"
>  #include "pcie_regs.h"
> +#include "pcie_aer.h"
>  
>  enum PCIExpressIndicator {
>      /* for attention and power indicator */
> @@ -52,6 +53,11 @@ struct PCIExpressDevice {
>  
>      /* FLR */
>      pcie_flr_fn flr;
> +
> +    /* AER */
> +    uint16_t aer_cap;
> +    pcie_aer_errmsg_fn aer_errmsg;
> +    PCIE_AERLog aer_log;
>  };
>  
>  void pcie_notify(PCIDevice *dev, uint16_t vector, bool trigger, int level);
> diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
> new file mode 100644
> index 0000000..9e3f48e
> --- /dev/null
> +++ b/hw/pcie_aer.c
> @@ -0,0 +1,796 @@
> +/*
> + * pcie_aer.c
> + *
> + * Copyright (c) 2010 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "sysemu.h"
> +#include "pci_bridge.h"
> +#include "pcie.h"
> +#include "msix.h"
> +#include "msi.h"
> +#include "pci_internals.h"
> +#include "pcie_regs.h"
> +
> +//#define DEBUG_PCIE
> +#ifdef DEBUG_PCIE
> +# define PCIE_DPRINTF(fmt, ...)                                         \
> +    fprintf(stderr, "%s:%d " fmt, __func__, __LINE__, ## __VA_ARGS__)
> +#else
> +# define PCIE_DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +#define PCIE_DEV_PRINTF(dev, fmt, ...)                                  \
> +    PCIE_DPRINTF("%s:%x "fmt, (dev)->name, (dev)->devfn, ## __VA_ARGS__)
> +
> +static void pcie_aer_clear_error(PCIDevice *dev);
> +static uint8_t pcie_aer_root_get_vector(PCIDevice *dev);
> +static AER_ERR_MSG_RESULT
> +pcie_aer_errmsg_alldev(PCIDevice *dev, const PCIE_AERErrMsg *msg);
> +static AER_ERR_MSG_RESULT
> +pcie_aer_errmsg_vbridge(PCIDevice *dev, const PCIE_AERErrMsg *msg);
> +
> +/* From 6.2.7 Error Listing and Rules. Table 6-2, 6-3 and 6-4 */
> +static enum PCIE_AER_SEVERITY pcie_aer_uncor_default_severity(uint32_t status)
> +{
> +    switch (status) {
> +    case PCI_ERR_UNC_INTN:
> +    case PCI_ERR_UNC_DLP:
> +    case PCI_ERR_UNC_SDN:
> +    case PCI_ERR_UNC_RX_OVER:
> +    case PCI_ERR_UNC_FCP:
> +    case PCI_ERR_UNC_MALF_TLP:
> +        return AER_ERR_FATAL;
> +    case PCI_ERR_UNC_POISON_TLP:
> +    case PCI_ERR_UNC_ECRC:
> +    case PCI_ERR_UNC_UNSUP:
> +    case PCI_ERR_UNC_COMP_TIME:
> +    case PCI_ERR_UNC_COMP_ABORT:
> +    case PCI_ERR_UNC_UNX_COMP:
> +    case PCI_ERR_UNC_ACSV:
> +    case PCI_ERR_UNC_MCBTLP:
> +    case PCI_ERR_UNC_ATOP_EBLOCKED:
> +    case PCI_ERR_UNC_TLP_PRF_BLOCKED:
> +        return AER_ERR_NONFATAL;
> +    default:
> +        break;
> +    }
> +    abort();
> +    return AER_ERR_FATAL;
> +}
> +
> +static uint32_t pcie_aer_log_next(uint32_t i, uint32_t max)


This is internal function, give it a short name like
aer_log_next and

> +{
> +    return (i + 1) % max;
> +}
> +
> +static bool pcie_aer_log_empty_index(uint32_t producer, uint32_t consumer)
> +{
> +    return producer == consumer;
> +}
> +
> +static bool pcie_aer_log_empty(PCIE_AERLog *aer_log)
> +{
> +    return pcie_aer_log_empty_index(aer_log->producer, aer_log->consumer);
> +}
> +
> +static bool pcie_aer_log_full(PCIE_AERLog *aer_log)
> +{
> +    return pcie_aer_log_next(aer_log->producer, aer_log->log_max) ==
> +        aer_log->consumer;
> +}
> +
> +static uint32_t pcie_aer_log_add(PCIE_AERLog *aer_log)
> +{
> +    uint32_t i = aer_log->producer;
> +    aer_log->producer = pcie_aer_log_next(aer_log->producer, aer_log->log_max);
> +    return i;
> +}
> +
> +static uint32_t pcie_aer_log_del(PCIE_AERLog *aer_log)
> +{
> +    uint32_t i = aer_log->consumer;
> +    aer_log->consumer = pcie_aer_log_next(aer_log->consumer, aer_log->log_max);
> +    return i;
> +}
> +
> +static int pcie_aer_log_add_err(PCIE_AERLog *aer_log, const PCIE_AERErr *err)
> +{
> +    uint32_t i;
> +    if (pcie_aer_log_full(aer_log)) {
> +        return -1;
> +    }
> +    i = pcie_aer_log_add(aer_log);
> +    memcpy(&aer_log->log[i], err, sizeof(*err));
> +    return 0;
> +}
> +
> +static const PCIE_AERErr* pcie_aer_log_del_err(PCIE_AERLog *aer_log)
> +{
> +    uint32_t i;
> +    assert(!pcie_aer_log_empty(aer_log));
> +    i = pcie_aer_log_del(aer_log);
> +    return &aer_log->log[i];
> +}
> +
> +static void pcie_aer_log_clear_all_err(PCIE_AERLog *aer_log)
> +{
> +    aer_log->producer = 0;
> +    aer_log->consumer = 0;
> +}
> +
> +void pcie_aer_init(PCIDevice *dev, uint16_t offset)
> +{
> +    PCIExpressDevice *exp;
> +
> +    pci_set_word(dev->wmask + PCI_COMMAND,
> +                 pci_get_word(dev->wmask + PCI_COMMAND) | PCI_COMMAND_SERR);
> +    pci_set_word(dev->w1cmask + PCI_STATUS,
> +                 pci_get_word(dev->w1cmask + PCI_STATUS) |
> +                 PCI_STATUS_SIG_SYSTEM_ERROR);
> +
> +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
> +                        offset, PCI_ERR_SIZEOF);
> +    exp = &dev->exp;
> +    exp->aer_cap = offset;
> +    if (dev->exp.aer_log.log_max == PCIE_AER_LOG_MAX_UNSET) {
> +        dev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
> +    }
> +    if (dev->exp.aer_log.log_max > PCIE_AER_LOG_MAX_MAX) {
> +        dev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_MAX;
> +    }
> +    dev->exp.aer_log.log = qemu_mallocz(sizeof(dev->exp.aer_log.log[0]) *
> +                                        dev->exp.aer_log.log_max);
> +
> +    /* On reset PCI_ERR_CAP_MHRE is disabled
> +     * PCI_ERR_CAP_MHRE is RWS so that reset doesn't affect related
> +     * registers
> +     */
> +    pci_set_long(dev->w1cmask + offset + PCI_ERR_UNCOR_STATUS,
> +                 PCI_ERR_UNC_SUPPORTED);
> +
> +    pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
> +                 PCI_ERR_UNC_SUPPORTED);
> +
> +    pci_set_long(dev->config + offset + PCI_ERR_UNCOR_SEVER,
> +                 PCI_ERR_UNC_SEVERITY_DEFAULT);
> +    pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_SEVER,
> +                 PCI_ERR_UNC_SUPPORTED);
> +
> +    pci_set_long(dev->w1cmask + offset + PCI_ERR_COR_STATUS,
> +                 pci_get_long(dev->w1cmask + offset + PCI_ERR_COR_STATUS) |
> +                 PCI_ERR_COR_STATUS);
> +
> +    pci_set_long(dev->config + offset + PCI_ERR_COR_MASK,
> +                 PCI_ERR_COR_MASK_DEFAULT);
> +    pci_set_long(dev->wmask + offset + PCI_ERR_COR_MASK,
> +                 PCI_ERR_COR_SUPPORTED);
> +
> +    /* capabilities and control. multiple header logging is supported */
> +    if (dev->exp.aer_log.log_max > 0) {
> +        pci_set_long(dev->config + offset + PCI_ERR_CAP,
> +                     PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_CHKC |
> +                     PCI_ERR_CAP_MHRC);
> +        pci_set_long(dev->wmask + offset + PCI_ERR_CAP,
> +                     PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE |
> +                     PCI_ERR_CAP_MHRE);
> +    } else {
> +        pci_set_long(dev->config + offset + PCI_ERR_CAP,
> +                     PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_CHKC);
> +        pci_set_long(dev->wmask + offset + PCI_ERR_CAP,
> +                     PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE);
> +    }
> +
> +    switch (pcie_cap_get_type(dev)) {
> +    case PCI_EXP_TYPE_ROOT_PORT:
> +        /* this case will be set by pcie_aer_root_init() */
> +        /* fallthrough */
> +    case PCI_EXP_TYPE_DOWNSTREAM:
> +    case PCI_EXP_TYPE_UPSTREAM:
> +        pci_set_word(dev->wmask + PCI_BRIDGE_CONTROL,
> +                     pci_get_word(dev->wmask + PCI_BRIDGE_CONTROL) |
> +                     PCI_BRIDGE_CTL_SERR);
> +        pci_set_long(dev->w1cmask + PCI_STATUS,
> +                     pci_get_long(dev->w1cmask + PCI_STATUS) |
> +                     PCI_SEC_STATUS_RCV_SYSTEM_ERROR);
> +        exp->aer_errmsg = pcie_aer_errmsg_vbridge;
> +        break;
> +    default:
> +        exp->aer_errmsg = pcie_aer_errmsg_alldev;
> +        break;
> +    }
> +}
> +
> +void pcie_aer_exit(PCIDevice *dev)
> +{
> +    pci_del_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_SIZEOF);
> +    qemu_free(dev->exp.aer_log.log);
> +}
> +
> +/* Multiple Header recording isn't implemented. Is it wanted? */
> +void pcie_aer_write_config(PCIDevice *dev,
> +                           uint32_t addr, uint32_t val, int len,
> +                           uint32_t uncorsta_prev)

Pls rename uncorsta_prev so its name tells us what it is.

> +{
> +    uint32_t pos = dev->exp.aer_cap;
> +
> +    /* uncorrectable */
> +    if (ranges_overlap(addr, len, pos + PCI_ERR_UNCOR_STATUS, 4)) {
> +        uint32_t written =
> +            pci_shift_long(addr, val, pos + PCI_ERR_UNCOR_STATUS) &
> +            PCI_ERR_UNC_SUPPORTED;
> +        uint32_t errcap = pci_get_long(dev->config + pos + PCI_ERR_CAP);
> +        uint32_t first_error = (1 << PCI_ERR_CAP_FEP(errcap));
> +
> +        if ((uncorsta_prev & first_error) && (written & first_error)) {
> +            pcie_aer_clear_error(dev);
> +        }

Is not this simple W1C? If yes we do not need custom code anymore?
If not pls make it writeable to avoid range checks.

> +    }
> +
> +    /* 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.

> +        }
> +    }
> +}
> +
> +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?

> +}
> +
> +static AER_ERR_MSG_RESULT
> +pcie_aer_errmsg_alldev(PCIDevice *dev, const PCIE_AERErrMsg *msg)
> +{
> +    uint16_t cmd = pci_get_word(dev->config + PCI_COMMAND);
> +    bool transmit1 =
> +        pcie_aer_err_msg_is_uncor(msg) && (cmd & PCI_COMMAND_SERR);
> +    uint32_t pos = pci_pcie_cap(dev);
> +    uint32_t devctl = pci_get_word(dev->config + pos + PCI_EXP_DEVCTL);
> +    bool transmit2 = msg->severity & devctl;
> +    PCIDevice *parent_port;
> +
> +    if (transmit1) {
> +        if (pcie_aer_err_msg_is_uncor(msg)) {
> +            /* Signaled System Error */
> +            uint8_t *status = dev->config + PCI_STATUS;
> +            pci_set_word(status,
> +                         pci_get_word(status) | PCI_STATUS_SIG_SYSTEM_ERROR);
> +        }
> +    }
> +
> +    if (!(transmit1 || transmit2)) {
> +        return AER_ERR_MSG_MASKED;
> +    }
> +
> +    /* send up error message */
> +    if (pci_is_express(dev) &&
> +        pcie_cap_get_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> +        /* Root port notify system itself,
> +           or send the error message to root complex event collector. */
> +        /*
> +         * if root port is associated to event collector, set
> +         * parent_port = root complex event collector
> +         * For now root complex event collector isn't supported.
> +         */
> +        parent_port = NULL;
> +    } else {
> +        parent_port = pci_bridge_get_device(dev->bus);
> +    }
> +    if (parent_port) {
> +        if (!pci_is_express(parent_port)) {
> +            /* What to do? */
> +            return AER_ERR_MSG_MASKED;
> +        }
> +        pcie_aer_errmsg(parent_port, msg);
> +    }
> +    return AER_ERR_MSG_SENT;
> +}
> +
> +static AER_ERR_MSG_RESULT
> +pcie_aer_errmsg_vbridge(PCIDevice *dev, const PCIE_AERErrMsg *msg)
> +{
> +    uint16_t bridge_control = pci_get_word(dev->config + PCI_BRIDGE_CONTROL);
> +
> +    if (pcie_aer_err_msg_is_uncor(msg)) {
> +        /* Received System Error */
> +        uint8_t *sec_status = dev->config + PCI_SEC_STATUS;
> +        pci_set_word(sec_status,
> +                     pci_get_word(sec_status) |
> +                     PCI_SEC_STATUS_RCV_SYSTEM_ERROR);
> +    }
> +
> +    if (!(bridge_control & PCI_BRIDGE_CTL_SERR)) {
> +        return AER_ERR_MSG_MASKED;
> +    }
> +    return pcie_aer_errmsg_alldev(dev, msg);
> +}
> +
> +static AER_ERR_MSG_RESULT
> +pcie_aer_errmsg_root_port(PCIDevice *dev, const PCIE_AERErrMsg *msg)
> +{
> +    AER_ERR_MSG_RESULT ret;
> +    uint16_t cmd;
> +    uint8_t *aer_cap;
> +    uint32_t root_cmd;
> +    uint32_t root_sta;
> +    bool trigger;
> +
> +    ret = pcie_aer_errmsg_vbridge(dev, msg);
> +    if (ret != AER_ERR_MSG_SENT) {
> +        return ret;
> +    }
> +
> +    ret = AER_ERR_MSG_MASKED;
> +    cmd = pci_get_word(dev->config + PCI_COMMAND);
> +    aer_cap = dev->config + pcie_aer_cap(dev);
> +    root_cmd = pci_get_long(aer_cap + PCI_ERR_ROOT_COMMAND);
> +    root_sta = pci_get_long(aer_cap + PCI_ERR_ROOT_STATUS);
> +    trigger = false;
> +
> +    if (cmd & PCI_COMMAND_SERR) {
> +        /* System Error. Platform Specific */
> +        /* ret = AER_ERR_MSG_SENT; */
> +    }
> +
> +    /* Errro Message Received: Root Error Status register */
> +    switch (msg->severity) {
> +    case AER_ERR_COR:
> +        if (root_sta & PCI_ERR_ROOT_COR_RCV) {
> +            root_sta |= PCI_ERR_ROOT_MULTI_COR_RCV;
> +        } else {
> +            if (root_cmd & PCI_ERR_ROOT_CMD_COR_EN) {
> +                trigger = true;
> +            }
> +            pci_set_word(aer_cap + PCI_ERR_ROOT_COR_SRC, msg->source_id);
> +        }
> +        root_sta |= PCI_ERR_ROOT_COR_RCV;
> +        break;
> +    case AER_ERR_NONFATAL:
> +        if (!(root_sta & PCI_ERR_ROOT_NONFATAL_RCV) &&
> +            root_cmd & PCI_ERR_ROOT_CMD_NONFATAL_EN) {
> +            trigger = true;
> +        }
> +        root_sta |= PCI_ERR_ROOT_NONFATAL_RCV;
> +        break;
> +    case AER_ERR_FATAL:
> +        if (!(root_sta & PCI_ERR_ROOT_FATAL_RCV) &&
> +            root_cmd & PCI_ERR_ROOT_CMD_FATAL_EN) {
> +            trigger = true;
> +        }
> +        if (!(root_sta & PCI_ERR_ROOT_UNCOR_RCV)) {
> +            root_sta |= PCI_ERR_ROOT_FIRST_FATAL;
> +        }
> +        root_sta |= PCI_ERR_ROOT_FATAL_RCV;
> +        break;
> +    }
> +    if (pcie_aer_err_msg_is_uncor(msg)) {
> +        if (root_sta & PCI_ERR_ROOT_UNCOR_RCV) {
> +            root_sta |= PCI_ERR_ROOT_MULTI_UNCOR_RCV;
> +        } else {
> +            pci_set_word(aer_cap + PCI_ERR_ROOT_SRC, msg->source_id);
> +        }
> +        root_sta |= PCI_ERR_ROOT_UNCOR_RCV;
> +    }
> +    pci_set_long(aer_cap + PCI_ERR_ROOT_STATUS, root_sta);
> +
> +    if (root_cmd & msg->severity) {
> +        /* Error Interrupt(INTx or MSI) */
> +        pcie_notify(dev, pcie_aer_root_get_vector(dev), trigger, 1);
> +        ret = AER_ERR_MSG_SENT;
> +    }
> +    return ret;
> +}
> +
> +static void pcie_aer_update_log(PCIDevice *dev, const PCIE_AERErr *err)
> +{
> +    uint8_t *aer_cap = dev->config + pcie_aer_cap(dev);
> +    uint8_t first_bit = ffsl(err->status) - 1;
> +    uint32_t errcap = pci_get_long(aer_cap + PCI_ERR_CAP);
> +    int i;
> +    uint32_t dw;
> +
> +    errcap &= ~(PCI_ERR_CAP_FEP_MASK | PCI_ERR_CAP_TLP);
> +    errcap |= PCI_ERR_CAP_FEP(first_bit);
> +
> +    if (err->flags & PCIE_AER_ERR_HEADER_VALID) {
> +        for (i = 0; i < ARRAY_SIZE(err->header); ++i) {
> +            /* 7.10.8 Header Log Register */
> +            cpu_to_be32wu(&dw, err->header[i]);
> +            memcpy(aer_cap + PCI_ERR_HEADER_LOG + sizeof(err->header[0]) * i,
> +                   &dw, sizeof(dw));
> +        }
> +    } else {
> +        assert(!(err->flags & PCIE_AER_ERR_TLP_PRESENT));
> +        memset(aer_cap + PCI_ERR_HEADER_LOG, 0, sizeof(err->header));
> +    }
> +
> +    if ((err->flags & PCIE_AER_ERR_TLP_PRESENT) &&
> +        (pci_get_long(dev->config + pci_pcie_cap(dev) + PCI_EXP_DEVCTL2) &
> +         PCI_EXP_DEVCAP2_EETLPP)) {
> +        for (i = 0; i < ARRAY_SIZE(err->prefix); ++i) {
> +            /* 7.10.12 tlp prefix log register */
> +            cpu_to_be32wu(&dw, err->prefix[i]);
> +            memcpy(aer_cap + PCI_ERR_TLP_PREFIX_LOG +
> +                   sizeof(err->prefix[0]) * i, &dw, sizeof(dw));
> +        }
> +        errcap |= PCI_ERR_CAP_TLP;
> +    } else {
> +        memset(aer_cap + PCI_ERR_TLP_PREFIX_LOG, 0, sizeof(err->prefix));
> +    }
> +    pci_set_long(aer_cap + PCI_ERR_CAP, errcap);
> +}
> +
> +static void pcie_aer_clear_log(PCIDevice *dev)
> +{
> +    PCIE_AERErr *err;
> +    uint8_t *aer_cap = dev->config + pcie_aer_cap(dev);
> +    uint32_t errcap = pci_get_long(aer_cap + PCI_ERR_CAP);
> +
> +    errcap &= ~(PCI_ERR_CAP_FEP_MASK | PCI_ERR_CAP_TLP);
> +    pci_set_long(aer_cap + PCI_ERR_CAP, errcap);
> +
> +    memset(aer_cap + PCI_ERR_HEADER_LOG, 0, sizeof(err->header));
> +    memset(aer_cap + PCI_ERR_TLP_PREFIX_LOG, 0, sizeof(err->prefix));
> +}
> +
> +static int pcie_aer_record_error(PCIDevice *dev,
> +                                 const PCIE_AERErr *err)
> +{
> +    uint8_t *aer_cap = dev->config + pcie_aer_cap(dev);
> +    uint32_t errcap = pci_get_long(aer_cap + PCI_ERR_CAP);
> +    int fep = PCI_ERR_CAP_FEP(errcap);
> +
> +    if (errcap & PCI_ERR_CAP_MHRE &&
> +        (pci_get_long(aer_cap + PCI_ERR_UNCOR_STATUS) & (1ULL << fep))) {
> +        /*  Not first error. queue error */
> +        if (pcie_aer_log_add_err(&dev->exp.aer_log, err) < 0) {
> +            /* overflow */
> +            return -1;
> +        }
> +        return 0;
> +    }
> +
> +    pcie_aer_update_log(dev, err);
> +    return 0;
> +}
> +
> +static void pcie_aer_clear_error(PCIDevice *dev)
> +{
> +    uint8_t *aer_cap = dev->config + pcie_aer_cap(dev);
> +    uint32_t errcap = pci_get_long(aer_cap + PCI_ERR_CAP);
> +    uint32_t old_err = (1UL << PCI_ERR_CAP_FEP(errcap));
> +    PCIE_AERLog *aer_log = &dev->exp.aer_log;
> +    const PCIE_AERErr *err;
> +    uint32_t consumer;
> +
> +    if (!(errcap & PCI_ERR_CAP_MHRE) || pcie_aer_log_empty(aer_log)) {
> +        pcie_aer_clear_log(dev);
> +        pci_set_long(aer_cap + PCI_ERR_UNCOR_STATUS,
> +                     pci_get_long(aer_cap + PCI_ERR_UNCOR_STATUS) & ~old_err);
> +        return;
> +    }
> +
> +    /* if no same error is queued, clear bit in uncorrectable error status */
> +    for (consumer = dev->exp.aer_log.consumer;
> +         !pcie_aer_log_empty_index(dev->exp.aer_log.producer, consumer);
> +         consumer = pcie_aer_log_next(consumer, dev->exp.aer_log.log_max)) {
> +        if (dev->exp.aer_log.log[consumer].status & old_err) {
> +            old_err = 0;
> +            break;
> +        }
> +    }
> +    if (old_err) {
> +        pci_set_long(aer_cap + PCI_ERR_UNCOR_STATUS,
> +                     pci_get_long(aer_cap + PCI_ERR_UNCOR_STATUS) & ~old_err);
> +    }
> +
> +    err = pcie_aer_log_del_err(aer_log);
> +    pcie_aer_update_log(dev, err);
> +}
> +
> +/*
> + * non-Function specific error must be recorded in all functions.
> + * It is the responsibility of the caller of this function.
> + * It is also caller's responsiblity to determine which function should
> + * report the rerror.
> + *
> + * 6.2.4 Error Logging
> + * 6.2.5 Sqeucne of Device Error Signaling and Logging Operations
> + * table 6-2: Flowchard Showing Sequence of Device Error Signaling and Logging
> + *            Operations
> + *
> + * Although this implementation can be shortened/optimized, this is kept
> + * parallel to table 6-2.
> + */
> +void pcie_aer_inject_error(PCIDevice *dev, const PCIE_AERErr *err)
> +{
> +    uint8_t *exp_cap;
> +    uint8_t *aer_cap = NULL;
> +    uint32_t devctl = 0;
> +    uint32_t devsta = 0;
> +    uint32_t status = err->status;
> +    uint32_t mask;
> +    bool is_unsupported_request =
> +        (!(err->flags & PCIE_AER_ERR_IS_CORRECTABLE) &&
> +         err->status == PCI_ERR_UNC_UNSUP);
> +    bool is_advisory_nonfatal = false;  /* for advisory non-fatal error */
> +    uint32_t uncor_status = 0;          /* for advisory non-fatal error */
> +    PCIE_AERErrMsg msg;
> +    int is_header_log_overflowed = 0;
> +
> +    if (!pci_is_express(dev)) {
> +        /* What to do? */
> +        return;
> +    }
> +
> +    if (err->flags & PCIE_AER_ERR_IS_CORRECTABLE) {
> +        status &= PCI_ERR_COR_SUPPORTED;
> +    } else {
> +        status &= PCI_ERR_UNC_SUPPORTED;
> +    }
> +    if (!status || status & (status - 1)) {
> +        /* invalid status bit. one and only one bit must be set */
> +        return;
> +    }
> +
> +    exp_cap = dev->config + pci_pcie_cap(dev);
> +    if (dev->exp.aer_cap) {
> +        aer_cap = dev->config + pcie_aer_cap(dev);
> +        devctl = pci_get_long(exp_cap + PCI_EXP_DEVCTL);
> +        devsta = pci_get_long(exp_cap + PCI_EXP_DEVSTA);
> +    }
> +    if (err->flags & PCIE_AER_ERR_IS_CORRECTABLE) {
> +    correctable_error:
> +        devsta |= PCI_EXP_DEVSTA_CED;
> +        if (is_unsupported_request) {
> +            devsta |= PCI_EXP_DEVSTA_URD;
> +        }
> +        pci_set_word(exp_cap + PCI_EXP_DEVSTA, devsta);
> +
> +        if (aer_cap) {
> +            pci_set_long(aer_cap + PCI_ERR_COR_STATUS,
> +                         pci_get_long(aer_cap + PCI_ERR_COR_STATUS) | status);
> +            mask = pci_get_long(aer_cap + PCI_ERR_COR_MASK);
> +            if (mask & status) {
> +                return;
> +            }
> +            if (is_advisory_nonfatal) {
> +                uint32_t uncor_mask =
> +                    pci_get_long(aer_cap + PCI_ERR_UNCOR_MASK);
> +                if (!(uncor_mask & uncor_status)) {
> +                    is_header_log_overflowed = pcie_aer_record_error(dev, err);
> +                }
> +                pci_set_long(aer_cap + PCI_ERR_UNCOR_STATUS,
> +                             pci_get_long(aer_cap + PCI_ERR_UNCOR_STATUS) |
> +                             uncor_status);
> +            }
> +        }
> +
> +        if (is_unsupported_request && !(devctl & PCI_EXP_DEVCTL_URRE)) {
> +            return;
> +        }
> +        if (!(devctl & PCI_EXP_DEVCTL_CERE)) {
> +            return;
> +        }
> +        msg.severity = AER_ERR_COR;
> +    } else {
> +        bool is_fatal =
> +            (pcie_aer_uncor_default_severity(status) == AER_ERR_FATAL);
> +        uint16_t cmd;
> +
> +        if (aer_cap) {
> +            is_fatal = status & pci_get_long(aer_cap + PCI_ERR_UNCOR_SEVER);
> +        }
> +        if (!is_fatal && (err->flags & PCIE_AER_ERR_MAYBE_ADVISORY)) {
> +            is_advisory_nonfatal = true;
> +            uncor_status = status;
> +            status = PCI_ERR_COR_ADV_NONFATAL;
> +            goto correctable_error;
> +        }
> +        if (is_fatal) {
> +            devsta |= PCI_EXP_DEVSTA_FED;
> +        } else {
> +            devsta |= PCI_EXP_DEVSTA_NFED;
> +        }
> +        if (is_unsupported_request) {
> +            devsta |= PCI_EXP_DEVSTA_URD;
> +        }
> +        pci_set_long(exp_cap + PCI_EXP_DEVSTA, devsta);
> +
> +        if (aer_cap) {
> +            mask = pci_get_long(aer_cap + PCI_ERR_UNCOR_MASK);
> +            if (mask & status) {
> +                pci_set_long(aer_cap + PCI_ERR_UNCOR_STATUS,
> +                             pci_get_long(aer_cap + PCI_ERR_UNCOR_STATUS) |
> +                             status);
> +                return;
> +            }
> +
> +            is_header_log_overflowed = pcie_aer_record_error(dev, err);
> +            pci_set_long(aer_cap + PCI_ERR_UNCOR_STATUS,
> +                         pci_get_long(aer_cap + PCI_ERR_UNCOR_STATUS) |
> +                         status);
> +        }
> +
> +        cmd = pci_get_word(dev->config + PCI_COMMAND);
> +        if (is_unsupported_request &&
> +            !(devctl & PCI_EXP_DEVCTL_URRE) && !(cmd & PCI_COMMAND_SERR)) {
> +            return;
> +        }
> +        if (is_fatal) {
> +            if (!((cmd & PCI_COMMAND_SERR) ||
> +                  (devctl & PCI_EXP_DEVCTL_FERE))) {
> +                return;
> +            }
> +            msg.severity = AER_ERR_FATAL;
> +        } else {
> +            if (!((cmd & PCI_COMMAND_SERR) ||
> +                  (devctl & PCI_EXP_DEVCTL_NFERE))) {
> +                return;
> +            }
> +            msg.severity = AER_ERR_NONFATAL;
> +        }
> +    }
> +
> +    /* send up error message */
> +    msg.source_id = err->source_id;
> +    pcie_aer_errmsg(dev, &msg);
> +
> +    if (is_header_log_overflowed) {
> +        PCIE_AERErr header_log_overflow = {
> +            .status = PCI_ERR_COR_HL_OVERFLOW,
> +            .flags = PCIE_AER_ERR_IS_CORRECTABLE,
> +            .header = {0, 0, 0, 0},
> +            .prefix = {0, 0, 0, 0},
> +        };
> +        pcie_aer_inject_error(dev, &header_log_overflow);
> +    }
> +}
> +
> +void pcie_aer_root_set_vector(PCIDevice *dev, uint8_t vector)
> +{
> +    uint8_t *aer_cap = dev->config + pcie_aer_cap(dev);
> +    uint32_t root_status = pci_get_long(aer_cap + PCI_ERR_ROOT_STATUS);
> +    root_status &= ~PCI_ERR_ROOT_IRQ;
> +    root_status |=
> +        (((uint32_t)vector) << PCI_ERR_ROOT_IRQ_SHIFT) & PCI_ERR_ROOT_IRQ;
> +    pci_set_long(aer_cap + PCI_ERR_ROOT_STATUS, root_status);
> +}
> +
> +static uint8_t pcie_aer_root_get_vector(PCIDevice *dev)
> +{
> +    uint8_t *aer_cap = dev->config + pcie_aer_cap(dev);
> +    uint32_t root_status = pci_get_long(aer_cap + PCI_ERR_ROOT_STATUS);
> +    return (root_status & PCI_ERR_ROOT_IRQ) >> PCI_ERR_ROOT_IRQ_SHIFT;
> +}
> +
> +void pcie_aer_root_init(PCIDevice *dev)
> +{
> +    uint16_t pos = pcie_aer_cap(dev);
> +
> +    pci_set_long(dev->wmask + pos + PCI_ERR_ROOT_COMMAND,
> +                 PCI_ERR_ROOT_CMD_EN_MASK);
> +    pci_set_long(dev->w1cmask + pos + PCI_ERR_ROOT_STATUS,
> +                 PCI_ERR_ROOT_STATUS_REPORT_MASK);
> +    dev->exp.aer_errmsg = pcie_aer_errmsg_root_port;
> +}
> +
> +void pcie_aer_root_reset(PCIDevice *dev)
> +{
> +    uint8_t* aer_cap = dev->config + pcie_aer_cap(dev);
> +
> +    pci_set_long(aer_cap + PCI_ERR_ROOT_COMMAND, 0);
> +
> +    /*
> +     * Advanced Error Interrupt Message Number in Root Error Status Register
> +     * must be updated by chip dependent code.
> +     */

Why?

> +}
> +
> +static bool pcie_aer_root_does_trigger(uint32_t cmd, uint32_t sta)

sta -> status

> +{
> +    return
> +        ((cmd & PCI_ERR_ROOT_CMD_COR_EN) && (sta & PCI_ERR_ROOT_COR_RCV)) ||
> +        ((cmd & PCI_ERR_ROOT_CMD_NONFATAL_EN) &&
> +         (sta & PCI_ERR_ROOT_NONFATAL_RCV)) ||
> +        ((cmd & PCI_ERR_ROOT_CMD_FATAL_EN) && (sta & PCI_ERR_ROOT_FATAL_RCV));
> +}
> +
> +void pcie_aer_root_write_config(PCIDevice *dev,
> +                                uint32_t addr, uint32_t val, int len,
> +                                uint32_t root_cmd_prev)
> +{
> +    uint16_t pos = pcie_aer_cap(dev);
> +    uint8_t *aer_cap = dev->config + pos;
> +    uint32_t root_status;
> +
> +    /* root command */
> +    if (ranges_overlap(addr, len, pos + PCI_ERR_ROOT_COMMAND, 4)) {
> +        uint32_t root_cmd = pci_get_long(aer_cap + PCI_ERR_ROOT_COMMAND);

make command writeable and then you won't need the tricky range checks.

> +        if (root_cmd & PCI_ERR_ROOT_CMD_EN_MASK) {
> +            bool trigger;
> +            int level;
> +            uint32_t root_cmd_set = (root_cmd_prev ^ root_cmd) & root_cmd;
> +
> +            /* 0 -> 1 */
> +            root_status = pci_get_long(aer_cap + PCI_ERR_ROOT_STATUS);
> +            if (pcie_aer_root_does_trigger(root_cmd_set, root_status)) {
> +                trigger = true;
> +            } else {
> +                trigger = false;
> +            }
> +            if (pcie_aer_root_does_trigger(root_cmd, root_status)) {
> +                level = 1;
> +            } else {
> +                level = 0;
> +            }
> +            pcie_notify(dev, pcie_aer_root_get_vector(dev), trigger, level);
> +        }
> +    }
> +}
> +
> +static const VMStateDescription vmstate_pcie_aer_err = {
> +    .name = "PCIE_AER_ERROR",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields     = (VMStateField[]) {
> +        VMSTATE_UINT32(status, PCIE_AERErr),
> +        VMSTATE_UINT16(source_id, PCIE_AERErr),
> +        VMSTATE_UINT16(flags, PCIE_AERErr),
> +        VMSTATE_UINT32_ARRAY(header, PCIE_AERErr, 4),
> +        VMSTATE_UINT32_ARRAY(prefix, PCIE_AERErr, 4),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +#define VMSTATE_PCIE_AER_ERRS(_field, _state, _field_num, _vmsd, _type) { \
> +    .name       = (stringify(_field)),                                    \
> +    .version_id = 0,                                                      \
> +    .num_offset = vmstate_offset_value(_state, _field_num, uint16_t),     \
> +    .size       = sizeof(_type),                                          \
> +    .vmsd       = &(_vmsd),                                               \
> +    .flags      = VMS_POINTER | VMS_VARRAY_UINT16 | VMS_STRUCT,           \
> +    .offset     = vmstate_offset_pointer(_state, _field, _type),          \
> +}
> +
> +const VMStateDescription vmstate_pcie_aer_log = {
> +    .name = "PCIE_AER_ERROR_LOG",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields     = (VMStateField[]) {
> +        VMSTATE_UINT32(producer, PCIE_AERLog),
> +        VMSTATE_UINT32(consumer, PCIE_AERLog),
> +        VMSTATE_UINT16(log_max, PCIE_AERLog),
> +        VMSTATE_PCIE_AER_ERRS(log, PCIE_AERLog, log_max,
> +                              vmstate_pcie_aer_err, PCIE_AERErr),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> diff --git a/hw/pcie_aer.h b/hw/pcie_aer.h
> new file mode 100644
> index 0000000..5a72bee
> --- /dev/null
> +++ b/hw/pcie_aer.h
> @@ -0,0 +1,105 @@
> +/*
> + * pcie_aer.h
> + *
> + * Copyright (c) 2010 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef QEMU_PCIE_AER_H
> +#define QEMU_PCIE_AER_H
> +
> +#include "hw.h"
> +
> +/* definitions which PCIExpressDevice uses */
> +enum AER_ERR_MSG_RESULT {
> +    AER_ERR_MSG_MASKED,
> +    AER_ERR_MSG_SENT,
> +};
> +typedef enum AER_ERR_MSG_RESULT AER_ERR_MSG_RESULT;
> +typedef AER_ERR_MSG_RESULT (*pcie_aer_errmsg_fn)(PCIDevice *dev, const PCIE_AERErrMsg *msg);
> +
> +/* AER log */
> +struct PCIE_AERLog {
> +    uint32_t producer;
> +    uint32_t consumer;

Make these unsigned, they are just numbers, right?
Their size does not matter.
And change to unsigned most of functions dealing with them.
Easier to print, and easier to follow to understand

> +
> +#define PCIE_AER_LOG_MAX_DEFAULT        8
> +#define PCIE_AER_LOG_MAX_MAX            128 /* what is appropriate? */
> +#define PCIE_AER_LOG_MAX_UNSET          (~(uint16_t)0)

This does not do what you want: you get ffffffff but I think you
wanted ffff.  So simply write 0xffff.

> +    uint16_t log_max;
> +
> +    PCIE_AERErr *log;
> +};

all enum names and struct names shoukd be changed: remove _
and make mixed case, add typedefs.

> +
> +/* aer error severity */
> +enum PCIE_AER_SEVERITY {

This is not how we name enums, is it? either enum pcie_aer_severity {},
or typedef enum {} PCIEAerSeverity.

> +    /* those value are same as
> +     * Root error command register in aer extended cap and
> +     * root control register in pci express cap.
> +     */
> +    AER_ERR_COR         = 0x1,
> +    AER_ERR_NONFATAL    = 0x2,
> +    AER_ERR_FATAL       = 0x4,
> +};
> +
> +/* aer error message: error signaling message has only error sevirity and
> +   source id. See 2.2.8.3 error signaling messages */
> +struct PCIE_AERErrMsg {

Same here: either struct pcie_aererrmsg  or struc PCIEAERErrMsg.
(Might be just Msg - AER includes error already).

> +    enum PCIE_AER_SEVERITY severity;
> +    uint16_t source_id; /* bdf */
> +};
> +
> +static inline bool
> +pcie_aer_err_msg_is_uncor(const PCIE_AERErrMsg *msg)
> +{
> +    return msg->severity == AER_ERR_NONFATAL || msg->severity == AER_ERR_FATAL;
> +}
> +
> +/* error */
> +struct PCIE_AERErr {
> +    uint32_t status;    /* error status bits */
> +    uint16_t source_id; /* bdf */
> +
> +#define PCIE_AER_ERR_IS_CORRECTABLE     0x1     /* correctable/uncorrectable */
> +#define PCIE_AER_ERR_MAYBE_ADVISORY     0x2     /* maybe advisory non-fatal */
> +#define PCIE_AER_ERR_HEADER_VALID       0x4     /* TLP header is logged */
> +#define PCIE_AER_ERR_TLP_PRESENT        0x8     /* TLP Prefix is logged */
> +    uint16_t flags;
> +
> +    uint32_t header[4]; /* TLP header */
> +    uint32_t prefix[4]; /* TLP header prefix */
> +};
> +
> +extern const VMStateDescription vmstate_pcie_aer_log;
> +
> +void pcie_aer_init(PCIDevice *dev, uint16_t offset);
> +void pcie_aer_exit(PCIDevice *dev);
> +void pcie_aer_write_config(PCIDevice *dev,
> +                           uint32_t addr, uint32_t val, int len,
> +                           uint32_t uncorsta_prev);
> +
> +/* aer root port */
> +void pcie_aer_root_set_vector(PCIDevice *dev, uint8_t vector);
> +void pcie_aer_root_init(PCIDevice *dev);
> +void pcie_aer_root_reset(PCIDevice *dev);
> +void pcie_aer_root_write_config(PCIDevice *dev,
> +                                uint32_t addr, uint32_t val, int len,
> +                                uint32_t root_cmd_prev);
> +
> +/* error injection */
> +void pcie_aer_inject_error(PCIDevice *dev, const PCIE_AERErr *err);
> +
> +#endif /* QEMU_PCIE_AER_H */
> diff --git a/qemu-common.h b/qemu-common.h
> index 6d9ee26..fee772e 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -221,6 +221,9 @@ typedef struct PCIBus PCIBus;
>  typedef struct PCIDevice PCIDevice;
>  typedef struct PCIExpressDevice PCIExpressDevice;
>  typedef struct PCIBridge PCIBridge;
> +typedef struct PCIE_AERErrMsg PCIE_AERErrMsg;
> +typedef struct PCIE_AERLog PCIE_AERLog;
> +typedef struct PCIE_AERErr PCIE_AERErr;
>  typedef struct SerialState SerialState;
>  typedef struct IRQState *qemu_irq;
>  typedef struct PCMCIACardState PCMCIACardState;
> -- 
> 1.7.1.1

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).


> > +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.

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

On Sun, Sep 26, 2010 at 02:46:51PM +0200, Michael S. Tsirkin wrote:
> > > > +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.

Not 2, but 3. root port, upstream or downstream and normal device.
So you want something like the following?
More than 2 is a good reason for me, I prefer function pointer.

switch (pcie_cap_get_type(dev))
    case PCI_EXP_TYPE_ROOT_PORT:
        pcie_aer_errmsg_root_port();
        break;
    case PCI_EXP_TYPE_DOWNSTREAM:
    case PCI_EXP_TYPE_UPSTREAM:
        pcie_aer_errmsg_vbridge();
        break;
    default:
       pcie_aer_errmsg_alldev();
       break;

On Mon, Sep 27, 2010 at 03:03:24PM +0900, Isaku Yamahata wrote:
> On Sun, Sep 26, 2010 at 02:46:51PM +0200, Michael S. Tsirkin wrote:
> > > > > +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.
> 
> Not 2, but 3. root port, upstream or downstream and normal device.
> So you want something like the following?
> More than 2 is a good reason for me, I prefer function pointer.

Heh, shall we change all switch statements to pointers then?
That's not a good idea IMO, automated tools like ctags
become useless for navigation, you have to look up where it's
actually set. We do have these things in places where
it brings benefit, which is typically where it can take
different values at runtime.

> switch (pcie_cap_get_type(dev))
>     case PCI_EXP_TYPE_ROOT_PORT:
>         pcie_aer_errmsg_root_port();
>         break;
>     case PCI_EXP_TYPE_DOWNSTREAM:
>     case PCI_EXP_TYPE_UPSTREAM:
>         pcie_aer_errmsg_vbridge();
>         break;
>     default:
>        pcie_aer_errmsg_alldev();
>        break;

Yes, and note that in fact it's not even that:
all of root and ports are also devices, so we end up
with:
	if (pcie_is_root_port(dev)) {
         	pcie_aer_errmsg_root_port();
	} else if (pcie_is_bridge_port(dev)) {
	         pcie_aer_errmsg_vbridge();
	}
        pcie_aer_errmsg_alldev();

which is in fact cleaner than calling pcie_aer_errmsg_alldev
from pcie_aer_errmsg_vbridge, I think.

> 
> -- 
> yamahata