| Message ID | 1386258131-755-4-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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 */ > > /* >
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 */ > > /*
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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
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.
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 */ /*
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(-)