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

[Rahul Singh <Rahul.Singh@arm.com>]

Hi Jan,

> On 24 Aug 2021, at 4:53 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> 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.

Let me remove the "Copyright Copyright (C) 2008,  Netronome Systems, Inc.” . 
Can you please help me what copyright I will add for the next patch ?
> 
>> + * 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.
Ok . Let me include here “xen/msi.h” and move other required includes to “xen/msi.h"
> 
>> +#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”?
Ok.
> 
>> @@ -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()
Ok.
>> --- /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.

Ok.

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

Ok. Let me do modification in next version.

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