[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 2/3] xen/pci: Refactor PCI MSI intercept related code
Hi Jan, > On 3 May 2021, at 3:38 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote: > > On 29.04.2021 16:46, Rahul Singh wrote: >> --- /dev/null >> +++ b/xen/drivers/passthrough/msi-intercept.c >> @@ -0,0 +1,53 @@ >> +/* >> + * 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/>. >> + */ >> + >> +#include <xen/init.h> >> +#include <xen/pci.h> >> +#include <asm/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; >> +} >> + >> +void pdev_dump_msi(const struct pci_dev *pdev) >> +{ >> + const struct msi_desc *msi; >> + >> + list_for_each_entry ( msi, &pdev->msi_list, list ) >> + printk("- MSIs < %d >", msi->irq); > > Only the %d and a blank should be part of the format string inside the > loop body; the rest wants printing exactly once. Yes I agree I missed this I will fix this in next patch. > >> +static inline size_t vmsix_table_size(const struct vpci *vpci, unsigned int >> nr) >> +{ >> + return >> + (nr == VPCI_MSIX_TABLE) ? vpci->msix->max_entries * >> PCI_MSIX_ENTRY_SIZE >> + : >> ROUNDUP(DIV_ROUND_UP(vpci->msix->max_entries, >> + 8), 8); > > I'm afraid I don't view this as an acceptable way of wrapping lines. > How about > > return (nr == VPCI_MSIX_TABLE) > ? vpci->msix->max_entries * PCI_MSIX_ENTRY_SIZE > : ROUNDUP(DIV_ROUND_UP(vpci->msix->max_entries, 8), 8); Ok. > >> @@ -428,6 +458,31 @@ int vpci_make_msix_hole(const struct pci_dev *pdev) >> return 0; >> } >> >> +int vpci_remove_msix_regions(const struct vpci *vpci, struct rangeset *mem) >> +{ >> + const struct vpci_msix *msix = vpci->msix; >> + unsigned int i; >> + int rc; >> + >> + for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ ) >> + { >> + unsigned long start = PFN_DOWN(vmsix_table_addr(vpci, i)); >> + unsigned long end = PFN_DOWN(vmsix_table_addr(vpci, i) + >> + vmsix_table_size(vpci, i) - 1); >> + >> + rc = rangeset_remove_range(mem, start, end); >> + if ( rc ) >> + { >> + printk(XENLOG_G_WARNING >> + "Failed to remove MSIX table [%lx, %lx]: %d\n", >> + start, end, rc); > > Indentation looks to be off by one space on the last two lines. Ok. > >> --- /dev/null >> +++ b/xen/include/xen/msi-intercept.h >> @@ -0,0 +1,49 @@ >> +/* >> + * 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_INTERCEPT_H_ >> +#define __XEN_MSI_INTERCEPT_H_ >> + >> +#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT >> + >> +#include <asm/msi.h> >> + >> +int pdev_msix_assign(struct domain *d, struct pci_dev *pdev); >> +void pdev_dump_msi(const struct pci_dev *pdev); >> + >> +#else /* !CONFIG_HAS_PCI_MSI_INTERCEPT */ >> + >> +static inline int pdev_msix_assign(struct domain *d, struct pci_dev *pdev) >> +{ >> + return 0; >> +} >> + >> +static inline void pdev_dump_msi(const struct pci_dev *pdev) {} >> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {} > > I don't think this last one is intercept related (and hence doesn't belong > here)? > Ok I will move this to next patch in series. >> @@ -148,6 +150,7 @@ struct vpci_vcpu { >> }; >> >> #ifdef __XEN__ >> +#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT > > Since both start and ... > >> +static inline void vpci_msi_free(struct vpci *vpci) {} >> +#endif /* CONFIG_HAS_PCI_MSI_INTERCEPT */ >> #endif /* __XEN__ */ > > ... end look to match, may I suggest to simply replace the __XEN__ ones, > as the test harness isn't supposed to (randomly) define CONFIG_*? Or > alternatively at least combine both #ifdef-s? Ok I will replace the line "#ifdef __XEN__" with " #ifdef CONFIG_HAS_PCI_MSI_INTERCEPT” Regards, Rahul > > Jan >
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |