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

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

Hi Jan,

> On 3 May 2021, at 3:38 pm, Jan Beulich <jbeulich@suse.com> 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
>