From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC for-4.5 08/12] xen/passthrough: iommu: Split generic IOMMU code Date: Mon, 10 Feb 2014 11:52:36 +0000 Message-ID: <52F8BD84.3040805@linaro.org> References: <1391794991-5919-1-git-send-email-julien.grall@linaro.org> <1391794991-5919-9-git-send-email-julien.grall@linaro.org> <52F89A5A020000780011A9C9@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WCpPk-0002qR-N9 for xen-devel@lists.xenproject.org; Mon, 10 Feb 2014 11:52:40 +0000 Received: by mail-wi0-f178.google.com with SMTP id cc10so2702413wib.11 for ; Mon, 10 Feb 2014 03:52:38 -0800 (PST) In-Reply-To: <52F89A5A020000780011A9C9@nat28.tlf.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: ian.campbell@citrix.com, patches@linaro.org, tim@xen.org, stefano.stabellini@citrix.com, xen-devel@lists.xenproject.org, Xiantao Zhang List-Id: xen-devel@lists.xenproject.org Hello Jan, Thanks for the review. On 10/02/14 08:22, Jan Beulich wrote: >>>> On 07.02.14 at 18:43, Julien Grall wrote: >> The generic IOMMU framework code (xen/drivers/passthrough/iommu.c) contains >> functions specific to x86 and PCI. >> >> Split the framework in 3 distincts files: >> - iommu.c: contains generic functions shared between x86 and ARM >> (when it will be supported) >> - iommu_pci.c: contains specific functions for PCI passthrough >> - iommu_x86.c: contains specific functions for x86 >> >> iommu_pci.c will be only compiled when PCI is supported by the architecture >> (eg. HAS_PCI is defined). >> >> This patch is mostly code movement in new files. >> >> Signed-off-by: Julien Grall >> Cc: Xiantao Zhang >> Cc: Jan Beulich >> --- >> xen/drivers/passthrough/Makefile | 6 +- >> xen/drivers/passthrough/iommu.c | 473 +---------------------------------- >> xen/drivers/passthrough/iommu_pci.c | 468 ++++++++++++++++++++++++++++++++++ > > There's xen/drivers/passthrough/pci.c already - any reason not to > move the code there? I didn't specific about moving the code directly in passthrough/pci.c. I will do on the next version. > >> xen/drivers/passthrough/iommu_x86.c | 65 +++++ > > Same here for xen/drivers/passthrough/x86/. > >> @@ -696,125 +344,6 @@ void iommu_crash_shutdown(void) >> iommu_enabled = iommu_intremap = 0; >> } >> >> -int iommu_do_domctl( >> - struct xen_domctl *domctl, struct domain *d, >> - XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > > The function itself should probably not be moved out. Either the > PCI-specific pieces of it should be made conditional, or a > descendant function be created. Since (afaict) you'll need all of > the domctl-s (with different arguments) too for pass-through on > ARM - what's your plan for them? PCI passthrough will be supported on ARM in the future. But we will also have to support passthrough (via the device tree). I think, we will add new DOMCTL for that. Didn't really think about that know. I will add a descendant function to handle PCI DOMCTL. > >> --- a/xen/drivers/passthrough/vtd/iommu.c >> +++ b/xen/drivers/passthrough/vtd/iommu.c >> @@ -1784,31 +1784,31 @@ static int intel_iommu_unmap_page(struct domain *d, >> unsigned long gfn) >> >> void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, >> int order, int present) >> -{ >> - struct acpi_drhd_unit *drhd; >> - struct iommu *iommu = NULL; >> - struct hvm_iommu *hd = domain_hvm_iommu(d); >> - int flush_dev_iotlb; >> - int iommu_domid; >> + { >> + struct acpi_drhd_unit *drhd; >> + struct iommu *iommu = NULL; >> + struct hvm_iommu *hd = domain_hvm_iommu(d); >> + int flush_dev_iotlb; >> + int iommu_domid; >> >> - iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); >> + iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); >> >> - for_each_drhd_unit ( drhd ) >> - { >> - iommu = drhd->iommu; >> - if ( !test_bit(iommu->index, &hd->iommu_bitmap) ) >> - continue; >> + for_each_drhd_unit ( drhd ) >> + { >> + iommu = drhd->iommu; >> + if ( !test_bit(iommu->index, &hd->iommu_bitmap) ) >> + continue; >> >> - flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; >> - iommu_domid= domain_iommu_domid(d, iommu); >> - if ( iommu_domid == -1 ) >> - continue; >> - if ( iommu_flush_iotlb_psi(iommu, iommu_domid, >> - (paddr_t)gfn << PAGE_SHIFT_4K, >> - order, !present, flush_dev_iotlb) ) >> - iommu_flush_write_buffer(iommu); >> + flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; >> + iommu_domid= domain_iommu_domid(d, iommu); >> + if ( iommu_domid == -1 ) >> + continue; >> + if ( iommu_flush_iotlb_psi(iommu, iommu_domid, >> + (paddr_t)gfn << PAGE_SHIFT_4K, >> + order, !present, flush_dev_iotlb) ) >> + iommu_flush_write_buffer(iommu); >> + } >> } >> -} > > What are these changes to indentation about? Are you > deliberately breaking common rules here, or is this some sort of > unintentional leftover? It's an error when I have created this patch. I will remove it. Regards, -- Julien Grall