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

[Jan Kiszka <jan.kiszka@siemens.com>]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux