[SLOF] [PATCH slof] pci: Put non-prefetchable 64bit BARs into 32bit MMIO window
Alexey Kardashevskiy
aik at ozlabs.ru
Thu Apr 27 23:56:35 AEST 2017
On Thu, 27 Apr 2017 11:52:18 +0200
Thomas Huth <thuth at redhat.com> wrote:
> 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 ...
This is what git commit logs are for :) But I'll add small comment
anyway.
>
> >> 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...
Yes, please.
--
Alexey
More information about the SLOF
mailing list