Re: [Xen-devel] [PATCH v3 02/27] x86/xen/MSI: Eliminate arch_msix_mask_irq() and arch_msi_mask_irq()

[Yijing Wang <wangyijing@xxxxxxxxxx>]

On 2014/10/23 12:25, Bjorn Helgaas wrote:
> On Wed, Oct 15, 2014 at 11:06:50AM +0800, Yijing Wang wrote:
>> Commit 0e4ccb1505a9 ("PCI: Add x86_msi.msi_mask_irq() and msix_mask_irq()")
>> introduced two __weak arch functions arch_msix_mask_irq() and
>> arch_msi_mask_irq() to work around a bug when running xen in x86.
>> These two functions made msi code more complex. This patch reverts
>> the commit and introduces a global flag to control msi mask action to
>> avoid the bug. The patch is also preparation for using MSI chip framework
>> instead of weak arch MSI functions in all platforms. Keep 
>> default_msi_mask_irq()
>> and default_msix_mask_irq() in linux/msi.h to make s390 MSI code compile
>> happy, they will be removed in the later patch.
> 
> This patch is basically a revert of 0e4ccb1505a9 intermingled with the
> addition of the new pci_msi_ignore_mask technique.
> 
> Can you please split this into two patches:
> 
>   - Add the pci_msi_ignore_mask stuff (alongside the existing way)
>   - Revert 0e4ccb1505a9
> 
> I think that will make it easier to see what's going on.

OK, I will do it, thanks.

Thanks!
Yijing.

> 
>> Signed-off-by: Yijing Wang <wangyijing@xxxxxxxxxx>
>> CC: David Vrabel <david.vrabel@xxxxxxxxxx>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> ---
>> Hi David and Konrad,
>>    I dropped the Acked-by and tested-by, because this version has a
>> lot changes compared to last. So, I guess you may want to check it again.
>> ---
>>  arch/x86/include/asm/x86_init.h |    3 ---
>>  arch/x86/kernel/x86_init.c      |   10 ----------
>>  arch/x86/pci/xen.c              |   14 ++------------
>>  drivers/pci/msi.c               |   29 ++++++++++++-----------------
>>  include/linux/msi.h             |    8 ++++++--
>>  5 files changed, 20 insertions(+), 44 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/x86_init.h 
>> b/arch/x86/include/asm/x86_init.h
>> index e45e4da..f58a9c7 100644
>> --- a/arch/x86/include/asm/x86_init.h
>> +++ b/arch/x86/include/asm/x86_init.h
>> @@ -172,7 +172,6 @@ struct x86_platform_ops {
>>  
>>  struct pci_dev;
>>  struct msi_msg;
>> -struct msi_desc;
>>  
>>  struct x86_msi_ops {
>>      int (*setup_msi_irqs)(struct pci_dev *dev, int nvec, int type);
>> @@ -183,8 +182,6 @@ struct x86_msi_ops {
>>      void (*teardown_msi_irqs)(struct pci_dev *dev);
>>      void (*restore_msi_irqs)(struct pci_dev *dev);
>>      int  (*setup_hpet_msi)(unsigned int irq, unsigned int id);
>> -    u32 (*msi_mask_irq)(struct msi_desc *desc, u32 mask, u32 flag);
>> -    u32 (*msix_mask_irq)(struct msi_desc *desc, u32 flag);
>>  };
>>  
>>  struct IO_APIC_route_entry;
>> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
>> index e48b674..234b072 100644
>> --- a/arch/x86/kernel/x86_init.c
>> +++ b/arch/x86/kernel/x86_init.c
>> @@ -116,8 +116,6 @@ struct x86_msi_ops x86_msi = {
>>      .teardown_msi_irqs      = default_teardown_msi_irqs,
>>      .restore_msi_irqs       = default_restore_msi_irqs,
>>      .setup_hpet_msi         = default_setup_hpet_msi,
>> -    .msi_mask_irq           = default_msi_mask_irq,
>> -    .msix_mask_irq          = default_msix_mask_irq,
>>  };
>>  
>>  /* MSI arch specific hooks */
>> @@ -140,14 +138,6 @@ void arch_restore_msi_irqs(struct pci_dev *dev)
>>  {
>>      x86_msi.restore_msi_irqs(dev);
>>  }
>> -u32 arch_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
>> -{
>> -    return x86_msi.msi_mask_irq(desc, mask, flag);
>> -}
>> -u32 arch_msix_mask_irq(struct msi_desc *desc, u32 flag)
>> -{
>> -    return x86_msi.msix_mask_irq(desc, flag);
>> -}
>>  #endif
>>  
>>  struct x86_io_apic_ops x86_io_apic_ops = {
>> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
>> index 093f5f4..e5b8b78 100644
>> --- a/arch/x86/pci/xen.c
>> +++ b/arch/x86/pci/xen.c
>> @@ -394,14 +394,6 @@ static void xen_teardown_msi_irq(unsigned int irq)
>>  {
>>      xen_destroy_irq(irq);
>>  }
>> -static u32 xen_nop_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
>> -{
>> -    return 0;
>> -}
>> -static u32 xen_nop_msix_mask_irq(struct msi_desc *desc, u32 flag)
>> -{
>> -    return 0;
>> -}
>>  #endif
>>  
>>  int __init pci_xen_init(void)
>> @@ -425,8 +417,7 @@ int __init pci_xen_init(void)
>>      x86_msi.setup_msi_irqs = xen_setup_msi_irqs;
>>      x86_msi.teardown_msi_irq = xen_teardown_msi_irq;
>>      x86_msi.teardown_msi_irqs = xen_teardown_msi_irqs;
>> -    x86_msi.msi_mask_irq = xen_nop_msi_mask_irq;
>> -    x86_msi.msix_mask_irq = xen_nop_msix_mask_irq;
>> +    pci_msi_ignore_mask = 1;
>>  #endif
>>      return 0;
>>  }
>> @@ -506,8 +497,7 @@ int __init pci_xen_initial_domain(void)
>>      x86_msi.setup_msi_irqs = xen_initdom_setup_msi_irqs;
>>      x86_msi.teardown_msi_irq = xen_teardown_msi_irq;
>>      x86_msi.restore_msi_irqs = xen_initdom_restore_msi_irqs;
>> -    x86_msi.msi_mask_irq = xen_nop_msi_mask_irq;
>> -    x86_msi.msix_mask_irq = xen_nop_msix_mask_irq;
>> +    pci_msi_ignore_mask = 1;
>>  #endif
>>      xen_setup_acpi_sci();
>>      __acpi_register_gsi = acpi_register_gsi_xen;
>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index ecb92a5..22e413c 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
>> @@ -23,6 +23,7 @@
>>  #include "pci.h"
>>  
>>  static int pci_msi_enable = 1;
>> +int pci_msi_ignore_mask;
>>  
>>  #define msix_table_size(flags)      ((flags & PCI_MSIX_FLAGS_QSIZE) + 1)
>>  
>> @@ -162,11 +163,11 @@ static inline __attribute_const__ u32 
>> msi_mask(unsigned x)
>>   * reliably as devices without an INTx disable bit will then generate a
>>   * level IRQ which will never be cleared.
>>   */
>> -u32 default_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
>> +u32 __msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
>>  {
>>      u32 mask_bits = desc->masked;
>>  
>> -    if (!desc->msi_attrib.maskbit)
>> +    if (pci_msi_ignore_mask || !desc->msi_attrib.maskbit)
>>              return 0;
>>  
>>      mask_bits &= ~mask;
>> @@ -176,14 +177,9 @@ u32 default_msi_mask_irq(struct msi_desc *desc, u32 
>> mask, u32 flag)
>>      return mask_bits;
>>  }
>>  
>> -__weak u32 arch_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
>> -{
>> -    return default_msi_mask_irq(desc, mask, flag);
>> -}
>> -
>>  static void msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
>>  {
>> -    desc->masked = arch_msi_mask_irq(desc, mask, flag);
>> +    desc->masked = __msi_mask_irq(desc, mask, flag);
>>  }
>>  
>>  /*
>> @@ -193,11 +189,15 @@ static void msi_mask_irq(struct msi_desc *desc, u32 
>> mask, u32 flag)
>>   * file.  This saves a few milliseconds when initialising devices with lots
>>   * of MSI-X interrupts.
>>   */
>> -u32 default_msix_mask_irq(struct msi_desc *desc, u32 flag)
>> +u32 __msix_mask_irq(struct msi_desc *desc, u32 flag)
>>  {
>>      u32 mask_bits = desc->masked;
>>      unsigned offset = desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
>>                                              PCI_MSIX_ENTRY_VECTOR_CTRL;
>> +
>> +    if (pci_msi_ignore_mask)
>> +            return 0;
>> +
>>      mask_bits &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
>>      if (flag)
>>              mask_bits |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
>> @@ -206,14 +206,9 @@ u32 default_msix_mask_irq(struct msi_desc *desc, u32 
>> flag)
>>      return mask_bits;
>>  }
>>  
>> -__weak u32 arch_msix_mask_irq(struct msi_desc *desc, u32 flag)
>> -{
>> -    return default_msix_mask_irq(desc, flag);
>> -}
>> -
>>  static void msix_mask_irq(struct msi_desc *desc, u32 flag)
>>  {
>> -    desc->masked = arch_msix_mask_irq(desc, flag);
>> +    desc->masked = __msix_mask_irq(desc, flag);
>>  }
>>  
>>  static void msi_set_mask_bit(struct irq_data *data, u32 flag)
>> @@ -866,7 +861,7 @@ void pci_msi_shutdown(struct pci_dev *dev)
>>      /* Return the device with MSI unmasked as initial states */
>>      mask = msi_mask(desc->msi_attrib.multi_cap);
>>      /* Keep cached state to be restored */
>> -    arch_msi_mask_irq(desc, mask, ~mask);
>> +    __msi_mask_irq(desc, mask, ~mask);
>>  
>>      /* Restore dev->irq to its default pin-assertion irq */
>>      dev->irq = desc->msi_attrib.default_irq;
>> @@ -964,7 +959,7 @@ void pci_msix_shutdown(struct pci_dev *dev)
>>      /* Return the device with MSI-X masked as initial states */
>>      list_for_each_entry(entry, &dev->msi_list, list) {
>>              /* Keep cached states to be restored */
>> -            arch_msix_mask_irq(entry, 1);
>> +            __msix_mask_irq(entry, 1);
>>      }
>>  
>>      msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
>> diff --git a/include/linux/msi.h b/include/linux/msi.h
>> index 44f4746..9ac1e3b 100644
>> --- a/include/linux/msi.h
>> +++ b/include/linux/msi.h
>> @@ -10,6 +10,8 @@ struct msi_msg {
>>      u32     data;           /* 16 bits of msi message data */
>>  };
>>  
>> +extern int pci_msi_ignore_mask;
>> +
>>  /* Helper functions */
>>  struct irq_data;
>>  struct msi_desc;
>> @@ -21,6 +23,8 @@ void __write_msi_msg(struct msi_desc *entry, struct 
>> msi_msg *msg);
>>  void read_msi_msg(unsigned int irq, struct msi_msg *msg);
>>  void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg);
>>  void write_msi_msg(unsigned int irq, struct msi_msg *msg);
>> +u32 __msix_mask_irq(struct msi_desc *desc, u32 flag);
>> +u32 __msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag);
>>  
>>  struct msi_desc {
>>      struct {
>> @@ -61,8 +65,8 @@ void arch_restore_msi_irqs(struct pci_dev *dev);
>>  
>>  void default_teardown_msi_irqs(struct pci_dev *dev);
>>  void default_restore_msi_irqs(struct pci_dev *dev);
>> -u32 default_msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag);
>> -u32 default_msix_mask_irq(struct msi_desc *desc, u32 flag);
>> +#define default_msi_mask_irq        __msi_mask_irq
>> +#define default_msix_mask_irq       __msix_mask_irq
>>  
>>  struct msi_chip {
>>      struct module *owner;
>> -- 
>> 1.7.1
>>
> 
> .
> 


-- 
Thanks!
Yijing


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel