diff mbox series

[PULL,v2,75/86] include/hw/pci/pcie_host: Correct PCIE_MMCFG_SIZE_MAX

Message ID 20220516204913.542894-76-mst@redhat.com (mailing list archive)
State New, archived
Headers show
Series [PULL,v2,01/86] virtio: fix feature negotiation for ACCESS_PLATFORM | expand

Commit Message

Michael S. Tsirkin May 16, 2022, 8:55 p.m. UTC 
From: Francisco Iglesias <frasse.iglesias@gmail.com>

According to 7.2.2 in [1] bit 27 is the last bit that can be part of the
bus number, this makes the ECAM max size equal to '1 << 28'. This patch
restores back this value into the PCIE_MMCFG_SIZE_MAX define (which was
changed in commit 58d5b22bbd5 ("ppc4xx: Add device models found in PPC440
core SoCs")).

[1] PCI Express® Base Specification Revision 5.0 Version 1.0

Signed-off-by: Francisco Iglesias <frasse.iglesias@gmail.com>
Message-Id: <20220411221836.17699-3-frasse.iglesias@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/pci/pcie_host.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Daniel Henrique Barboza May 26, 2022, 1:27 p.m. UTC | #1 
Hi,

This patch broke the boot of the sam460ex ppc machine:

qemu-system-ppc -M sam460ex -kernel ./buildroot/qemu_ppc_sam460ex-latest/vmlinux \
-device virtio-net-pci,netdev=net0 -netdev user,id=net0 -serial mon:stdio \
-nographic -snapshot
qemu-system-ppc: ../hw/pci/pcie_host.c:97: pcie_host_mmcfg_init: Assertion `size <= PCIE_MMCFG_SIZE_MAX' failed.

The reason is that it changed commit 58d5b22bbd5 ("ppc4xx: Add device models
found in PPC440 core SoCs")) in a way that it wasn't expected by the board. The
code seems to believe that, for a reason that isn't stated in the 58d5b22bbd5 commit
message, PCIE_MMCFG_SIZE_MAX must be set to 1 << 29.

I'm CCing BALATON Zoltan since he's the author of 58d5b22bbd5 and can provide context of
his initial change and why the board seems to rely on it. qemu-ppc is being CCed for
awareness of the sam460ex problem.


Zoltan, I wasn't able to amend to quickly amend the code in a way that I could
preserve the current PCIE_MMCFG_SIZE_MAX setting and make sam460ex work again.
Can you please take a look?


Thanks,


Daniel



On 5/16/22 17:55, Michael S. Tsirkin wrote:
> From: Francisco Iglesias <frasse.iglesias@gmail.com>
> 
> According to 7.2.2 in [1] bit 27 is the last bit that can be part of the
> bus number, this makes the ECAM max size equal to '1 << 28'. This patch
> restores back this value into the PCIE_MMCFG_SIZE_MAX define (which was
> changed in commit 58d5b22bbd5 ("ppc4xx: Add device models found in PPC440
> core SoCs")).
> 
> [1] PCI Express® Base Specification Revision 5.0 Version 1.0
> 
> Signed-off-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> Message-Id: <20220411221836.17699-3-frasse.iglesias@gmail.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   include/hw/pci/pcie_host.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/pci/pcie_host.h b/include/hw/pci/pcie_host.h
> index b3c8ce973c..82d92177da 100644
> --- a/include/hw/pci/pcie_host.h
> +++ b/include/hw/pci/pcie_host.h
> @@ -65,7 +65,7 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
>    * bit 12 - 14: function number
>    * bit  0 - 11: offset in configuration space of a given device
>    */
> -#define PCIE_MMCFG_SIZE_MAX             (1ULL << 29)
> +#define PCIE_MMCFG_SIZE_MAX             (1ULL << 28)
>   #define PCIE_MMCFG_SIZE_MIN             (1ULL << 20)
>   #define PCIE_MMCFG_BUS_BIT              20
>   #define PCIE_MMCFG_BUS_MASK             0xff
BALATON Zoltan May 26, 2022, 3:54 p.m. UTC | #2 
Hello,

On Thu, 26 May 2022, Daniel Henrique Barboza wrote:
> Hi,
>
> This patch broke the boot of the sam460ex ppc machine:
>
> qemu-system-ppc -M sam460ex -kernel 
> ./buildroot/qemu_ppc_sam460ex-latest/vmlinux \
> -device virtio-net-pci,netdev=net0 -netdev user,id=net0 -serial mon:stdio \
> -nographic -snapshot
> qemu-system-ppc: ../hw/pci/pcie_host.c:97: pcie_host_mmcfg_init: Assertion 
> `size <= PCIE_MMCFG_SIZE_MAX' failed.

Thanks for noticing this. I usually only test it during the freeze. Wasn't 
there a test patch submitted by Philippe before? Isn't that yet merged or 
included in CI? That should catch these before breaking it.

> The reason is that it changed commit 58d5b22bbd5 ("ppc4xx: Add device 
> models found in PPC440 core SoCs")) in a way that it wasn't expected by 
> the board. The code seems to believe that, for a reason that isn't 
> stated in the 58d5b22bbd5 commit message, PCIE_MMCFG_SIZE_MAX must be 
> set to 1 << 29.
>
> I'm CCing BALATON Zoltan since he's the author of 58d5b22bbd5 and can 
> provide context of his initial change and why the board seems to rely on 
> it. qemu-ppc is being CCed for awareness of the sam460ex problem.

I'm afraid I don't remember but maybe I did not have a definitive answer 
even back then as the docs for this PCIe controller were not available so 
I've mostly worked from docs for similar SoCs and U-Boot and Linux sources 
so there were a lot of guessing. Maybe it's related to that the board maps 
peripheral addresses above 4GB as the first 4GB is reserved for memory? Or 
maybe there's some mixup between address spaces and the PCIe controller 
should have a separate address space that's mapped in the system? I did 
not have any knowledge about this back then and my understanding may still 
be lacking on how this should work.

> Zoltan, I wasn't able to amend to quickly amend the code in a way that I 
> could preserve the current PCIE_MMCFG_SIZE_MAX setting and make sam460ex 
> work again. Can you please take a look?

The PCIe controllers of the 460EX are implemented at the end of 
hw/ppc/ppc440_uc.c (a lot of these 4xx SoCs are sharing components and the 
code organisation is a bit messy). As the comment near it says it's not 
really fully tested and working. only good enough for firmware and OSes 
get past testing it. I think trying to attach any device to it probably 
would fail or I would be surprised if the OS could actually talk to it as 
there may be some missing parts. So I'm happy with any solution that keeps 
the current state of being able to boot the OSes running on it (some of 
which like AmigaOS and MorphOS are closed source though so I don't know 
what their drivers need; closest open source OS to them is AROS but not 
sure that's working on real hardware). Some advice from somebody more 
knowledgeable about PCIe emulation in QEMU would be welcome here.

Regards,
BALATON Zoltan

>
> Thanks,
>
>
> Daniel
>
>
>
> On 5/16/22 17:55, Michael S. Tsirkin wrote:
>> From: Francisco Iglesias <frasse.iglesias@gmail.com>
>> 
>> According to 7.2.2 in [1] bit 27 is the last bit that can be part of the
>> bus number, this makes the ECAM max size equal to '1 << 28'. This patch
>> restores back this value into the PCIE_MMCFG_SIZE_MAX define (which was
>> changed in commit 58d5b22bbd5 ("ppc4xx: Add device models found in PPC440
>> core SoCs")).
>> 
>> [1] PCI Express® Base Specification Revision 5.0 Version 1.0
>> 
>> Signed-off-by: Francisco Iglesias <frasse.iglesias@gmail.com>
>> Message-Id: <20220411221836.17699-3-frasse.iglesias@gmail.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>   include/hw/pci/pcie_host.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/include/hw/pci/pcie_host.h b/include/hw/pci/pcie_host.h
>> index b3c8ce973c..82d92177da 100644
>> --- a/include/hw/pci/pcie_host.h
>> +++ b/include/hw/pci/pcie_host.h
>> @@ -65,7 +65,7 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
>>    * bit 12 - 14: function number
>>    * bit  0 - 11: offset in configuration space of a given device
>>    */
>> -#define PCIE_MMCFG_SIZE_MAX             (1ULL << 29)
>> +#define PCIE_MMCFG_SIZE_MAX             (1ULL << 28)
>>   #define PCIE_MMCFG_SIZE_MIN             (1ULL << 20)
>>   #define PCIE_MMCFG_BUS_BIT              20
>>   #define PCIE_MMCFG_BUS_MASK             0xff
>
>
BALATON Zoltan May 26, 2022, 4:43 p.m. UTC | #3 
On Thu, 26 May 2022, BALATON Zoltan wrote:
> Hello,
>
> On Thu, 26 May 2022, Daniel Henrique Barboza wrote:
>> Hi,
>> 
>> This patch broke the boot of the sam460ex ppc machine:
>> 
>> qemu-system-ppc -M sam460ex -kernel 
>> ./buildroot/qemu_ppc_sam460ex-latest/vmlinux \
>> -device virtio-net-pci,netdev=net0 -netdev user,id=net0 -serial mon:stdio \
>> -nographic -snapshot
>> qemu-system-ppc: ../hw/pci/pcie_host.c:97: pcie_host_mmcfg_init: Assertion 
>> `size <= PCIE_MMCFG_SIZE_MAX' failed.

With just qemu-system-ppc -M sam460ex the assert seems to happen when the 
guest (board firmware?) writes a value to CFGMSK reg:

(gdb) bt
#0  0x00007ffff68ff4a0 in raise () at /lib64/libc.so.6
#1  0x00007ffff68ea536 in abort () at /lib64/libc.so.6
#2  0x00007ffff68ea42f in _nl_load_domain.cold () at /lib64/libc.so.6
#3  0x00007ffff68f7ed2 in  () at /lib64/libc.so.6
#4  0x000055555596646f in pcie_host_mmcfg_init (e=e@entry=0x5555567942f0, size=size@entry=0x20000000) at ../hw/pci/pcie_host.c:97
#5  0x000055555596653b in pcie_host_mmcfg_map (size=0x20000000, addr=0xd20000000, e=0x5555567942f0) at ../hw/pci/pcie_host.c:105
#6  pcie_host_mmcfg_update (e=0x5555567942f0, enable=0x1, addr=0xd20000000, size=0x20000000) at ../hw/pci/pcie_host.c:118
#7  0x0000555555a70d7c in ppc_dcr_write (dcr_env=0x555556669c10, dcrn=0x122, val=0xe0000001) at ../hw/ppc/ppc.c:1418
#8  0x0000555555abdabb in helper_store_dcr (env=0x555556633360, dcrn=0x122, val=0xe0000001) at ../target/ppc/timebase_helper.c:188

This is done in the board firmware here:

https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD#l963

when trying to map config space. Here the size is calculated as 0x20000000 
which does not fit the assert. I'm not sure what this means though and 
where is the problem. Any ideas?

Regards,
BALATON Zoltan

> Thanks for noticing this. I usually only test it during the freeze. Wasn't 
> there a test patch submitted by Philippe before? Isn't that yet merged or 
> included in CI? That should catch these before breaking it.
>
>> The reason is that it changed commit 58d5b22bbd5 ("ppc4xx: Add device 
>> models found in PPC440 core SoCs")) in a way that it wasn't expected by the 
>> board. The code seems to believe that, for a reason that isn't stated in 
>> the 58d5b22bbd5 commit message, PCIE_MMCFG_SIZE_MAX must be set to 1 << 29.
>> 
>> I'm CCing BALATON Zoltan since he's the author of 58d5b22bbd5 and can 
>> provide context of his initial change and why the board seems to rely on 
>> it. qemu-ppc is being CCed for awareness of the sam460ex problem.
>
> I'm afraid I don't remember but maybe I did not have a definitive answer even 
> back then as the docs for this PCIe controller were not available so I've 
> mostly worked from docs for similar SoCs and U-Boot and Linux sources so 
> there were a lot of guessing. Maybe it's related to that the board maps 
> peripheral addresses above 4GB as the first 4GB is reserved for memory? Or 
> maybe there's some mixup between address spaces and the PCIe controller 
> should have a separate address space that's mapped in the system? I did not 
> have any knowledge about this back then and my understanding may still be 
> lacking on how this should work.
>
>> Zoltan, I wasn't able to amend to quickly amend the code in a way that I 
>> could preserve the current PCIE_MMCFG_SIZE_MAX setting and make sam460ex 
>> work again. Can you please take a look?
>
> The PCIe controllers of the 460EX are implemented at the end of 
> hw/ppc/ppc440_uc.c (a lot of these 4xx SoCs are sharing components and the 
> code organisation is a bit messy). As the comment near it says it's not 
> really fully tested and working. only good enough for firmware and OSes get 
> past testing it. I think trying to attach any device to it probably would 
> fail or I would be surprised if the OS could actually talk to it as there may 
> be some missing parts. So I'm happy with any solution that keeps the current 
> state of being able to boot the OSes running on it (some of which like 
> AmigaOS and MorphOS are closed source though so I don't know what their 
> drivers need; closest open source OS to them is AROS but not sure that's 
> working on real hardware). Some advice from somebody more knowledgeable about 
> PCIe emulation in QEMU would be welcome here.
>
> Regards,
> BALATON Zoltan
>
>> 
>> Thanks,
>> 
>> 
>> Daniel
>> 
>> 
>> 
>> On 5/16/22 17:55, Michael S. Tsirkin wrote:
>>> From: Francisco Iglesias <frasse.iglesias@gmail.com>
>>> 
>>> According to 7.2.2 in [1] bit 27 is the last bit that can be part of the
>>> bus number, this makes the ECAM max size equal to '1 << 28'. This patch
>>> restores back this value into the PCIE_MMCFG_SIZE_MAX define (which was
>>> changed in commit 58d5b22bbd5 ("ppc4xx: Add device models found in PPC440
>>> core SoCs")).
>>> 
>>> [1] PCI Express® Base Specification Revision 5.0 Version 1.0
>>> 
>>> Signed-off-by: Francisco Iglesias <frasse.iglesias@gmail.com>
>>> Message-Id: <20220411221836.17699-3-frasse.iglesias@gmail.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>   include/hw/pci/pcie_host.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/include/hw/pci/pcie_host.h b/include/hw/pci/pcie_host.h
>>> index b3c8ce973c..82d92177da 100644
>>> --- a/include/hw/pci/pcie_host.h
>>> +++ b/include/hw/pci/pcie_host.h
>>> @@ -65,7 +65,7 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
>>>    * bit 12 - 14: function number
>>>    * bit  0 - 11: offset in configuration space of a given device
>>>    */
>>> -#define PCIE_MMCFG_SIZE_MAX             (1ULL << 29)
>>> +#define PCIE_MMCFG_SIZE_MAX             (1ULL << 28)
>>>   #define PCIE_MMCFG_SIZE_MIN             (1ULL << 20)
>>>   #define PCIE_MMCFG_BUS_BIT              20
>>>   #define PCIE_MMCFG_BUS_MASK             0xff
>> 
>
Michael S. Tsirkin May 26, 2022, 7:13 p.m. UTC | #4 
On Thu, May 26, 2022 at 06:43:25PM +0200, BALATON Zoltan wrote:
> On Thu, 26 May 2022, BALATON Zoltan wrote:
> > Hello,
> > 
> > On Thu, 26 May 2022, Daniel Henrique Barboza wrote:
> > > Hi,
> > > 
> > > This patch broke the boot of the sam460ex ppc machine:
> > > 
> > > qemu-system-ppc -M sam460ex -kernel
> > > ./buildroot/qemu_ppc_sam460ex-latest/vmlinux \
> > > -device virtio-net-pci,netdev=net0 -netdev user,id=net0 -serial mon:stdio \
> > > -nographic -snapshot
> > > qemu-system-ppc: ../hw/pci/pcie_host.c:97: pcie_host_mmcfg_init:
> > > Assertion `size <= PCIE_MMCFG_SIZE_MAX' failed.
> 
> With just qemu-system-ppc -M sam460ex the assert seems to happen when the
> guest (board firmware?) writes a value to CFGMSK reg:
> 
> (gdb) bt
> #0  0x00007ffff68ff4a0 in raise () at /lib64/libc.so.6
> #1  0x00007ffff68ea536 in abort () at /lib64/libc.so.6
> #2  0x00007ffff68ea42f in _nl_load_domain.cold () at /lib64/libc.so.6
> #3  0x00007ffff68f7ed2 in  () at /lib64/libc.so.6
> #4  0x000055555596646f in pcie_host_mmcfg_init (e=e@entry=0x5555567942f0, size=size@entry=0x20000000) at ../hw/pci/pcie_host.c:97
> #5  0x000055555596653b in pcie_host_mmcfg_map (size=0x20000000, addr=0xd20000000, e=0x5555567942f0) at ../hw/pci/pcie_host.c:105
> #6  pcie_host_mmcfg_update (e=0x5555567942f0, enable=0x1, addr=0xd20000000, size=0x20000000) at ../hw/pci/pcie_host.c:118
> #7  0x0000555555a70d7c in ppc_dcr_write (dcr_env=0x555556669c10, dcrn=0x122, val=0xe0000001) at ../hw/ppc/ppc.c:1418
> #8  0x0000555555abdabb in helper_store_dcr (env=0x555556633360, dcrn=0x122, val=0xe0000001) at ../target/ppc/timebase_helper.c:188
> 
> This is done in the board firmware here:
> 
> https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD#l963
> 
> when trying to map config space. Here the size is calculated as 0x20000000
> which does not fit the assert. I'm not sure what this means though and where
> is the problem. Any ideas?
> 
> Regards,
> BALATON Zoltan


It says so, does it not?

1051         switch (port) {
1052         case 0:
1053                 mtdcr(DCRN_PEGPL_CFGBAH(PCIE0), high);
1054                 mtdcr(DCRN_PEGPL_CFGBAL(PCIE0), low);
1055                 mtdcr(DCRN_PEGPL_CFGMSK(PCIE0), 0xe0000001); /* 512MB region, valid */
1056                 break;
1057         case 1:
1058                 mtdcr(DCRN_PEGPL_CFGBAH(PCIE1), high);
1059                 mtdcr(DCRN_PEGPL_CFGBAL(PCIE1), low);
1060                 mtdcr(DCRN_PEGPL_CFGMSK(PCIE1), 0xe0000001); /* 512MB region, valid */
1061                 break;
1062 #if CONFIG_SYS_PCIE_NR_PORTS > 2
1063         case 2:
1064                 mtdcr(DCRN_PEGPL_CFGBAH(PCIE2), high);
1065                 mtdcr(DCRN_PEGPL_CFGBAL(PCIE2), low);
1066                 mtdcr(DCRN_PEGPL_CFGMSK(PCIE2), 0xe0000001); /* 512MB region, valid */
1067                 break;
1068 #endif

and basically according to spec max size is 256MB.

we can fix the firmware of course, or we can just drop the assert,
or force it within range in the ppc code.

Fixing firmware seems cleaner ... want to try?
Any preference? 



> > Thanks for noticing this. I usually only test it during the freeze.
> > Wasn't there a test patch submitted by Philippe before? Isn't that yet
> > merged or included in CI? That should catch these before breaking it.
> > 
> > > The reason is that it changed commit 58d5b22bbd5 ("ppc4xx: Add
> > > device models found in PPC440 core SoCs")) in a way that it wasn't
> > > expected by the board. The code seems to believe that, for a reason
> > > that isn't stated in the 58d5b22bbd5 commit message,
> > > PCIE_MMCFG_SIZE_MAX must be set to 1 << 29.
> > > 
> > > I'm CCing BALATON Zoltan since he's the author of 58d5b22bbd5 and
> > > can provide context of his initial change and why the board seems to
> > > rely on it. qemu-ppc is being CCed for awareness of the sam460ex
> > > problem.
> > 
> > I'm afraid I don't remember but maybe I did not have a definitive answer
> > even back then as the docs for this PCIe controller were not available
> > so I've mostly worked from docs for similar SoCs and U-Boot and Linux
> > sources so there were a lot of guessing. Maybe it's related to that the
> > board maps peripheral addresses above 4GB as the first 4GB is reserved
> > for memory? Or maybe there's some mixup between address spaces and the
> > PCIe controller should have a separate address space that's mapped in
> > the system? I did not have any knowledge about this back then and my
> > understanding may still be lacking on how this should work.
> > 
> > > Zoltan, I wasn't able to amend to quickly amend the code in a way
> > > that I could preserve the current PCIE_MMCFG_SIZE_MAX setting and
> > > make sam460ex work again. Can you please take a look?
> > 
> > The PCIe controllers of the 460EX are implemented at the end of
> > hw/ppc/ppc440_uc.c (a lot of these 4xx SoCs are sharing components and
> > the code organisation is a bit messy). As the comment near it says it's
> > not really fully tested and working. only good enough for firmware and
> > OSes get past testing it. I think trying to attach any device to it
> > probably would fail or I would be surprised if the OS could actually
> > talk to it as there may be some missing parts. So I'm happy with any
> > solution that keeps the current state of being able to boot the OSes
> > running on it (some of which like AmigaOS and MorphOS are closed source
> > though so I don't know what their drivers need; closest open source OS
> > to them is AROS but not sure that's working on real hardware). Some
> > advice from somebody more knowledgeable about PCIe emulation in QEMU
> > would be welcome here.
> > 
> > Regards,
> > BALATON Zoltan
> > 
> > > 
> > > Thanks,
> > > 
> > > 
> > > Daniel
> > > 
> > > 
> > > 
> > > On 5/16/22 17:55, Michael S. Tsirkin wrote:
> > > > From: Francisco Iglesias <frasse.iglesias@gmail.com>
> > > > 
> > > > According to 7.2.2 in [1] bit 27 is the last bit that can be part of the
> > > > bus number, this makes the ECAM max size equal to '1 << 28'. This patch
> > > > restores back this value into the PCIE_MMCFG_SIZE_MAX define (which was
> > > > changed in commit 58d5b22bbd5 ("ppc4xx: Add device models found in PPC440
> > > > core SoCs")).
> > > > 
> > > > [1] PCI Express® Base Specification Revision 5.0 Version 1.0
> > > > 
> > > > Signed-off-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> > > > Message-Id: <20220411221836.17699-3-frasse.iglesias@gmail.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >   include/hw/pci/pcie_host.h | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/hw/pci/pcie_host.h b/include/hw/pci/pcie_host.h
> > > > index b3c8ce973c..82d92177da 100644
> > > > --- a/include/hw/pci/pcie_host.h
> > > > +++ b/include/hw/pci/pcie_host.h
> > > > @@ -65,7 +65,7 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
> > > >    * bit 12 - 14: function number
> > > >    * bit  0 - 11: offset in configuration space of a given device
> > > >    */
> > > > -#define PCIE_MMCFG_SIZE_MAX             (1ULL << 29)
> > > > +#define PCIE_MMCFG_SIZE_MAX             (1ULL << 28)
> > > >   #define PCIE_MMCFG_SIZE_MIN             (1ULL << 20)
> > > >   #define PCIE_MMCFG_BUS_BIT              20
> > > >   #define PCIE_MMCFG_BUS_MASK             0xff
> > > 
> >
BALATON Zoltan May 26, 2022, 7:34 p.m. UTC | #5 
On Thu, 26 May 2022, Michael S. Tsirkin wrote:
> On Thu, May 26, 2022 at 06:43:25PM +0200, BALATON Zoltan wrote:
>> On Thu, 26 May 2022, BALATON Zoltan wrote:
>>> Hello,
>>>
>>> On Thu, 26 May 2022, Daniel Henrique Barboza wrote:
>>>> Hi,
>>>>
>>>> This patch broke the boot of the sam460ex ppc machine:
>>>>
>>>> qemu-system-ppc -M sam460ex -kernel
>>>> ./buildroot/qemu_ppc_sam460ex-latest/vmlinux \
>>>> -device virtio-net-pci,netdev=net0 -netdev user,id=net0 -serial mon:stdio \
>>>> -nographic -snapshot
>>>> qemu-system-ppc: ../hw/pci/pcie_host.c:97: pcie_host_mmcfg_init:
>>>> Assertion `size <= PCIE_MMCFG_SIZE_MAX' failed.
>>
>> With just qemu-system-ppc -M sam460ex the assert seems to happen when the
>> guest (board firmware?) writes a value to CFGMSK reg:
>>
>> (gdb) bt
>> #0  0x00007ffff68ff4a0 in raise () at /lib64/libc.so.6
>> #1  0x00007ffff68ea536 in abort () at /lib64/libc.so.6
>> #2  0x00007ffff68ea42f in _nl_load_domain.cold () at /lib64/libc.so.6
>> #3  0x00007ffff68f7ed2 in  () at /lib64/libc.so.6
>> #4  0x000055555596646f in pcie_host_mmcfg_init (e=e@entry=0x5555567942f0, size=size@entry=0x20000000) at ../hw/pci/pcie_host.c:97
>> #5  0x000055555596653b in pcie_host_mmcfg_map (size=0x20000000, addr=0xd20000000, e=0x5555567942f0) at ../hw/pci/pcie_host.c:105
>> #6  pcie_host_mmcfg_update (e=0x5555567942f0, enable=0x1, addr=0xd20000000, size=0x20000000) at ../hw/pci/pcie_host.c:118
>> #7  0x0000555555a70d7c in ppc_dcr_write (dcr_env=0x555556669c10, dcrn=0x122, val=0xe0000001) at ../hw/ppc/ppc.c:1418
>> #8  0x0000555555abdabb in helper_store_dcr (env=0x555556633360, dcrn=0x122, val=0xe0000001) at ../target/ppc/timebase_helper.c:188
>>
>> This is done in the board firmware here:
>>
>> https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD#l963
>>
>> when trying to map config space. Here the size is calculated as 0x20000000
>> which does not fit the assert. I'm not sure what this means though and where
>> is the problem. Any ideas?
>>
>> Regards,
>> BALATON Zoltan
>
>
> It says so, does it not?
>
> 1051         switch (port) {
> 1052         case 0:
> 1053                 mtdcr(DCRN_PEGPL_CFGBAH(PCIE0), high);
> 1054                 mtdcr(DCRN_PEGPL_CFGBAL(PCIE0), low);
> 1055                 mtdcr(DCRN_PEGPL_CFGMSK(PCIE0), 0xe0000001); /* 512MB region, valid */
> 1056                 break;
> 1057         case 1:
> 1058                 mtdcr(DCRN_PEGPL_CFGBAH(PCIE1), high);
> 1059                 mtdcr(DCRN_PEGPL_CFGBAL(PCIE1), low);
> 1060                 mtdcr(DCRN_PEGPL_CFGMSK(PCIE1), 0xe0000001); /* 512MB region, valid */
> 1061                 break;
> 1062 #if CONFIG_SYS_PCIE_NR_PORTS > 2
> 1063         case 2:
> 1064                 mtdcr(DCRN_PEGPL_CFGBAH(PCIE2), high);
> 1065                 mtdcr(DCRN_PEGPL_CFGBAL(PCIE2), low);
> 1066                 mtdcr(DCRN_PEGPL_CFGMSK(PCIE2), 0xe0000001); /* 512MB region, valid */
> 1067                 break;
> 1068 #endif

Yes, the size matches what the firmware programs it.

> and basically according to spec max size is 256MB.

Maybe this SoC does not follow the spec you're referring to? Complies to 
some other spec like a newer version or has its own idea? I don't have 
docs to tell.

> we can fix the firmware of course, or we can just drop the assert,
> or force it within range in the ppc code.

According to this random Cisco IOS dump I've found:

https://www.hardware.com.br/comunidade/switch-cisco/1128380/

looks like this value is valid on that hardware so that likely means we 
should not "fix" the firmware. (Also not because it's what the real board 
uses or at least very close to it so it should work with the emulated 
board too and keeping it close to the real hardware ensures the emulation 
is accurate.) That means we should either revert this back (why was this 
changed back in the first place, did it cause any problems?) or maybe try 
restricting the value in the PPC model.

> Fixing firmware seems cleaner ... want to try?
> Any preference?

I'd leave making a patch to someone else but can help testing it. As per 
the above if we can't revert it maybe we can try restricting size in 
ppc440_uc.c where pcie_host_mmcfg_update() is called. Since the PCIe bus 
on this board probably does not work now anyway probably nothing will 
notice this for now.

Regards,
BALATON Zoltan

>
>
>>> Thanks for noticing this. I usually only test it during the freeze.
>>> Wasn't there a test patch submitted by Philippe before? Isn't that yet
>>> merged or included in CI? That should catch these before breaking it.
>>>
>>>> The reason is that it changed commit 58d5b22bbd5 ("ppc4xx: Add
>>>> device models found in PPC440 core SoCs")) in a way that it wasn't
>>>> expected by the board. The code seems to believe that, for a reason
>>>> that isn't stated in the 58d5b22bbd5 commit message,
>>>> PCIE_MMCFG_SIZE_MAX must be set to 1 << 29.
>>>>
>>>> I'm CCing BALATON Zoltan since he's the author of 58d5b22bbd5 and
>>>> can provide context of his initial change and why the board seems to
>>>> rely on it. qemu-ppc is being CCed for awareness of the sam460ex
>>>> problem.
>>>
>>> I'm afraid I don't remember but maybe I did not have a definitive answer
>>> even back then as the docs for this PCIe controller were not available
>>> so I've mostly worked from docs for similar SoCs and U-Boot and Linux
>>> sources so there were a lot of guessing. Maybe it's related to that the
>>> board maps peripheral addresses above 4GB as the first 4GB is reserved
>>> for memory? Or maybe there's some mixup between address spaces and the
>>> PCIe controller should have a separate address space that's mapped in
>>> the system? I did not have any knowledge about this back then and my
>>> understanding may still be lacking on how this should work.
>>>
>>>> Zoltan, I wasn't able to amend to quickly amend the code in a way
>>>> that I could preserve the current PCIE_MMCFG_SIZE_MAX setting and
>>>> make sam460ex work again. Can you please take a look?
>>>
>>> The PCIe controllers of the 460EX are implemented at the end of
>>> hw/ppc/ppc440_uc.c (a lot of these 4xx SoCs are sharing components and
>>> the code organisation is a bit messy). As the comment near it says it's
>>> not really fully tested and working. only good enough for firmware and
>>> OSes get past testing it. I think trying to attach any device to it
>>> probably would fail or I would be surprised if the OS could actually
>>> talk to it as there may be some missing parts. So I'm happy with any
>>> solution that keeps the current state of being able to boot the OSes
>>> running on it (some of which like AmigaOS and MorphOS are closed source
>>> though so I don't know what their drivers need; closest open source OS
>>> to them is AROS but not sure that's working on real hardware). Some
>>> advice from somebody more knowledgeable about PCIe emulation in QEMU
>>> would be welcome here.
>>>
>>> Regards,
>>> BALATON Zoltan
>>>
>>>>
>>>> Thanks,
>>>>
>>>>
>>>> Daniel
>>>>
>>>>
>>>>
>>>> On 5/16/22 17:55, Michael S. Tsirkin wrote:
>>>>> From: Francisco Iglesias <frasse.iglesias@gmail.com>
>>>>>
>>>>> According to 7.2.2 in [1] bit 27 is the last bit that can be part of the
>>>>> bus number, this makes the ECAM max size equal to '1 << 28'. This patch
>>>>> restores back this value into the PCIE_MMCFG_SIZE_MAX define (which was
>>>>> changed in commit 58d5b22bbd5 ("ppc4xx: Add device models found in PPC440
>>>>> core SoCs")).
>>>>>
>>>>> [1] PCI Express® Base Specification Revision 5.0 Version 1.0
>>>>>
>>>>> Signed-off-by: Francisco Iglesias <frasse.iglesias@gmail.com>
>>>>> Message-Id: <20220411221836.17699-3-frasse.iglesias@gmail.com>
>>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> ---
>>>>>   include/hw/pci/pcie_host.h | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/hw/pci/pcie_host.h b/include/hw/pci/pcie_host.h
>>>>> index b3c8ce973c..82d92177da 100644
>>>>> --- a/include/hw/pci/pcie_host.h
>>>>> +++ b/include/hw/pci/pcie_host.h
>>>>> @@ -65,7 +65,7 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
>>>>>    * bit 12 - 14: function number
>>>>>    * bit  0 - 11: offset in configuration space of a given device
>>>>>    */
>>>>> -#define PCIE_MMCFG_SIZE_MAX             (1ULL << 29)
>>>>> +#define PCIE_MMCFG_SIZE_MAX             (1ULL << 28)
>>>>>   #define PCIE_MMCFG_SIZE_MIN             (1ULL << 20)
>>>>>   #define PCIE_MMCFG_BUS_BIT              20
>>>>>   #define PCIE_MMCFG_BUS_MASK             0xff
>>>>
>>>
>
>
>
Michael S. Tsirkin May 26, 2022, 7:55 p.m. UTC | #6 
On Thu, May 26, 2022 at 09:34:08PM +0200, BALATON Zoltan wrote:
> On Thu, 26 May 2022, Michael S. Tsirkin wrote:
> > On Thu, May 26, 2022 at 06:43:25PM +0200, BALATON Zoltan wrote:
> > > On Thu, 26 May 2022, BALATON Zoltan wrote:
> > > > Hello,
> > > > 
> > > > On Thu, 26 May 2022, Daniel Henrique Barboza wrote:
> > > > > Hi,
> > > > > 
> > > > > This patch broke the boot of the sam460ex ppc machine:
> > > > > 
> > > > > qemu-system-ppc -M sam460ex -kernel
> > > > > ./buildroot/qemu_ppc_sam460ex-latest/vmlinux \
> > > > > -device virtio-net-pci,netdev=net0 -netdev user,id=net0 -serial mon:stdio \
> > > > > -nographic -snapshot
> > > > > qemu-system-ppc: ../hw/pci/pcie_host.c:97: pcie_host_mmcfg_init:
> > > > > Assertion `size <= PCIE_MMCFG_SIZE_MAX' failed.
> > > 
> > > With just qemu-system-ppc -M sam460ex the assert seems to happen when the
> > > guest (board firmware?) writes a value to CFGMSK reg:
> > > 
> > > (gdb) bt
> > > #0  0x00007ffff68ff4a0 in raise () at /lib64/libc.so.6
> > > #1  0x00007ffff68ea536 in abort () at /lib64/libc.so.6
> > > #2  0x00007ffff68ea42f in _nl_load_domain.cold () at /lib64/libc.so.6
> > > #3  0x00007ffff68f7ed2 in  () at /lib64/libc.so.6
> > > #4  0x000055555596646f in pcie_host_mmcfg_init (e=e@entry=0x5555567942f0, size=size@entry=0x20000000) at ../hw/pci/pcie_host.c:97
> > > #5  0x000055555596653b in pcie_host_mmcfg_map (size=0x20000000, addr=0xd20000000, e=0x5555567942f0) at ../hw/pci/pcie_host.c:105
> > > #6  pcie_host_mmcfg_update (e=0x5555567942f0, enable=0x1, addr=0xd20000000, size=0x20000000) at ../hw/pci/pcie_host.c:118
> > > #7  0x0000555555a70d7c in ppc_dcr_write (dcr_env=0x555556669c10, dcrn=0x122, val=0xe0000001) at ../hw/ppc/ppc.c:1418
> > > #8  0x0000555555abdabb in helper_store_dcr (env=0x555556633360, dcrn=0x122, val=0xe0000001) at ../target/ppc/timebase_helper.c:188
> > > 
> > > This is done in the board firmware here:
> > > 
> > > https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD#l963
> > > 
> > > when trying to map config space. Here the size is calculated as 0x20000000
> > > which does not fit the assert. I'm not sure what this means though and where
> > > is the problem. Any ideas?
> > > 
> > > Regards,
> > > BALATON Zoltan
> > 
> > 
> > It says so, does it not?
> > 
> > 1051         switch (port) {
> > 1052         case 0:
> > 1053                 mtdcr(DCRN_PEGPL_CFGBAH(PCIE0), high);
> > 1054                 mtdcr(DCRN_PEGPL_CFGBAL(PCIE0), low);
> > 1055                 mtdcr(DCRN_PEGPL_CFGMSK(PCIE0), 0xe0000001); /* 512MB region, valid */
> > 1056                 break;
> > 1057         case 1:
> > 1058                 mtdcr(DCRN_PEGPL_CFGBAH(PCIE1), high);
> > 1059                 mtdcr(DCRN_PEGPL_CFGBAL(PCIE1), low);
> > 1060                 mtdcr(DCRN_PEGPL_CFGMSK(PCIE1), 0xe0000001); /* 512MB region, valid */
> > 1061                 break;
> > 1062 #if CONFIG_SYS_PCIE_NR_PORTS > 2
> > 1063         case 2:
> > 1064                 mtdcr(DCRN_PEGPL_CFGBAH(PCIE2), high);
> > 1065                 mtdcr(DCRN_PEGPL_CFGBAL(PCIE2), low);
> > 1066                 mtdcr(DCRN_PEGPL_CFGMSK(PCIE2), 0xe0000001); /* 512MB region, valid */
> > 1067                 break;
> > 1068 #endif
> 
> Yes, the size matches what the firmware programs it.
> 
> > and basically according to spec max size is 256MB.
> 
> Maybe this SoC does not follow the spec you're referring to? Complies to
> some other spec like a newer version or has its own idea? I don't have docs
> to tell.
> 
> > we can fix the firmware of course, or we can just drop the assert,
> > or force it within range in the ppc code.
> 
> According to this random Cisco IOS dump I've found:
> 
> https://www.hardware.com.br/comunidade/switch-cisco/1128380/
> 
> looks like this value is valid on that hardware so that likely means we
> should not "fix" the firmware. (Also not because it's what the real board
> uses or at least very close to it so it should work with the emulated board
> too and keeping it close to the real hardware ensures the emulation is
> accurate.) That means we should either revert this back (why was this
> changed back in the first place, did it cause any problems?) or maybe try
> restricting the value in the PPC model.

It didn't it was just weird behaviour. High bit in the address was
ignored, so the config space was mapped many times at offsets
0,256M, etc up to whatever size was.
> > Fixing firmware seems cleaner ... want to try?
> > Any preference?
> 
> I'd leave making a patch to someone else but can help testing it. As per the
> above if we can't revert it maybe we can try restricting size in ppc440_uc.c
> where pcie_host_mmcfg_update() is called. Since the PCIe bus on this board
> probably does not work now anyway probably nothing will notice this for now.
> 
> Regards,
> BALATON Zoltan


I am guessing the unused space is just ignored.
so I am inclined to restrict in PPC model,
where we also have a chance to document what is going on.




diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index 993e3ba955..a1ecf6dd1c 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -1180,6 +1180,14 @@ static void dcr_write_pcie(void *opaque, int dcrn, uint32_t val)
     case PEGPL_CFGMSK:
         s->cfg_mask = val;
         size = ~(val & 0xfffffffe) + 1;
+        /*
+         * Firmware sets this register to E0000001. Why we are not sure,
+         * but the current guess is anything above PCIE_MMCFG_SIZE_MAX is
+         * ignored.
+         */
+        if (size > PCIE_MMCFG_SIZE_MAX) {
+            size = PCIE_MMCFG_SIZE_MAX;
+        }
         pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, size);
         break;
     case PEGPL_MSGBAH:
BALATON Zoltan May 26, 2022, 8:51 p.m. UTC | #7 
On Thu, 26 May 2022, Michael S. Tsirkin wrote:
> On Thu, May 26, 2022 at 09:34:08PM +0200, BALATON Zoltan wrote:
>> On Thu, 26 May 2022, Michael S. Tsirkin wrote:
>>> On Thu, May 26, 2022 at 06:43:25PM +0200, BALATON Zoltan wrote:
>>>> On Thu, 26 May 2022, BALATON Zoltan wrote:
>>>>> Hello,
>>>>>
>>>>> On Thu, 26 May 2022, Daniel Henrique Barboza wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This patch broke the boot of the sam460ex ppc machine:
>>>>>>
>>>>>> qemu-system-ppc -M sam460ex -kernel
>>>>>> ./buildroot/qemu_ppc_sam460ex-latest/vmlinux \
>>>>>> -device virtio-net-pci,netdev=net0 -netdev user,id=net0 -serial mon:stdio \
>>>>>> -nographic -snapshot
>>>>>> qemu-system-ppc: ../hw/pci/pcie_host.c:97: pcie_host_mmcfg_init:
>>>>>> Assertion `size <= PCIE_MMCFG_SIZE_MAX' failed.
>>>>
>>>> With just qemu-system-ppc -M sam460ex the assert seems to happen when the
>>>> guest (board firmware?) writes a value to CFGMSK reg:
>>>>
>>>> (gdb) bt
>>>> #0  0x00007ffff68ff4a0 in raise () at /lib64/libc.so.6
>>>> #1  0x00007ffff68ea536 in abort () at /lib64/libc.so.6
>>>> #2  0x00007ffff68ea42f in _nl_load_domain.cold () at /lib64/libc.so.6
>>>> #3  0x00007ffff68f7ed2 in  () at /lib64/libc.so.6
>>>> #4  0x000055555596646f in pcie_host_mmcfg_init (e=e@entry=0x5555567942f0, size=size@entry=0x20000000) at ../hw/pci/pcie_host.c:97
>>>> #5  0x000055555596653b in pcie_host_mmcfg_map (size=0x20000000, addr=0xd20000000, e=0x5555567942f0) at ../hw/pci/pcie_host.c:105
>>>> #6  pcie_host_mmcfg_update (e=0x5555567942f0, enable=0x1, addr=0xd20000000, size=0x20000000) at ../hw/pci/pcie_host.c:118
>>>> #7  0x0000555555a70d7c in ppc_dcr_write (dcr_env=0x555556669c10, dcrn=0x122, val=0xe0000001) at ../hw/ppc/ppc.c:1418
>>>> #8  0x0000555555abdabb in helper_store_dcr (env=0x555556633360, dcrn=0x122, val=0xe0000001) at ../target/ppc/timebase_helper.c:188
>>>>
>>>> This is done in the board firmware here:
>>>>
>>>> https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD#l963
>>>>
>>>> when trying to map config space. Here the size is calculated as 0x20000000
>>>> which does not fit the assert. I'm not sure what this means though and where
>>>> is the problem. Any ideas?
>>>>
>>>> Regards,
>>>> BALATON Zoltan
>>>
>>>
>>> It says so, does it not?
>>>
>>> 1051         switch (port) {
>>> 1052         case 0:
>>> 1053                 mtdcr(DCRN_PEGPL_CFGBAH(PCIE0), high);
>>> 1054                 mtdcr(DCRN_PEGPL_CFGBAL(PCIE0), low);
>>> 1055                 mtdcr(DCRN_PEGPL_CFGMSK(PCIE0), 0xe0000001); /* 512MB region, valid */
>>> 1056                 break;
>>> 1057         case 1:
>>> 1058                 mtdcr(DCRN_PEGPL_CFGBAH(PCIE1), high);
>>> 1059                 mtdcr(DCRN_PEGPL_CFGBAL(PCIE1), low);
>>> 1060                 mtdcr(DCRN_PEGPL_CFGMSK(PCIE1), 0xe0000001); /* 512MB region, valid */
>>> 1061                 break;
>>> 1062 #if CONFIG_SYS_PCIE_NR_PORTS > 2
>>> 1063         case 2:
>>> 1064                 mtdcr(DCRN_PEGPL_CFGBAH(PCIE2), high);
>>> 1065                 mtdcr(DCRN_PEGPL_CFGBAL(PCIE2), low);
>>> 1066                 mtdcr(DCRN_PEGPL_CFGMSK(PCIE2), 0xe0000001); /* 512MB region, valid */
>>> 1067                 break;
>>> 1068 #endif
>>
>> Yes, the size matches what the firmware programs it.
>>
>>> and basically according to spec max size is 256MB.
>>
>> Maybe this SoC does not follow the spec you're referring to? Complies to
>> some other spec like a newer version or has its own idea? I don't have docs
>> to tell.
>>
>>> we can fix the firmware of course, or we can just drop the assert,
>>> or force it within range in the ppc code.
>>
>> According to this random Cisco IOS dump I've found:
>>
>> https://www.hardware.com.br/comunidade/switch-cisco/1128380/
>>
>> looks like this value is valid on that hardware so that likely means we
>> should not "fix" the firmware. (Also not because it's what the real board
>> uses or at least very close to it so it should work with the emulated board
>> too and keeping it close to the real hardware ensures the emulation is
>> accurate.) That means we should either revert this back (why was this
>> changed back in the first place, did it cause any problems?) or maybe try
>> restricting the value in the PPC model.
>
> It didn't it was just weird behaviour. High bit in the address was
> ignored, so the config space was mapped many times at offsets
> 0,256M, etc up to whatever size was.
>>> Fixing firmware seems cleaner ... want to try?
>>> Any preference?
>>
>> I'd leave making a patch to someone else but can help testing it. As per the
>> above if we can't revert it maybe we can try restricting size in ppc440_uc.c
>> where pcie_host_mmcfg_update() is called. Since the PCIe bus on this board
>> probably does not work now anyway probably nothing will notice this for now.
>>
>> Regards,
>> BALATON Zoltan
>
>
> I am guessing the unused space is just ignored.
> so I am inclined to restrict in PPC model,
> where we also have a chance to document what is going on.
>

Works for me. Verified that AmigaOS and MorphOS boot with this.

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

Thanks.

>
>
> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> index 993e3ba955..a1ecf6dd1c 100644
> --- a/hw/ppc/ppc440_uc.c
> +++ b/hw/ppc/ppc440_uc.c
> @@ -1180,6 +1180,14 @@ static void dcr_write_pcie(void *opaque, int dcrn, uint32_t val)
>     case PEGPL_CFGMSK:
>         s->cfg_mask = val;
>         size = ~(val & 0xfffffffe) + 1;
> +        /*
> +         * Firmware sets this register to E0000001. Why we are not sure,
> +         * but the current guess is anything above PCIE_MMCFG_SIZE_MAX is
> +         * ignored.
> +         */
> +        if (size > PCIE_MMCFG_SIZE_MAX) {
> +            size = PCIE_MMCFG_SIZE_MAX;
> +        }
>         pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, size);
>         break;
>     case PEGPL_MSGBAH:
>
>
>
Thomas Huth May 30, 2022, 9:42 a.m. UTC | #8 
On 26/05/2022 17.54, BALATON Zoltan wrote:
> Hello,
> 
> On Thu, 26 May 2022, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> This patch broke the boot of the sam460ex ppc machine:
>>
>> qemu-system-ppc -M sam460ex -kernel 
>> ./buildroot/qemu_ppc_sam460ex-latest/vmlinux \
>> -device virtio-net-pci,netdev=net0 -netdev user,id=net0 -serial mon:stdio \
>> -nographic -snapshot
>> qemu-system-ppc: ../hw/pci/pcie_host.c:97: pcie_host_mmcfg_init: Assertion 
>> `size <= PCIE_MMCFG_SIZE_MAX' failed.
> 
> Thanks for noticing this. I usually only test it during the freeze. Wasn't 
> there a test patch submitted by Philippe before? Isn't that yet merged or 
> included in CI? That should catch these before breaking it.

Seems like there is only the (primitive) boot-serial test so far:

$ grep -r sam460ex tests/
tests/qtest/boot-serial-test.c:    { "ppc", "sam460ex", "-m 256", "DRAM:  256 MiB" },
tests/qtest/boot-serial-test.c:    { "ppc64", "sam460ex", "-device e1000", "8086  100e" },

If there is a guest kernel available for public download somewhere,
it would be great if you could add an avocado test for this machine
(see e.g. the tests/avocado/ppc_*.py files for a template).

  Thomas
Cédric Le Goater May 30, 2022, 10:09 a.m. UTC | #9 
On 5/30/22 11:42, Thomas Huth wrote:
> On 26/05/2022 17.54, BALATON Zoltan wrote:
>> Hello,
>>
>> On Thu, 26 May 2022, Daniel Henrique Barboza wrote:
>>> Hi,
>>>
>>> This patch broke the boot of the sam460ex ppc machine:
>>>
>>> qemu-system-ppc -M sam460ex -kernel ./buildroot/qemu_ppc_sam460ex-latest/vmlinux \
>>> -device virtio-net-pci,netdev=net0 -netdev user,id=net0 -serial mon:stdio \
>>> -nographic -snapshot
>>> qemu-system-ppc: ../hw/pci/pcie_host.c:97: pcie_host_mmcfg_init: Assertion `size <= PCIE_MMCFG_SIZE_MAX' failed.
>>
>> Thanks for noticing this. I usually only test it during the freeze. Wasn't there a test patch submitted by Philippe before? Isn't that yet merged or included in CI? That should catch these before breaking it.
> 
> Seems like there is only the (primitive) boot-serial test so far:
> 
> $ grep -r sam460ex tests/
> tests/qtest/boot-serial-test.c:    { "ppc", "sam460ex", "-m 256", "DRAM:  256 MiB" },
> tests/qtest/boot-serial-test.c:    { "ppc64", "sam460ex", "-device e1000", "8086  100e" },
> 
> If there is a guest kernel available for public download somewhere,
> it would be great if you could add an avocado test for this machine
> (see e.g. the tests/avocado/ppc_*.py files for a template).

We use these buildroot kernel and rootfs images :

    https://github.com/legoater/qemu-ppc-boot

The whole takes about 5m40s to run on a 8*2 Intel(R) Core(TM)
i7-10700 CPU @ 2.90GHz.

Thanks,

C.


./ppc-boot.sh -q --prefix=/home/legoater/work/qemu/qemu-ppc.git/install/
ref405ep : Linux /init login DONE (PASSED)
bamboo : Linux /init net login DONE (PASSED)
sam460ex : ASSERT (FAILED)
g3beige-604 : FW Linux Linux /init net login DONE (PASSED)
g3beige-g3 : FW Linux Linux /init net login DONE (PASSED)
mac99-g4 : FW Linux Linux /init net login DONE (PASSED)
mac99-7447 : FW Linux Linux /init net login DONE (PASSED)
mac99-7448 : FW Linux Linux /init net login DONE (PASSED)
mac99-7450 : FW Linux Linux /init net login DONE (PASSED)
mpc8544ds : Linux /init net login DONE (PASSED)
e500mc : Linux /init net login DONE (PASSED)
40p : FW login DONE (PASSED)
e5500 : Linux /init net login DONE (PASSED)
e6500 : Linux /init net login DONE (PASSED)
g5-32 : FW Linux Linux /init net login DONE (PASSED)
g5-64 : FW Linux Linux /init net login DONE (PASSED)
pseries-970 : FW Linux Linux /init net login DONE (PASSED)
pseries-970mp : FW Linux Linux /init net login DONE (PASSED)
pseries-POWER5+ : FW Linux Linux /init net login DONE (PASSED)
pseries : FW Linux Linux /init net login DONE (PASSED)
pseriesle8 : FW Linux Linux /init net login DONE (PASSED)
pseriesle9 : FW Linux Linux /init net login DONE (PASSED)
pseriesle-vof : Linux /init net login DONE (PASSED)
pseriesle10 : FW Linux Linux /init net login DONE (PASSED)
powernv8 : FW Linux /init net login DONE (PASSED)
powernv9 : FW Linux /init net login DONE (PASSED)
powernv10 : FW Linux /init net login DONE (PASSED)
Michael S. Tsirkin May 30, 2022, 4 p.m. UTC | #10 
On Mon, May 30, 2022 at 11:42:41AM +0200, Thomas Huth wrote:
> On 26/05/2022 17.54, BALATON Zoltan wrote:
> > Hello,
> > 
> > On Thu, 26 May 2022, Daniel Henrique Barboza wrote:
> > > Hi,
> > > 
> > > This patch broke the boot of the sam460ex ppc machine:
> > > 
> > > qemu-system-ppc -M sam460ex -kernel
> > > ./buildroot/qemu_ppc_sam460ex-latest/vmlinux \
> > > -device virtio-net-pci,netdev=net0 -netdev user,id=net0 -serial mon:stdio \
> > > -nographic -snapshot
> > > qemu-system-ppc: ../hw/pci/pcie_host.c:97: pcie_host_mmcfg_init:
> > > Assertion `size <= PCIE_MMCFG_SIZE_MAX' failed.
> > 
> > Thanks for noticing this. I usually only test it during the freeze.
> > Wasn't there a test patch submitted by Philippe before? Isn't that yet
> > merged or included in CI? That should catch these before breaking it.
> 
> Seems like there is only the (primitive) boot-serial test so far:
> 
> $ grep -r sam460ex tests/
> tests/qtest/boot-serial-test.c:    { "ppc", "sam460ex", "-m 256", "DRAM:  256 MiB" },
> tests/qtest/boot-serial-test.c:    { "ppc64", "sam460ex", "-device e1000", "8086  100e" },

Hmm why don't these tests catch the issue?

> If there is a guest kernel available for public download somewhere,
> it would be great if you could add an avocado test for this machine
> (see e.g. the tests/avocado/ppc_*.py files for a template).
> 
>  Thomas
diff mbox series

Patch

diff --git a/include/hw/pci/pcie_host.h b/include/hw/pci/pcie_host.h
index b3c8ce973c..82d92177da 100644
--- a/include/hw/pci/pcie_host.h
+++ b/include/hw/pci/pcie_host.h
@@ -65,7 +65,7 @@  void pcie_host_mmcfg_update(PCIExpressHost *e,
  * bit 12 - 14: function number
  * bit  0 - 11: offset in configuration space of a given device
  */
-#define PCIE_MMCFG_SIZE_MAX             (1ULL << 29)
+#define PCIE_MMCFG_SIZE_MAX             (1ULL << 28)
 #define PCIE_MMCFG_SIZE_MIN             (1ULL << 20)
 #define PCIE_MMCFG_BUS_BIT              20
 #define PCIE_MMCFG_BUS_MASK             0xff