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 <address@hidden>
Signed-off-by: Gabriel Somlo <address@hidden>
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 ?