[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 2/3] applesmc: consolidate port i/o into sing
From: |
Gabriel L. Somlo |
Subject: |
Re: [Qemu-devel] [PATCH v1 2/3] applesmc: consolidate port i/o into single contiguous region |
Date: |
Mon, 3 Apr 2017 20:04:35 -0400 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
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 ?
I guess that doesn't matter if we stick with option #2... :)
Any further advice on which way to go much appreciated!
Thanks,
--Gabriel