[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 09/17] xen/arm: p2m type definitions and changes



H Tamas,

On 12/09/14 01:15, Tamas K Lengyel wrote:


On Thu, Sep 11, 2014 at 10:25 PM, Julien Grall <julien.grall@xxxxxxxxxx
<mailto:julien.grall@xxxxxxxxxx>> 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 <public/memory.h>
        +#include <public/mem_event.h>


    Why do you need those 2 includes?



<public/memory.h> 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 <xen/p2m-common.h>

        @@ -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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.