[Re-added linux-pci, which got lost again somewhere.]
On Monday, May 31, 2010 05:12:00 am Mike Travis wrote:
> H. Peter Anvin wrote:
> > On 05/28/2010 10:10 AM, Mike Travis wrote:
> >> H. Peter Anvin wrote:
> >>> On 05/28/2010 09:53 AM, Mike Travis wrote:
> >>>> Any further consideration for this patch, or has it been rejected?
I'm disappointed that you didn't rework this to make it generic,
not x86-specific. That would be pretty easy and would remove
the need for somebody else to come and clean it up later.
> >>> Well, it's really up to Jesse, but as far as I can see, this patch is a
> >>> net loss of functionality and doesn't actually add anything. Without
> >>> this patch, some resources that were not assigned by BIOS will be
> >>> unreachable. With this patch, *all* resources that were not assigned by
> >>> BIOS will be unreachable...
> >>>
> >>> -hpa
> >>>
> >> Apparently you're missing the point of the patch? The patch is needed
> >> because BIOS is purposely not assigning I/O BAR's to devices that won't
> >> use them, freeing up the resource for devices that do need them. Where
> >> is the "all" resources that are not reachable?
> >
> > No, the patch isn't needed for those.
> >
> > Without your patch:
> >
> > - Devices assigned by BIOS remain assigned;
> > - Devices not assigned by BIOS get assigned until address space
> > exhausted.
> >
> > With your patch:
> >
> > - Devices assigned by BIOS remain assigned;
> > - Devices not assigned by BIOS never get assigned at all.
> >
> > What am I missing here?
>
> BIOS still assigns the MMIO BAR's so the devices are alive.
I'm sorry; I don't follow this. BIOS assigns MMIO BARs regardless
of whether we have your patch.
I'm still having trouble reconciling the stated purpose, i.e., the
changelog, with the behavior. The changelog implies that the patch
is required to make >16 devices with I/O BARs work at all, but per
Mike Habeck, the patch just gets rid of some warnings and maybe helps
with hot-add of devices using I/O space.
Is there a deeper problem that happens if we exhaust I/O space?
Are we releasing device resources in pci_assign_unassigned_bridge_resources()
and then we fail to reassign even MMIO resources after we exhaust
I/O space?
Maybe a complete dmesg log showing the failure would be helpful. if
so, you could open a kernel.org bugzilla and reference it in your
changelog so we can take this issue into account in future PCI work.
Bjorn
On 06/01/2010 03:49 PM, Bjorn Helgaas wrote:
>>
>> BIOS still assigns the MMIO BAR's so the devices are alive.
>
> I'm sorry; I don't follow this. BIOS assigns MMIO BARs regardless
> of whether we have your patch.
>
I'm assuming that that Mike is implying is that the allocation code runs
out of I/O space and as a result shuts down the entire device.
-hpa
On Wednesday, June 02, 2010 01:31:18 am H. Peter Anvin wrote:
> On 06/01/2010 03:49 PM, Bjorn Helgaas wrote:
> >>
> >> BIOS still assigns the MMIO BAR's so the devices are alive.
> >
> > I'm sorry; I don't follow this. BIOS assigns MMIO BARs regardless
> > of whether we have your patch.
>
> I'm assuming that that Mike is implying is that the allocation code runs
> out of I/O space and as a result shuts down the entire device.
Yeah, that's why I asked about a deeper problem. There's not really a
"shut down this device" flag, so the only way I can think of that we
might make a device completely unusable is if we release all the device
resources and then fail to reassign them.
A concrete example, e.g., a dmesg log, would go a long ways toward
clarifying this.
Bjorn
On 06/02/2010 05:45 PM, Bjorn Helgaas wrote:
> On Wednesday, June 02, 2010 01:31:18 am H. Peter Anvin wrote:
>> On 06/01/2010 03:49 PM, Bjorn Helgaas wrote:
>>>>
>>>> BIOS still assigns the MMIO BAR's so the devices are alive.
>>>
>>> I'm sorry; I don't follow this. BIOS assigns MMIO BARs regardless
>>> of whether we have your patch.
>>
>> I'm assuming that that Mike is implying is that the allocation code runs
>> out of I/O space and as a result shuts down the entire device.
>
> Yeah, that's why I asked about a deeper problem. There's not really a
> "shut down this device" flag, so the only way I can think of that we
> might make a device completely unusable is if we release all the device
> resources and then fail to reassign them.
>
> A concrete example, e.g., a dmesg log, would go a long ways toward
> clarifying this.
>
That's what I thought, which I guess means my original question to Mike
still stands...
-hpa
Bjorn Helgaas wrote:
> [Re-added linux-pci, which got lost again somewhere.]
>
> On Monday, May 31, 2010 05:12:00 am Mike Travis wrote:
>> H. Peter Anvin wrote:
>>> On 05/28/2010 10:10 AM, Mike Travis wrote:
>>>> H. Peter Anvin wrote:
>>>>> On 05/28/2010 09:53 AM, Mike Travis wrote:
>>>>>> Any further consideration for this patch, or has it been rejected?
>
> I'm disappointed that you didn't rework this to make it generic,
> not x86-specific. That would be pretty easy and would remove
> the need for somebody else to come and clean it up later.
>
>>>>> Well, it's really up to Jesse, but as far as I can see, this patch is a
>>>>> net loss of functionality and doesn't actually add anything. Without
>>>>> this patch, some resources that were not assigned by BIOS will be
>>>>> unreachable. With this patch, *all* resources that were not assigned by
>>>>> BIOS will be unreachable...
>>>>>
>>>>> -hpa
>>>>>
>>>> Apparently you're missing the point of the patch? The patch is needed
>>>> because BIOS is purposely not assigning I/O BAR's to devices that won't
>>>> use them, freeing up the resource for devices that do need them. Where
>>>> is the "all" resources that are not reachable?
>>> No, the patch isn't needed for those.
>>>
>>> Without your patch:
>>>
>>> - Devices assigned by BIOS remain assigned;
>>> - Devices not assigned by BIOS get assigned until address space
>>> exhausted.
>>>
>>> With your patch:
>>>
>>> - Devices assigned by BIOS remain assigned;
>>> - Devices not assigned by BIOS never get assigned at all.
>>>
>>> What am I missing here?
>> BIOS still assigns the MMIO BAR's so the devices are alive.
>
> I'm sorry; I don't follow this. BIOS assigns MMIO BARs regardless
> of whether we have your patch.
>
> I'm still having trouble reconciling the stated purpose, i.e., the
> changelog, with the behavior. The changelog implies that the patch
> is required to make >16 devices with I/O BARs work at all, but per
> Mike Habeck, the patch just gets rid of some warnings and maybe helps
> with hot-add of devices using I/O space.
greaterthan 16 devices will still work if the BIOS doesn't assign
the I/O BARs and the kernel does (including the devices that don't
get assigned due to the kernel running out of I/O Port resources).
And when the kernel runs out of I/O Port space it will warn for
those devices it couldn't assign: (example):
pci 0002:03:00.0: BAR 5: can't allocate I/O resource [0x0-0x7f]
But if the kernel assigns all the I/O Port space to these devices
we know don't need it (thus the reason the BIOS didn't assign it)
then I believe hot-add of devices using I/O space will fail.
What the patch is attempting to do is allow the BIOS a way to
not assign resources it knows are not needed and thus make sure
the kernel doesn't override that.
There is also the issue that quirk_system_pci_resources() thinks
those unassigned I/O resources are using I/O port space 0x0-0xFF.
Since the BIOS never assigned the BAR the kernel reads it as
having a base of 0x0 and a limit of whatever the BAR size is when
it writes all 1's to obtain the size. So that results in
quirk_system_pci_resources() disabling pnp devices: (example):
pnp 00:11: io resource (0x92-0x92) overlaps 0002:03:00.0 BAR 0 (0x0-0xff), disabling
pnp 00:11: io resource (0x10-0x1f) overlaps 0002:03:00.0 BAR 0 (0x0-0xff), disabling
pnp 00:11: io resource (0x72-0x73) overlaps 0002:03:00.0 BAR 0 (0x0-0xff), disabling
pnp 00:11: io resource (0x80-0x80) overlaps 0002:03:00.0 BAR 0 (0x0-0xff), disabling
pnp 00:11: io resource (0x84-0x86) overlaps 0002:03:00.0 BAR 0 (0x0-0xff), disabling
pnp 00:11: io resource (0x88-0x88) overlaps 0002:03:00.0 BAR 0 (0x0-0xff), disabling
pnp 00:11: io resource (0x8c-0x8e) overlaps 0002:03:00.0 BAR 0 (0x0-0xff), disabling
pnp 00:11: io resource (0x90-0x9f) overlaps 0002:03:00.0 BAR 0 (0x0-0xff), disabling
One could argue this is a quirk_system_pci_resources() issue and
should be handled there rather than "zeroing out the resource if
the bios didn't assign it" as the patch does, but what the patch
is attempting to do (as stated above) is to allow the BIOS a way
to not assign resources it knows are not needed and thus make sure
the kernel doesn't override that... and in doing that the quirk
issue goes away too.
-mike
>
> Is there a deeper problem that happens if we exhaust I/O space?
> Are we releasing device resources in pci_assign_unassigned_bridge_resources()
> and then we fail to reassign even MMIO resources after we exhaust
> I/O space?
>
> Maybe a complete dmesg log showing the failure would be helpful. if
> so, you could open a kernel.org bugzilla and reference it in your
> changelog so we can take this issue into account in future PCI work.
>
> Bjorn
On Wed, 02 Jun 2010 17:47:23 +0200
"H. Peter Anvin" <[email protected]> wrote:
> On 06/02/2010 05:45 PM, Bjorn Helgaas wrote:
> > On Wednesday, June 02, 2010 01:31:18 am H. Peter Anvin wrote:
> >> On 06/01/2010 03:49 PM, Bjorn Helgaas wrote:
> >>>>
> >>>> BIOS still assigns the MMIO BAR's so the devices are alive.
> >>>
> >>> I'm sorry; I don't follow this. BIOS assigns MMIO BARs regardless
> >>> of whether we have your patch.
> >>
> >> I'm assuming that that Mike is implying is that the allocation code runs
> >> out of I/O space and as a result shuts down the entire device.
> >
> > Yeah, that's why I asked about a deeper problem. There's not really a
> > "shut down this device" flag, so the only way I can think of that we
> > might make a device completely unusable is if we release all the device
> > resources and then fail to reassign them.
> >
> > A concrete example, e.g., a dmesg log, would go a long ways toward
> > clarifying this.
> >
>
> That's what I thought, which I guess means my original question to Mike
> still stands...
I thought the whole reason for this was hotplug; we don't want to
exhaust I/O space unnecessarily by allocating resources for BARs the
BIOS didn't assign so we can keep them around for later hotplug
activity.
If there's some other issue, it's not too late to drop this patch.
Mike or Mike, can you clarify?
--
Jesse Barnes, Intel Open Source Technology Center
On Wednesday, June 02, 2010 09:53:23 am Mike Habeck wrote:
> Bjorn Helgaas wrote:
> > I'm still having trouble reconciling the stated purpose, i.e., the
> > changelog, with the behavior. The changelog implies that the patch
> > is required to make >16 devices with I/O BARs work at all, but per
> > Mike Habeck, the patch just gets rid of some warnings and maybe helps
> > with hot-add of devices using I/O space.
>
> greaterthan 16 devices will still work if the BIOS doesn't assign
> the I/O BARs and the kernel does (including the devices that don't
> get assigned due to the kernel running out of I/O Port resources).
> And when the kernel runs out of I/O Port space it will warn for
> those devices it couldn't assign: (example):
>
> pci 0002:03:00.0: BAR 5: can't allocate I/O resource [0x0-0x7f]
[For anybody else grepping for this message, it is now "can't assign",
not "can't allocate".]
> But if the kernel assigns all the I/O Port space to these devices
> we know don't need it (thus the reason the BIOS didn't assign it)
> then I believe hot-add of devices using I/O space will fail.
Yes, I understand and agree with this motivation, though I hope
"pci=nobar" is recognized as only a short-term workaround. I think
it's completely unacceptable in terms of the user experience. Using
DMI quirks to turn it on automatically removes some user pain, but
it's not a real solution and still leaves a maintenance problem.
How about if we write a changelog that mentions hot-add of devices
that need I/O space? :-)
> There is also the issue that quirk_system_pci_resources() thinks
> those unassigned I/O resources are using I/O port space 0x0-0xFF.
> Since the BIOS never assigned the BAR the kernel reads it as
> having a base of 0x0 and a limit of whatever the BAR size is when
> it writes all 1's to obtain the size. So that results in
> quirk_system_pci_resources() disabling pnp devices: (example):
>
> pnp 00:11: io resource (0x92-0x92) overlaps 0002:03:00.0 BAR 0 (0x0-0xff), disabling
[...]
>
> One could argue this is a quirk_system_pci_resources() issue and
> should be handled there rather than "zeroing out the resource if
> the bios didn't assign it" as the patch does, but what the patch
> is attempting to do (as stated above) is to allow the BIOS a way
> to not assign resources it knows are not needed and thus make sure
> the kernel doesn't override that... and in doing that the quirk
> issue goes away too.
This is definitely a problem, but I think it should be handled
separately. The fact that this patch makes the "overlap" messages
go away is a nice coincidence, but it doesn't fix the underlying
issue.
I think setting the resource start/end to zero to "disable" it as in
this patch (and elsewhere) is just a hack -- we no longer know the size
of the BAR, the struct resource no longer corresponds to the BAR contents,
and I don't think we do anything with the PCI_COMMAND_IO/PCI_COMMAND_MEM
bits to actually disable the BAR, so the device probably still responds
at whatever we left in the BAR.
In quirk_system_pci_resources(), we set IORESOURCE_DISABLED rather than
setting the PNP resource start/end to zero, but that's a bit of a hack,
too. All that does is prevent the PNP motherboard driver (pnp/system.c)
from requesting that region. We don't actually do anything to stop the
device from responding to that address.
Bjorn
On 06/02/2010 08:53 AM, Jesse Barnes wrote:
>>
>> That's what I thought, which I guess means my original question to Mike
>> still stands...
>
> I thought the whole reason for this was hotplug; we don't want to
> exhaust I/O space unnecessarily by allocating resources for BARs the
> BIOS didn't assign so we can keep them around for later hotplug
> activity.
>
> If there's some other issue, it's not too late to drop this patch.
>
Okay, now... this means that if a device that the BIOS doesn't know
about, but which needs I/O addresses, then it will work if hotplugged,
but not if it is plugged in on system boot?
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On Tue, 08 Jun 2010 17:53:19 -0700
"H. Peter Anvin" <[email protected]> wrote:
> On 06/02/2010 08:53 AM, Jesse Barnes wrote:
> >>
> >> That's what I thought, which I guess means my original question to Mike
> >> still stands...
> >
> > I thought the whole reason for this was hotplug; we don't want to
> > exhaust I/O space unnecessarily by allocating resources for BARs the
> > BIOS didn't assign so we can keep them around for later hotplug
> > activity.
> >
> > If there's some other issue, it's not too late to drop this patch.
> >
>
> Okay, now... this means that if a device that the BIOS doesn't know
> about, but which needs I/O addresses, then it will work if hotplugged,
> but not if it is plugged in on system boot?
Depends on the BIOS interactions on this platform; if the kernel ends
up doing all the allocations itself, we'll allocate space for every BAR
unconditionally, meaning that any hotplugged device should work.
But really the SGI guys should comment here.
--
Jesse Barnes, Intel Open Source Technology Center
Jesse Barnes wrote:
> On Tue, 08 Jun 2010 17:53:19 -0700
> "H. Peter Anvin" <[email protected]> wrote:
>
>> On 06/02/2010 08:53 AM, Jesse Barnes wrote:
>>>> That's what I thought, which I guess means my original question to Mike
>>>> still stands...
>>> I thought the whole reason for this was hotplug; we don't want to
>>> exhaust I/O space unnecessarily by allocating resources for BARs the
>>> BIOS didn't assign so we can keep them around for later hotplug
>>> activity.
>>>
>>> If there's some other issue, it's not too late to drop this patch.
>>>
>> Okay, now... this means that if a device that the BIOS doesn't know
>> about, but which needs I/O addresses, then it will work if hotplugged,
>> but not if it is plugged in on system boot?
>
> Depends on the BIOS interactions on this platform; if the kernel ends
> up doing all the allocations itself, we'll allocate space for every BAR
> unconditionally, meaning that any hotplugged device should work.
Correct, our BIOS allocates I/O space for all devices except for a
few that it knows don't use it. On a hotplug attach the kernel
will be unconditionally allocating the I/O space for all devices..
The pci=nobar option strictly prevents the kernel from allocating
BAR resources to device BARs that the BIOS didn't assign (similar
to how the pci=norom option works for the device's ROM BAR)
>
> But really the SGI guys should comment here.
>