Hi Jan,
> On 3 May 2021, at 3:38 pm, Jan Beulich <[email protected]> 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
>
