From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH 3/8] xen/arm: Implement p2m_type_t as an enum Date: Thu, 05 Dec 2013 16:44:01 +0000 Message-ID: <52A0AD51.6050203@linaro.org> References: <1386258131-755-1-git-send-email-julien.grall@linaro.org> <1386258131-755-4-git-send-email-julien.grall@linaro.org> <1386258750.20047.84.camel@kazak.uk.xensource.com> <52A0A36F.7070908@linaro.org> <1386260093.20047.101.camel@kazak.uk.xensource.com> <52A0A9B5.5060705@linaro.org> <1386261523.20047.120.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" 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 1Voc21-0003H3-RH for xen-devel@lists.xenproject.org; Thu, 05 Dec 2013 16:44:06 +0000 Received: by mail-ea0-f182.google.com with SMTP id a15so1189354eae.13 for ; Thu, 05 Dec 2013 08:44:04 -0800 (PST) In-Reply-To: <1386261523.20047.120.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, stefano.stabellini@citrix.com, patches@linaro.org List-Id: xen-devel@lists.xenproject.org On 12/05/2013 04:38 PM, Ian Campbell wrote: > On Thu, 2013-12-05 at 16:28 +0000, Julien Grall wrote: >> >> On 12/05/2013 04:14 PM, Ian Campbell wrote: >>> On Thu, 2013-12-05 at 16:01 +0000, Julien Grall wrote: >>>> >>>> On 12/05/2013 03:52 PM, Ian Campbell wrote: >>>>> On Thu, 2013-12-05 at 15:42 +0000, Julien Grall wrote: >>>>>> Until now, Xen doesn't know the type of the page (ram, foreign page, mmio,...). >>>>>> Introduce p2m_type_t with basic types: >>>>>> - p2m_invalid: Nothing is mapped here >>>>> >>>>> Do we really need this? Is it not equivalent to not setting the present >>>>> bit? I see x86 has the same type though -- Tim can you explain why. >>>> >>>> We need a default value when Xen retrieves the p2m type. I don't think >>>> we can assume that p2m_ram_rw (or any other type) is used by default. >>>> >>>>> Since the avail bits in the p2m pte are in pretty short supply I think >>>>> we can avoid unnecessary types. >>>> >>>> I plan to use directly the decimal value. So we can store up to 16 values. >>> >>> 16 is short supply in my book ;-) >>> >>> Having got a bit further through the series I see how p2m_invalid is >>> being used now. It is a useful pseudo-type but it doesn't need to be >>> represented in the avail bits I don't think. How about: >>> >>> typedef enum { >>> p2m_ram_rw, /* Normal read/write guest RAM */ >>> p2m_ram_ro, /* Read-only; writes are silently dropped */ >>> p2m_mmio_direct, /* Read/write mapping of genuine MMIO area / >>> p2m_map_foreign, /* Ram pages from foreign domain */ >>> >>> p2m_max_real_type = 16, /* Types after this are pseudo-types. */ >>> >>> p2m_invalid, /* Nothing mapped here */ >>> >>> } p2m_type_t; >>> >>> BUILD_BUG_ON(p2m_max_real_type >= 2^4); >>> >>> Now you can return it etc but it never needs to get put in an actual >>> pte? >> >> >> This solution was easier to avoid extra code in the different function. >> I will rework it for the next series. > > "This" is what I suggested here or what you wrote already? The code I wrote. > >>> Maybe this is one for the future when we get a bit short on bits. >>> >>>>>> - p2m_ram_rw: Normal read/write guest RAM >>>>>> - p2m_ram_ro: Read-only guest RAM >>>>>> - p2m_mmio_direct: Read/write mapping of device memory >>>>>> - p2m_map_foreign: RAM page from foreign guest >>>>> >>>>> Is there no need for an entry for a grant mapping (and a ro >>>>> counterpart)? >>>> >>>> Hmmm .. actually grant table is mapped as RAM (so read/write and >>>> execute). Do we want to allow code execution from grant-mapping page? >>>> If not, then we will need to introduce specific p2m type from grant-mapping. >>> >>> If a guest is stupid enough to execute code from a page owned by another >>> guest then it gets what it deserves ;-) >> >> Actually X86, disable execution on grant and foreign mapping. > > I guess consistency is a good reason to do the same then. Ok. So I will add p2m_grant_map_ro and p2m_grant_map_rw. -- Julien Grall