Re: [Qemu-devel] [PATCH 21/24] isa: add isa_address_space()

[Avi Kivity <avi@redhat.com>]

On 08/10/2011 07:26 PM, Richard Henderson wrote:
> On 08/10/2011 09:24 AM, Richard Henderson wrote:
> >  Of course, as far as I can see, this variable is only used by
> >  the VGA devices.  Surely we can arrange to pass down some address
> >  space during setup of the VGA?
>
> ... Which seems to be what you've done in patch 23.
>
> So what's the point of this patch?
>
>

Some more about this:

We have a few dual (or even tripe) interface devices, of which vga is an 
example.  This requires a common interface when we add a memory region 
to a device's bus.  Let's enumerate the options:

1. just ignore the parent bus

this is cpu_register_physical_memory()

2. pass the right MemoryRegion to the device

this series

3. Integrate qdev and the memory API

introduce qdev_add_memory(), have vga.c call that.

4. Have the specialized device constructor (isa-vga.c's vga_initfn() 
pass down a callback to the generaized device (vga_init_vbe()) that 
wraps the bus-specific memory registration function:

     static void isa_vga_add_region(void *opaque, ...)
     {
          ISAVGAState *d = opaque;

          isa_add_region(&d->dev, ...);
     }

     vga_initfn()
     {
         ...
         vga_init_vbe(s, isa_vga_add_region, d);
         ...
     }

1 and 2 are layering violations in that they don't allow bookkeeping by 
the specific bus.  3 allows it to some extent but I still feel it is 
wrong.  4 is correct in this sense but takes some work.

It may be that no bookkeeping is actually required and we can do 3, but 
that will have to wait until qdev/memory integration.  If not we'll 
switch to 4.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.