On Mon, Apr 03, 2017 at 12:27:15PM +0200, Paolo Bonzini wrote: > > > On 03/04/2017 11:32, Alexander Graf wrote: > > > >> Newer AppleSMC revisions support an error status (read) port > >> in addition to the data and command ports currently supported. > >> > >> Register the full 32-bit region at once, and handle individual > >> ports at various offsets within the region from the top-level > >> applesmc_io_[write|read]() methods (ctual support for reading > >> the error status port to be added by a subsequent patch). > >> > >> Originally proposed by Eric Shelton <eshel...@pobox.com> > >> > >> Signed-off-by: Gabriel Somlo <gso...@gmail.com> > > > > I would prefer if we could leave the multiplexing to the layer that > > knows how to do that the best: Memory Regions. > > > > Why don't you just define a big region that ecompasses all of the IO > > space (with fallback I/O handlers for warnings) and then just sprinkle > > the ones we handle on top with higher prio? > > You don't need priority at all, "contained" regions always win over the > container (docs/memory.txt just before "Region names"). > > Both what you propose and what Gabriel did makes sense. In general QEMU > does things more like in this patch, but there are exceptions (e.g. ACPI).
So, option 1 would be: Leave the region covering the entire AppleSMC port range (APPLESMC_NUM_PORTS) as-is: static const MemoryRegionOps applesmc_io_ops = { .write = applesmc_io_write, .read = applesmc_io_read, .endianness = DEVICE_NATIVE_ENDIAN, .impl = { .min_access_size = 1, .max_access_size = 1, }, }; but modify applesmc_io_write() and applesmc_io_read() to do nothing or return 0xff, respectively. Then, add three separate regions covering 1 byte, for each of the three ports, with their own methods pointing at the existing port-specific read/write methods. Option 2 would be to leave everything as-is, per Paolo's suggestion that it's the more common way of doing things. I personally find this one easier to follow, but really don't mind doing either. If I end up going with #1, I'd probably be bringing back applesmc_data_io_ops and applesmc_cmd_io_ops, in which case I'd like to know why the access size was set to 4 bytes to begin with: - memory_region_init_io(&s->io_data, OBJECT(s), &applesmc_data_io_ops, s, - "applesmc-data", 4); - isa_register_ioport(&s->parent_obj, &s->io_data, - s->iobase + APPLESMC_DATA_PORT); - - memory_region_init_io(&s->io_cmd, OBJECT(s), &applesmc_cmd_io_ops, s, - "applesmc-cmd", 4); - isa_register_ioport(&s->parent_obj, &s->io_cmd, - s->iobase + APPLESMC_CMD_PORT); Each port is 8-bits wide only, so why 4 and not 1, above ? I guess that doesn't matter if we stick with option #2... :) Any further advice on which way to go much appreciated! Thanks, --Gabriel