[3/8] xen/arm: Implement p2m_type_t as an enum

Message ID 1386258131-755-4-git-send-email-julien.grall@linaro.org
State Superseded
Headers show

Commit Message

Julien Grall Dec. 5, 2013, 3:42 p.m.
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
    - 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

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/include/asm-arm/p2m.h |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Egger, Christoph Dec. 5, 2013, 3:51 p.m. | #1
On 05.12.13 16:42, 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
>     - 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
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/include/asm-arm/p2m.h |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index f079f00..b24f94a 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -20,6 +20,16 @@ struct p2m_domain {
>      uint8_t vmid;
>  };
>  
> +typedef enum {
> +    p2m_invalid = 0,        /* Nothing mapped here */
> +    p2m_ram_rw = 1,         /* Normal read/write guest RAM */
> +    p2m_ram_ro = 2,         /* Read-only; writes are silently dropped */
> +    p2m_mmio_direct = 3,    /* Read/write mapping of genuine MMIO area */
> +    p2m_map_foreign = 4,    /* Ram pages from foreign domain */
> +} p2m_type_t;
> +
> +#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
> +

Is it possible to merge p2m_type_t with x86 and move into common code?

Christoph


>  /* Initialise vmid allocator */
>  void p2m_vmid_allocator_init(void);
>  
> @@ -72,7 +82,6 @@ p2m_pod_decrease_reservation(struct domain *d,
>                               unsigned int order);
>  
>  /* Look up a GFN and take a reference count on the backing page. */
> -typedef int p2m_type_t;
>  typedef unsigned int p2m_query_t;
>  #define P2M_ALLOC    (1u<<0)   /* Populate PoD and paged-out entries */
>  #define P2M_UNSHARE  (1u<<1)   /* Break CoW sharing */
> @@ -110,8 +119,6 @@ static inline int get_page_and_type(struct page_info *page,
>      return rc;
>  }
>  
> -#define p2m_is_foreign(_t) (0)
> -
>  #endif /* _XEN_P2M_H */
>  
>  /*
>
Ian Campbell Dec. 5, 2013, 3:52 p.m. | #2
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.

Since the avail bits in the p2m pte are in pretty short supply I think
we can avoid unnecessary types.

>     - 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)?


> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/include/asm-arm/p2m.h |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index f079f00..b24f94a 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -20,6 +20,16 @@ struct p2m_domain {
>      uint8_t vmid;
>  };
>  
> +typedef enum {
> +    p2m_invalid = 0,        /* Nothing mapped here */
> +    p2m_ram_rw = 1,         /* Normal read/write guest RAM */
> +    p2m_ram_ro = 2,         /* Read-only; writes are silently dropped */
> +    p2m_mmio_direct = 3,    /* Read/write mapping of genuine MMIO area */
> +    p2m_map_foreign = 4,    /* Ram pages from foreign domain */
> +} p2m_type_t;
> +
> +#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
> +
>  /* Initialise vmid allocator */
>  void p2m_vmid_allocator_init(void);
>  
> @@ -72,7 +82,6 @@ p2m_pod_decrease_reservation(struct domain *d,
>                               unsigned int order);
>  
>  /* Look up a GFN and take a reference count on the backing page. */
> -typedef int p2m_type_t;
>  typedef unsigned int p2m_query_t;
>  #define P2M_ALLOC    (1u<<0)   /* Populate PoD and paged-out entries */
>  #define P2M_UNSHARE  (1u<<1)   /* Break CoW sharing */
> @@ -110,8 +119,6 @@ static inline int get_page_and_type(struct page_info *page,
>      return rc;
>  }
>  
> -#define p2m_is_foreign(_t) (0)
> -
>  #endif /* _XEN_P2M_H */
>  
>  /*
Ian Campbell Dec. 5, 2013, 3:56 p.m. | #3
On Thu, 2013-12-05 at 16:51 +0100, Egger, Christoph wrote:
> On 05.12.13 16:42, 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
> >     - 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
> > 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > ---
> >  xen/include/asm-arm/p2m.h |   13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> > index f079f00..b24f94a 100644
> > --- a/xen/include/asm-arm/p2m.h
> > +++ b/xen/include/asm-arm/p2m.h
> > @@ -20,6 +20,16 @@ struct p2m_domain {
> >      uint8_t vmid;
> >  };
> >  
> > +typedef enum {
> > +    p2m_invalid = 0,        /* Nothing mapped here */
> > +    p2m_ram_rw = 1,         /* Normal read/write guest RAM */
> > +    p2m_ram_ro = 2,         /* Read-only; writes are silently dropped */
> > +    p2m_mmio_direct = 3,    /* Read/write mapping of genuine MMIO area */
> > +    p2m_map_foreign = 4,    /* Ram pages from foreign domain */
> > +} p2m_type_t;
> > +
> > +#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
> > +
> 
> Is it possible to merge p2m_type_t with x86 and move into common code?

