Re: [Qemu-devel] [RFC v3 03/10] aer: introduce pcie_aer_setup to setup aer related bits

[Alex Williamson]

On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
> because function pcie_aer_init() is for adding a new aer capability,
> but for vfio device, we only need to capture the aer capability from
> vfio device configuration space, so here we introduce pcie_aer_setup()
> to init all raw devices.


I don't see why pcie_add_capability is split out, see for instance
msix_init() where the call still includes a call to
pci_add_capability().

> Signed-off-by: Chen Fan <address@hidden>
> ---
>  hw/pci/pcie_aer.c         | 63 
> +++++++++++++++++++++++++++++------------------
>  include/hw/pci/pcie_aer.h |  1 +
>  2 files changed, 40 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index ece1487..18caf43 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -94,53 +94,31 @@ static void aer_log_clear_all_err(PCIEAERLog *aer_log)
>      aer_log->log_num = 0;
>  }
>  
> -int pcie_aer_init(PCIDevice *dev, uint16_t offset)
> +void pcie_aer_setup(PCIDevice *dev, uint16_t offset, uint16_t log_max)
>  {
>      PCIExpressDevice *exp;
>  
> -    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
> -                        offset, PCI_ERR_SIZEOF);
>      exp = &dev->exp;
>      exp->aer_cap = offset;
>  
> -    /* log_max is property */
> -    if (dev->exp.aer_log.log_max == PCIE_AER_LOG_MAX_UNSET) {
> -        dev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
> -    }
> -    /* clip down the value to avoid unreasobale memory usage */
> -    if (dev->exp.aer_log.log_max > PCIE_AER_LOG_MAX_LIMIT) {
> -        return -EINVAL;
> -    }
> -    dev->exp.aer_log.log = g_malloc0(sizeof dev->exp.aer_log.log[0] *
> -                                        dev->exp.aer_log.log_max);
>  
>      pci_set_long(dev->w1cmask + offset + PCI_ERR_UNCOR_STATUS,
>                   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_long_test_and_set_mask(dev->w1cmask + offset + PCI_ERR_COR_STATUS,
>                                 PCI_ERR_COR_SUPPORTED);
>  
> -    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);
> +    if (log_max > 0) {
>          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);
>      }
> @@ -160,6 +138,43 @@ int pcie_aer_init(PCIDevice *dev, uint16_t offset)
>          /* nothing */
>          break;
>      }
> +}
> +
> +int pcie_aer_init(PCIDevice *dev, uint16_t offset)
> +{
> +
> +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, PCI_ERR_VER,
> +                        offset, PCI_ERR_SIZEOF);
> +
> +    /* log_max is property */
> +    if (dev->exp.aer_log.log_max == PCIE_AER_LOG_MAX_UNSET) {
> +        dev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
> +    }
> +    /* clip down the value to avoid unreasobale memory usage */
> +    if (dev->exp.aer_log.log_max > PCIE_AER_LOG_MAX_LIMIT) {
> +        return -EINVAL;
> +    }
> +
> +    dev->exp.aer_log.log = g_malloc0(sizeof dev->exp.aer_log.log[0] *
> +                                     dev->exp.aer_log.log_max);
> +
> +    /* 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);
> +    } else {
> +        pci_set_long(dev->config + offset + PCI_ERR_CAP,
> +                     PCI_ERR_CAP_ECRC_GENC | PCI_ERR_CAP_ECRC_CHKC);
> +    }
> +
> +    pci_set_long(dev->config + offset + PCI_ERR_UNCOR_SEVER,
> +                 PCI_ERR_UNC_SEVERITY_DEFAULT);
> +    pci_set_long(dev->config + offset + PCI_ERR_COR_MASK,
> +                 PCI_ERR_COR_MASK_DEFAULT);
> +
> +    pcie_aer_setup(dev, offset, dev->exp.aer_log.log_max);
> +
>      return 0;
>  }
>  
> diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
> index bcac80a..e675c7d 100644
> --- a/include/hw/pci/pcie_aer.h
> +++ b/include/hw/pci/pcie_aer.h
> @@ -87,6 +87,7 @@ struct PCIEAERErr {
>  
>  extern const VMStateDescription vmstate_pcie_aer_log;
>  
> +void pcie_aer_setup(PCIDevice *dev, uint16_t offset, uint16_t log_max);
>  int pcie_aer_init(PCIDevice *dev, uint16_t offset);
>  void pcie_aer_exit(PCIDevice *dev);
>  void pcie_aer_write_config(PCIDevice *dev,