From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39248) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XH8VE-0002nK-GZ for qemu-devel@nongnu.org; Tue, 12 Aug 2014 05:36:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XH8V8-000144-7X for qemu-devel@nongnu.org; Tue, 12 Aug 2014 05:36:24 -0400 Message-ID: <53E9E010.9030309@suse.de> Date: Tue, 12 Aug 2014 11:36:16 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1406799254-25223-1-git-send-email-aik@ozlabs.ru> <1406799254-25223-9-git-send-email-aik@ozlabs.ru> <53E8B02F.1040908@suse.de> <53E8E097.5080603@ozlabs.ru> <53E8FD76.4080602@suse.de> <53E95C10.8040402@ozlabs.ru> <53E9910A.8060904@ozlabs.ru> In-Reply-To: <53E9910A.8060904@ozlabs.ru> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH 08/10] spapr_pci: Enable DDW List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy , qemu-devel@nongnu.org Cc: Alex Williamson , qemu-ppc@nongnu.org On 12.08.14 05:59, Alexey Kardashevskiy wrote: > On 08/12/2014 10:13 AM, Alexey Kardashevskiy wrote: >> On 08/12/2014 03:29 AM, Alexander Graf wrote: >>> On 11.08.14 17:26, Alexey Kardashevskiy wrote: >>>> On 08/11/2014 09:59 PM, Alexander Graf wrote: >>>>> On 31.07.14 11:34, Alexey Kardashevskiy wrote: >>>>>> This implements DDW for emulated PHB. >>>>>> >>>>>> This advertises DDW in device tree. >>>>>> >>>>>> Signed-off-by: Alexey Kardashevskiy >>>>>> --- >>>>>> >>>>>> The DDW has not been tested as QEMU does not implement any 64bit DMA >>>>>> capable >>>>>> device and existing linux guests do not use DDW for 32bit DMA. >>>>> Can't you just add the pci config space bit for it to the e1000 emulation? >>>> Sorry, I am not following you here. What bit in config space can enable >>>> 64bit DMA? >>> Apparently there's nothing at all required. The igb driver simply tries to >>> use 64bit DMA masks. >> A driver should use 64bit addresses (unsigned long, u64) for DMA, not 32bit >> (unsigned, u32). >> >> >>>> I tried patching the guest driver, that did not work so I did not dig >>>> further. >>> Which driver did you try it with? >> >> drivers/net/ethernet/intel/e1000/e1000_main.c >> >> I looked again, the driver uses 64bit DMA if it is PCI-X-capable adapter >> which e1000 form QEMU is not. Does it decide this only based on the pci id? If so, fake it. >> >> >>> >>>>> That one should be pretty safe, no? >>>>> >>>>>> --- >>>>>> hw/ppc/spapr_pci.c | 65 >>>>>> +++++++++++++++++++++++++++++++++++++++++++++ >>>>>> include/hw/pci-host/spapr.h | 5 ++++ >>>>>> 2 files changed, 70 insertions(+) >>>>>> >>>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>>>>> index 230b59c..d1f4c86 100644 >>>>>> --- a/hw/ppc/spapr_pci.c >>>>>> +++ b/hw/ppc/spapr_pci.c >>>>>> @@ -22,6 +22,7 @@ >>>>>> * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER >>>>>> DEALINGS IN >>>>>> * THE SOFTWARE. >>>>>> */ >>>>>> +#include "sysemu/sysemu.h" >>>>>> #include "hw/hw.h" >>>>>> #include "hw/pci/pci.h" >>>>>> #include "hw/pci/msi.h" >>>>>> @@ -650,6 +651,8 @@ static void spapr_phb_finish_realize(sPAPRPHBState >>>>>> *sphb, Error **errp) >>>>>> /* Register default 32bit DMA window */ >>>>>> memory_region_add_subregion(&sphb->iommu_root, 0, >>>>>> spapr_tce_get_iommu(tcet)); >>>>>> + >>>>>> + sphb->ddw_supported = true; >>>>> Unconditionally? >>>> Yes. Why not? I cannot think of any case when we would not want this. In >>>> practice there is very little chance it will ever be used anyway :) There >>>> is still a machine option to disable it completely. >>>> >>>> >>>>> Also, can't you make the ddw enable/disable flow go set-only? Basically >>>>> have the flag in the machine struct if you must, but then on every PHB >>>>> instantiation you set a QOM property that sets ddw_supported respectively? >>>> Uff. Very confusing review comments today :) >>>> >>>> For VFIO, ddw_supported comes from the host kernel and totally depends on >>>> hardware. >>>> >>>> For emulated, there is just one emulated PHB (yes, can be many but noone >>>> seems to be using more in reality) and what you suggest seems to be too >>>> complicated. >>>> >>>> This DDW thing - it is not really dynamic in the way it is used by the >>>> existing linux guest. At the boot time the guest driver looks at DMA mask >>>> and only if it is >32bit, it creates DDW, once, and after that the windows >>>> remains active while the guest is running. >>> What I'm asking is that rather than having >>> >>> if (machine->ddw_enabled && phb->ddw_supported) >>> >>> to instead only have >>> >>> if (phb->ddw_enabled) >>> >>> which gets set by the machine to true if machine->ddw_enabled. If you make >>> it a qom property you can control the setter, so you can at the point when >>> the machine wants to set it also ignore the set to true if your vfio >>> implementation doesn't support ddw, leaving ddw_enabled as false. >>> >>>> >>>>> Also keep in mind that we will have to at least disable ddw by default for >>>>> existing machine types to maintain backwards compatibility. >>>> Where exactly does the default setting "on" break in compatibility? >>> Different device tree? Different return values on rtas calls? These are >>> guest visible changes, so in theory we would have to make sure we don't >>> change any of them. >>> >>> Of course we can always consciously declare them as unimportant enough that >>> they in reality shouldn't have side effects we care about for hot and live >>> migration, but there'd have to be a good reasoning on why we shouldn't have >>> it disabled rather than why we should have backwards compatibility. >> "hot" migration? What is that? :) >> >> There is a machine option to disable it and migrate to older guest (which >> we do not support afair or do we?). If we migrate to newer QEMU, these DDW >> tokens will be missing in the destination guest's tree and DDW won't be >> used, everybody is happy. I really fail to see a scenario when I would not >> use DDW... > Ok, Paul explained. So by default "ddw" must be off for the pseries-2.0 > machine and on for pseries-2.2 and we'll be fine, right? Yes. Alex