Re: [PATCH v3] mac_oldworld: Add machine ID register

[BALATON Zoltan]

On Sun, 14 Jun 2020, Mark Cave-Ayland wrote:
> On 13/06/2020 18:57, BALATON Zoltan wrote:
>
> > The G3 beige machine has a machine ID register that is accessed by the
> > firmware to deternine the board config. Add basic emulation of it.
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > ---
> > v3: add empty write function in case anything tries to write reg
> >
> > hw/ppc/mac_oldworld.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> > index 3812adc441..acaf468458 100644
> > --- a/hw/ppc/mac_oldworld.c
> > +++ b/hw/ppc/mac_oldworld.c
> > @@ -80,6 +80,22 @@ static void ppc_heathrow_reset(void *opaque)
> >      cpu_reset(CPU(cpu));
> >  }
> >
> > +static uint64_t machine_id_read(void *opaque, hwaddr addr, unsigned size)
> > +{
> > +    return (addr == 0 && size == 2 ? 0x3d8c : 0);
> > +}
> > +
> > +static void machine_id_write(void *opaque, hwaddr addr,
> > +                             uint64_t val, unsigned size)
> > +{
> > +    return;
> > +}
> > +
> > +const MemoryRegionOps machine_id_reg_ops = {
> > +    .read = machine_id_read,
> > +    .write = machine_id_write,
> > +};
> > +
> >  static void ppc_heathrow_init(MachineState *machine)
> >  {
> >      ram_addr_t ram_size = machine->ram_size;
> > @@ -93,6 +109,7 @@ static void ppc_heathrow_init(MachineState *machine)
> >      char *filename;
> >      int linux_boot, i;
> >      MemoryRegion *bios = g_new(MemoryRegion, 1);
> > +    MemoryRegion *machine_id = g_new(MemoryRegion, 1);
> >      uint32_t kernel_base, initrd_base, cmdline_base = 0;
> >      int32_t kernel_size, initrd_size;
> >      PCIBus *pci_bus;
> > @@ -227,6 +244,10 @@ static void ppc_heathrow_init(MachineState *machine)
> >          }
> >      }
> >
> > +    memory_region_init_io(machine_id, OBJECT(machine), &machine_id_reg_ops,
> > +                          NULL, "machine_id", 2);
> > +    memory_region_add_subregion(get_system_memory(), 0xff000004, machine_id);
> > +
> >      /* XXX: we register only 1 output pin for heathrow PIC */
> >      pic_dev = qdev_create(NULL, TYPE_HEATHROW);
> >      qdev_init_nofail(pic_dev);
>
> I'm guessing this supersedes the version from the v2 patchset? Given that you're

Yes, I've realized that a write method is needed otherwise it will crash 
if something writes the reg. I've assumed it would check for NULL but 
seems it doesn't. I've found that out when adding similar code for a (yet 
unsubmitted) dummy screamer reg which was written so I've corrected this 
and resent only this patch. I've added the screamer reg to see if it would 
go farther but it seems it's missing i2c so cannot find RAM. I have a 
sketch to implement i2c in cuda but it does not work yet.

> introducing a HeathrowMachineState in patch 5, I'd be inclined to store the
> MemoryRegions there to prevent these references leaking after init(). A 
> constant for
> the address of the register would also be good here too.

Yes, this could be included in MachineState just I've done this before 
that and haven't thought about it. I might move it there. I'm not sure 
about the constant as this value is only used at this one place and for 
those I prefer writing the value directly as it's easier to read (no need 
to look up constant value) so I'd only use a constant for values needed 
more than once but could add one for this.

Regards,
BALATON Zoltan