| Message ID | 20170427192125.23bd4af4@aik.ozlabs.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
On 27.04.2017 11:21, Alexey Kardashevskiy wrote: > On Thu, 27 Apr 2017 10:51:12 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 27.04.2017 10:30, Alexey Kardashevskiy wrote: >>> On Thu, 27 Apr 2017 09:50:36 +0200 >>> Thomas Huth <thuth@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@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@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
On Thu, 27 Apr 2017 11:52:18 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 27.04.2017 11:21, Alexey Kardashevskiy wrote: > > On Thu, 27 Apr 2017 10:51:12 +0200 > > Thomas Huth <thuth@redhat.com> wrote: > > > >> On 27.04.2017 10:30, Alexey Kardashevskiy wrote: > >>> On Thu, 27 Apr 2017 09:50:36 +0200 > >>> Thomas Huth <thuth@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@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@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
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