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

From: BALATON Zoltan <balaton@eik.bme.hu>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Daniel Henrique Barboza <danielhb413@gmail.com>,
	qemu-devel@nongnu.org,  Peter Maydell <peter.maydell@linaro.org>,
	 Francisco Iglesias <frasse.iglesias@gmail.com>,
	 Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
	 "qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>,
	 Cedric Le Goater <clg@kaod.org>
Subject: Re: [PULL v2 75/86] include/hw/pci/pcie_host: Correct PCIE_MMCFG_SIZE_MAX
Date: Thu, 26 May 2022 22:51:45 +0200 (CEST)	[thread overview]
Message-ID: <29d5144-82e8-e715-e461-c21f7229f30@eik.bme.hu> (raw)
In-Reply-To: <20220526153640-mutt-send-email-mst@kernel.org>

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:
>
>
>