Re: [PATCH] ppc: fix boot with sam460ex

[BALATON Zoltan]

On Fri, 27 May 2022, Michael S. Tsirkin wrote:
> On Fri, May 27, 2022 at 12:46:57PM +0200, BALATON Zoltan wrote:
> > Hello,
> >
> > Some changes to commit message (patch is OK).
>
> Want to write the commit message for me then?

How about:

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 PPC 4xx 
actually allows guest to configure different sizes, and some firmwares 
seem to set it to 1 << 29.

This caused e.g. qemu-system-ppc -M sam460ex to exit with an assert when 
the guest writes a value to CFGMSK register when trying to map config 
space. This is done in the board firmware in ppc4xx_init_pcie_port() in 
roms/u-boot-sam460ex/arch/powerpc/cpu/ppc4xx/4xx_pcie.c

It's not clear what the proper fix should be but for now let's force the 
size to 256MB, so 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.
> > >
> > > 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: