Re: [PATCH 3/3] q800: add machine id register

2019-12-14 Thread Philippe Mathieu-Daudé

On 12/12/19 9:01 PM, Laurent Vivier wrote:

MacOS reads this address to identify the hardware.

This is a basic implementation returning the ID of Quadra 800.

Details:

   http://mess.redump.net/mess/driver_info/mac_technical_notes

"There are 3 ID schemes [...]
  The third and most scalable is a machine ID register at 0x5ffc.
  The top word must be 0xa55a to be valid. Then bits 15-11 are 0 for
  consumer Macs, 1 for portables, 2 for high-end 68k, and 3 for high-end
  PowerPC. Bit 10 is 1 if additional ID bits appear elsewhere (e.g. in VIA1).
  The rest of the bits are a per-model identifier.

  Model  Lower 16 bits of ID
...
  Quadra/Centris 610/650/800 0x2BAD"

Signed-off-by: Laurent Vivier 
---
  hw/m68k/q800.c | 22 ++
  1 file changed, 22 insertions(+)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 9ee0cb1141..c2b2aa779f 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -97,6 +97,23 @@ static void main_cpu_reset(void *opaque)
  cpu->env.pc = ldl_phys(cs->as, 4);
  }
  
+static uint64_t machine_id_read(void *opaque, hwaddr addr, unsigned size)

+{
+return 0xa55a2bad; /* Quadra 800 ID */
+}
+
+static void machine_id_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned size)
+{


Maybe worth adding:

   qemu_log_mask(LOG_GUEST_ERROR, ...);


+}
+
+static const MemoryRegionOps machine_id_ops = {
+.read = machine_id_read,
+.write = machine_id_write,
+.valid.min_access_size = 4,
+.valid.max_access_size = 4,


I think you want s/valid/impl/, because i.e. the guest could use 16-bit 
access right?



+};
+
  static void q800_init(MachineState *machine)
  {
  M68kCPU *cpu = NULL;
@@ -110,6 +127,7 @@ static void q800_init(MachineState *machine)
  MemoryRegion *rom;
  MemoryRegion *ram;
  MemoryRegion *io;
+MemoryRegion *machine_id;
  const int io_slice_nb = (IO_SIZE / IO_SLICE) - 1;
  int i;
  ram_addr_t ram_size = machine->ram_size;
@@ -159,6 +177,10 @@ static void q800_init(MachineState *machine)
  g_free(name);
  }
  
+machine_id = g_malloc(sizeof(*machine_id));


We now prefer g_new(MemoryRegion, 1).


+memory_region_init_io(machine_id, NULL, &machine_id_ops, NULL, "Machine 
ID", 4);
+memory_region_add_subregion(get_system_memory(), 0x5ffc, machine_id);


To keep the style consistent, can you use a MACHINEID_BASE definition 
instead?



+
  /* djMEMC memory and interrupt controller */
  
  djmemc_dev = qdev_create(NULL, TYPE_DJMEMC);







Re: [PATCH 3/3] q800: add machine id register

2019-12-14 Thread Mark Cave-Ayland
On 12/12/2019 20:01, Laurent Vivier wrote:

> MacOS reads this address to identify the hardware.
> 
> This is a basic implementation returning the ID of Quadra 800.
> 
> Details:
> 
>   http://mess.redump.net/mess/driver_info/mac_technical_notes
> 
> "There are 3 ID schemes [...]
>  The third and most scalable is a machine ID register at 0x5ffc.
>  The top word must be 0xa55a to be valid. Then bits 15-11 are 0 for
>  consumer Macs, 1 for portables, 2 for high-end 68k, and 3 for high-end
>  PowerPC. Bit 10 is 1 if additional ID bits appear elsewhere (e.g. in VIA1).
>  The rest of the bits are a per-model identifier.
> 
>  Model  Lower 16 bits of ID
> ...
>  Quadra/Centris 610/650/800 0x2BAD"
> 
> Signed-off-by: Laurent Vivier 
> ---
>  hw/m68k/q800.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 9ee0cb1141..c2b2aa779f 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -97,6 +97,23 @@ static void main_cpu_reset(void *opaque)
>  cpu->env.pc = ldl_phys(cs->as, 4);
>  }
>  
> +static uint64_t machine_id_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +return 0xa55a2bad; /* Quadra 800 ID */
> +}
> +
> +static void machine_id_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned size)
> +{
> +}
> +
> +static const MemoryRegionOps machine_id_ops = {
> +.read = machine_id_read,
> +.write = machine_id_write,
> +.valid.min_access_size = 4,
> +.valid.max_access_size = 4,
> +};

