Re: [PATCH v1 01/14] xen/pci: Refactor MSI code that implements MSI functionality within XEN

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.08.2021 14:02, Rahul Singh wrote:
> --- /dev/null
> +++ b/xen/drivers/passthrough/msi.c
> @@ -0,0 +1,96 @@
> +/*
> + * Copyright (C) 2008,  Netronome Systems, Inc.

While generally copying copyright statements when splitting source
files is probably wanted (or even necessary) I doubt this is
suitable here: None of the MSI code that you move was contributed
by them afaict.

> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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 <xen/init.h>
> +#include <xen/pci.h>
> +#include <asm/msi.h>

You surely mean xen/msi.h here: Headers like this one should always
be included by the producer, no matter that it builds fine without.
Else you risk declarations and definitions to go out of sync.

> +#include <asm/hvm/io.h>
> +
> +int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    if ( pdev->msix )
> +    {
> +        rc = pci_reset_msix_state(pdev);
> +        if ( rc )
> +            return rc;
> +        msixtbl_init(d);
> +    }
> +
> +    return 0;
> +}
> +
> +int pdev_msi_init(struct pci_dev *pdev)
> +{
> +    unsigned int pos;
> +
> +    INIT_LIST_HEAD(&pdev->msi_list);
> +
> +    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
> +    if ( pos )
> +    {
> +        uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
> +
> +        pdev->msi_maxvec = multi_msi_capable(ctrl);
> +    }
> +
> +    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
> +    if ( pos )
> +    {
> +        struct arch_msix *msix = xzalloc(struct arch_msix);
> +        uint16_t ctrl;
> +
> +        if ( !msix )
> +            return -ENOMEM;
> +
> +        spin_lock_init(&msix->table_lock);
> +
> +        ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
> +        msix->nr_entries = msix_table_size(ctrl);
> +
> +        pdev->msix = msix;
> +    }
> +
> +    return 0;
> +}
> +
> +void pdev_msi_deinit(struct pci_dev *pdev)
> +{
> +    XFREE(pdev->msix);
> +}
> +
> +void pdev_dump_msi(const struct pci_dev *pdev)
> +{
> +    const struct msi_desc *msi;
> +
> +    printk("- MSIs < ");
> +    list_for_each_entry ( msi, &pdev->msi_list, list )
> +        printk("%d ", msi->irq);
> +    printk(">");

While not an exact equivalent of the original code then, could I
talk you into adding an early list_empty() check, suppressing any
output from this function if that one returned "true"?

> @@ -1271,18 +1249,16 @@ bool_t pcie_aer_get_firmware_first(const struct 
> pci_dev *pdev)
>  static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
>  {
>      struct pci_dev *pdev;
> -    struct msi_desc *msi;
>  
>      printk("==== segment %04x ====\n", pseg->nr);
>  
>      list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>      {
> -        printk("%pp - %pd - node %-3d - MSIs < ",
> +        printk("%pp - %pd - node %-3d ",

Together with the request above the trailin blank here also wants to
become a leading blank in pdev_dump_msi().

> --- /dev/null
> +++ b/xen/include/xen/msi.h
> @@ -0,0 +1,56 @@
> +/*
> + * Copyright (C) 2008,  Netronome Systems, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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 __XEN_MSI_H_
> +#define __XEN_MSI_H_
> +
> +#ifdef CONFIG_HAS_PCI_MSI
> +
> +#include <asm/msi.h>
> +
> +int pdev_msix_assign(struct domain *d, struct pci_dev *pdev);
> +int pdev_msi_init(struct pci_dev *pdev);
> +void pdev_msi_deinit(struct pci_dev *pdev);
> +void pdev_dump_msi(const struct pci_dev *pdev);
> +
> +#else /* !CONFIG_HAS_PCI_MSI */
> +static inline int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)

Please be consistent with blank lines you add; here you also want one
after the #else.

> +{
> +    return 0;
> +}
> +
> +static inline int pdev_msi_init(struct pci_dev *pdev)
> +{
> +    return 0;
> +}
> +
> +static inline void pdev_msi_deinit(struct pci_dev *pdev) {}
> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
> +static inline void pdev_dump_msi(const struct pci_dev *pdev) {}

Especially for (but perhaps not limited to) this !HAS_PCI_MSI case
(where you don't include asm/msi.h and its possible dependents)
please forward-declare struct-s you use in prototypes or inline
stubs (outside the #ifdef, that is). This will allow including
this header without having to care about prereq headers.

If you agree with and make all the suggested or requested changes,
feel free to add
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan