| Message ID | 1422503683-19709-1-git-send-email-wangyijing@huawei.com |
|---|---|
| State | Accepted |
| Headers | show |
>>> On 29.01.15 at 04:54, <wangyijing@huawei.com> wrote: > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -694,11 +694,16 @@ static void __iomem *msix_map_region(struct pci_dev *dev, unsigned nr_entries) > { > resource_size_t phys_addr; > u32 table_offset; > + unsigned long flags; > u8 bir; > > pci_read_config_dword(dev, dev->msix_cap + PCI_MSIX_TABLE, > &table_offset); > bir = (u8)(table_offset & PCI_MSIX_TABLE_BIR); > + flags = pci_resource_flags(dev, bir); > + if (!flags || (flags & IORESOURCE_UNSET)) > + return NULL; Mind explaining what relevance the checking of flags against zero has (also in the Xen counterpart)? The patch description says nothing in this regard... Jan -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 29, 2015 at 7:15 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 29.01.15 at 04:54, <wangyijing@huawei.com> wrote: >> --- a/drivers/pci/msi.c >> +++ b/drivers/pci/msi.c >> @@ -694,11 +694,16 @@ static void __iomem *msix_map_region(struct pci_dev *dev, unsigned nr_entries) >> { >> resource_size_t phys_addr; >> u32 table_offset; >> + unsigned long flags; >> u8 bir; >> >> pci_read_config_dword(dev, dev->msix_cap + PCI_MSIX_TABLE, >> &table_offset); >> bir = (u8)(table_offset & PCI_MSIX_TABLE_BIR); >> + flags = pci_resource_flags(dev, bir); >> + if (!flags || (flags & IORESOURCE_UNSET)) >> + return NULL; > > Mind explaining what relevance the checking of flags against zero has > (also in the Xen counterpart)? The patch description says nothing in > this regard... It's buried in there: > reset_resource() -->res->start/end/flags = 0 If we fail to assign resources to a BAR, in some cases we set res->flags = 0. There are other cases where we set IORESOURCE_UNSET, and that's the direction I want to go for all situations where we don't assign resources. But right now, we have a mix where flags is zero in some cases (including the one Zhang found), and has IORESOURCE_UNSET in others. I'll try to wordsmith this in the changelog when I apply this. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/1/29 22:12, Bjorn Helgaas wrote: > On Thu, Jan 29, 2015 at 7:15 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 29.01.15 at 04:54, <wangyijing@huawei.com> wrote: >>> --- a/drivers/pci/msi.c >>> +++ b/drivers/pci/msi.c >>> @@ -694,11 +694,16 @@ static void __iomem *msix_map_region(struct pci_dev *dev, unsigned nr_entries) >>> { >>> resource_size_t phys_addr; >>> u32 table_offset; >>> + unsigned long flags; >>> u8 bir; >>> >>> pci_read_config_dword(dev, dev->msix_cap + PCI_MSIX_TABLE, >>> &table_offset); >>> bir = (u8)(table_offset & PCI_MSIX_TABLE_BIR); >>> + flags = pci_resource_flags(dev, bir); >>> + if (!flags || (flags & IORESOURCE_UNSET)) >>> + return NULL; >> >> Mind explaining what relevance the checking of flags against zero has >> (also in the Xen counterpart)? The patch description says nothing in >> this regard... > > It's buried in there: > >> reset_resource() -->res->start/end/flags = 0 > > If we fail to assign resources to a BAR, in some cases we set > res->flags = 0. There are other cases where we set IORESOURCE_UNSET, > and that's the direction I want to go for all situations where we > don't assign resources. But right now, we have a mix where flags is > zero in some cases (including the one Zhang found), and has > IORESOURCE_UNSET in others. > > I'll try to wordsmith this in the changelog when I apply this. Thanks to Bjorn help explain, sorry for my poor description. > > Bjorn > > . >
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c index c489ef2..34fc418 100644 --- a/arch/x86/pci/xen.c +++ b/arch/x86/pci/xen.c @@ -298,12 +298,16 @@ static int xen_initdom_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) map_irq.entry_nr = nvec; } else if (type == PCI_CAP_ID_MSIX) { int pos; + unsigned long flags; u32 table_offset, bir; pos = dev->msix_cap; pci_read_config_dword(dev, pos + PCI_MSIX_TABLE, &table_offset); bir = (u8)(table_offset & PCI_MSIX_TABLE_BIR); + flags = pci_resource_flags(dev, bir); + if (!flags || (flags & IORESOURCE_UNSET)) + return -EINVAL; map_irq.table_base = pci_resource_start(dev, bir); map_irq.entry_nr = msidesc->msi_attrib.entry_nr; diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index fd60806..c3e7dfc 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -694,11 +694,16 @@ static void __iomem *msix_map_region(struct pci_dev *dev, unsigned nr_entries) { resource_size_t phys_addr; u32 table_offset; + unsigned long flags; u8 bir; pci_read_config_dword(dev, dev->msix_cap + PCI_MSIX_TABLE, &table_offset); bir = (u8)(table_offset & PCI_MSIX_TABLE_BIR); + flags = pci_resource_flags(dev, bir); + if (!flags || (flags & IORESOURCE_UNSET)) + return NULL; + table_offset &= PCI_MSIX_TABLE_OFFSET; phys_addr = pci_resource_start(dev, bir) + table_offset;