| Message ID | 1490978921-3782-3-git-send-email-gsomlo@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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");
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(-)