[Top][All Lists]

[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)

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!


reply via email to

[Prev in Thread] Current Thread [Next in Thread]