[v1,2/3] applesmc: consolidate port i/o into single contiguous region

Submitted by Gabriel L. Somlo on March 31, 2017, 4:48 p.m.

Details

Message ID 1490978921-3782-3-git-send-email-gsomlo@gmail.com
State New
Headers show

Commit Message

Gabriel L. Somlo March 31, 2017, 4:48 p.m.
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 <eshelton@pobox.com>

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
 hw/misc/applesmc.c | 56 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 23 deletions(-)

Comments

Alexander Graf April 3, 2017, 9:32 a.m.
On 03/31/2017 06:48 PM, Gabriel L. Somlo 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 <eshelton@pobox.com>
>
> Signed-off-by: Gabriel Somlo <gsomlo@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?


Alex
Paolo Bonzini April 3, 2017, 10:27 a.m.
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 <eshelton@pobox.com>
>>
>> Signed-off-by: Gabriel Somlo <gsomlo@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).

Paolo
Gabriel L. Somlo April 4, 2017, 12:04 a.m.
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 <eshelton@pobox.com>
> >>
> >> Signed-off-by: Gabriel Somlo <gsomlo@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
Alexander Graf April 4, 2017, 9:44 a.m.
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 <eshelton@pobox.com>
>>>>
>>>> Signed-off-by: Gabriel Somlo <gsomlo@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.

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.


Alex
Gabriel L. Somlo April 4, 2017, 2:32 p.m.
On Tue, Apr 04, 2017 at 11:44:29AM +0200, Alexander Graf wrote:
> 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 <eshelton@pobox.com>
> > > > > 
> > > > > Signed-off-by: Gabriel Somlo <gsomlo@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.
> 
> 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.

At first glance, I found the idea of having a single pair of
read/write access methods for the whole 32-byte region, with
supported/unsupported ports handled internally to be aesthetically
appealing. Now I'm not so sure anymore... :)

What about if I leave everything alone, and just add a third region to
represent the error port only, with no fallback?

Also, I'll probably be able to reverse engineer this by staring at the
code long enough, but if something doesn't support e.g. a write method,
is it better to not provide a .write method to the respective region at
all, or to provide one that does nothing and returns immediately ?
(same question for the fallback/container region, if it turns out to
be reauired: provide empty read/write methods, or don't set .read and
.write at all ?)

Thanks,
--Gabriel

Patch hide | download patch | download mbox

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 986f2ac..e581e02 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -75,8 +75,7 @@  typedef struct AppleSMCState AppleSMCState;
 struct AppleSMCState {
     ISADevice parent_obj;
 
-    MemoryRegion io_data;
-    MemoryRegion io_cmd;
+    MemoryRegion io_reg;
     uint32_t iobase;
     uint8_t cmd;
     uint8_t status;
@@ -207,19 +206,36 @@  static void qdev_applesmc_isa_reset(DeviceState *dev)
     applesmc_add_key(s, "MSSD", 1, "\0x3");
 }
 
-static const MemoryRegionOps applesmc_data_io_ops = {
-    .write = applesmc_io_data_write,
-    .read = applesmc_io_data_read,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-    .impl = {
-        .min_access_size = 1,
-        .max_access_size = 1,
-    },
-};
+static void applesmc_io_write(void *opaque, hwaddr addr, uint64_t val,
+                              unsigned size)
+{
+    switch (addr) {
+    case APPLESMC_DATA_PORT:
+        applesmc_io_data_write(opaque, addr, val, size);
+        break;
+    case APPLESMC_CMD_PORT:
+        applesmc_io_cmd_write(opaque, addr, val, size);
+        break;
+    default:
+        break;
+    }
+}
 
-static const MemoryRegionOps applesmc_cmd_io_ops = {
-    .write = applesmc_io_cmd_write,
-    .read = applesmc_io_cmd_read,
+static uint64_t applesmc_io_read(void *opaque, hwaddr addr, unsigned size)
+{
+    switch (addr) {
+    case APPLESMC_DATA_PORT:
+        return applesmc_io_data_read(opaque, addr, size);
+    case APPLESMC_CMD_PORT:
+        return applesmc_io_cmd_read(opaque, addr, size);
+    default:
+        return 0xff;
+    }
+}
+
+static const MemoryRegionOps applesmc_io_ops = {
+    .write = applesmc_io_write,
+    .read = applesmc_io_read,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .impl = {
         .min_access_size = 1,
@@ -231,15 +247,9 @@  static void applesmc_isa_realize(DeviceState *dev, Error **errp)
 {
     AppleSMCState *s = APPLE_SMC(dev);
 
-    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);
+    memory_region_init_io(&s->io_reg, OBJECT(s), &applesmc_io_ops, s,
+                          "applesmc", APPLESMC_NUM_PORTS);
+    isa_register_ioport(&s->parent_obj, &s->io_reg, s->iobase);
 
     if (!s->osk || (strlen(s->osk) != 64)) {
         fprintf(stderr, "WARNING: Using AppleSMC with invalid key\n");