From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 16/24] xen/passthrough: Introduce iommu_construct Date: Wed, 21 Jan 2015 10:37:19 +0000 Message-ID: <54BF815F.5090106@linaro.org> References: <1421159133-31526-1-git-send-email-julien.grall@linaro.org> <1421159133-31526-17-git-send-email-julien.grall@linaro.org> <54BD46C60200007800056A6B@mail.emea.novell.com> <54BE6611.1070701@linaro.org> <54BE932502000078000572A8@mail.emea.novell.com> <54BE8C4A.7030801@linaro.org> <54BF8C470200007800057611@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YDsbz-0004iE-Bh for xen-devel@lists.xenproject.org; Wed, 21 Jan 2015 10:34:11 +0000 Received: by mail-wg0-f53.google.com with SMTP id a1so1724534wgh.12 for ; Wed, 21 Jan 2015 02:34:10 -0800 (PST) In-Reply-To: <54BF8C470200007800057611@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel@lists.xenproject.org, stefano.stabellini@citrix.com, ian.campbell@citrix.com, tim@xen.org List-Id: xen-devel@lists.xenproject.org On 21/01/2015 10:23, Jan Beulich wrote: >>>> On 20.01.15 at 18:11, wrote: >> On 20/01/15 16:40, Jan Beulich wrote: >>>>>> On 20.01.15 at 15:28, wrote: >>>> On 19/01/15 17:02, Jan Beulich wrote: >>>>>>>> On 13.01.15 at 15:25, wrote: >>>>>> --- a/xen/drivers/passthrough/device_tree.c >>>>>> +++ b/xen/drivers/passthrough/device_tree.c >>>>>> @@ -41,6 +41,10 @@ int iommu_assign_dt_device(struct domain *d, struct >> dt_device_node *dev) >>>>>> if ( !list_empty(&dev->domain_list) ) >>>>>> goto fail; >>>>>> >>>>>> + rc = iommu_construct(d); >>>>>> + if ( rc ) >>>>>> + goto fail; >>>>> >>>>> Considering that the only (current) caller of this it domain_build.c I'm >>>>> afraid you're going to get into trouble if you get back -ERESTART >>>>> here. Note that on x86 Dom0 setup works via iommu_hwdom_init(), >>>>> which deals with the preemption needs (at that point in time) by >>>>> calling process_pending_softirqs() every once in a while. >>>> >>>> iommu_hwdom_init is also called for ARM (it's part of the common domain >>>> creation code). So, I don't see any issue here as we match the same >>>> behavior as PCI. >>>> >>>> FWIW, on the previous version you asked to check the need_iommu(d) in >>>> iommu_construct. For DOM0 it will return 0 and therefore never return >>>> -ERESTART. >>> >>> Quoting the function: >>> >>> +int iommu_construct(struct domain *d) >>> +{ >>> + int rc = 0; >>> + >>> + if ( need_iommu(d) > 0 ) >>> + return 0; >>> + >>> + if ( !iommu_use_hap_pt(d) ) >>> + { >>> + rc = arch_iommu_populate_page_table(d); >>> + if ( rc ) >>> + return rc; >>> + } >>> + >>> + d->need_iommu = 1; >>> + >>> + return rc; >>> +} >> >>> If need_iommu() returns 0 for Dom0, then the early return won't get >>> used. Hence I don't follow your comment above. And if what you say >>> there was correct, then I don't understand why you add the call >>> quoted at the very top in the first place (again taking into consideration >>> that - afaict - the only [current] caller is in domain_build.c). >> >> I don't understand what is the issue in the device tree use case. As I >> said, assign_device in the pci do exactly the same things. > > Sure, but it's not being called for Dom0, but only out of the domctl > handler. Hmmm right. It's the main difference between non-PCI and PCI passthrough. > >> While this function is currently only used for DOM0, this will be used >> in a later patch for guest non-PCI passthrough. > > Okay, but you shouldn't break (or alter in [seemingly] benign ways) the > Dom0 case imo. As iommu_hwdom_init is initialized correctly the IOMMU for DOM0, iommu_construct is a no-op. Would an if ( need_iommu(d) ) will be more clear? Maybe we an assert (!is_hardware_domain(d)). Regards, -- Julien Grall