From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH v2 8/9] qemu-kvm: Refresh MSI settings after vmload Date: Wed, 27 Apr 2011 18:20:52 +0200 Message-ID: <4DB84264.4040007@siemens.com> References: <4DB7E315.6030004@redhat.com> <4DB80059.7080308@siemens.com> <20110427141631.GO15788@redhat.com> <4DB82828.7040800@siemens.com> <20110427143049.GQ15788@redhat.com> <4DB82AB9.4090500@siemens.com> <20110427150926.GU15788@redhat.com> <4DB83487.7070102@siemens.com> <20110427160220.GA25494@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Avi Kivity , Marcelo Tosatti , "kvm@vger.kernel.org" To: "Michael S. Tsirkin" Return-path: Received: from goliath.siemens.de ([192.35.17.28]:27949 "EHLO goliath.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751123Ab1D0QVA (ORCPT ); Wed, 27 Apr 2011 12:21:00 -0400 In-Reply-To: <20110427160220.GA25494@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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