Hi Jan, > On 24 Aug 2021, at 4:53 pm, Jan Beulich <[email protected]> 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 <[email protected]> > > Jan >
