Re: [PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload

["Michael S. Tsirkin" <mst@redhat.com>]

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