From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v3 12/13] xen/iommu: smmu: Add Xen specific code to be able to use the driver Date: Fri, 20 Feb 2015 13:23:40 +0000 Message-ID: <1424438620.30924.225.camel@citrix.com> References: <1422643768-23614-1-git-send-email-julien.grall@linaro.org> <1422643768-23614-13-git-send-email-julien.grall@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YOnYZ-0007wM-GN for xen-devel@lists.xenproject.org; Fri, 20 Feb 2015 13:23:47 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: xen-devel@lists.xenproject.org, Julien Grall , tim@xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org On Fri, 2015-02-06 at 13:20 +0000, Stefano Stabellini wrote: > > +#define iommu_group_get_iommudata(group) (group)->cfg > > I would move all the Xen stuff at the beginning of the file or maybe to > a separate file. You could #include the linux smmu driver into it. > Maybe we cannot have runtime patching of this file, but at least we can > keep Xen and Linux stuff clearly separate. I was going to suggest something similar e.g. #include or something. > > > > static DEFINE_SPINLOCK(arm_smmu_devices_lock); > > static LIST_HEAD(arm_smmu_devices); > > > > @@ -455,6 +690,8 @@ static void parse_driver_options(struct arm_smmu_device *smmu) > > > > static struct device_node *dev_get_dev_node(struct device *dev) > > { > > + /* Xen: TODO: Add support for PCI */ > > +#if 0 > > if (dev_is_pci(dev)) { > > struct pci_bus *bus = to_pci_dev(dev)->bus; > > > > @@ -462,7 +699,7 @@ static struct device_node *dev_get_dev_node(struct device *dev) > > bus = bus->parent; > > return bus->bridge->parent->of_node; > > } > > - > > +#endif > > Would it be possible instead to add: > > #define dev_is_pci (0) > > above among the Xen definitions? Would be preferable if possible. > > > @@ -700,11 +938,10 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev) > > /* Retry or terminate any stalled transactions */ > > if (fsr & FSR_SS) > > writel_relaxed(resume, cb_base + ARM_SMMU_CB_RESUME); > > - > > - return ret; > > } > > For the sake of minimizing the changes to Linux functions, could you > write a wrapper around this function instead? That might allow you to > remove the changes related to the return value. typedef void irqreturn_t; and "#define IRQ_NONE /**/" etc perhaps? Or even just cast the function in the call to request_irq, the differing return type shouldn't cause a problem in this context I think. > > > > +/* Xen: Page tables are shared with the processor */ > > +#if 0 > > static void arm_smmu_flush_pgtable(struct arm_smmu_device *smmu, void *addr, > > size_t size) > > { > > @@ -749,6 +987,7 @@ static void arm_smmu_flush_pgtable(struct arm_smmu_device *smmu, void *addr, > > DMA_TO_DEVICE); > > } > > } > > +#endif > > But then you need to fix all the call sites. I think it is best to add a > return at the beginning of the function. Or add a nop stub... (return sounds better though) > > @@ -886,9 +1131,21 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain) > > reg |= (64 - smmu->s1_input_size) << TTBCR_T0SZ_SHIFT; > > } > > } else { > > +#if CONFIG_ARM_32 > > + /* Xen: Midway is using 40-bit address space. I'm a bit surprised that the upstream driver isn't prepared to cope with this sort of thing, there are a few LPAE arm32 systems around these days. I had the same thought around the " /* Xen: The fault address maybe higher than 32 bits on arm32 */" comment too. Ian.