Should there be a .endianness here just to be really safe?

>  static void q800_init(MachineState *machine)
>  {
>  M68kCPU *cpu = NULL;
> @@ -110,6 +127,7 @@ static void q800_init(MachineState *machine)
>  MemoryRegion *rom;
>  MemoryRegion *ram;
>  MemoryRegion *io;
> +MemoryRegion *machine_id;
>  const int io_slice_nb = (IO_SIZE / IO_SLICE) - 1;
>  int i;
>  ram_addr_t ram_size = machine->ram_size;
> @@ -159,6 +177,10 @@ static void q800_init(MachineState *machine)
>  g_free(name);
>  }
>  
> +machine_id = g_malloc(sizeof(*machine_id));
> +memory_region_init_io(machine_id, NULL, &machine_id_ops, NULL, "Machine 
> ID", 4);
> +memory_region_add_subregion(get_system_memory(), 0x5ffc, machine_id);
> +
>  /* djMEMC memory and interrupt controller */
>  
>  djmemc_dev = qdev_create(NULL, TYPE_DJMEMC);

Otherwise looks good and I see it being poked by a MacOS ROM so:

Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.



[PATCH 3/3] q800: add machine id register

2019-12-12 Thread Laurent Vivier
MacOS reads this address to identify the hardware.

This is a basic implementation returning the ID of Quadra 800.

Details:

  http://mess.redump.net/mess/driver_info/mac_technical_notes

"There are 3 ID schemes [...]
 The third and most scalable is a machine ID register at 0x5ffc.
 The top word must be 0xa55a to be valid. Then bits 15-11 are 0 for
 consumer Macs, 1 for portables, 2 for high-end 68k, and 3 for high-end
 PowerPC. Bit 10 is 1 if additional ID bits appear elsewhere (e.g. in VIA1).
 The rest of the bits are a per-model identifier.

 Model  Lower 16 bits of ID
...
 Quadra/Centris 610/650/800 0x2BAD"

Signed-off-by: Laurent Vivier 
---
 hw/m68k/q800.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 9ee0cb1141..c2b2aa779f 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -97,6 +97,23 @@ static void main_cpu_reset(void *opaque)
 cpu->env.pc = ldl_phys(cs->as, 4);
 }
 
+static uint64_t machine_id_read(void *opaque, hwaddr addr, unsigned size)
+{
+return 0xa55a2bad; /* Quadra 800 ID */
+}
+
+static void machine_id_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned size)
+{
+}
+
+static const MemoryRegionOps machine_id_ops = {
+.read = machine_id_read,
+.write = machine_id_write,
+.valid.min_access_size = 4,
+.valid.max_access_size = 4,
+};
+
 static void q800_init(MachineState *machine)
 {
 M68kCPU *cpu = NULL;
@@ -110,6 +127,7 @@ static void q800_init(MachineState *machine)
 MemoryRegion *rom;
 MemoryRegion *ram;
 MemoryRegion *io;
+MemoryRegion *machine_id;
 const int io_slice_nb = (IO_SIZE / IO_SLICE) - 1;
 int i;
 ram_addr_t ram_size = machine->ram_size;
@@ -159,6 +177,10 @@ static void q800_init(MachineState *machine)
 g_free(name);
 }
 
+machine_id = g_malloc(sizeof(*machine_id));
+memory_region_init_io(machine_id, NULL, &machine_id_ops, NULL, "Machine 
ID", 4);
+memory_region_add_subregion(get_system_memory(), 0x5ffc, machine_id);
+
 /* djMEMC memory and interrupt controller */
 
 djmemc_dev = qdev_create(NULL, TYPE_DJMEMC);
-- 
2.23.0