From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v8 4/4] xen/arm: grant: Add another entry to map MFN 1:1 in dom0 p2m Date: Wed, 21 May 2014 15:01:51 +0100 Message-ID: <537CB1CF.1030206@linaro.org> References: <1400516640-7175-1-git-send-email-julien.grall@linaro.org> <1400516640-7175-5-git-send-email-julien.grall@linaro.org> <1400678863.4856.87.camel@kazak.uk.xensource.com> <537CAD5B.1080207@linaro.org> <1400680247.4856.100.camel@kazak.uk.xensource.com> 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 1Wn75e-0008W4-NU for xen-devel@lists.xenproject.org; Wed, 21 May 2014 14:01:55 +0000 Received: by mail-ee0-f49.google.com with SMTP id e53so1631889eek.36 for ; Wed, 21 May 2014 07:01:53 -0700 (PDT) In-Reply-To: <1400680247.4856.100.camel@kazak.uk.xensource.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: Ian Campbell Cc: xen-devel@lists.xenproject.org, tim@xen.org, Jan Beulich , stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org On 05/21/2014 02:50 PM, Ian Campbell wrote: >>>> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c >>>> index 21b4572..9f85800 100644 >>>> --- a/xen/drivers/passthrough/arm/smmu.c >>>> +++ b/xen/drivers/passthrough/arm/smmu.c >>>> @@ -1536,6 +1536,48 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d) >>>> xfree(smmu_domain); >>>> } >>>> >>>> +static int arm_smmu_map_page(struct domain *d, unsigned long gfn, >>>> + unsigned long mfn, unsigned int flags) >>>> +{ >>>> + p2m_type_t t; >>>> + >>>> + /* This function should only be used by gnttab code when the domain >>>> + * is direct mapped and gfn == mfn. >>> >>> Is gfn !+ mfn an ASSERT-worthy condition? >> >> The ASSERT would only be for debug build. I'd like to have a safe guard >> for non-debug build just in case. > > That's a BUG_ON then I think, assuming it would be a coding error in the > hypervisor (rather than e.g. a guest trying to exploit the issue > somehow). The guest should not be able to exploit this issue. I will add a BUG_ON. >>> Is gnttab the only possible user? >> >> For ARM yes. > > OK > > (out of curiosity what are the other users on x86?) It's used for create the IOMMU PT. >>>> + * This is only valid when the domain is directed mapped >>>> + */ >>>> + return guest_physmap_add_entry(d, gfn, mfn, 0, t); >>>> +} >>>> + >>>> +static int arm_smmu_unmap_page(struct domain *d, unsigned long gfn) >>>> +{ >>>> + /* This function should only be used by gnttab code when the domain >>>> + * is direct mapped >>>> + */ >>>> + if ( !is_domain_direct_mapped(d) ) >>>> + return -EINVAL; >>>> + >>>> + guest_physmap_remove_page(d, gfn, gfn, 0); >>> >>> I think 0 here is really PAGE_ORDER_4K, is it? (other callers of this >>> function seem to be inconsistent about this) >> >> Yes, assuming the guest page will always be 4K. > > Even if not then PAGE_ORDER_4K will make good fodder for grep... I will use it in the next version. >> What about introducing "dummy type" such as p2m_notype_{ro,rw} which >> could be use in such case? > > notype is effectively "ram" I think, but that doesn't seem quite right > either. > > I'm just worried that p2m type bits are in limited supply so I want to > be sure using new ones is justified. We don't really need to store those type in the P2M. We only need them to choose the page attributes. We could introduce a virtual type (i.e value > p2m_max_real_type) and store p2m_invalid in the P2M. Regards, -- Julien Grall