From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" 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 08:18:17 +0100 Message-ID: <537334D90200007800012009@mail.emea.novell.com> References: <1399996230-18201-1-git-send-email-julien.grall@linaro.org> <1399996230-18201-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.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WkTSG-0006Dq-OK for xen-devel@lists.xenproject.org; Wed, 14 May 2014 07:18:20 +0000 In-Reply-To: <1399996230-18201-13-git-send-email-julien.grall@linaro.org> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall 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 > --- 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. > @@ -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. Jan