Not really, the p2m handling is very different on the two arches, even
if they look superficially similar at this level.

x86 has more types than can fit in the available pte space on ARM for a
start.

I'd like to keep them separate please.
Julien Grall Dec. 5, 2013, 4:01 p.m. | #4
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.

>>      - 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.
Tim Deegan Dec. 5, 2013, 4:07 p.m. | #5
At 15:52 +0000 on 05 Dec (1386255150), 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.

There are other not-present types on x86: PoD, paged-out, emulated-MMIO.  

Tim.
Ian Campbell Dec. 5, 2013, 4:14 p.m. | #6
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?

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 ;-)

My question wasn't about that though -- just whether it is useful for
Xen to track whether the particular RAM mapping is normal or a grant
mapping.

x86 has some special handling, but I don't know if that is for
correctness or just a historical legacy of something else.

Ian.
Ian Campbell Dec. 5, 2013, 4:19 p.m. | #7
On Thu, 2013-12-05 at 17:07 +0100, Tim Deegan wrote:
> At 15:52 +0000 on 05 Dec (1386255150), 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.
> 
> There are other not-present types on x86: PoD, paged-out, emulated-MMIO.  

This means not Present doesn't imply invalid, I see. Even if we don't
currently have such types I can see how this makes sense. In theory when
we get short of bits we could consider the P bit one of the type bits,
which would give us 16 present and 16 not-present types. Overkill for
now I expect.

Is it ever the case that an actual p2m entry contains the p2m_invalid
bit pattern or is it more of a pseudo-type which is used internally and
as an API token in the hypervisor?

Ian.
Julien Grall Dec. 5, 2013, 4:28 p.m. | #8
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.

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

> My question wasn't about that though -- just whether it is useful for
> Xen to track whether the particular RAM mapping is normal or a grant
> mapping.

For now, I don't see a specific reason to track it.
Tim Deegan Dec. 5, 2013, 4:31 p.m. | #9
At 16:19 +0000 on 05 Dec (1386256758), Ian Campbell wrote:
> On Thu, 2013-12-05 at 17:07 +0100, Tim Deegan wrote:
> > At 15:52 +0000 on 05 Dec (1386255150), 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.
> > 
> > There are other not-present types on x86: PoD, paged-out, emulated-MMIO.  
> 
> This means not Present doesn't imply invalid, I see. Even if we don't
> currently have such types I can see how this makes sense. In theory when
> we get short of bits we could consider the P bit one of the type bits,
> which would give us 16 present and 16 not-present types. Overkill for
> now I expect.
> 
> Is it ever the case that an actual p2m entry contains the p2m_invalid
> bit pattern

Yes, IIRC if an existing entry is removed, it's replaced with an
explicit invalid entry.  It used to be the case that inclid was type 0
so by default all uninitialised entries were invalid too, but ram_rw
had to be type 0 (becasue of an inconvenient interacion with AMD
IOMMUs).  For added confusion on x86 we still have the legacy rule
that invalid entries are reported as mmio_dm, because the default path
is to ask qemu.

Tim.
Ian Campbell Dec. 5, 2013, 4:38 p.m. | #10
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?

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

> > My question wasn't about that though -- just whether it is useful for
> > Xen to track whether the particular RAM mapping is normal or a grant
> > mapping.
> 
> For now, I don't see a specific reason to track it.

