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

[Gerd Hoffmann]

  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