From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v5 12/14] xen/arm: p2m: Clean cache PT when the IOMMU doesn't support coherent walk Date: Wed, 14 May 2014 10:09:25 +0100 Message-ID: <537332C5.2010302@linaro.org> References: <1399996230-18201-1-git-send-email-julien.grall@linaro.org> <1399996230-18201-13-git-send-email-julien.grall@linaro.org> <537334D90200007800012009@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.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WkVBp-00044V-Fs for xen-devel@lists.xenproject.org; Wed, 14 May 2014 09:09:29 +0000 Received: by mail-wi0-f173.google.com with SMTP id bs8so7635526wib.12 for ; Wed, 14 May 2014 02:09:27 -0700 (PDT) In-Reply-To: <537334D90200007800012009@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, Xiantao Zhang , tim@xen.org List-Id: xen-devel@lists.xenproject.org Hi Ian, On 14/05/14 08:18, Jan Beulich wrote: >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -344,6 +344,17 @@ void iommu_crash_shutdown(void) >> iommu_enabled = iommu_intremap = 0; >> } >> >> +bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature) >> +{ >> + const struct iommu_ops *ops = domain_hvm_iommu(d)->platform_ops; >> + uint32_t features = 0; > > Please here and further down - don't use fixed width type unless > you really need to. > >> --- a/xen/include/xen/iommu.h >> +++ b/xen/include/xen/iommu.h >> @@ -67,6 +67,14 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, >> unsigned int flags); >> int iommu_unmap_page(struct domain *d, unsigned long gfn); >> >> +enum iommu_feature >> +{ >> + IOMMU_FEAT_COHERENT_WALK = 1, > > Why 1? Enumerations are defined to start at zero, and starting at > zero is what you really want here. Don't specify a value at all. It's a mistake when I create the enum. I will drop the 1. >> @@ -139,6 +147,7 @@ struct iommu_ops { >> void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count); >> void (*iotlb_flush_all)(struct domain *d); >> void (*dump_p2m_table)(struct domain *d); >> + uint32_t (*features)(struct domain *d); > > I think I said this on an earlier round already - for future extensibility > this should return "const unsigned long *", and get accessed by the > wrapper function using test_bit(). Or even better without an accessor > function at all, just directly having a "const unsigned long *" field here. > Unless of course the backend implementation - which isn't part of this > patch - would have difficulty setting up a suitable bitfield during (early) > initialization. The SMMU drivers handle multiple SMMUs. Each SMMU can have different specifications (e.g coherent walk support or not). As each domain doesn't use all SMMUs, we might be able to avoid flushing PT on some of them. That's why I've choose to use a callback with the domain in parameter. I don't like the solution which return "unsigned long *" because we are assuming the driver will always a valid pointer (for instance with 2 unsigned long), even if he doesn't need it. Regards, -- Julien Grall