From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v5 09/17] xen/arm: p2m type definitions and changes Date: Fri, 12 Sep 2014 12:23:56 -0700 Message-ID: <5413484C.3010104@linaro.org> References: <1410355726-5599-1-git-send-email-tklengyel@sec.in.tum.de> <1410355726-5599-10-git-send-email-tklengyel@sec.in.tum.de> <5412053B.6040005@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: Ian Campbell , Tim Deegan , Ian Jackson , "xen-devel@lists.xen.org" , Stefano Stabellini , Andres Lagar-Cavilla , Jan Beulich , Daniel De Graaf , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org H Tamas, On 12/09/14 01:15, Tamas K Lengyel wrote: > > > On Thu, Sep 11, 2014 at 10:25 PM, Julien Grall > wrote: > > Hi Tamas, > > You skipped my comments/questions on v4. > > On 10/09/14 06:28, Tamas K Lengyel wrote: > > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 08ce07b..b4ca86d 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -2,6 +2,9 @@ > > > [..] > > > +#include > +#include > > > Why do you need those 2 includes? > > > > might not be necessary in this patch yet, but it will > be required for defining the functions that mem_access will call passing > xenmem_access_t access. I can move the header include into the next > patch in the series if that is cleaner. The mem_event header is not > actually required so I'll remove it. Yes please. > > #include > > @@ -11,6 +14,48 @@ struct domain; > > extern void memory_type_changed(struct domain *); > > +/* List of possible type for each page in the p2m entry. > + * The number of available bit per page in the pte for this > purpose is 4 bits. > + * So it's possible to only have 16 fields. If we run out of > value in the > + * future, it's possible to use higher value for pseudo-type > and don't store > + * them in the p2m entry. > + */ > +typedef enum { > + p2m_invalid = 0, /* Nothing mapped here */ > + 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_grant_map_rw, /* Read/write grant mapping */ > + p2m_grant_map_ro, /* Read-only grant mapping */ > + /* The types below are only used to decide the page > attribute in the P2M */ > + p2m_iommu_map_rw, /* Read/write iommu mapping */ > + p2m_iommu_map_ro, /* Read-only iommu mapping */ > + p2m_max_real_type, /* Types after this won't be store in > the p2m */ > +} p2m_type_t; > > > Any reason to move the enum earlier? If not, I would prefer to keep > at the same place. It will be easier with git-blame to find when a > new type has been added. > > > Stylistically it made more sense to have p2m_type_t and p2m_access_t > together (as it is on the x86 as well). And they are defined here as the > p2m_domain does have a field now defining default_access with p2m_access_t. If it's only for the style I wouldn't move them. Anyway, I'll Ian/Stefano decides about this. > > +/* > + * Additional access types, which are used to further restrict > + * the permissions given by the p2m_type_t memory type. Violations > + * caused by p2m_access_t restrictions are sent to the mem_event > + * interface. > + * > + * The access permissions are soft state: when any ambigious > change of page > > > ambiguous. > > > Copy-pasta but will fix. > > > [..] > > + /* Default P2M access type for each page in the the domain: > new pages, > + * swapped in pages, cleared pages, and pages that are > ambiquously > > > Did you intend to mean ambiguously rather than ambiquously? > > > Copy-pasta again but will fix. Maybe in a separate patch where I fix it > here and in the x86 side as well? I'm OK for a separate patch fixing x86 side, but there is no reason to fix the spelling for ARM outside this patch. Regards, -- Julien Grall