[SLOF] [PATCH] pci: Avoid 32-bit prefetchable memory area if possible

[Alexey Kardashevskiy]

On 20/07/17 18:47, Thomas Huth wrote:
> On 20.07.2017 08:54, Alexey Kardashevskiy wrote:
>> On 17/07/17 20:05, Thomas Huth wrote:
>>> On 17.07.2017 08:18, Alexey Kardashevskiy wrote:
>>>> On 14/07/17 19:45, Thomas Huth wrote:
>>>>> PCI bridges can only have one prefetchable memory area. If we are
>>>>> already using 64-bit prefetchable memory regions, we can not use
>>>>> a dedicated 32-bit prefetchable memory region anymore. In that
>>>>> case the 32-bit BARs should all be located in the 32-bit non-
>>>>> prefetchable memory space instead.
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth at redhat.com>
>>>>> ---
>>>>>  board-qemu/slof/pci-phb.fs | 16 +++++++++++-----
>>>>>  slof/fs/pci-properties.fs  |  7 ++++++-
>>>>>  2 files changed, 17 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/board-qemu/slof/pci-phb.fs b/board-qemu/slof/pci-phb.fs
>>>>> index b7bf9cf..926efba 100644
>>>>> --- a/board-qemu/slof/pci-phb.fs
>>>>> +++ b/board-qemu/slof/pci-phb.fs
>>>>> @@ -253,12 +253,9 @@ setup-puid
>>>>>              THEN
>>>>>           ENDOF
>>>>>           2000000 OF                             \ 32-bit memory space?
>>>>> -            decode-64 pci-next-mem !            \ Decode mem base address
>>>>> +            decode-64 dup >r pci-next-mmio !    \ Decode base address
>>>>>              decode-64 drop                      \ Forget the parent address
>>>>> -            decode-64 2 / dup >r                \ Decode and calc size/2
>>>>> -            pci-next-mem @ + dup pci-max-mem !  \ and calc max mem address
>>>>> -            dup pci-next-mmio !                 \ which is the same as MMIO base
>>>>> -            r> + pci-max-mmio !                 \ calc max MMIO address
>>>>> +            decode-64 r> + pci-max-mmio !       \ calc max MMIO address
>>>>>           ENDOF
>>>>>           3000000 OF                             \ 64-bit memory space?
>>>>>              decode-64 dup >r pci-next-mem64 !
>>>>> @@ -270,6 +267,15 @@ setup-puid
>>>>>     ( prop-addr prop-len )
>>>>>     2drop
>>>>>  
>>>>> +   \ If we do not have 64-bit prefetchable memory, split the 32-bit space:
>>>>
>>>> When is this ^^^^^^^^^^^^^^^^^^^ possible?
>>>
>>> This happens if you either use SLOF with an older version of QEMU, or
>>> start a recent QEMU with an older machine type, e.g. "-M pseries-2.1".
>>> That means, we've still got to support this for running older VMs on
>>> current QEMU.
>>>
>>>>> +   pci-next-mem64 @ 0= IF
>>>>> +      pci-next-mmio @ pci-next-mem !            \ Start of 32-bit prefetchable
>>>>> +      pci-max-mmio @ pci-next-mmio @ - 2 /      \ Calculate new size
>>>>> +      pci-next-mmio @ +                         \ The middle of the area
>>>>> +      dup pci-max-mem !
>>>>> +      pci-next-mmio !
>>>>> +   THEN
>>>>> +
>>>>>     phb-debug? IF
>>>>>       ." pci-next-io   = " pci-next-io @ . cr
>>>>>       ." pci-max-io    = " pci-max-io  @ . cr
>>>>> diff --git a/slof/fs/pci-properties.fs b/slof/fs/pci-properties.fs
>>>>> index b7bb534..6f8f013 100644
>>>>> --- a/slof/fs/pci-properties.fs
>>>>> +++ b/slof/fs/pci-properties.fs
>>>>> @@ -159,7 +159,12 @@
>>>>>  \ Setup a prefetchable 32bit BAR and return its size
>>>>>  : assign-mem32-bar ( bar-addr -- 4 )
>>>>>          dup pci-bar-size-mem32          \ fetch size
>>>>> -        pci-next-mem                    \ var to change
>>>>> +        \ Do we have a dedicated 32-bit prefetchable area? If not, use MMIO
>>>>> +        pci-next-mem @ IF
>>>>> +            pci-next-mem
>>>>> +        ELSE
>>>>> +            pci-next-mmio
>>>>> +        THEN
>>>>
>>>>
>>>> The commit log explains this chunk but not the other chunks.
>>>
>>> We've got to avoid to create that fake "pci-next-mem" region to be
>>> able to check pci-next-mem != 0 here. Shall I respin the patch
>>> and elaborate this in the commit message?
>>>
>>>> How did you test the change to get different behaviour?
>>>
>>> Run QEMU with "-M pseries-2.1"
>>
>> I did not go that far, I tried this:
>>
>> qemu-system-ppc64 \
>> -nodefaults \
>> -chardev stdio,id=STDIO0,signal=off,mux=on \
>> -device spapr-vty,id=svty0,chardev=STDIO0,reg=0x71000100 \
>> -mon id=MON0,chardev=STDIO0,mode=readline -vnc "localhost:100" \
>> -device pci-bridge,id=pci.0_1,bus=pci.0,addr=1.0,chassis_nr=1 \
>> -device VGA,id=VGA0,bus=pci.0,addr=2.0 -enable-kvm \
>> -smp 8,threads=8 \
>> -machine pseries
>>
>> Note that both bridge and VGA are on the root bus, the bridge goes first.
>> Without this patch, VNC shows what is expected, with the patch - it is a
>> black screen.
>>
>> "pci: Translate PCI addresses to host addresses at the end of map-in" does
>> not change anything here.
>>
>> Ideas?
> 
> You also need my "Fix pci-bridge-set-mem-base and
> pci-bridge-set-mem-limit" patch in this case. The problem is that the
> old pci-bridge-set-mem-limit function messes around with pci-next-mem -
> even if that memory region should not be used at all:
> 
> : pci-bridge-set-mem-limit ( addr -- )
>         pci-next-mem @ 100000 +            \ add space for hot-plugging
>         100000 #aligned                    \ align to 1MB boundary
>         dup pci-next-mem !                 \ and write it back
> 
> So if pci-next-mem was initially set to 0, it is set to the non-sense
> value 0x100000 after the bridge has been scanned, so the code in
> assign-mem32-bar sets the BAR to a wrong value.


Ok, that patch helps, thanks. I'd still like to get a cleaner version of that.

Another observation - the vga does not work if the bridge it is connected
to has multifunction=on:

-device
pci-bridge,id=pci.0_1,bus=pci.0,addr=1.0,chassis_nr=1,multifunction=on \
-device VGA,id=VGA0,bus=pci.0_1,addr=2.0

Multifunction bridges do not make much sense (or at all) but the difference
in behaviour is still not clear to me, ideas?




-- 
Alexey