Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses

[Gerd Hoffmann <kraxel@redhat.com>]

   Hi,

>> The trick of having a way to register N callbacks with one shot is worth
>> growing.  Ideally each register in a BAR would have a callback and we'd
>> do something like
>>
>>      MemoryRegionOps mydev_ops = {
>>          .registers = {
>>               { MYDEV_REG_x, 4, 4, mydev_reg_x_read, mydev_reg_x_write, },
>>               ...
>>           },
>>      }
>>
>> with hints to the core like "this register sits at this offset, use it
>> for reads instead of a callback", or, "this is a read-only register".
>
> This has pros and cons. If you have n registers to dispatch, you then
> have to write n function prologues and maybe epilogues instead of just
> one. Specifically if the register access is trivial, that could case
> quite some LoC blowup on the device side.

If the register access is trivial then you don't need to call into the 
driver at all ...

You can have a look at hw/intel-hda.c which actually implements 
something like this, with some commonly needed features:

   * The "offset" field already mentioned by avi is there, so trivial
     reads/writes can be handled by the core.
   * A "wmask" field to specify which bits are guest writable.
   * A "wclear" field to specify which bits have write-one-to-clear
     semantics.
   * A "reset" field which specified the value this field has after
     device reset.  Also serves as value for read-only registers.
   * read/write handlers of course.  The write handler is called after
     the core applied sanity checks and calculated the new register
     value (using wmask+wclear).
   * A "name" field (for debug logging).

It's pretty nice, alot more readable that a big switch, forces you to 
think which bits the guest can set (not specifying a wmask gives you a 
read-only register ;).

Also no bloat.  With this moving to memory core the all the handlers 
will gain a line with a container_of(), but that isn't too bad too IMHO.

cheers,
   Gerd