* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 3:31 ` Yinghai Lu
@ 2012-05-23 4:07 ` Yinghai Lu
2012-05-23 15:53 ` Jason Baron
2012-05-23 19:13 ` Michael S. Tsirkin
2012-05-23 20:52 ` Jason Baron
2 siblings, 1 reply; 20+ messages in thread
From: Yinghai Lu @ 2012-05-23 4:07 UTC (permalink / raw)
To: Jason Baron; +Cc: bhelgaas, linux-pci, mst
On Tue, May 22, 2012 at 8:31 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, May 22, 2012 at 7:43 PM, Jason Baron <jbaron@redhat.com> wrote:
>> On Tue, May 22, 2012 at 02:21:13PM -0700, Yinghai Lu wrote:
>>> On Tue, May 22, 2012 at 1:11 PM, Jason Baron <jbaron@redhat.com> wrote:
>>> > I'm tyring to support bridge hotplug and devices below it in qemu via acpi
>>> > hotplug. Currently only 1 level or 32 slots are supported. By allowing for a
>>> > second level, we will be able to support 32^2 devices.
>>> >
>>> > If I first hotplug the bridge with no devices intially below it, the hotplug
>>> > code sets the bridge memory window to 0 and does not increase it when
>>> > subsequent devices are added below it.
>>> >
>>> > Fix this, by calling pci_rescan_bus_bridge_resize(), on the bridge directly
>>> > below the root to re-size all the birdge windows that may have changed.
>>> >
>>> > Signed-off-by: Jason Baron <jbaron@redhat.com>
>>> > ---
>>> > drivers/pci/hotplug/acpiphp_glue.c | 8 ++++++++
>>> > 1 files changed, 8 insertions(+), 0 deletions(-)
>>> >
>>> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>>> > index 806c44f..8960c1e 100644
>>> > --- a/drivers/pci/hotplug/acpiphp_glue.c
>>> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>> > @@ -792,6 +792,7 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>>> > {
>>> > struct pci_dev *dev;
>>> > struct pci_bus *bus = slot->bridge->pci_bus;
>>> > + struct pci_bus *rescan_bus;
>>> > struct acpiphp_func *func;
>>> > int retval = 0;
>>> > int num, max, pass;
>>> > @@ -821,6 +822,13 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>>> > }
>>> > }
>>> >
>>> > + /* Ensure we rescan/setup a bridge for new devs hanging off of it */
>>> > + rescan_bus = bus;
>>> > + while (rescan_bus->parent && rescan_bus->parent->self)
>>> > + rescan_bus = rescan_bus->parent;
>>> > + if (rescan_bus->self)
>>> > + pci_rescan_bus_bridge_resize(rescan_bus->self);
>>> > +
>>>
>>> No, you can not do that. some parents bus could have other devices
>>> and driver could be loaded for those devices.
>>> so can not release resources that are used by those devices to resize
>>> parent bridges.
>>>
>>
>> hmmm...this patch also does what I want:
>>
>>
>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>> index 806c44f..be63c72 100644
>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> @@ -821,6 +821,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>> }
>> }
>>
>> + /* Ensure we rescan/setup a bridge for new devs hanging off of it */
>> + if (bus->self)
>> + pci_assign_unassigned_bridge_resources(bus->self);
>> +
>> list_for_each_entry(func, &slot->funcs, sibling)
>> acpiphp_bus_add(func);
>>
>>
>> There appears to be a precedant for something very similar in:
>> drivers/pci/hotplug/pciehp_pci.c:pciehp_configure_device(), where there
>> is a call to 'pci_assign_unassigned_bridge_resources(), when a new
>> device is added...
>
> the bus could have other devices under that bridge already.
>
> so pci_assign_unassigned_bridge_resources() will still try to release
> the bridge resource and
> those devices resources.
>
> but pciehp we could do that, because it will only have one device
> under that pcie root port or downstream port.
>
> so you can make qemu to support pcie and pciehp.
>
> for hot add one hotplug bridge support, we have
> bus->self->is_hotplug_bridge to set, so it will have allocate
> minimum resource.
>
> so you also could try to use quirk etc to get that bit set.
> please check
> drivers/pci/quirks.c::
> /* Allow manual resource allocation for PCI hotplug bridges
> * via pci=hpmemsize=nnM and pci=hpiosize=nnM parameters. For
> * some PCI-PCI hotplug bridges, like PLX 6254 (former HINT HB6),
> * kernel fails to allocate resources when hotplug device is
> * inserted and PCI bus is rescanned.
> */
> static void __devinit quirk_hotplug_bridge(struct pci_dev *dev)
> {
> dev->is_hotplug_bridge = 1;
> }
>
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_HINT, 0x0020, quirk_hotplug_bridge);
>
> but still would prefer you to make qemu to support pciehp.
another solution could be:
in qemu acpi dsdt, you could set bridge size for new added bridge.
current pbus_size_mem() will not shrink the old bridge resource size.
Thanks
Yinghai Lu
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 4:07 ` Yinghai Lu
@ 2012-05-23 15:53 ` Jason Baron
2012-05-23 17:16 ` Yinghai Lu
0 siblings, 1 reply; 20+ messages in thread
From: Jason Baron @ 2012-05-23 15:53 UTC (permalink / raw)
To: Yinghai Lu; +Cc: bhelgaas, linux-pci, mst
On Tue, May 22, 2012 at 09:07:54PM -0700, Yinghai Lu wrote:
> On Tue, May 22, 2012 at 8:31 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Tue, May 22, 2012 at 7:43 PM, Jason Baron <jbaron@redhat.com> wrote:
> >> On Tue, May 22, 2012 at 02:21:13PM -0700, Yinghai Lu wrote:
> >>> On Tue, May 22, 2012 at 1:11 PM, Jason Baron <jbaron@redhat.com> wrote:
> >>> > I'm tyring to support bridge hotplug and devices below it in qemu via acpi
> >>> > hotplug. Currently only 1 level or 32 slots are supported. By allowing for a
> >>> > second level, we will be able to support 32^2 devices.
> >>> >
> >>> > If I first hotplug the bridge with no devices intially below it, the hotplug
> >>> > code sets the bridge memory window to 0 and does not increase it when
> >>> > subsequent devices are added below it.
> >>> >
> >>> > Fix this, by calling pci_rescan_bus_bridge_resize(), on the bridge directly
> >>> > below the root to re-size all the birdge windows that may have changed.
> >>> >
> >>> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> >>> > ---
> >>> > drivers/pci/hotplug/acpiphp_glue.c | 8 ++++++++
> >>> > 1 files changed, 8 insertions(+), 0 deletions(-)
> >>> >
> >>> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> >>> > index 806c44f..8960c1e 100644
> >>> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> >>> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> >>> > @@ -792,6 +792,7 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> >>> > {
> >>> > struct pci_dev *dev;
> >>> > struct pci_bus *bus = slot->bridge->pci_bus;
> >>> > + struct pci_bus *rescan_bus;
> >>> > struct acpiphp_func *func;
> >>> > int retval = 0;
> >>> > int num, max, pass;
> >>> > @@ -821,6 +822,13 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> >>> > }
> >>> > }
> >>> >
> >>> > + /* Ensure we rescan/setup a bridge for new devs hanging off of it */
> >>> > + rescan_bus = bus;
> >>> > + while (rescan_bus->parent && rescan_bus->parent->self)
> >>> > + rescan_bus = rescan_bus->parent;
> >>> > + if (rescan_bus->self)
> >>> > + pci_rescan_bus_bridge_resize(rescan_bus->self);
> >>> > +
> >>>
> >>> No, you can not do that. some parents bus could have other devices
> >>> and driver could be loaded for those devices.
> >>> so can not release resources that are used by those devices to resize
> >>> parent bridges.
> >>>
> >>
> >> hmmm...this patch also does what I want:
> >>
> >>
> >> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> >> index 806c44f..be63c72 100644
> >> --- a/drivers/pci/hotplug/acpiphp_glue.c
> >> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> >> @@ -821,6 +821,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> >> }
> >> }
> >>
> >> + /* Ensure we rescan/setup a bridge for new devs hanging off of it */
> >> + if (bus->self)
> >> + pci_assign_unassigned_bridge_resources(bus->self);
> >> +
> >> list_for_each_entry(func, &slot->funcs, sibling)
> >> acpiphp_bus_add(func);
> >>
> >>
> >> There appears to be a precedant for something very similar in:
> >> drivers/pci/hotplug/pciehp_pci.c:pciehp_configure_device(), where there
> >> is a call to 'pci_assign_unassigned_bridge_resources(), when a new
> >> device is added...
> >
> > the bus could have other devices under that bridge already.
> >
> > so pci_assign_unassigned_bridge_resources() will still try to release
> > the bridge resource and
> > those devices resources.
> >
> > but pciehp we could do that, because it will only have one device
> > under that pcie root port or downstream port.
> >
> > so you can make qemu to support pcie and pciehp.
> >
> > for hot add one hotplug bridge support, we have
> > bus->self->is_hotplug_bridge to set, so it will have allocate
> > minimum resource.
> >
> > so you also could try to use quirk etc to get that bit set.
> > please check
> > drivers/pci/quirks.c::
> > /* Allow manual resource allocation for PCI hotplug bridges
> > * via pci=hpmemsize=nnM and pci=hpiosize=nnM parameters. For
> > * some PCI-PCI hotplug bridges, like PLX 6254 (former HINT HB6),
> > * kernel fails to allocate resources when hotplug device is
> > * inserted and PCI bus is rescanned.
> > */
> > static void __devinit quirk_hotplug_bridge(struct pci_dev *dev)
> > {
> > dev->is_hotplug_bridge = 1;
> > }
> >
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_HINT, 0x0020, quirk_hotplug_bridge);
> >
> > but still would prefer you to make qemu to support pciehp.
>
> another solution could be:
>
> in qemu acpi dsdt, you could set bridge size for new added bridge.
>
> current pbus_size_mem() will not shrink the old bridge resource size.
>
> Thanks
>
> Yinghai Lu
ok, I also tried hard-wiring the bridge io/mem base and limit registers on the
qemu side. That seems to work without any guest-side hotplug code changes. And
would seem to be more flexible than putting the limits in acpi.
Thanks,
-Jason
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 15:53 ` Jason Baron
@ 2012-05-23 17:16 ` Yinghai Lu
2012-05-23 18:44 ` Jason Baron
0 siblings, 1 reply; 20+ messages in thread
From: Yinghai Lu @ 2012-05-23 17:16 UTC (permalink / raw)
To: Jason Baron; +Cc: bhelgaas, linux-pci, mst
On Wed, May 23, 2012 at 8:53 AM, Jason Baron <jbaron@redhat.com> wrote:
>> > but still would prefer you to make qemu to support pciehp.
>>
>> another solution could be:
>>
>> in qemu acpi dsdt, you could set bridge size for new added bridge.
>>
>> current pbus_size_mem() will not shrink the old bridge resource size.
>
> ok, I also tried hard-wiring the bridge io/mem base and limit registers on the
> qemu side. That seems to work without any guest-side hotplug code changes. And
> would seem to be more flexible than putting the limits in acpi.
that should be acpi asl code or SMI work. and should make sure that
range is not overlapped with resources that are used by other bridges
and pci devices.
Thanks
Yinghai
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 17:16 ` Yinghai Lu
@ 2012-05-23 18:44 ` Jason Baron
2012-05-23 19:08 ` Michael S. Tsirkin
2012-05-23 20:31 ` Yinghai Lu
0 siblings, 2 replies; 20+ messages in thread
From: Jason Baron @ 2012-05-23 18:44 UTC (permalink / raw)
To: Yinghai Lu; +Cc: bhelgaas, linux-pci, mst
On Wed, May 23, 2012 at 10:16:06AM -0700, Yinghai Lu wrote:
> On Wed, May 23, 2012 at 8:53 AM, Jason Baron <jbaron@redhat.com> wrote:
> >> > but still would prefer you to make qemu to support pciehp.
> >>
> >> another solution could be:
> >>
> >> in qemu acpi dsdt, you could set bridge size for new added bridge.
> >>
> >> current pbus_size_mem() will not shrink the old bridge resource size.
> >
> > ok, I also tried hard-wiring the bridge io/mem base and limit registers on the
> > qemu side. That seems to work without any guest-side hotplug code changes. And
> > would seem to be more flexible than putting the limits in acpi.
>
> that should be acpi asl code or SMI work. and should make sure that
> range is not overlapped with resources that are used by other bridges
> and pci devices.
>
Ok. So you are saying to define a 'Name (_CRS, ResourceTemplate() {
Memory() IO() })' block for each bridge? The acpi code I currently have,
has one 'Device()' definition for each top-level hotplug slot. Would it
be ok to have the '_CRS' apply to either one?
Also, I'm wondering where in the acpiphp code it picks up the memory/io
ranges to configure them as bridge ranges?
I also see in ACPIspec40a.pdf, section "9.11 Module Device":
"
If no _CRS object is present, OSPM will assume that the module device is
a simple container object that does not produce the resources consumed by its
child devices. In this case, OSPM will assign resources to the child devices as
if they were direct children of the module device's parent object.
"
So not sure if that applies to hotplug, but that is not what acpiphp is
doing atm.
Finally, I don't see why putting the bridge window range logic into qemu is a
bad solution. Qemu knows memory ranges consumed by various resoures,
looking at 'info mtree', so why can't qemu hand out the bridge ranges
dynamically? In that way, it can re-size the windows more optimally than
something hard-coded into acpi. So to me, it seems like a better
approach.
Thanks,
-Jason
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 18:44 ` Jason Baron
@ 2012-05-23 19:08 ` Michael S. Tsirkin
2012-05-23 19:20 ` Michael S. Tsirkin
2012-05-23 20:53 ` Yinghai Lu
2012-05-23 20:31 ` Yinghai Lu
1 sibling, 2 replies; 20+ messages in thread
From: Michael S. Tsirkin @ 2012-05-23 19:08 UTC (permalink / raw)
To: Jason Baron; +Cc: Yinghai Lu, bhelgaas, linux-pci
On Wed, May 23, 2012 at 02:44:49PM -0400, Jason Baron wrote:
> On Wed, May 23, 2012 at 10:16:06AM -0700, Yinghai Lu wrote:
> > On Wed, May 23, 2012 at 8:53 AM, Jason Baron <jbaron@redhat.com> wrote:
> > >> > but still would prefer you to make qemu to support pciehp.
> > >>
> > >> another solution could be:
> > >>
> > >> in qemu acpi dsdt, you could set bridge size for new added bridge.
> > >>
> > >> current pbus_size_mem() will not shrink the old bridge resource size.
> > >
> > > ok, I also tried hard-wiring the bridge io/mem base and limit registers on the
> > > qemu side. That seems to work without any guest-side hotplug code changes. And
> > > would seem to be more flexible than putting the limits in acpi.
> >
> > that should be acpi asl code or SMI work. and should make sure that
> > range is not overlapped with resources that are used by other bridges
> > and pci devices.
> >
>
> Ok. So you are saying to define a 'Name (_CRS, ResourceTemplate() {
> Memory() IO() })' block for each bridge?
We are talking about hotplugging bridges.
This sounds strange.
> The acpi code I currently have,
> has one 'Device()' definition for each top-level hotplug slot. Would it
> be ok to have the '_CRS' apply to either one?
>
> Also, I'm wondering where in the acpiphp code it picks up the memory/io
> ranges to configure them as bridge ranges?
>
> I also see in ACPIspec40a.pdf, section "9.11 Module Device":
>
> "
> If no _CRS object is present, OSPM will assume that the module device is
> a simple container object that does not produce the resources consumed by its
> child devices. In this case, OSPM will assign resources to the child devices as
> if they were direct children of the module device's parent object.
> "
>
> So not sure if that applies to hotplug, but that is not what acpiphp is
> doing atm.
>
> Finally, I don't see why putting the bridge window range logic into qemu is a
> bad solution. Qemu knows memory ranges consumed by various resoures,
> looking at 'info mtree', so why can't qemu hand out the bridge ranges
> dynamically? In that way, it can re-size the windows more optimally than
> something hard-coded into acpi. So to me, it seems like a better
> approach.
>
> Thanks,
>
> -Jason
Because this is not how real hardware behaves.
Fundamentally BARs are under guest control.
So while you are handing out memory for the bridges you would have to
tiptoe around guest assigned BARs.
Also what happens after reboot? bios currently
assigns memory to bridges that are present on boot.
So this makes it even messier as who assigns memory
depends on whether device is hotplugged.
Cleaner to have guest do it.
I think Windows even can even rebalance BARs
as needed. So this is just a linux bug that needs
to be fixed.
--
MST
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 19:08 ` Michael S. Tsirkin
@ 2012-05-23 19:20 ` Michael S. Tsirkin
2012-05-23 20:53 ` Yinghai Lu
1 sibling, 0 replies; 20+ messages in thread
From: Michael S. Tsirkin @ 2012-05-23 19:20 UTC (permalink / raw)
To: Jason Baron; +Cc: Yinghai Lu, bhelgaas, linux-pci
On Wed, May 23, 2012 at 10:08:41PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 23, 2012 at 02:44:49PM -0400, Jason Baron wrote:
> > On Wed, May 23, 2012 at 10:16:06AM -0700, Yinghai Lu wrote:
> > > On Wed, May 23, 2012 at 8:53 AM, Jason Baron <jbaron@redhat.com> wrote:
> > > >> > but still would prefer you to make qemu to support pciehp.
> > > >>
> > > >> another solution could be:
> > > >>
> > > >> in qemu acpi dsdt, you could set bridge size for new added bridge.
> > > >>
> > > >> current pbus_size_mem() will not shrink the old bridge resource size.
> > > >
> > > > ok, I also tried hard-wiring the bridge io/mem base and limit registers on the
> > > > qemu side. That seems to work without any guest-side hotplug code changes. And
> > > > would seem to be more flexible than putting the limits in acpi.
> > >
> > > that should be acpi asl code or SMI work. and should make sure that
> > > range is not overlapped with resources that are used by other bridges
> > > and pci devices.
> > >
> >
> > Ok. So you are saying to define a 'Name (_CRS, ResourceTemplate() {
> > Memory() IO() })' block for each bridge?
>
> We are talking about hotplugging bridges.
> This sounds strange.
However if it works it is certainly simple.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 19:08 ` Michael S. Tsirkin
2012-05-23 19:20 ` Michael S. Tsirkin
@ 2012-05-23 20:53 ` Yinghai Lu
2012-05-23 21:18 ` Michael S. Tsirkin
1 sibling, 1 reply; 20+ messages in thread
From: Yinghai Lu @ 2012-05-23 20:53 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Baron, bhelgaas, linux-pci
On Wed, May 23, 2012 at 12:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> Fundamentally BARs are under guest control.
> So while you are handing out memory for the bridges you would have to
> tiptoe around guest assigned BARs.
>
> Also what happens after reboot? bios currently
> assigns memory to bridges that are present on boot.
> So this makes it even messier as who assigns memory
> depends on whether device is hotplugged.
>
> Cleaner to have guest do it.
assume here "guest' mean: BIOS + kernel + (acpi dsl code from BIOS).
>
> I think Windows even can even rebalance BARs
> as needed. So this is just a linux bug that needs
> to be fixed.
Now pciehp in Linux will resize the bridge.
but not for acpiphp. reason: acpiphp could handle more complicated case.
does not mean new add card have to been under one bridge existing.
slot just on root bus, you could just add device or bridge in it.
and the bridge could have other hotlpug slots.
in that case, if one of children slot already have card
installed/driver loaded, later
after another card is put in another child slot, we can not simple
resize the bridge in parent slot.
because other child devices is operating...
solution will be
1. add quirks to set is_hotplug_bridge for apciphp....
2. or probe_resource in busn_alloc patchset, that maybe still need to
extend to support alignment.
BTW, I was thinking Linux is the ONLY one that will scratch bridge BAR
if that is big enough for children devices.
Do you have evidence that other os does that?
Yinghai
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 20:53 ` Yinghai Lu
@ 2012-05-23 21:18 ` Michael S. Tsirkin
0 siblings, 0 replies; 20+ messages in thread
From: Michael S. Tsirkin @ 2012-05-23 21:18 UTC (permalink / raw)
To: Yinghai Lu; +Cc: Jason Baron, bhelgaas, linux-pci
On Wed, May 23, 2012 at 01:53:10PM -0700, Yinghai Lu wrote:
> On Wed, May 23, 2012 at 12:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Fundamentally BARs are under guest control.
> > So while you are handing out memory for the bridges you would have to
> > tiptoe around guest assigned BARs.
> >
> > Also what happens after reboot? bios currently
> > assigns memory to bridges that are present on boot.
> > So this makes it even messier as who assigns memory
> > depends on whether device is hotplugged.
> >
> > Cleaner to have guest do it.
>
> assume here "guest' mean: BIOS + kernel + (acpi dsl code from BIOS).
>
> >
> > I think Windows even can even rebalance BARs
> > as needed. So this is just a linux bug that needs
> > to be fixed.
>
> Now pciehp in Linux will resize the bridge.
> but not for acpiphp. reason: acpiphp could handle more complicated case.
> does not mean new add card have to been under one bridge existing.
expess has this too. It just needs a bigger hierarchy:
upsteam ports can have many downstream ports behind them.
> slot just on root bus, you could just add device or bridge in it.
> and the bridge could have other hotlpug slots.
>
> in that case, if one of children slot already have card
> installed/driver loaded, later
> after another card is put in another child slot, we can not simple
> resize the bridge in parent slot.
> because other child devices is operating...
>
> solution will be
> 1. add quirks to set is_hotplug_bridge for apciphp....
> 2. or probe_resource in busn_alloc patchset, that maybe still need to
> extend to support alignment.
>
> BTW, I was thinking Linux is the ONLY one that will scratch bridge BAR
> if that is big enough for children devices.
> Do you have evidence that other os does that?
>
> Yinghai
I saw some docs on the net but do not have them handy. Google it.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 18:44 ` Jason Baron
2012-05-23 19:08 ` Michael S. Tsirkin
@ 2012-05-23 20:31 ` Yinghai Lu
2012-05-23 20:49 ` Michael S. Tsirkin
1 sibling, 1 reply; 20+ messages in thread
From: Yinghai Lu @ 2012-05-23 20:31 UTC (permalink / raw)
To: Jason Baron; +Cc: bhelgaas, linux-pci, mst
On Wed, May 23, 2012 at 11:44 AM, Jason Baron <jbaron@redhat.com> wrote:
> On Wed, May 23, 2012 at 10:16:06AM -0700, Yinghai Lu wrote:
>> On Wed, May 23, 2012 at 8:53 AM, Jason Baron <jbaron@redhat.com> wrote:
>> >> > but still would prefer you to make qemu to support pciehp.
>> >>
>> >> another solution could be:
>> >>
>> >> in qemu acpi dsdt, you could set bridge size for new added bridge.
>> >>
>> >> current pbus_size_mem() will not shrink the old bridge resource size.
>> >
>> > ok, I also tried hard-wiring the bridge io/mem base and limit registers on the
>> > qemu side. That seems to work without any guest-side hotplug code changes. And
>> > would seem to be more flexible than putting the limits in acpi.
>>
>> that should be acpi asl code or SMI work. and should make sure that
>> range is not overlapped with resources that are used by other bridges
>> and pci devices.
>>
>
> Ok. So you are saying to define a 'Name (_CRS, ResourceTemplate() {
> Memory() IO() })' block for each bridge? The acpi code I currently have,
> has one 'Device()' definition for each top-level hotplug slot. Would it
> be ok to have the '_CRS' apply to either one?
>
> Also, I'm wondering where in the acpiphp code it picks up the memory/io
> ranges to configure them as bridge ranges?
>
> I also see in ACPIspec40a.pdf, section "9.11 Module Device":
>
> "
> If no _CRS object is present, OSPM will assume that the module device is
> a simple container object that does not produce the resources consumed by its
> child devices. In this case, OSPM will assign resources to the child devices as
> if they were direct children of the module device's parent object.
> "
>
> So not sure if that applies to hotplug, but that is not what acpiphp is
> doing atm.
_CRS is only used for pci root bus.
I mean in you _PS0 in your socket.
>
> Finally, I don't see why putting the bridge window range logic into qemu is a
> bad solution. Qemu knows memory ranges consumed by various resoures,
> looking at 'info mtree', so why can't qemu hand out the bridge ranges
> dynamically? In that way, it can re-size the windows more optimally than
> something hard-coded into acpi. So to me, it seems like a better
> approach.
acpi could be dynamically too.
Yinghai
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 20:31 ` Yinghai Lu
@ 2012-05-23 20:49 ` Michael S. Tsirkin
0 siblings, 0 replies; 20+ messages in thread
From: Michael S. Tsirkin @ 2012-05-23 20:49 UTC (permalink / raw)
To: Yinghai Lu; +Cc: Jason Baron, bhelgaas, linux-pci
On Wed, May 23, 2012 at 01:31:23PM -0700, Yinghai Lu wrote:
> On Wed, May 23, 2012 at 11:44 AM, Jason Baron <jbaron@redhat.com> wrote:
> > On Wed, May 23, 2012 at 10:16:06AM -0700, Yinghai Lu wrote:
> >> On Wed, May 23, 2012 at 8:53 AM, Jason Baron <jbaron@redhat.com> wrote:
> >> >> > but still would prefer you to make qemu to support pciehp.
> >> >>
> >> >> another solution could be:
> >> >>
> >> >> in qemu acpi dsdt, you could set bridge size for new added bridge.
> >> >>
> >> >> current pbus_size_mem() will not shrink the old bridge resource size.
> >> >
> >> > ok, I also tried hard-wiring the bridge io/mem base and limit registers on the
> >> > qemu side. That seems to work without any guest-side hotplug code changes. And
> >> > would seem to be more flexible than putting the limits in acpi.
> >>
> >> that should be acpi asl code or SMI work. and should make sure that
> >> range is not overlapped with resources that are used by other bridges
> >> and pci devices.
> >>
> >
> > Ok. So you are saying to define a 'Name (_CRS, ResourceTemplate() {
> > Memory() IO() })' block for each bridge? The acpi code I currently have,
> > has one 'Device()' definition for each top-level hotplug slot. Would it
> > be ok to have the '_CRS' apply to either one?
> >
> > Also, I'm wondering where in the acpiphp code it picks up the memory/io
> > ranges to configure them as bridge ranges?
> >
> > I also see in ACPIspec40a.pdf, section "9.11 Module Device":
> >
> > "
> > If no _CRS object is present, OSPM will assume that the module device is
> > a simple container object that does not produce the resources consumed by its
> > child devices. In this case, OSPM will assign resources to the child devices as
> > if they were direct children of the module device's parent object.
> > "
> >
> > So not sure if that applies to hotplug, but that is not what acpiphp is
> > doing atm.
>
> _CRS is only used for pci root bus.
>
> I mean in you _PS0 in your socket.
>
> >
> > Finally, I don't see why putting the bridge window range logic into qemu is a
> > bad solution. Qemu knows memory ranges consumed by various resoures,
> > looking at 'info mtree', so why can't qemu hand out the bridge ranges
> > dynamically? In that way, it can re-size the windows more optimally than
> > something hard-coded into acpi. So to me, it seems like a better
> > approach.
>
> acpi could be dynamically too.
>
> Yinghai
There isn't much you can do besides loading a new table, is there?
--
MST
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 3:31 ` Yinghai Lu
2012-05-23 4:07 ` Yinghai Lu
@ 2012-05-23 19:13 ` Michael S. Tsirkin
2012-05-23 20:52 ` Jason Baron
2 siblings, 0 replies; 20+ messages in thread
From: Michael S. Tsirkin @ 2012-05-23 19:13 UTC (permalink / raw)
To: Yinghai Lu; +Cc: Jason Baron, bhelgaas, linux-pci
On Tue, May 22, 2012 at 08:31:30PM -0700, Yinghai Lu wrote:
> but pciehp we could do that, because it will only have one device
> under that pcie root port or downstream port.
>
> so you can make qemu to support pcie and pciehp.
This does not solve the problem I think because
there are still multiple devices behind an upstream port.
Also, swiching to pcie creates a host of other complexities
so we want to support pci bridges too.
--
MST
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 3:31 ` Yinghai Lu
2012-05-23 4:07 ` Yinghai Lu
2012-05-23 19:13 ` Michael S. Tsirkin
@ 2012-05-23 20:52 ` Jason Baron
2012-05-24 0:00 ` Yinghai Lu
2 siblings, 1 reply; 20+ messages in thread
From: Jason Baron @ 2012-05-23 20:52 UTC (permalink / raw)
To: Yinghai Lu; +Cc: bhelgaas, linux-pci, mst
On Tue, May 22, 2012 at 08:31:30PM -0700, Yinghai Lu wrote:
> On Tue, May 22, 2012 at 7:43 PM, Jason Baron <jbaron@redhat.com> wrote:
> > On Tue, May 22, 2012 at 02:21:13PM -0700, Yinghai Lu wrote:
> >> On Tue, May 22, 2012 at 1:11 PM, Jason Baron <jbaron@redhat.com> wrote:
> >> > I'm tyring to support bridge hotplug and devices below it in qemu via acpi
> >> > hotplug. Currently only 1 level or 32 slots are supported. By allowing for a
> >> > second level, we will be able to support 32^2 devices.
> >> >
> >> > If I first hotplug the bridge with no devices intially below it, the hotplug
> >> > code sets the bridge memory window to 0 and does not increase it when
> >> > subsequent devices are added below it.
> >> >
> >> > Fix this, by calling pci_rescan_bus_bridge_resize(), on the bridge directly
> >> > below the root to re-size all the birdge windows that may have changed.
> >> >
> >> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> >> > ---
> >> > drivers/pci/hotplug/acpiphp_glue.c | 8 ++++++++
> >> > 1 files changed, 8 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> >> > index 806c44f..8960c1e 100644
> >> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> >> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> >> > @@ -792,6 +792,7 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> >> > {
> >> > struct pci_dev *dev;
> >> > struct pci_bus *bus = slot->bridge->pci_bus;
> >> > + struct pci_bus *rescan_bus;
> >> > struct acpiphp_func *func;
> >> > int retval = 0;
> >> > int num, max, pass;
> >> > @@ -821,6 +822,13 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> >> > }
> >> > }
> >> >
> >> > + /* Ensure we rescan/setup a bridge for new devs hanging off of it */
> >> > + rescan_bus = bus;
> >> > + while (rescan_bus->parent && rescan_bus->parent->self)
> >> > + rescan_bus = rescan_bus->parent;
> >> > + if (rescan_bus->self)
> >> > + pci_rescan_bus_bridge_resize(rescan_bus->self);
> >> > +
> >>
> >> No, you can not do that. some parents bus could have other devices
> >> and driver could be loaded for those devices.
> >> so can not release resources that are used by those devices to resize
> >> parent bridges.
> >>
> >
> > hmmm...this patch also does what I want:
> >
> >
> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > index 806c44f..be63c72 100644
> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -821,6 +821,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> > }
> > }
> >
> > + /* Ensure we rescan/setup a bridge for new devs hanging off of it */
> > + if (bus->self)
> > + pci_assign_unassigned_bridge_resources(bus->self);
> > +
> > list_for_each_entry(func, &slot->funcs, sibling)
> > acpiphp_bus_add(func);
> >
> >
> > There appears to be a precedant for something very similar in:
> > drivers/pci/hotplug/pciehp_pci.c:pciehp_configure_device(), where there
> > is a call to 'pci_assign_unassigned_bridge_resources(), when a new
> > device is added...
>
> the bus could have other devices under that bridge already.
>
> so pci_assign_unassigned_bridge_resources() will still try to release
> the bridge resource and
> those devices resources.
>
> but pciehp we could do that, because it will only have one device
> under that pcie root port or downstream port.
>
> so you can make qemu to support pcie and pciehp.
>
> for hot add one hotplug bridge support, we have
> bus->self->is_hotplug_bridge to set, so it will have allocate
> minimum resource.
>
> so you also could try to use quirk etc to get that bit set.
> please check
> drivers/pci/quirks.c::
> /* Allow manual resource allocation for PCI hotplug bridges
> * via pci=hpmemsize=nnM and pci=hpiosize=nnM parameters. For
> * some PCI-PCI hotplug bridges, like PLX 6254 (former HINT HB6),
> * kernel fails to allocate resources when hotplug device is
> * inserted and PCI bus is rescanned.
> */
> static void __devinit quirk_hotplug_bridge(struct pci_dev *dev)
> {
> dev->is_hotplug_bridge = 1;
> }
>
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_HINT, 0x0020, quirk_hotplug_bridge);
>
> but still would prefer you to make qemu to support pciehp.
>
Hi,
Ok, I've also verified that quirk approach resolves the issue for me as
well. It might not be a bad solution, especially if we anticipate moving
to pciehp. We can also always change bridge vendor id to not hit the
quirk, if we choose to implement more complex guest, or acpi fixes. The
defaults for i/o and mem seem pretty sane too:
#define DEFAULT_HOTPLUG_IO_SIZE (256)
#define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
And further can be set on the guest command line via:
/* pci=hpmemsize=nnM,hpiosize=nn can override this */
So I don't see any down-side really to the quirk approach (which is a
quite simple 1-line patch to the guest), even if it is not a long-term
solution.
Thanks,
-Jason
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-23 20:52 ` Jason Baron
@ 2012-05-24 0:00 ` Yinghai Lu
2012-05-24 13:43 ` Jason Baron
0 siblings, 1 reply; 20+ messages in thread
From: Yinghai Lu @ 2012-05-24 0:00 UTC (permalink / raw)
To: Jason Baron; +Cc: bhelgaas, linux-pci, mst
[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]
On Wed, May 23, 2012 at 1:52 PM, Jason Baron <jbaron@redhat.com> wrote:
> Ok, I've also verified that quirk approach resolves the issue for me as
> well. It might not be a bad solution, especially if we anticipate moving
> to pciehp. We can also always change bridge vendor id to not hit the
> quirk, if we choose to implement more complex guest, or acpi fixes. The
> defaults for i/o and mem seem pretty sane too:
>
>
> #define DEFAULT_HOTPLUG_IO_SIZE (256)
> #define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
>
> And further can be set on the guest command line via:
>
> /* pci=hpmemsize=nnM,hpiosize=nn can override this */
>
> So I don't see any down-side really to the quirk approach (which is a
> quite simple 1-line patch to the guest), even if it is not a long-term
> solution.
Good. please check attached patch,
it should auto set that bit for apciphp when complex slot are
supported by platform.
next, we still need to extend probe_resource() to support
mmio/pref_mmio extension.
that is for when you have a lot slots on hotplug bridge.
Thanks
Yinghai
[-- Attachment #2: acpi_slot_hotplug_bridge_detection.patch --]
[-- Type: application/octet-stream, Size: 1831 bytes --]
Subject: [PATCH] PCI, acpiphp: add is_hotplug_bridge detection
When system support hotplug bridge with children hotplug slots, we need to make sure
that parent bridge get preallocated resource so later when device is plugged into
children slot, those children devices will get resource allocated.
We do not meet this problem, because for pcie hotplug card, when acpiphp is used,
pci_scan_bridge will set that for us when detect hotplug bit in slot cap.
Reported-by: Jason Baron <jbaron@redhat.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 62d0ae4..5c834d6 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -781,6 +781,26 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
}
}
+static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
+{
+ struct acpiphp_func *func;
+
+ if (!dev->subordinate)
+ return;
+
+ /* quirk, or pcie could set it already */
+ if (dev->is_hotplug_bridge)
+ return;
+
+ list_for_each_entry(func, &slot->funcs, sibling) {
+ if (PCI_FUNC(dev->devfn) == func->function) {
+ /* check if this bridge has ejectable slots */
+ if ((detect_ejectable_slots(func->handle) > 0))
+ dev->is_hotplug_bridge = 1;
+ break;
+ }
+ }
+}
/**
* enable_device - enable, configure a slot
* @slot: slot to be enabled
@@ -815,8 +835,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
max = pci_scan_bridge(bus, dev, max, pass);
- if (pass && dev->subordinate)
+ if (pass && dev->subordinate) {
+ check_hotplug_bridge(slot, dev);
pci_bus_size_bridges(dev->subordinate);
+ }
}
}
}
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH] pci hotplug: rescan bridge after device hotplug
2012-05-24 0:00 ` Yinghai Lu
@ 2012-05-24 13:43 ` Jason Baron
0 siblings, 0 replies; 20+ messages in thread
From: Jason Baron @ 2012-05-24 13:43 UTC (permalink / raw)
To: Yinghai Lu; +Cc: bhelgaas, linux-pci, mst
On Wed, May 23, 2012 at 05:00:05PM -0700, Yinghai Lu wrote:
> On Wed, May 23, 2012 at 1:52 PM, Jason Baron <jbaron@redhat.com> wrote:
> > Ok, I've also verified that quirk approach resolves the issue for me as
> > well. It might not be a bad solution, especially if we anticipate moving
> > to pciehp. We can also always change bridge vendor id to not hit the
> > quirk, if we choose to implement more complex guest, or acpi fixes. The
> > defaults for i/o and mem seem pretty sane too:
> >
> >
> > #define DEFAULT_HOTPLUG_IO_SIZE (256)
> > #define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
> >
> > And further can be set on the guest command line via:
> >
> > /* pci=hpmemsize=nnM,hpiosize=nn can override this */
> >
> > So I don't see any down-side really to the quirk approach (which is a
> > quite simple 1-line patch to the guest), even if it is not a long-term
> > solution.
>
> Good. please check attached patch,
> it should auto set that bit for apciphp when complex slot are
> supported by platform.
>
> next, we still need to extend probe_resource() to support
> mmio/pref_mmio extension.
> that is for when you have a lot slots on hotplug bridge.
>
> Thanks
>
> Yinghai
Yes, I can comfirm that the patch works with my qemu hotplug bridge
code. Thanks!
Acked-by: Jason Baron <jbaron@redhat.com>
-Jason
^ permalink raw reply [flat|nested] 20+ messages in thread