Re: [coreboot] PC Engines apu2 boot regression

[Piotr Król]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 05/05/2018 11:53 AM, Kyösti Mälkki wrote:
> On Sat, May 5, 2018 at 3:06 AM, Marshall Dawson 
>  wrote:
> > > My current guess is AP CPUs do not see initialised memory for 
> > > _car_region_start .. _end. That region is set up using fixed
> > > MTRRs in low memory and probably not synced between cores so
> > > early in romstage.
> >
> > Kyosti, I haven't kept a close watch on CAR changes in the other
> > AMD systems, however in experimenting with CZ using ST source I
> > found that 4 cores had a problem using CAR globals, so I think
> > you're on the right track. AMD's CAR setup code assumes each core
> > only needs fixed MTRRs configured for their own particular region
> > of storage -- they all don't get access to 100% of the CAR area.
> > Going from 2 cores (ST) to 4 cores (CZ), the higher two cores
> > could no longer access the CAR globals at the bottom of the
> > region. Try this hack and see if it improves for you:
>
> Hi Marshall
>
> Thanks, your suggested MTRR change seems to solve the regression.
> It was definetely about CAR_GLOBALs failing for AP CPUs in general.
> We just had not hit this case before for typical builds, as use of 
> POSTCAR_STAGE barely avoided the error from triggering.
>
> So did you hit problems with CAR_GLOBAL in agesa_get_dispatcher(), 
> when experimeting CZ vs ST? I don't see why this code works even
> for dual-core ST but I did not evaluate CAR layout. Documentation
> part for fixed MTRRs in gcccar.inc appears to be same.
>
> Now, does this MTRR change have potential to actually become _the_ 
> fix? I don't see why it could not. It's pretty much all that we can
> do with binaryPI, anything else touches the blobs.

Hi Marshall, Kyösti,
also confirm that this patch fix regression on recent master. Will you
sent patch?

Best Regards,
- -- 
Piotr Król
Embedded Systems Consultant
https://3mdeb.com | @3mdeb_com
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE4DCbLYWmfoRjKeNLsu5x6WeqnkwFAlrthdYACgkQsu5x6Weq
nkwqPQ//QGpX6w23CWwN/lJQPMIB2Qpw0rdL5GTa2L1axsdL8GLWr+TgRkeleGi4
q77x+jG9xPP5fBkoEiZfBOkZTwhfKIKygtSdwHybs0Qn7aammItCgdOyz1tJj9Gu
/i6ncXK3dsWYwOj1r+qkMm0phGQDgq+uW17CnsmOUfXMIMm6ULIF6f/8t/aNvdGC
N6gNPIjI7oDlkI73CRZjLV+bWIsFzfvoJWTZrilNz41zxI5ERCKpmGCNy93qPsMl
p5GoGZvzE2rwniRqju8oDlw2P+o3OZ3ukAJWbQxid83dn7NHAo95ZFZ8cx7umLvN
J+vFrD9n1CzwQRXtqWoSelYmGJkJWxx/p5Co8JK+x2yLDShrojJraV02NX32k4Yd
Bf4RZW9nK4QnPqrH647FfBEOBZfX0ruizjTQzFLd7moTYDry8zIOwOuLc3IftT09
edPrxnwlp6q6F8EopGXsnTbqFfbxGRRVzGICvd2/eFtSXZBULFQBfutKYrK8KVaS
/VLXSpPQ6f07vZnWQMrCndaHIC0cShnHE1ekb2uY8kmkqzgXZetaKGsDkCXlJVRF
vlJEoEu9qWmglqMYzxPF8MnHBx9DLAgFk/AMMmQk5yzv8Ki1m3kgJu9Fxdc0iJ3y
Ka9voYxzP2r+K8CaJgvtXYwSeHoIqDdERBAz0lN3KTqGuXtewSs=
=0Gj7
-----END PGP SIGNATURE-----

PC Engines apu2 boot regression

[Marshall Dawson]

> So did you hit problems with CAR_GLOBAL in agesa_get_dispatcher(),
> when experimeting CZ vs ST? I don't see why this code works even for
> dual-core ST but I did not evaluate CAR layout. Documentation part for
> fixed MTRRs in gcccar.inc appears to be same.

I don't remember specifically where the condition caused a problem.  It
kind of seems like it was when the third core tried to run printk, wanted a
global pointer to a global variable, and got back 0xffffffff.  It's been
too long to say for certain.  IIRC, cores 0 & 1 received fixed MTRR
settings allowing 0x30000-0x3ffff, but 2 & 3 started at 0x40000 and
couldn't access the globals just above 0x30000.  It's worth noting that
cores in Family 15h share their MTRRs within a compute unit, so it was
never possible for 0 & 1 to be anything but identical.

Now, does this MTRR change have potential to actually become _the_
> fix? I don't see why it could not. It's pretty much all that we can do
> with binaryPI, anything else touches the blobs.

Will you sent patch?
>

I don't think _the_ fix has been pursued yet.  It's certainly lower
priority than other things I have going on right now.  But in my mind, it's
still a little too hacky.  It assumes the hardcoded value of
BSP_STACK_BASE_ADDR stays consistent (which needs to also equal
DCACHE_RAM_BASE).  Maybe a better approach is to allow all cores to have
access to 100% of the CAR region.  Maybe just their own plus the globals
area?  I haven't given much thought to why cores received differing regions
in AMD's CAR setup code.  What I would really enjoy seeing is moving CAR
above 1MB and skipping all the fixed MTRR setup, but that's a pretty big
rewrite.

Kyosti, do you want to submit a patch since you're able to test it on other
platforms?  I'd be OK with having the 64K region at BSP_STACK_BASE_ADDR
being WB for all cores until a more robust solution is available.  I don't
think this is a characteristic of PI blob implementations, by the way.  I
would expect all the native AGESA platforms to have the same limitation too.

Thanks,
Marshall

On Sat, May 5, 2018 at 4:22 AM, Piotr Król  wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> On 05/05/2018 11:53 AM, Kyösti Mälkki wrote:
> > On Sat, May 5, 2018 at 3:06 AM, Marshall Dawson
> >  wrote:
> > > > My current guess is AP CPUs do not see initialised memory for
> > > > _car_region_start .. _end. That region is set up using fixed
> > > > MTRRs in low memory and probably not synced between cores so
> > > > early in romstage.
> > >
> > > Kyosti, I haven't kept a close watch on CAR changes in the other
> > > AMD systems, however in experimenting with CZ using ST source I
> > > found that 4 cores had a problem using CAR globals, so I think
> > > you're on the right track. AMD's CAR setup code assumes each core
> > > only needs fixed MTRRs configured for their own particular region
> > > of storage -- they all don't get access to 100% of the CAR area.
> > > Going from 2 cores (ST) to 4 cores (CZ), the higher two cores
> > > could no longer access the CAR globals at the bottom of the
> > > region. Try this hack and see if it improves for you:
> >
> > Hi Marshall
> >
> > Thanks, your suggested MTRR change seems to solve the regression.
> > It was definetely about CAR_GLOBALs failing for AP CPUs in general.
> > We just had not hit this case before for typical builds, as use of
> > POSTCAR_STAGE barely avoided the error from triggering.
> >
> > So did you hit problems with CAR_GLOBAL in agesa_get_dispatcher(),
> > when experimeting CZ vs ST? I don't see why this code works even
> > for dual-core ST but I did not evaluate CAR layout. Documentation
> > part for fixed MTRRs in gcccar.inc appears to be same.
> >
> > Now, does this MTRR change have potential to actually become _the_
> > fix? I don't see why it could not. It's pretty much all that we can
> > do with binaryPI, anything else touches the blobs.
>
> Hi Marshall, Kyösti,
> also confirm that this patch fix regression on recent master. Will you
> sent patch?
>
> Best Regards,
>
> Piotr Król
> Embedded Systems Consultant
> https://3mdeb.com | @3mdeb_com
> -----BEGIN PGP SIGNATURE-----
>
> iQIzBAEBCAAdFiEE4DCbLYWmfoRjKeNLsu5x6WeqnkwFAlrthdYACgkQsu5x6Weq
> nkwqPQ//QGpX6w23CWwN/lJQPMIB2Qpw0rdL5GTa2L1axsdL8GLWr+TgRkeleGi4
> q77x+jG9xPP5fBkoEiZfBOkZTwhfKIKygtSdwHybs0Qn7aammItCgdOyz1tJj9Gu
> /i6ncXK3dsWYwOj1r+qkMm0phGQDgq+uW17CnsmOUfXMIMm6ULIF6f/8t/aNvdGC
> N6gNPIjI7oDlkI73CRZjLV+bWIsFzfvoJWTZrilNz41zxI5ERCKpmGCNy93qPsMl
> p5GoGZvzE2rwniRqju8oDlw2P+o3OZ3ukAJWbQxid83dn7NHAo95ZFZ8cx7umLvN
> J+vFrD9n1CzwQRXtqWoSelYmGJkJWxx/p5Co8JK+x2yLDShrojJraV02NX32k4Yd
> Bf4RZW9nK4QnPqrH647FfBEOBZfX0ruizjTQzFLd7moTYDry8zIOwOuLc3IftT09
> edPrxnwlp6q6F8EopGXsnTbqFfbxGRRVzGICvd2/eFtSXZBULFQBfutKYrK8KVaS
> /VLXSpPQ6f07vZnWQMrCndaHIC0cShnHE1ekb2uY8kmkqzgXZetaKGsDkCXlJVRF
> vlJEoEu9qWmglqMYzxPF8MnHBx9DLAgFk/AMMmQk5yzv8Ki1m3kgJu9Fxdc0iJ3y
> Ka9voYxzP2r+K8CaJgvtXYwSeHoIqDdERBAz0lN3KTqGuXtewSs=
> =0Gj7
> -----END PGP SIGNATURE-----