Re: [PATCH] ppc: fix boot with sam460ex

[Michael S. Tsirkin]

On Mon, Jun 06, 2022 at 10:51:23AM -0300, Daniel Henrique Barboza wrote:
> Michael,
> 
> 
> I'll queue this patch with the commit msg proposed by Zoltan as follows:
> 
> 
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Thu May 26 18:43:43 2022 -0400
> 
>     ppc: fix boot with sam460ex
>     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.
> 
> 
> Is that ok with you?
> 
> 
> Thanks,
> 
> 
> Daniel


ACK

> 
> On 5/26/22 19: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.
> > 
> >   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: