Re: [Qemu-ppc] [Qemu-devel] [PATCH 13/15] ppc4xx: Add more PLB registers

[BALATON Zoltan]

On Sun, 20 Aug 2017, Philippe Mathieu-Daudé wrote:
> On 08/20/2017 02:23 PM, BALATON Zoltan wrote:
> > These registers are present in 440 SoCs (and maybe in others too) and
> > U-Boot accesses them when printing register info. We don't emulate
> > these but add them to avoid crashing when they are read or written.
>
> Your code isn't incorrect but it doesn't sound the right way to fix your 
> problem. Your firmware shouldn't *crash* on unimplemented dcr.

The firmware shouldn't crash but it's just isn't written expecting missing 
DCRs (and we aim to run the board's patched U-Boot version as it is 
because it is needed by the OSes running on the board). QEMU makes it 
crash by raising an exception for unknown DCRs, I think in 
target/ppc/timebase_helper.c:150

> Looking at ppc_dcr_read() I see that *valp isn't updated on unimp dcrn, while 
> the dcr_read_plb() callback you are using return 0 on unimp (with an "avoid 
> gcc warning" misleading comment).
>
> What is the hardware behavior on implemented dcr? return 0? In that case this 
> should be used in ppc_dcr_read(), also adding some 
> qemu_log_mask(LOG_UNIMP,...) log entry there.

Of course the right way would be to emulate what these registers do on 
hardware but I don't know what the hardware does and for a lot of DCRs the 
firmware just writes some values to initialise the hadware which is not 
needed on QEMU so we can just do read zero, ignore writes which we are 
doing here. (That's what I was trying to say in the commit message too.) 
Actually these registers aren't even used by the firmware, only included 
in register dumps so this should be safe until we find something tries to 
use it.

> Regards,
>
> Phil.
>
> > Signed-off-by: BALATON Zoltan <address@hidden>
> > ---
> >   hw/ppc/ppc405_uc.c | 12 +++++++++---
> >   1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> > index e621d0a..8e58065 100644
> > --- a/hw/ppc/ppc405_uc.c
> > +++ b/hw/ppc/ppc405_uc.c
> > @@ -105,9 +105,12 @@ ram_addr_t ppc405_set_bootinfo (CPUPPCState *env, 
> > ppc4xx_bd_info_t *bd,
> >   
> > /*****************************************************************************/
> >   /* Peripheral local bus arbitrer */
> >   enum {
> > -    PLB0_BESR = 0x084,
> > -    PLB0_BEAR = 0x086,
> > -    PLB0_ACR  = 0x087,
> > +    PLB3A0_ACR = 0x077,
> > +    PLB4A0_ACR = 0x081,
> > +    PLB0_BESR  = 0x084,
> > +    PLB0_BEAR  = 0x086,
> > +    PLB0_ACR   = 0x087,
> > +    PLB4A1_ACR = 0x089,
> >   };
> >     typedef struct ppc4xx_plb_t ppc4xx_plb_t;
> > @@ -179,9 +182,12 @@ void ppc4xx_plb_init(CPUPPCState *env)
> >       ppc4xx_plb_t *plb;
> >         plb = g_malloc0(sizeof(ppc4xx_plb_t));
> > +    ppc_dcr_register(env, PLB3A0_ACR, plb, &dcr_read_plb, &dcr_write_plb);
> > +    ppc_dcr_register(env, PLB4A0_ACR, plb, &dcr_read_plb, &dcr_write_plb);
> >       ppc_dcr_register(env, PLB0_ACR, plb, &dcr_read_plb, &dcr_write_plb);
> >       ppc_dcr_register(env, PLB0_BEAR, plb, &dcr_read_plb, &dcr_write_plb);
> >       ppc_dcr_register(env, PLB0_BESR, plb, &dcr_read_plb, &dcr_write_plb);
> > +    ppc_dcr_register(env, PLB4A1_ACR, plb, &dcr_read_plb, &dcr_write_plb);
> >       qemu_register_reset(ppc4xx_plb_reset, plb);
> >   }