Re: [PATCH 2/2] pci: Use Qemu created PCI device nodes

[Thomas Huth <thuth@redhat.com>]

 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