Re: [Xen-devel] [PATCH v3 1/6] xen/arm: p2m: Add support for normal non-cacheable memory

[Julien Grall <julien.grall@xxxxxxx>]

Hi Edgar,

On 07/09/2016 08:56, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxxx>
>
> Add support for describing normal non-cacheable memory.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
> ---
>  xen/arch/arm/p2m.c         | 18 ++++++++++++++++++
>  xen/include/asm-arm/p2m.h  |  1 +
>  xen/include/asm-arm/page.h |  1 +
>  3 files changed, 20 insertions(+)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index b648a9d..bfef77b 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -282,6 +282,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, 
> p2m_access_t a)
>      /* First apply type permissions */
>      switch ( t )
>      {
> +    case p2m_mem_nc:
>      case p2m_ram_rw:
>          e->p2m.xn = 0;
>          e->p2m.write = 1;
> @@ -376,6 +377,23 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, 
> p2m_access_t a)
>          e.p2m.sh = LPAE_SH_OUTER;
>          break;
>
> +    /*
> +     * ARM ARM: Overlaying the shareability attribute (DDI
> +     * 0406C.b B3-1376 to 1377)
> +     *
> +     * A memory region with a resultant memory type attribute of Normal,
> +     * and a resultant cacheability attribute of Inner Non-cacheable,
> +     * Outer Non-cacheable, must have a resultant shareability attribute
> +     * of Outer Shareable, otherwise shareability is UNPREDICTABLE.
> +     *
> +     * On ARMv8 sharability is ignored and explicitly treated as Outer
I know you copied it from mfn_to_xen_entry, but can we fixed the copy with:

s/sharability/shareability/

> +     * Shareable for Normal Inner Non_cacheable, Outer Non-cacheable.
s/_/-/

Also I would like to see a spec reference for the ARMv8. I think it is 
the note in D4-1788 ARM DDI 0487A.j.
> +     */
> +    case p2m_mem_nc:
> +        e.p2m.mattr = MATTR_MEM_NC;
> +        e.p2m.sh = LPAE_SH_OUTER;
> +        break;
> +
>      default:
>          e.p2m.mattr = MATTR_MEM;
>          e.p2m.sh = LPAE_SH_INNER;
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 53c4d78..b012d50 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -93,6 +93,7 @@ typedef enum {
>      p2m_ram_ro,         /* Read-only; writes are silently dropped */
>      p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area 
> non-cacheable */
>      p2m_mmio_direct_c,  /* Read/write mapping of genuine MMIO area cacheable */
> +    p2m_mem_nc,         /* Read/write mapping of Non-cacheable Memory */
I find the name a bit confusing. Technically p2m_mem_nc is 
p2m_mmio_direct_c version but non-cacheable.
I have got the feeling that the naming I used on a recent patch is not 
correct. Because p2m_mmio_direct_nc is not doing what is expect (i.e 
mapping non-cacheable). It maps with the device memory attribute.
Maybe we should rename p2m_mmio_direct_nc to p2m_mmio_direct_dev 
(because it will use the device memory attribute) and then use 
p2m_mmio_direct_nc for your purpose.
Any opinions?


>      p2m_map_foreign,    /* Ram pages from foreign domain */
>      p2m_grant_map_rw,   /* Read/write grant mapping */
>      p2m_grant_map_ro,   /* Read-only grant mapping */
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 05d9f82..9adc973 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -73,6 +73,7 @@
>   *
>   */
>  #define MATTR_DEV     0x1
> +#define MATTR_MEM_NC  0x5
>  #define MATTR_MEM     0xf
>
>  /* Flags for get_page_from_gva, gvirt_to_maddr etc */
Regards,

--
Julien Grall

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