[8/9] PCI: Ignore BAR contents when firmware left decoding disabled

Commit Message

Bjorn Helgaas March 19, 2014, 6:54 p.m. UTC

[+cc Ming, Rusty, Pekka, Sasha]

On Wed, Feb 26, 2014 at 12:37:57PM -0700, Bjorn Helgaas wrote:
> Don't rely on BAR contents when the command register says the BAR is
> disabled.
> 
> If we receive a PCI device from firmware (or a hot-added device that was
> just powered up) with the MEMORY or IO enable bits in the PCI command
> register cleared, there's no reason to believe the BARs contain valid
> addresses.
> 
> In that case, we still know the type and size of the BAR, but this
> patch marks the resource as "unset" so we have a chance to reassign it.
> 
> Historically, we often used "BAR == 0" to decide the BAR is invalid.  But 0
> is a legal BAR value, especially if the host bridge translates addresses,
> so I think it's better to decide based on the PCI command register, and
> store the conclusion in the IORESOURCE_UNSET bit.

I plan to replace this patch with the following, which only sets
IORESOURCE_UNSET when we already have been clearing the bus region start
address.  (This probably should have been a separate patch to begin with,
mea culpa.)

This is intended for the v3.15 merge window, so I made the minimal change
to reduce risk.

Thanks to Ming Lei for prompting me to look at this; I think the issue he
reported with the original patch is really a problem somewhere else that
the patch just happened to expose, but the original patch was more
aggressive than necessary, so this revision tones it down a bit.

Bjorn


PCI: Mark 64-bit resource as IORESOURCE_UNSET if we only support 32-bit

From: Bjorn Helgaas <[e-mail]>

If we don't support 64-bit addresses, i.e., CONFIG_PHYS_ADDR_T_64BIT is not
set, we can't deal with BARs above 4GB.  In this case we already pretend
the BAR contained zero; this patch also sets IORESOURCE_UNSET so we can try
to reallocate it later.

I don't think this is exactly correct: what we care about here are *bus*
addresses, not CPU addresses, so the tests of sizeof(resource_size_t)
probably should be on sizeof(dma_addr_t) instead.  But this is what's been
in -next, so we'll fix that later.

Signed-off-by: Bjorn Helgaas <[e-mail]>
---
 drivers/pci/probe.c |    1 +
 1 file changed, 1 insertion(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bjorn Helgaas March 19, 2014, 9:16 p.m. UTC | #1

On Wed, Mar 19, 2014 at 12:54 PM, Bjorn Helgaas <[e-mail]> wrote:
> [+cc Ming, Rusty, Pekka, Sasha]
> ...
> I plan to replace this patch with the following, which only sets
> IORESOURCE_UNSET when we already have been clearing the bus region start
> address.  (This probably should have been a separate patch to begin with,
> mea culpa.)
>
> This is intended for the v3.15 merge window, so I made the minimal change
> to reduce risk.

I put this patch in my pci/resource branch and re-merged it into my
"next" branch.  This rebased both pci/resource and next, which is
unfortunate, but I think it's the cleanest and least risky way at this
point.

Bjorn

> PCI: Mark 64-bit resource as IORESOURCE_UNSET if we only support 32-bit
>
> From: Bjorn Helgaas <[e-mail]>
>
> If we don't support 64-bit addresses, i.e., CONFIG_PHYS_ADDR_T_64BIT is not
> set, we can't deal with BARs above 4GB.  In this case we already pretend
> the BAR contained zero; this patch also sets IORESOURCE_UNSET so we can try
> to reallocate it later.
>
> I don't think this is exactly correct: what we care about here are *bus*
> addresses, not CPU addresses, so the tests of sizeof(resource_size_t)
> probably should be on sizeof(dma_addr_t) instead.  But this is what's been
> in -next, so we'll fix that later.
>
> Signed-off-by: Bjorn Helgaas <[e-mail]>
> ---
>  drivers/pci/probe.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6e34498ec9f0..78335efbbb74 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -252,6 +252,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>                         /* Address above 32-bit boundary; disable the BAR */
>                         pci_write_config_dword(dev, pos, 0);
>                         pci_write_config_dword(dev, pos + 4, 0);
> +                       res->flags |= IORESOURCE_UNSET;
>                         region.start = 0;
>                         region.end = sz64;
>                         bar_disabled = true;
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sasha Levin March 19, 2014, 9:23 p.m. UTC | #2

On 03/19/2014 05:16 PM, Bjorn Helgaas wrote:
> On Wed, Mar 19, 2014 at 12:54 PM, Bjorn Helgaas<[e-mail]>  wrote:
>> >[+cc Ming, Rusty, Pekka, Sasha]
>> >...
>> >I plan to replace this patch with the following, which only sets
>> >IORESOURCE_UNSET when we already have been clearing the bus region start
>> >address.  (This probably should have been a separate patch to begin with,
>> >mea culpa.)
>> >
>> >This is intended for the v3.15 merge window, so I made the minimal change
>> >to reduce risk.
> I put this patch in my pci/resource branch and re-merged it into my
> "next" branch.  This rebased both pci/resource and next, which is
> unfortunate, but I think it's the cleanest and least risky way at this
> point.

Thanks for pointing out the issue Ming. I must admit that I haven't referred
to the PCI spec when sending in my "fix" since the upstream patch sort of
made sense. OTOH, you can't really call the kvm tool PCI implementation spec
compliant :)

Pekka, we can either revert my patch completely since the issue won't ever be
visible (since the pci tree got rebase) or just keep it in. Let me know what you
prefer.


Thanks,
Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6e34498ec9f0..78335efbbb74 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -252,6 +252,7 @@  int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 			/* Address above 32-bit boundary; disable the BAR */
 			pci_write_config_dword(dev, pos, 0);
 			pci_write_config_dword(dev, pos + 4, 0);
+			res->flags |= IORESOURCE_UNSET;
 			region.start = 0;
 			region.end = sz64;
 			bar_disabled = true;