OK.
Ian Campbell Dec. 5, 2013, 4:39 p.m. | #11
On Thu, 2013-12-05 at 17:31 +0100, Tim Deegan wrote:
> At 16:19 +0000 on 05 Dec (1386256758), Ian Campbell wrote:
> > On Thu, 2013-12-05 at 17:07 +0100, Tim Deegan wrote:
> > > At 15:52 +0000 on 05 Dec (1386255150), 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.
> > > 
> > > There are other not-present types on x86: PoD, paged-out, emulated-MMIO.  
> > 
> > This means not Present doesn't imply invalid, I see. Even if we don't
> > currently have such types I can see how this makes sense. In theory when
> > we get short of bits we could consider the P bit one of the type bits,
> > which would give us 16 present and 16 not-present types. Overkill for
> > now I expect.
> > 
> > Is it ever the case that an actual p2m entry contains the p2m_invalid
> > bit pattern
> 
> Yes, IIRC if an existing entry is removed, it's replaced with an
> explicit invalid entry.  It used to be the case that inclid was type 0
> so by default all uninitialised entries were invalid too, but ram_rw
> had to be type 0 (becasue of an inconvenient interacion with AMD
> IOMMUs).  For added confusion on x86 we still have the legacy rule
> that invalid entries are reported as mmio_dm, because the default path
> is to ask qemu.

I think we avoid all those pitfalls on arm right now, so probably
invalid == P==0b0,AVAIL=0b0000 makes sense?

Ian.
Julien Grall Dec. 5, 2013, 4:44 p.m. | #12
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.
Ian Campbell Dec. 5, 2013, 4:56 p.m. | #13
On Thu, 2013-12-05 at 16:44 +0000, Julien Grall wrote:
> 
> 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 we should just keep this trick in our pocket for when we run out
of bits then?

Ian.
Tim Deegan Dec. 5, 2013, 5:24 p.m. | #14
B11;rgb:0000/0000/0000At 16:39 +0000 on 05 Dec (1386257979), Ian Campbell wrote:
> On Thu, 2013-12-05 at 17:31 +0100, Tim Deegan wrote:
> > At 16:19 +0000 on 05 Dec (1386256758), Ian Campbell wrote:
> > > On Thu, 2013-12-05 at 17:07 +0100, Tim Deegan wrote:
> > > > At 15:52 +0000 on 05 Dec (1386255150), 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.
> > > > 
> > > > There are other not-present types on x86: PoD, paged-out, emulated-MMIO.  
> > > 
> > > This means not Present doesn't imply invalid, I see. Even if we don't
> > > currently have such types I can see how this makes sense. In theory when
> > > we get short of bits we could consider the P bit one of the type bits,
> > > which would give us 16 present and 16 not-present types. Overkill for
> > > now I expect.
> > > 
> > > Is it ever the case that an actual p2m entry contains the p2m_invalid
> > > bit pattern
> > 
> > Yes, IIRC if an existing entry is removed, it's replaced with an
> > explicit invalid entry.  It used to be the case that inclid was type 0
> > so by default all uninitialised entries were invalid too, but ram_rw
> > had to be type 0 (becasue of an inconvenient interacion with AMD
> > IOMMUs).  For added confusion on x86 we still have the legacy rule
> > that invalid entries are reported as mmio_dm, because the default path
> > is to ask qemu.
> 
> I think we avoid all those pitfalls on arm right now, so probably
> invalid == P==0b0,AVAIL=0b0000 makes sense?

Sure.

Tim.
Julien Grall Dec. 5, 2013, 9:26 p.m. | #15
On 12/05/2013 04:56 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 16:44 +0000, Julien Grall wrote:
>>
>> 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 we should just keep this trick in our pocket for when we run out
> of bits then?

Sounds good to me. I will add a comment in the code with your solution.
Egger, Christoph Dec. 9, 2013, 11:16 a.m. | #16
On 05.12.13 16:56, Ian Campbell wrote:
> On Thu, 2013-12-05 at 16:51 +0100, Egger, Christoph wrote:
>> On 05.12.13 16:42, 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
>>>     - 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
>>>
>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>> ---
>>>  xen/include/asm-arm/p2m.h |   13 ++++++++++---
>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>>> index f079f00..b24f94a 100644
>>> --- a/xen/include/asm-arm/p2m.h
>>> +++ b/xen/include/asm-arm/p2m.h
>>> @@ -20,6 +20,16 @@ struct p2m_domain {
>>>      uint8_t vmid;
>>>  };
>>>  
>>> +typedef enum {
>>> +    p2m_invalid = 0,        /* Nothing mapped here */
>>> +    p2m_ram_rw = 1,         /* Normal read/write guest RAM */
>>> +    p2m_ram_ro = 2,         /* Read-only; writes are silently dropped */
>>> +    p2m_mmio_direct = 3,    /* Read/write mapping of genuine MMIO area */
>>> +    p2m_map_foreign = 4,    /* Ram pages from foreign domain */
>>> +} p2m_type_t;
>>> +
>>> +#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
>>> +
>>
>> Is it possible to merge p2m_type_t with x86 and move into common code?
> 
> Not really, the p2m handling is very different on the two arches, even
> if they look superficially similar at this level.

