Re: [PATCH v4 2/3] xen/pci: Refactor PCI MSI intercept related code

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> +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);

> @@ -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.

> --- /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)?

> @@ -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?

Jan