[SLOF] [PATCH slof] pci: Put non-prefetchable 64bit BARs into 32bit MMIO window
Thomas Huth
thuth at redhat.com
Thu Apr 27 19:52:18 AEST 2017
On 27.04.2017 11:21, Alexey Kardashevskiy wrote:
> On Thu, 27 Apr 2017 10:51:12 +0200
> Thomas Huth <thuth at redhat.com> wrote:
>
>> On 27.04.2017 10:30, Alexey Kardashevskiy wrote:
>>> On Thu, 27 Apr 2017 09:50:36 +0200
>>> Thomas Huth <thuth at redhat.com> wrote:
>>>
>>>> On 27.04.2017 09:10, Thomas Huth wrote:
>>>>> On 27.04.2017 08:54, Alexey Kardashevskiy wrote:
>>>>>> On Wed, 26 Apr 2017 15:23:40 +0200
>>>>>> Thomas Huth <thuth at redhat.com> wrote:
>>>>>>
>>>>>>> On 24.04.2017 05:01, Alexey Kardashevskiy wrote:
>>>>>>>> At the moment 64bit non-prefetchable BARs of devices behind PCI
>>>>>>>> p2p bridge go to a 64bit prefetchable windows which is not
>>>>>>>> correct and causes linux guests to fail to ioremap() such
>>>>>>>> resources.
>>>>>>>>
>>>>>>>> This moves 64bit non-prefetchable BARs 32bit non-prefetchable
>>>>>>>> window.
>>>>>>>>
>>>>>>>> Note that this does not make distinction between P2P and PHB so
>>>>>>>> from now on XHCI BARs will be allocated from 32bit MMIO space.
>>>>>>>> However since most 64bit-MMIO-capable devices have prefetchable
>>>>>>>> BARs, and XHCI BAR is just 4K (so it is unlikely to cause any
>>>>>>>> space problems), this should not affect usual behavior.
>>>>>>>>
>>>>>>>> Signed-off-by: Alexey Kardashevskiy <aik at ozlabs.ru>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> This fixes QEMU's XHCI when it is put on a P2P PCI bridge.
>>>>>>>>
>>>>>>>> There is a little naming confusion as it may look like the
>>>>>>>> patch is changing assignment for all 64bit BAR but it does not
>>>>>>>> as:
>>>>>>>> - "mmio" is used for non-prefetchable memory,
>>>>>>>> - "mem" is used for prefetchable memory.
>>>>>>>> ---
>>>>>>>> slof/fs/pci-properties.fs | 6 +-----
>>>>>>>> 1 file changed, 1 insertion(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/slof/fs/pci-properties.fs
>>>>>>>> b/slof/fs/pci-properties.fs index e6fd843..8594e5d 100644
>>>>>>>> --- a/slof/fs/pci-properties.fs
>>>>>>>> +++ b/slof/fs/pci-properties.fs
>>>>>>>> @@ -166,11 +166,7 @@
>>>>>>>> \ Setup a non-prefetchable 64bit BAR and return its size
>>>>>>>> : assign-mmio64-bar ( bar-addr -- 8 )
>>>>>>>> dup pci-bar-size-mem64 \ fetch size
>>>>>>>> - pci-next-mem64 @ 0 = IF \ Check if we have
>>>>>>>> 64-bit memory range
>>>>>>>> - pci-next-mmio
>>>>>>>> - ELSE
>>>>>>>> - pci-next-mem64 \ for board-qemu we
>>>>>>>> will use same range
>>>>>>>> - THEN
>>>>>>>> + pci-next-mmio
>>>>>>>> assign-bar-value64 \ and set it all
>>>>>>>> ;
>>>>>>>
>>>>>>> I'm sorry, but this is now causing trouble with the USB keyboard
>>>>>>> in *SLOF* instead. If I start QEMU now like this:
>>>>>>>
>>>>>>> qemu-system-ppc64 -nodefaults -serial mon:stdio \
>>>>>>> -device pci-bridge,bus=pci.0,id=bridge1,chassis_nr=1 \
>>>>>>> -device nec-usb-xhci,id=xhci1,bus=bridge1,addr=0x3 -vga std \
>>>>>>> -device usb-kbd,bus=xhci1.0 -bios boot_rom.bin
>>>>>>>
>>>>>>> then SLOF complains: "usb-xhci: failed to initialize XHCI
>>>>>>> controller" and the keyboard is not working at the SLOF prompt
>>>>>>> anymore.
>>>>>>>
>>>>>>> Any idea why this is happening now?
>>>>>>
>>>>>>
>>>>>> XHCI's usb_hcd_dev::base == 0xc0000000 which is a bus address
>>>>>> while it is supposed to be a mapped address, i.e.
>>>>>> 800000020000000.
>>>>>>
>>>>>> It used to work before the patch as pci address is the same as
>>>>>> mapped address - 210000000000 - and I am not sure if this is not
>>>>>> an accident, could be QEMU's PCI MMIO windows/spacing rework
>>>>>> David did some time ago.
>>>>>>
>>>>>> I am trying to figure out now where usb_hcd_dev::base is set in
>>>>>> SLOF, it is tricky :-/
>>>>>
>>>>> I think the value comes from usb-setup-hcidev in
>>>>> pci-class_0c.fs ... so it's likely that translate-my-address is
>>>>> failing now.
>>>>>
>>>>> ... but thinking about this issue again, I now wonder whether your
>>>>> original patch was really the right solution here, since QEMU only
>>>>> provides three memory regions to the guest:
>>>>> 1) I/O
>>>>> 2) 32-bit memory
>>>>> 3) 64-bit memory
>>>>> There is no distinction between prefetchable and non-prefetchable
>>>>> here. So I think the original problem might have been caused by
>>>>> something else instead... (not sure about that yet, though)
>>>>
>>>> Yup, looks like the real culprit is the "ranges" property of the
>>>> PCI bridges. This can not deal with 64-bit memory regions yet. I
>>>> think we should revert the patch to assign-mmio64-bar and fix
>>>> pci-bridge-range-props and friends instead...
>>>
>>> Try this:
>>>
>>> diff --git a/slof/fs/pci-properties.fs b/slof/fs/pci-properties.fs
>>> index 8594e5d..66e364d 100644
>>> --- a/slof/fs/pci-properties.fs
>>> +++ b/slof/fs/pci-properties.fs
>>> @@ -349,7 +349,7 @@
>>> 1 OF gen-io-bar-prop ENDOF \ - IO-BAR
>>> 2 OF gen-mem32-bar-prop ENDOF \ - MEM32
>>> 3 OF gen-pmem32-bar-prop ENDOF \ - MEM32
>>> prefetchable
>>> - 4 OF gen-mem64-bar-prop ENDOF \ - MEM64
>>> + 4 OF gen-mem32-bar-prop ENDOF \ - MEM64
>>> 5 OF gen-pmem64-bar-prop ENDOF \ - MEM64
>>> prefetchable ENDCASE \ ESAC
>>> ( paddr plen bsize ) ;
>>>
>>> Works for me, for both PHB and P2P bridges.
>>
>> That might work as long as the device has only one BAR. But as soon
>> as you've got a device with multiple BARs, you'll run into problems
>> since pci-add-assigned-address now returns the wrong BAR size.
>
> Ah, right. This should do it instead:
>
> diff --git a/slof/fs/pci-properties.fs b/slof/fs/pci-properties.fs
> index 8594e5d..d446473 100644
> --- a/slof/fs/pci-properties.fs
> +++ b/slof/fs/pci-properties.fs
> @@ -254,13 +254,13 @@
> : gen-mem64-bar-prop ( prop-addr prop-len bar-addr -- prop-addr prop-len 8 )
> dup pci-bar-size-mem64 \ fetch BAR Size ( paddr plen baddr bsize )
> dup IF \ IF Size > 0
> >r dup rtas-config-l@ \ | save size and fetch lower 32 bits ( paddr plen baddr val.lo R: size)
> over 4 + rtas-config-l@ \ | fetch upper 32 bits ( paddr plen baddr val.lo val.hi R: size)
> 20 lshift + -10 and >r \ | calc 64 bit value and save it ( paddr plen baddr R: size val )
> - 83000000 or encode-int+ \ | Encode config addr ( paddr plen R: size val )
> + 82000000 or encode-int+ \ | Encode config addr ( paddr plen R: size val )
> r> encode-64+ \ | Encode assigned addr ( paddr plen R: size )
> r> encode-64+ \ | Encode size ( paddr plen )
> ELSE \ ELSE
> 2drop \ | don't do anything
> THEN \ FI
> 8 \ sizeof(BAR) = 8 Bytes
OK, but I think you should also add a comment before that line with an
explanation for the 32-bit space here ... otherwise we will likely not
understand this anymore in a year or two ...
>> Please try this instead: Revert your original patch to
>> assign-mmio64-bar and apply this patch instead:
>>
>> diff --git a/slof/fs/pci-properties.fs b/slof/fs/pci-properties.fs
>> index e6fd843..625692a 100644
>> --- a/slof/fs/pci-properties.fs
>> +++ b/slof/fs/pci-properties.fs
>> @@ -411,12 +411,14 @@
>> : pci-bridge-gen-mem-range ( addr prop-addr prop-len -- addr
>> prop-addr prop-len ) 2 pick 24 + rtas-config-l@ \ fetch
>> Value ( addr paddr plen val ) dup 000FFFFF
>> or \ calc limit Bits 31:0 ( addr paddr plen val
>> limit.31:0 )
>> - swap 0000FFF0 and 10 lshift \ calc base Bits 31:0
>> ( addr paddr plen limit.31:0 base.31:0 )
>> + swap dup F and >r \ R: 64-bit?
>> + 0000FFF0 and 10 lshift \ calc base Bits 31:0
>> ( addr paddr plen limit.31:0 base.31:0 ) 4 pick 28 +
>> rtas-config-l@ \ fetch upper Basebits ( addr paddr plen
>> limit.31:0 base.31:0 base.63:32 ) 20 lshift or swap \
>> and calc Base ( addr paddr plen base.63:0 limit.31:0 ) 4 pick
>> 2C + rtas-config-l@ \ fetch upper Limitbits ( addr paddr plen
>> base.63:0 limit.31:0 limit.63:32 ) 20 lshift or \
>> and calc Limit ( addr paddr plen base.63:0 limit.63:0 )
>> - 42000000 pci-bridge-gen-range \ and generate it
>> ( addr paddr plen )
>> + r> IF 03000000 ELSE 42000000 THEN \ 64-bit or 32-bit ?
>> + pci-bridge-gen-range \ and generate it
>> ( addr paddr plen ) ;
>>
>> This fixes all the XHCI problems for me.
>
> Yes it does fix XHCI but it also puts 64bit non-prefetchable BAR to
> prefetchable window which we decided is not correct.
True... but I should likely still send such a patch with the
prefetchable bit set instead, I guess...
Thomas
More information about the SLOF
mailing list