[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Gerd Hoffmann
Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory: Fix old portio word accesses
Date: Mon, 19 Sep 2011 15:55:18 +0200
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv: Gecko/20110904 Red Hat/3.1.14-1.el6_1 Thunderbird/3.1.14


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
  * 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.


reply via email to

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