That does not imply there is no common logic from the algorithm POV.
Do you see a way split the algorithm into architecture-specific and
architecture-independent parts?

> x86 has more types than can fit in the available pte space on ARM for a
> start.
>
> I'd like to keep them separate please.

Ok. Then leave the declaration in the architecture specific part
and make the accessors available for the common code.
In OO-languages this is called an 'interface'.

Christoph
Ian Campbell Dec. 9, 2013, 11:38 a.m. | #17
On Mon, 2013-12-09 at 12:16 +0100, Egger, Christoph wrote:
> On 05.12.13 16:56, Ian Campbell wrote:
> > On Thu, 2013-12-05 at 16:51 +0100, Egger, Christoph wrote:
> >> On 05.12.13 16:42, 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
> >>>     - 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
> >>>
> >>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>> ---
> >>>  xen/include/asm-arm/p2m.h |   13 ++++++++++---
> >>>  1 file changed, 10 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> >>> index f079f00..b24f94a 100644
> >>> --- a/xen/include/asm-arm/p2m.h
> >>> +++ b/xen/include/asm-arm/p2m.h
> >>> @@ -20,6 +20,16 @@ struct p2m_domain {
> >>>      uint8_t vmid;
> >>>  };
> >>>  
> >>> +typedef enum {
> >>> +    p2m_invalid = 0,        /* Nothing mapped here */
> >>> +    p2m_ram_rw = 1,         /* Normal read/write guest RAM */
> >>> +    p2m_ram_ro = 2,         /* Read-only; writes are silently dropped */
> >>> +    p2m_mmio_direct = 3,    /* Read/write mapping of genuine MMIO area */
> >>> +    p2m_map_foreign = 4,    /* Ram pages from foreign domain */
> >>> +} p2m_type_t;
> >>> +
> >>> +#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
> >>> +
> >>
> >> Is it possible to merge p2m_type_t with x86 and move into common code?
> > 
> > Not really, the p2m handling is very different on the two arches, even
> > if they look superficially similar at this level.
> 
> That does not imply there is no common logic from the algorithm POV.
> Do you see a way split the algorithm into architecture-specific and
> architecture-independent parts?

The question was can we move p2m_type_t to common code. This is clearly
not possible because the PTEs in both cases have different number of
available bits, and the x86 case has particular requirements about which
type is represented by pattern 0 (due to some AMD IOMMU behaviour,
judging from the comment)

> > x86 has more types than can fit in the available pte space on ARM for a
> > start.
> >
> > I'd like to keep them separate please.
> 
> Ok. Then leave the declaration in the architecture specific part
> and make the accessors available for the common code.
> In OO-languages this is called an 'interface'.

Thanks, I had never heard of an 'interface'. </sarcasm>.

What do you think we are defining here if it is not an interface?

Ian.

Patch

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index f079f00..b24f94a 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -20,6 +20,16 @@  struct p2m_domain {
     uint8_t vmid;
 };
 
+typedef enum {
+    p2m_invalid = 0,        /* Nothing mapped here */
+    p2m_ram_rw = 1,         /* Normal read/write guest RAM */
+    p2m_ram_ro = 2,         /* Read-only; writes are silently dropped */
+    p2m_mmio_direct = 3,    /* Read/write mapping of genuine MMIO area */
+    p2m_map_foreign = 4,    /* Ram pages from foreign domain */
+} p2m_type_t;
+
+#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
+
 /* Initialise vmid allocator */
 void p2m_vmid_allocator_init(void);
 
@@ -72,7 +82,6 @@  p2m_pod_decrease_reservation(struct domain *d,
                              unsigned int order);
 
 /* Look up a GFN and take a reference count on the backing page. */
-typedef int p2m_type_t;
 typedef unsigned int p2m_query_t;
 #define P2M_ALLOC    (1u<<0)   /* Populate PoD and paged-out entries */
 #define P2M_UNSHARE  (1u<<1)   /* Break CoW sharing */
@@ -110,8 +119,6 @@  static inline int get_page_and_type(struct page_info *page,
     return rc;
 }
 
-#define p2m_is_foreign(_t) (0)
-
 #endif /* _XEN_P2M_H */
 
 /*