| Message ID | 1429700240-32373-2-git-send-email-nikunj@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Michael Ellerman |
| Headers | show |
Hi Nikunj, On Wed, 22 Apr 2015 16:27:20 +0530 Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote: > PCI Enumeration has been part of SLOF. Now with hotplug code addition > in Qemu, it makes more sense to have this code a one place, i.e. Qemu. s/Qemu/QEMU/ and s/code a one place/code in one place/ ? > Adding routines to walk through the device nodes created by Qemu. SLOF > will configure the device/bridges and program the BARs for > communicating with the devices. I wonder whether it would make more sense to also set up the BARs etc. in QEMU instead of SLOF? > > diff --git a/board-qemu/slof/pci-phb.fs b/board-qemu/slof/pci-phb.fs > index e307d95..30b7443 100644 > --- a/board-qemu/slof/pci-phb.fs > +++ b/board-qemu/slof/pci-phb.fs > @@ -283,6 +283,41 @@ setup-puid > THEN > ; > > +: phb-pci-walk-bridge ( -- ) > + phb-debug? IF ." Calling pci-walk-bridge " pwd cr THEN > + > + get-node child ?dup 0= IF EXIT THEN \ get and check if we have children > + BEGIN > + dup \ Continue as long as there are children > + WHILE Most Forth code uses the same indentation for the code between BEGIN...WHILE and WHILE...REPEAT ... so I think you could decrease the indentation of the following block by one level. > + \ Set child node as current node: > + dup set-node Below you are calling pci-device-setup which in turn might include some pci-class_*.fs or pci-device_*.fs files (or even run some FCODE?). At least pci-class_02.fs seems to use an INSTANCE VARIABLE, i.e. the instance template should get modified in that case ==> Please double-check whether you need to use extend-device here instead (I'm not 100% sure right now ... what happens for example when you run qemu with a network device that SLOF does not provide a pci-device_*.fs for? I guess it will try to include pci-class_02.fs and fail due to the INSTANCE VARIABLE ?) > + my-space pci-set-slot \ set the slot bit pci-set-slot seems to rely on the pci-device-slots global variable. This is normally initialized by pci-probe-bus. Now that you provide your own implementation of that function below, I think it should likely also set up the pci-device-slots variable, shouldn't it? > + my-space pci-htype@ \ read HEADER-Type > + 7f and \ Mask bit 7 - multifunction device > + CASE > + 0 OF my-space pci-device-setup ENDOF \ | set up the device > + 1 OF my-space pci-bridge-setup ENDOF \ | set up the bridge > + dup OF my-space pci-htype@ pci-out ENDOF > + ENDCASE > + peer > + REPEAT drop > + get-parent set-node > +; The remaining part of the patch looks ok to me. Thomas
On 04/25/2015 05:30 AM, Thomas Huth wrote: > > Hi Nikunj, > > On Wed, 22 Apr 2015 16:27:20 +0530 > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote: > >> PCI Enumeration has been part of SLOF. Now with hotplug code addition >> in Qemu, it makes more sense to have this code a one place, i.e. Qemu. > > s/Qemu/QEMU/ and s/code a one place/code in one place/ ? > >> Adding routines to walk through the device nodes created by Qemu. SLOF >> will configure the device/bridges and program the BARs for >> communicating with the devices. > > I wonder whether it would make more sense to also set up the BARs etc. > in QEMU instead of SLOF? We need BAR setup in 2 cases: when SLOF needs to boot from a PCI device (and SLOF can do BAR setup) and when we do PCI hotplug - and BARs are set by the guest, otherwise we hit races here (Michael Roth can tell more). So as for today there is no reason for doing this in QEMU.
On Sat, 2015-04-25 at 17:31 +1000, Alexey Kardashevskiy wrote: > We need BAR setup in 2 cases: when SLOF needs to boot from a PCI device > (and SLOF can do BAR setup) and when we do PCI hotplug - and BARs are set > by the guest, otherwise we hit races here (Michael Roth can tell more). So > as for today there is no reason for doing this in QEMU. Doing BAR setup in the guest is a violation of PAPR though .... we do it now to workaround bugs I believe but normally the BARs are expected to be owned and setup by FW on a PAPR system. Ben.
Hi Thomas, Thomas Huth <thuth@redhat.com> writes: > Hi Nikunj, > > On Wed, 22 Apr 2015 16:27:20 +0530 > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote: > >> PCI Enumeration has been part of SLOF. Now with hotplug code addition >> in Qemu, it makes more sense to have this code a one place, i.e. Qemu. > > s/Qemu/QEMU/ and s/code a one place/code in one place/ ? Sure > >> Adding routines to walk through the device nodes created by Qemu. SLOF >> will configure the device/bridges and program the BARs for >> communicating with the devices. > > I wonder whether it would make more sense to also set up the BARs etc. > in QEMU instead of SLOF? > >> >> diff --git a/board-qemu/slof/pci-phb.fs b/board-qemu/slof/pci-phb.fs >> index e307d95..30b7443 100644 >> --- a/board-qemu/slof/pci-phb.fs >> +++ b/board-qemu/slof/pci-phb.fs >> @@ -283,6 +283,41 @@ setup-puid >> THEN >> ; >> >> +: phb-pci-walk-bridge ( -- ) >> + phb-debug? IF ." Calling pci-walk-bridge " pwd cr THEN >> + >> + get-node child ?dup 0= IF EXIT THEN \ get and check if we have children >> + BEGIN >> + dup \ Continue as long as there are children >> + WHILE > > Most Forth code uses the same indentation for the code between > BEGIN...WHILE and WHILE...REPEAT ... so I think you could decrease the > indentation of the following block by one level. Sure, emacs is autoindenting this, so i ended up like this. > >> + \ Set child node as current node: >> + dup set-node > > Below you are calling pci-device-setup which in turn might include some > pci-class_*.fs or pci-device_*.fs files (or even run some FCODE?). At > least pci-class_02.fs seems to use an INSTANCE VARIABLE, i.e. the > instance template should get modified in that case ==> Please > double-check whether you need to use extend-device here instead (I'm > not 100% sure right now ... what happens > for example when you run qemu with a network device that SLOF does not > provide a pci-device_*.fs for? I guess it will try to include > pci-class_02.fs and fail due to the INSTANCE VARIABLE ?) I tried using rtl8139 device, SLOF does not provide driver for that. And that did not create any problem. Not sure about FCODE though, i will do more experiments with non supported devices to check if things are fine. > >> + my-space pci-set-slot \ set the slot bit > > pci-set-slot seems to rely on the pci-device-slots global variable. > This is normally initialized by pci-probe-bus. Now that you provide > your own implementation of that function below, I think it should > likely also set up the pci-device-slots variable, shouldn't it? Yes, I will need to initialize it for every bus probe to 0. > >> + my-space pci-htype@ \ read HEADER-Type >> + 7f and \ Mask bit 7 - multifunction device >> + CASE >> + 0 OF my-space pci-device-setup ENDOF \ | set up the device >> + 1 OF my-space pci-bridge-setup ENDOF \ | set up the bridge >> + dup OF my-space pci-htype@ pci-out ENDOF >> + ENDCASE >> + peer >> + REPEAT drop >> + get-parent set-node >> +; > > The remaining part of the patch looks ok to me. Regards, Nikunj
diff --git a/board-qemu/slof/pci-phb.fs b/board-qemu/slof/pci-phb.fs index e307d95..30b7443 100644 --- a/board-qemu/slof/pci-phb.fs +++ b/board-qemu/slof/pci-phb.fs @@ -283,6 +283,41 @@ setup-puid THEN ; +: phb-pci-walk-bridge ( -- ) + phb-debug? IF ." Calling pci-walk-bridge " pwd cr THEN + + get-node child ?dup 0= IF EXIT THEN \ get and check if we have children + BEGIN + dup \ Continue as long as there are children + WHILE + \ Set child node as current node: + dup set-node + my-space pci-set-slot \ set the slot bit + my-space pci-htype@ \ read HEADER-Type + 7f and \ Mask bit 7 - multifunction device + CASE + 0 OF my-space pci-device-setup ENDOF \ | set up the device + 1 OF my-space pci-bridge-setup ENDOF \ | set up the bridge + dup OF my-space pci-htype@ pci-out ENDOF + ENDCASE + peer + REPEAT drop + get-parent set-node +; + +\ Landing routing to probe the popuated device tree +: phb-pci-probe-bus ( busnr -- ) + drop phb-pci-walk-bridge +; + +\ Stub routine, as qemu has enumerated, we already have the device +\ properties set. +: phb-pci-device-props ( addr -- ) + dup pci-class-name device-name + dup pci-device-assigned-addresses-prop + drop +; + \ Scan the child nodes of the pci root node to assign bars, fixup \ properties etc. : phb-setup-children @@ -290,7 +325,14 @@ setup-puid my-puid TO puid \ Set current puid phb-parse-ranges 1 TO pci-hotplug-enabled - 1 0 (probe-pci-host-bridge) + s" qemu,phb-enumerated" get-node get-property 0<> IF + 1 0 (probe-pci-host-bridge) + ELSE + 2drop + ['] phb-pci-probe-bus TO func-pci-probe-bus + ['] phb-pci-device-props TO func-pci-device-props + phb-pci-walk-bridge \ PHB device tree is already populated. + THEN r> TO puid \ Restore previous puid ; phb-setup-children diff --git a/slof/fs/pci-properties.fs b/slof/fs/pci-properties.fs index 9efa87e..4f13402 100644 --- a/slof/fs/pci-properties.fs +++ b/slof/fs/pci-properties.fs @@ -651,6 +651,8 @@ r> TO pci-device-slots \ and reset the slot array ; +DEFER func-pci-device-props + \ used for an gerneric device set up \ if a device has no special handling for setup \ the device file (pci-device_VENDOR_DEVICE.fs) can call @@ -659,6 +661,8 @@ dup assign-all-device-bars \ calc all BARs dup pci-set-irq-line \ set the interrupt pin dup pci-set-capabilities \ set up the capabilities - dup pci-device-props \ and generate all properties + dup func-pci-device-props \ and generate all properties drop \ forget the config-addr ; + +' pci-device-props TO func-pci-device-props
PCI Enumeration has been part of SLOF. Now with hotplug code addition in Qemu, it makes more sense to have this code a one place, i.e. Qemu. Adding routines to walk through the device nodes created by Qemu. SLOF will configure the device/bridges and program the BARs for communicating with the devices. Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> --- board-qemu/slof/pci-phb.fs | 44 +++++++++++++++++++++++++++++++++++++++++++- slof/fs/pci-properties.fs | 6 +++++- 2 files changed, 48 insertions(+), 2 deletions(-)