On Wed, Apr 27, 2011 at 06:20:52PM +0200, Jan Kiszka wrote: > On 2011-04-27 18:02, Michael S. Tsirkin wrote: > > On Wed, Apr 27, 2011 at 05:21:43PM +0200, Jan Kiszka wrote: > >> On 2011-04-27 17:09, Michael S. Tsirkin wrote: > >>> On Wed, Apr 27, 2011 at 04:39:53PM +0200, Jan Kiszka wrote: > >>>> On 2011-04-27 16:30, Michael S. Tsirkin wrote: > >>>>>>>> --- a/hw/pci.c > >>>>>>>> +++ b/hw/pci.c > >>>>>>>> @@ -34,6 +34,7 @@ > >>>>>>>> #include "device-assignment.h" > >>>>>>>> #include "qemu-objects.h" > >>>>>>>> #include "range.h" > >>>>>>>> +#include "msi.h" > >>>>>>>> > >>>>>>>> //#define DEBUG_PCI > >>>>>>>> #ifdef DEBUG_PCI > >>>>>>>> @@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) > >>>>>>>> memcpy(s->config, config, size); > >>>>>>>> > >>>>>>>> pci_update_mappings(s); > >>>>>>>> + msi_post_load(s); > >>>>>>> > >>>>>>> Pls don't do this: I'm trying to keep just the core in > >>>>>>> pci.c and all capabilities in separate files. > >>>>>>> msix has msix_load, msi will just need one too, > >>>>>>> and let all devices call that. > >>>>>>> > >>>>>> > >>>>>> Preferred alternatives are...? Registering a vmstate for msi? > >>>>>> > >>>>>> Jan > >>>>> > >>>>> Add msi_load and call that from devices that need it. > >>>>> Like msix_load does now. > >>>>> > >>>> > >>>> msix_load/save are refactoring candidates IMHO. MSI-X has a real need > >>>> for storing additional state information, so it should register its own > >>>> subsection. > >>> > >>> That's an implementation detail though, isn't it. > >>> > >>>> I don't want to offload this burden to the devices also for > >>>> MSI. > >>>> From the devices' POV, why shouldn't msi_init suffice? > >>>> > >>>> Jan > >>> > >>> One can also claim this about config writes: > >>> pci_bridge_write_config(d, address, val, len); > >>> pcie_cap_flr_write_config(d, address, val, len); > >>> pcie_cap_slot_write_config(d, address, val, len); > >>> msi_write_config(d, address, val, len); > >>> pcie_aer_write_config(d, address, val, len); > >>> which arguably just duplicates the initialization sequence. > >>> > >>> What I'm trying to do though is to keep it modular and > >>> keep module inter-dependencies to a minimum, > >>> so that pci is the core and msix depends on it > >>> but not the other way around. > >> > >> I still don't see the bigger benefit in saving a single bidirectional > >> dependency at core level vs. saving additional callbacks at each and > >> every MSI user. The latter is also a source for bugs. > >> > >> Jan > > > > Yes but let us be consistent with how e.g. config writes are > > handled. As I said > > > >>> What I think we should do is to add a pci subdirectory, move all > >>> of the stuff there, move pci.c to pci/core.c and > >>> add a high level module that depends on them all > >>> and deals with all the capabilities. > > > > Which will solve both issues without need for tradeoffs. > > OK, but this patch is not about doing a pci refactoring. And it should > not touch any device without a need. So what is your suggestion again? > > Jan Either add msi_load call in 4 msi users or keep your patch as is. I'll fix it up when I do the refactoring. > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
