[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: Alexander Graf
Subject: Re: [Qemu-devel] [PATCH v1 2/3] applesmc: consolidate port i/o into single contiguous region
Date: Tue, 4 Apr 2017 11:44:29 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0

On 04/04/2017 02:04 AM, Gabriel L. Somlo wrote:
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.

Basically, yes. I think it would reduce the code churn quite a bit, as 99% of it stays the same. You only add the fall-through layer.

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 don't fully remember, but I suppose Mac OS X or Linux were accessing it using 32bit read/write operations, so we had to encompass the full size. I don't think you need to do that if you have the fall-through.

I guess that doesn't matter if we stick with option #2... :)

Any further advice on which way to go much appreciated!

I was mostly curious on why you would change so much code without any obvious gain. I'm perfectly fine with moving to a switch based approach for the memory region, but I didn't really understand *why* it was done in the first place :). It just felt like a lot of patch for no reason.


reply via email to

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