Re: [PATCH] ppc: fix boot with sam460ex

[Cédric Le Goater]

[ resend with a reply-all ]

On 5/27/22 00:43, Michael S. Tsirkin wrote:
> Recent changes to pcie_host corrected size of its internal region to
> match what it expects - only the low 28 bits are ever decoded. Previous
> code just ignored bit 29 (if size was 1 << 29) in the address which does
> not make much sense.  We are now asserting on size > 1 << 28 instead,
> but it so happened that ppc actually allows guest to configure as large
> a size as it wants to, and current firmware set it to 1 << 29.
>
> With just qemu-system-ppc -M sam460ex this triggers an assert which
> seems to happen when the guest (board firmware?) writes a value to
> CFGMSK reg:
>
> (gdb) bt
>
> 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
>
> when trying to map config space.
>
> Note that what firmware does matches
> https://www.hardware.com.br/comunidade/switch-cisco/1128380/
>
> So it's not clear what the proper fix should be.
>
> However, allowing guest to trigger an assert in qemu is not good practice 
> anyway.
>
> For now let's just force the mask to 256MB on guest write, this way
> anything outside the expected address range is ignored.
>
> Fixes: commit 1f1a7b2269 ("include/hw/pci/pcie_host: Correct 
> PCIE_MMCFG_SIZE_MAX")
> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Affected system is orphan so I guess I will merge the patch unless
> someone objects.

Fine with me.

Acked-by: Cédric Le Goater <clg@kaod.org>


On the orphan status,

MAINTAINERS file says the sam460ex machine is maintained. I understand that
these files :

  hw/ppc/ppc440_uc.c
  hw/ppc/ppc440.h
  hw/ppc/ppc440_pcix.c
  hw/ppc/ppc4xx_devs.c
  include/hw/ppc/ppc4xx.h

and these

  include/hw/i2c/ppc4xx_i2c.h
  hw/i2c/ppc4xx_i2c.c
  hw/intc/ppc-uic.c
  include/hw/intc/ppc-uic.h

should be under the same entry since sam460ex depends on it.

The ppc440 support is a bit of a mess to be honest. We have two 440
machines bamboo and sam460ex which have a lot in common a part from
the PCI host bridge.

Thanks,

C.

>   hw/ppc/ppc440_uc.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> 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: