| Message ID | 1370348041-6768-7-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 4 June 2013 13:13, Paolo Bonzini <pbonzini@redhat.com> wrote: > This matches sysbus_pass_irq in cases where a device is a thin wrapper > of another. MMIO regions will keep the subdevice as the owner. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/core/sysbus.c | 12 ++++++++++++ > hw/cpu/arm11mpcore.c | 2 +- > include/hw/sysbus.h | 1 + > 3 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c > index 9004d8c..6dbd1f8 100644 > --- a/hw/core/sysbus.c > +++ b/hw/core/sysbus.c > @@ -117,6 +117,18 @@ void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory) > dev->mmio[n].memory = memory; > } > > +/* Pass MMIOs from a target device. */ > +void sysbus_pass_mmio(SysBusDevice *dev, SysBusDevice *target) > +{ > + int i; > + assert(dev->num_mmio == 0); > + dev->num_mmio = target->num_mmio; > + for (i = 0; i < dev->num_mmio; i++) { > + assert(target->mmio[i].addr == -1); > + dev->mmio[i] = target->mmio[i]; > + } > +} This is much less flexible than just using sysbus_mmio_get_region(), because it only lets you pass the whole set of MMIOs from the other device through, not just the ones you want. Please just make reference counting work properly with passing MemoryRegion*s around. -- PMM
Il 04/06/2013 14:24, Peter Maydell ha scritto: > On 4 June 2013 13:13, Paolo Bonzini <pbonzini@redhat.com> wrote: >> This matches sysbus_pass_irq in cases where a device is a thin wrapper >> of another. MMIO regions will keep the subdevice as the owner. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> hw/core/sysbus.c | 12 ++++++++++++ >> hw/cpu/arm11mpcore.c | 2 +- >> include/hw/sysbus.h | 1 + >> 3 files changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c >> index 9004d8c..6dbd1f8 100644 >> --- a/hw/core/sysbus.c >> +++ b/hw/core/sysbus.c >> @@ -117,6 +117,18 @@ void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory) >> dev->mmio[n].memory = memory; >> } >> >> +/* Pass MMIOs from a target device. */ >> +void sysbus_pass_mmio(SysBusDevice *dev, SysBusDevice *target) >> +{ >> + int i; >> + assert(dev->num_mmio == 0); >> + dev->num_mmio = target->num_mmio; >> + for (i = 0; i < dev->num_mmio; i++) { >> + assert(target->mmio[i].addr == -1); >> + dev->mmio[i] = target->mmio[i]; >> + } >> +} > > This is much less flexible than just using sysbus_mmio_get_region(), > because it only lets you pass the whole set of MMIOs from the > other device through, not just the ones you want. How is this different from sysbus_pass_irq? > Please just make reference counting work properly with passing > MemoryRegion*s around. Do you have any idea that doesn't require touch 800 invocation of the region creation functions? This looks like a solution in search of a problem to me. Paolo
On 4 June 2013 13:31, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 04/06/2013 14:24, Peter Maydell ha scritto: >> On 4 June 2013 13:13, Paolo Bonzini <pbonzini@redhat.com> wrote: >> This is much less flexible than just using sysbus_mmio_get_region(), >> because it only lets you pass the whole set of MMIOs from the >> other device through, not just the ones you want. > > How is this different from sysbus_pass_irq? sysbus_pass_irq is also an annoyingly inflexible function. With MMIOs we have the advantage of being able to do better. >> Please just make reference counting work properly with passing >> MemoryRegion*s around. > > Do you have any idea that doesn't require touch 800 invocation of the > region creation functions? I think that would be a straightforward and easy to understand way to define the ownership rules so I would much rather we did that. I really don't like the way your current patch is doing something complicated in an attempt to avoid this. thanks -- PMM
Il 04/06/2013 14:36, Peter Maydell ha scritto: > On 4 June 2013 13:31, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 04/06/2013 14:24, Peter Maydell ha scritto: >>> On 4 June 2013 13:13, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> This is much less flexible than just using sysbus_mmio_get_region(), >>> because it only lets you pass the whole set of MMIOs from the >>> other device through, not just the ones you want. >> >> How is this different from sysbus_pass_irq? > > sysbus_pass_irq is also an annoyingly inflexible function. > With MMIOs we have the advantage of being able to do better. I prefer consistency to useless flexibility. The day someone will need it, they can add sysbus_pass_one_{irq,mmio}. >>> Please just make reference counting work properly with passing >>> MemoryRegion*s around. >> >> Do you have any idea that doesn't require touch 800 invocation of the >> region creation functions? > > I think that would be a straightforward and easy to understand > way to define the ownership rules so I would much rather we > did that. I really don't like the way your current patch > is doing something complicated in an attempt to avoid this. They are straightforward, documented, and the wide majority of the devices need not care at all about them. By contrast, changing 800 invocations of the functions would be impossible to review seriously, it would have to be redone when boards are qdev/QOM-ified, would be worse for submitters of new boards. There are an order of magnitude less calls to memory_region_set_owner than to memory_region_init_*. Changing four places suffices to get ownership for 97% of the devices (309 files in hw/ call memory_region_init*, 9 devices call memory_region_set_owner): hw/core/sysbus.c:1 hw/isa/isa-bus.c:1 hw/pci/pci.c:1 ioport.c:2 Of the remaining calls, 2/3 of them are concentrated in a handful of devices: hw/display/cirrus_vga.c:7 hw/display/vga.c:4 hw/i386/kvm/pci-assign.c:7 hw/misc/vfio.c:8 and all the others could probably be refactored away, and are all for PC devices, other targets are unaffected (I did review them and sysbus catches everything): hw/acpi/ich9.c:1 hw/acpi/piix4.c:4 hw/char/serial-pci.c:1 hw/isa/apm.c:1 hw/misc/pc-testdev.c:4 To me, it seems a pretty good abstraction. Paolo
On 4 June 2013 14:24, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 04/06/2013 14:36, Peter Maydell ha scritto: >> On 4 June 2013 13:31, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> Il 04/06/2013 14:24, Peter Maydell ha scritto: >>>> On 4 June 2013 13:13, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> This is much less flexible than just using sysbus_mmio_get_region(), >>>> because it only lets you pass the whole set of MMIOs from the >>>> other device through, not just the ones you want. >>> >>> How is this different from sysbus_pass_irq? >> >> sysbus_pass_irq is also an annoyingly inflexible function. >> With MMIOs we have the advantage of being able to do better. > > I prefer consistency to useless flexibility. > > The day someone will need it, they can add sysbus_pass_one_{irq,mmio}. We've already got a working implementation, in the shape of sysbus_mmio_get_region(). This is exactly the right way to do this API -- we have one API which says "give me a MemoryRegion*" and one which says "I have a MemoryRegion*, please expose it". All I'm asking you to do is not break it. >> I think that would be a straightforward and easy to understand >> way to define the ownership rules so I would much rather we >> did that. I really don't like the way your current patch >> is doing something complicated in an attempt to avoid this. > > They are straightforward, documented, and the wide majority of the > devices need not care at all about them. I still don't understand them. Why should "hey, please use this MMIO region as a PCI BAR" imply "and by the way set the ownership"? Why does "here's an MMIO region which should be exposed to users of this device" imply "and by the way set the ownership"? This is just yoking together two separate things. And it seems wrong that devices shouldn't care about memory region ownership -- devices *are* the memory region owners typically. Memory regions should just have owners always from the start, if we need them to have owners. > By contrast, changing 800 > invocations of the functions would be impossible to review seriously, it > would have to be redone when boards are qdev/QOM-ified, would be worse > for submitters of new boards. If it's not clear who ought to be the owner when mmio_init_region is called then there are problems anyway. > There are an order of magnitude less calls to memory_region_set_owner > than to memory_region_init_*. That's because you've optimised for "minimise number of places to put calls". The downside is it's much harder to review new patches. An owner parameter to the mmio_init_region* functions means (a) it's impossible to forget to set the owner (b) it's easy to check when looking at the patch whether the owner is appropriate (c) you don't have to worry about weird cases where something else might try to set the owner later. As a concrete example, if somebody submitted cirrus_vga as a new driver, I have no idea how to tell that it needs to set the owner for its memory regions, when 99% of other devices don't. I think this is going to result in "forgot to set owner" bugs. thanks -- PMM
Il 04/06/2013 16:11, Peter Maydell ha scritto: > We've already got a working implementation, in the shape > of sysbus_mmio_get_region(). This is exactly the right way > to do this API -- we have one API which says "give me a > MemoryRegion*" and one which says "I have a MemoryRegion*, > please expose it". All I'm asking you to do is not break it. I can add a conditional to sysbus_add_mmio if you prefer. I think it's uglier, but I can live with it. > I still don't understand them. Why should "hey, please use > this MMIO region as a PCI BAR" imply "and by the way set the > ownership"? Why does "here's an MMIO region which should be > exposed to users of this device" imply "and by the way > set the ownership"? Because both places are _already_ tying a MemoryRegion to a device. >> By contrast, changing 800 >> invocations of the functions would be impossible to review seriously, it >> would have to be redone when boards are qdev/QOM-ified, would be worse >> for submitters of new boards. > > If it's not clear who ought to be the owner when mmio_init_region > is called then there are problems anyway. It is clear, but this doesn't make a mechanical-but-not-quite patch easy to review. It's not that I cannot do it. I simply believe it is a worse choice. >> There are an order of magnitude less calls to memory_region_set_owner >> than to memory_region_init_*. > > That's because you've optimised for "minimise number of places > to put calls". The downside is it's much harder to review new > patches. An owner parameter to the mmio_init_region* functions > means (a) it's impossible to forget to set the owner (b) it's > easy to check when looking at the patch whether the owner is > appropriate (c) you don't have to worry about weird cases > where something else might try to set the owner later. That's true, I cannot deny that. > As a concrete example, if somebody submitted cirrus_vga > as a new driver, I have no idea how to tell that it needs > to set the owner for its memory regions, when 99% of > other devices don't. I think this is going to result in > "forgot to set owner" bugs. Because cirrus is adding regions directly to address_space_memory/io. As documented: * The device must set the owner itself * only if it uses memory_region_add_subregion directly on some address * space, or after the parent region is passed to the bus (for example * dynamically while the device runs). Paolo
On 4 June 2013 15:27, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 04/06/2013 16:11, Peter Maydell ha scritto: >> We've already got a working implementation, in the shape >> of sysbus_mmio_get_region(). This is exactly the right way >> to do this API -- we have one API which says "give me a >> MemoryRegion*" and one which says "I have a MemoryRegion*, >> please expose it". All I'm asking you to do is not break it. > > I can add a conditional to sysbus_add_mmio if you prefer. I think it's > uglier, but I can live with it. No, I just think it shouldn't be setting the owner. >> As a concrete example, if somebody submitted cirrus_vga >> as a new driver, I have no idea how to tell that it needs >> to set the owner for its memory regions, when 99% of >> other devices don't. I think this is going to result in >> "forgot to set owner" bugs. > > Because cirrus is adding regions directly to address_space_memory/io. > As documented: > > * The device must set the owner itself > * only if it uses memory_region_add_subregion directly on some address > * space, or after the parent region is passed to the bus (for example > * dynamically while the device runs). OK, so why doesn't your patchset make the places in hw/arm/omap1.c which add memory regions directly to a subregion set the owner of the region? (or any of the many other places where we do similar things). thanks -- PMM
Il 04/06/2013 16:56, Peter Maydell ha scritto: >>> As a concrete example, if somebody submitted cirrus_vga >>> as a new driver, I have no idea how to tell that it needs >>> to set the owner for its memory regions, when 99% of >>> other devices don't. I think this is going to result in >>> "forgot to set owner" bugs. >> >> Because cirrus is adding regions directly to address_space_memory/io. >> As documented: >> >> * The device must set the owner itself >> * only if it uses memory_region_add_subregion directly on some address >> * space, or after the parent region is passed to the bus (for example >> * dynamically while the device runs). > > OK, so why doesn't your patchset make the places in > hw/arm/omap1.c which add memory regions directly > to a subregion set the owner of the region? Because these aren't qdevified. > (or any of the many other places where we do similar things). Note that it's only necessary to do so when you add those to the address space, not to other regions. Paolo
Il 04/06/2013 15:24, Paolo Bonzini ha scritto: > Il 04/06/2013 14:36, Peter Maydell ha scritto: >> On 4 June 2013 13:31, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> Il 04/06/2013 14:24, Peter Maydell ha scritto: >>>> On 4 June 2013 13:13, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> This is much less flexible than just using sysbus_mmio_get_region(), >>>> because it only lets you pass the whole set of MMIOs from the >>>> other device through, not just the ones you want. >>> >>> How is this different from sysbus_pass_irq? >> >> sysbus_pass_irq is also an annoyingly inflexible function. >> With MMIOs we have the advantage of being able to do better. > > I prefer consistency to useless flexibility. > > The day someone will need it, they can add sysbus_pass_one_{irq,mmio}. > >>>> Please just make reference counting work properly with passing >>>> MemoryRegion*s around. >>> >>> Do you have any idea that doesn't require touch 800 invocation of the >>> region creation functions? >> >> I think that would be a straightforward and easy to understand >> way to define the ownership rules so I would much rather we >> did that. I really don't like the way your current patch >> is doing something complicated in an attempt to avoid this. > > They are straightforward, documented, and the wide majority of the > devices need not care at all about them. By contrast, changing 800 > invocations of the functions would be impossible to review seriously, it > would have to be redone when boards are qdev/QOM-ified, would be worse > for submitters of new boards. > > There are an order of magnitude less calls to memory_region_set_owner > than to memory_region_init_*. Changing four places suffices to get > ownership for 97% of the devices (309 files in hw/ call > memory_region_init*, 9 devices call memory_region_set_owner): > > hw/core/sysbus.c:1 > hw/isa/isa-bus.c:1 > hw/pci/pci.c:1 > ioport.c:2 > > Of the remaining calls, 2/3 of them are concentrated in a handful of > devices: > > hw/display/cirrus_vga.c:7 > hw/display/vga.c:4 > hw/i386/kvm/pci-assign.c:7 > hw/misc/vfio.c:8 A closer look at the code, and a better grep command, changed this down to: hw/display/cirrus_vga.c:2 hw/display/qxl.c:1 hw/display/vga-isa.c:1 hw/display/vga.c:6 hw/misc/vfio.c:8 (plus the ones quoted below) where all the calls in cirrus, qxl and vfio could be removed realtively easily. In particular, VGA card implementations do not use pci_register_vga yet, because it's a new API. So, with further refactoring, it could be brought down to 1 or 2 calls in hw/display/vga.c and one in vga-isa.c. hw/display/vga.c needs the memory_region_set_owner calls, both because of optimization tricks, and because the code tries to cover both ISA and PCI. vga-isa.c needs it because VGAs are special ISA devices, the only ones that do MMIO. Now, doing all the refactoring may not be worthwhile, but it shows that the abstraction is even less leaky than it sounds. I asked Alex Williamson to read the thread and share his opinion. Interestingly, he had a different mental model of building the memory regions (passing them to PCI core early rather than late, and that's why VFIO needed 8 calls in this series). So I believe his input will be useful. Paolo > and all the others could probably be refactored away, and are all for PC > devices, other targets are unaffected (I did review them and sysbus > catches everything): > > hw/acpi/ich9.c:1 > hw/acpi/piix4.c:4 > hw/char/serial-pci.c:1 > hw/isa/apm.c:1 > hw/misc/pc-testdev.c:4 > > > To me, it seems a pretty good abstraction. > > Paolo > >
On Tue, 2013-06-04 at 18:50 +0200, Paolo Bonzini wrote: > Il 04/06/2013 15:24, Paolo Bonzini ha scritto: > > Il 04/06/2013 14:36, Peter Maydell ha scritto: > >> On 4 June 2013 13:31, Paolo Bonzini <pbonzini@redhat.com> wrote: > >>> Il 04/06/2013 14:24, Peter Maydell ha scritto: > >>>> On 4 June 2013 13:13, Paolo Bonzini <pbonzini@redhat.com> wrote: > >>>> This is much less flexible than just using sysbus_mmio_get_region(), > >>>> because it only lets you pass the whole set of MMIOs from the > >>>> other device through, not just the ones you want. > >>> > >>> How is this different from sysbus_pass_irq? > >> > >> sysbus_pass_irq is also an annoyingly inflexible function. > >> With MMIOs we have the advantage of being able to do better. > > > > I prefer consistency to useless flexibility. > > > > The day someone will need it, they can add sysbus_pass_one_{irq,mmio}. > > > >>>> Please just make reference counting work properly with passing > >>>> MemoryRegion*s around. > >>> > >>> Do you have any idea that doesn't require touch 800 invocation of the > >>> region creation functions? > >> > >> I think that would be a straightforward and easy to understand > >> way to define the ownership rules so I would much rather we > >> did that. I really don't like the way your current patch > >> is doing something complicated in an attempt to avoid this. > > > > They are straightforward, documented, and the wide majority of the > > devices need not care at all about them. By contrast, changing 800 > > invocations of the functions would be impossible to review seriously, it > > would have to be redone when boards are qdev/QOM-ified, would be worse > > for submitters of new boards. > > > > There are an order of magnitude less calls to memory_region_set_owner > > than to memory_region_init_*. Changing four places suffices to get > > ownership for 97% of the devices (309 files in hw/ call > > memory_region_init*, 9 devices call memory_region_set_owner): > > > > hw/core/sysbus.c:1 > > hw/isa/isa-bus.c:1 > > hw/pci/pci.c:1 > > ioport.c:2 > > > > Of the remaining calls, 2/3 of them are concentrated in a handful of > > devices: > > > > hw/display/cirrus_vga.c:7 > > hw/display/vga.c:4 > > hw/i386/kvm/pci-assign.c:7 > > hw/misc/vfio.c:8 > > A closer look at the code, and a better grep command, changed this down to: > > hw/display/cirrus_vga.c:2 > hw/display/qxl.c:1 > hw/display/vga-isa.c:1 > hw/display/vga.c:6 > hw/misc/vfio.c:8 > > (plus the ones quoted below) where all the calls in cirrus, qxl and vfio > could be removed realtively easily. In particular, VGA card > implementations do not use pci_register_vga yet, because it's a new API. > > So, with further refactoring, it could be brought down to 1 or 2 calls > in hw/display/vga.c and one in vga-isa.c. hw/display/vga.c needs the > memory_region_set_owner calls, both because of optimization tricks, and > because the code tries to cover both ISA and PCI. vga-isa.c needs it > because VGAs are special ISA devices, the only ones that do MMIO. > > Now, doing all the refactoring may not be worthwhile, but it shows that > the abstraction is even less leaky than it sounds. > > I asked Alex Williamson to read the thread and share his opinion. > Interestingly, he had a different mental model of building the memory > regions (passing them to PCI core early rather than late, and that's why > VFIO needed 8 calls in this series). So I believe his input will be useful. We'll see about that ;) It's true that it's simply a mental model of doing the required steps, then optimizing that makes vfio need a sprinkling of set ownership calls. Paolo, your patch to move the PCI/VGA registration later solves this and completely hides memory region ownership from vfio. That's great, but as Peter is arguing, leaves a hole that I'm not even aware that an owner is required for a memory region and the API still leaves me lots of opportunities to get it wrong. So, I have to go back to Rusty's API design guidelines that an API should difficult to use incorrectly. From what I see, I'm not sure we have that here. An ugly compromise might be a runtime checks for orphan memory regions after a device is initialized, but that has it's own set of problems. Thanks, Alex
Il 04/06/2013 19:47, Alex Williamson ha scritto: > We'll see about that ;) At the very least you broke the tie. :) > It's true that it's simply a mental model of > doing the required steps, then optimizing that makes vfio need a > sprinkling of set ownership calls. Paolo, your patch to move the > PCI/VGA registration later solves this and completely hides memory > region ownership from vfio. That's great, but as Peter is arguing, > leaves a hole that I'm not even aware that an owner is required for a > memory region and the API still leaves me lots of opportunities to get > it wrong. So, I have to go back to Rusty's API design guidelines that > an API should difficult to use incorrectly. From what I see, I'm not > sure we have that here. An ugly compromise might be a runtime checks > for orphan memory regions after a device is initialized, but that has > it's own set of problems. Thanks, Then I'll hunt for the 800 owners. In the meanwhile, patch 2/3/4/14/15/16/17 won't change from this series to the next, so a review of those is welcome. :) Paolo
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c index 9004d8c..6dbd1f8 100644 --- a/hw/core/sysbus.c +++ b/hw/core/sysbus.c @@ -117,6 +117,18 @@ void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory) dev->mmio[n].memory = memory; } +/* Pass MMIOs from a target device. */ +void sysbus_pass_mmio(SysBusDevice *dev, SysBusDevice *target) +{ + int i; + assert(dev->num_mmio == 0); + dev->num_mmio = target->num_mmio; + for (i = 0; i < dev->num_mmio; i++) { + assert(target->mmio[i].addr == -1); + dev->mmio[i] = target->mmio[i]; + } +} + MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n) { return dev->mmio[n].memory; diff --git a/hw/cpu/arm11mpcore.c b/hw/cpu/arm11mpcore.c index 90dcead..cc885d1 100644 --- a/hw/cpu/arm11mpcore.c +++ b/hw/cpu/arm11mpcore.c @@ -213,7 +213,7 @@ static int realview_mpcore_init(SysBusDevice *dev) } } qdev_init_gpio_in(&dev->qdev, mpcore_rirq_set_irq, 64); - sysbus_init_mmio(dev, sysbus_mmio_get_region(s->priv, 0)); + sysbus_pass_mmio(dev, s->priv); return 0; } diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h index 7c2e316..0639343 100644 --- a/include/hw/sysbus.h +++ b/include/hw/sysbus.h @@ -48,6 +48,7 @@ struct SysBusDevice { void *sysbus_new(void); void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory); +void sysbus_pass_mmio(SysBusDevice *dev, SysBusDevice *target); MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n); void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p); void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target);
This matches sysbus_pass_irq in cases where a device is a thin wrapper of another. MMIO regions will keep the subdevice as the owner. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/core/sysbus.c | 12 ++++++++++++ hw/cpu/arm11mpcore.c | 2 +- include/hw/sysbus.h | 1 + 3 files changed, 14 insertions(+), 1 deletion(-)