Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.04.2021 12:49, Roger Pau Monné wrote:
> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
>>          return;
>>  }
>>  
>> +int alloc_pci_msi(struct pci_dev *pdev)
> 
> I would rather name it pci_msi_init...

Or maybe pdev_msi_init(), as pci_msi_init() looks more like a one-
time, global init function?

>> +void free_pci_msi(struct pci_dev *pdev)
> 
> ...and pci_msi_free.

The counterpart of "init" really would be "deinit", IOW I'd like to
ask for either alloc/free or init/deinit.

>> +{
>> +    xfree(pdev->msix);

Could this maybe become XFREE() at this occasion?

>> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
> 
> This kind of a confusing name - what about pci_msix_assign?
> 
>> +{
>> +    int rc;
>> +
>> +    if ( pdev->msix )

Wouldn't this check better live in the sole caller?


>> +void dump_pci_msi(struct pci_dev *pdev)

pdev_dump_msi() or some such?

Also - const here and ...

>> +{
>> +    struct msi_desc *msi;

... here please, while you already move this code.

>> +    list_for_each_entry ( msi, &pdev->msi_list, list )
>> +        printk("%d ", msi->irq);
>> +}
> 
> I wonder, those those function really want to be in a x86 specific
> file? There's nothing x86 specific about them AFAICT.
> 
> Would it make sense to create a separate msi-intercept.c file with
> those that gets included when CONFIG_PCI_MSI_INTERCEPT?

+1

>> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>>  }
>>  __initcall(setup_dump_pcidevs);
>>  
>> +
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT

No double blank lines please.

>>  int iommu_update_ire_from_msi(
>>      struct msi_desc *msi_desc, struct msi_msg *msg)
>>  {
>>      return iommu_intremap
>>             ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>>  }
>> +#endif
> 
> This is not exactly related to MSI intercepts, the IOMMU interrupt
> remapping table will also be used for interrupt entries for devices
> being used by Xen directly, where no intercept is required.
> 
> And then you also want to gate the hook from iommu_ops itself with
> CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.

I think the two aspects of MSI should be kept separate.

>> --- a/xen/drivers/pci/Kconfig
>> +++ b/xen/drivers/pci/Kconfig
>> @@ -1,3 +1,7 @@
>>  
>>  config HAS_PCI
>>      bool
>> +
>> +config PCI_MSI_INTERCEPT
>> +    def_bool y
>> +    depends on X86 && HAS_PCI

Depending on HAS_PCI is fine (albeit not strictly needed afaict),
but this shouldn't have a default (like HAS_PCI doesn't) and
instead be selected by x86.

>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -48,8 +48,7 @@ void vpci_remove_device(struct pci_dev *pdev)
>>          xfree(r);
>>      }
>>      spin_unlock(&pdev->vpci->lock);
>> -    xfree(pdev->vpci->msix);
>> -    xfree(pdev->vpci->msi);
>> +    free_vpci_msi(pdev);

I don't think the function needs to be passed a pdev, and ...
>>      xfree(pdev->vpci);
>>      pdev->vpci = NULL;

... it would only be consistent with this if pdev->vpci was passed
instead.

>> --- /dev/null
>> +++ b/xen/include/asm-arm/msi.h
>> @@ -0,0 +1,44 @@
>> +/*
>> + * Copyright (C) 2021 Arm Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that 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 __ASM_MSI_H_
>> +#define __ASM_MSI_H_
>> +
>> +static inline int alloc_pci_msi(struct pci_dev *pdev)
>> +{
>> +    return 0;
>> +}
>> +
>> +static inline int pci_assign_msix_init(struct domain *d, struct pci_dev 
>> *pdev)
>> +{
>> +    return 0;
>> +}
>> +
>> +static inline void dump_pci_msi(struct pci_dev *pdev) {}
>> +static inline void free_pci_msi(struct pci_dev *pdev) {}
>> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
>> +
>> +#endif /* __ASM_MSI_H */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
> 
> Should you instead have a non-arch specific file with those dummy
> helpers that get used when !CONFIG_PCI_MSI_INTERCEPT?

+1

>> --- a/xen/include/asm-x86/msi.h
>> +++ b/xen/include/asm-x86/msi.h
>> @@ -252,5 +252,9 @@ void unmask_msi_irq(struct irq_desc *);
>>  void guest_mask_msi_irq(struct irq_desc *, bool mask);
>>  void ack_nonmaskable_msi_irq(struct irq_desc *);
>>  void set_msi_affinity(struct irq_desc *, const cpumask_t *);
>> +int alloc_pci_msi(struct pci_dev *pdev);
>> +void free_pci_msi(struct pci_dev *pdev);
>> +void dump_pci_msi(struct pci_dev *pdev);
>> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev);

These should then live in the other "half" of that header.

>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -79,10 +79,6 @@ struct pci_dev {
>>      struct list_head alldevs_list;
>>      struct list_head domain_list;
>>  
>> -    struct list_head msi_list;
>> -
>> -    struct arch_msix *msix;
>> -
>>      struct domain *domain;
>>  
>>      const union {
>> @@ -94,7 +90,14 @@ struct pci_dev {
>>          pci_sbdf_t sbdf;
>>      };
>>  
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>> +    struct list_head msi_list;
>> +
>> +    struct arch_msix *msix;
>> +
>>      uint8_t msi_maxvec;
>> +#endif
>> +
>>      uint8_t phantom_stride;

Like Roger also said for struct vpci, it's not clear this is worth
it. And while you may have paid attention (and there simply is no
better arrangement) I'd also like to point out that such field
movement should be done while keeping padding hole sizes in mind.

>> @@ -174,7 +177,7 @@ int __must_check vpci_msix_arch_disable_entry(struct 
>> vpci_msix_entry *entry,
>>                                                const struct pci_dev *pdev);
>>  void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry);
>>  int vpci_msix_arch_print(const struct vpci_msix *msix);
>> -
>> +int remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem);
>>  /*
>>   * Helper functions to fetch MSIX related data. They are used by both the
>>   * emulated MSIX code and the BAR handlers.

Please don't remove blank lines like this one, unless you actually
see a reason.

Jan