diff mbox

[RFC,04/24] ARM: GICv3 ITS: map ITS command buffer

Message ID 20160928182457.12433-5-andre.przywara@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andre Przywara Sept. 28, 2016, 6:24 p.m. UTC
Instead of directly manipulating the tables in memory, an ITS driver
sends commands via a ring buffer to the ITS h/w to create or alter the
LPI mappings.
Allocate memory for that buffer and tell the ITS about it to be able
to send ITS commands.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 xen/arch/arm/gic-its.c        | 25 +++++++++++++++++++++++++
 xen/include/asm-arm/gic-its.h |  1 +
 2 files changed, 26 insertions(+)

Comments

Vijay Kilari Oct. 24, 2016, 2:31 p.m. UTC | #1
On Wed, Sep 28, 2016 at 11:54 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> Instead of directly manipulating the tables in memory, an ITS driver
> sends commands via a ring buffer to the ITS h/w to create or alter the
> LPI mappings.
> Allocate memory for that buffer and tell the ITS about it to be able
> to send ITS commands.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/gic-its.c        | 25 +++++++++++++++++++++++++
>  xen/include/asm-arm/gic-its.h |  1 +
>  2 files changed, 26 insertions(+)
>
> diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c
> index 40238a2..c8a7a7e 100644
> --- a/xen/arch/arm/gic-its.c
> +++ b/xen/arch/arm/gic-its.c
> @@ -18,6 +18,7 @@
>
>  #include <xen/config.h>
>  #include <xen/lib.h>
> +#include <xen/err.h>
>  #include <xen/device_tree.h>
>  #include <xen/libfdt/libfdt.h>
>  #include <asm/p2m.h>
> @@ -56,6 +57,26 @@ static uint64_t encode_phys_addr(paddr_t addr, int page_bits)
>      return ret | (addr & GENMASK(51, 48)) >> (48 - 12);
>  }
>
> +static void *gicv3_map_cbaser(void __iomem *cbasereg)
> +{
> +    uint64_t attr, reg;
> +    void *buffer;
> +
> +    attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
> +    attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
> +    attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT;
> +
> +    buffer = alloc_xenheap_pages(0, 0);
> +    if ( !buffer )
> +        return ERR_PTR(-ENOMEM);
> +
> +    /* We use exactly one 4K page, so the "Size" field is 0. */
> +    reg = attr | BIT(63) | (virt_to_maddr(buffer) & GENMASK(51, 12));

Isn't GENMASK(47, 12)?. Am I referring to wrong spec?
Stefano Stabellini Oct. 26, 2016, 11:03 p.m. UTC | #2
On Wed, 28 Sep 2016, Andre Przywara wrote:
> Instead of directly manipulating the tables in memory, an ITS driver
> sends commands via a ring buffer to the ITS h/w to create or alter the
> LPI mappings.
> Allocate memory for that buffer and tell the ITS about it to be able
> to send ITS commands.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/gic-its.c        | 25 +++++++++++++++++++++++++
>  xen/include/asm-arm/gic-its.h |  1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c
> index 40238a2..c8a7a7e 100644
> --- a/xen/arch/arm/gic-its.c
> +++ b/xen/arch/arm/gic-its.c
> @@ -18,6 +18,7 @@
>  
>  #include <xen/config.h>
>  #include <xen/lib.h>
> +#include <xen/err.h>
>  #include <xen/device_tree.h>
>  #include <xen/libfdt/libfdt.h>
>  #include <asm/p2m.h>
> @@ -56,6 +57,26 @@ static uint64_t encode_phys_addr(paddr_t addr, int page_bits)
>      return ret | (addr & GENMASK(51, 48)) >> (48 - 12);
>  }
>  
> +static void *gicv3_map_cbaser(void __iomem *cbasereg)

Shouldn't it be its_map_cbaser?


> +{
> +    uint64_t attr, reg;
> +    void *buffer;
> +
> +    attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
> +    attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
> +    attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT;
> +
> +    buffer = alloc_xenheap_pages(0, 0);
> +    if ( !buffer )
> +        return ERR_PTR(-ENOMEM);

We haven't use much ERR_PTR on arm so far. In this case I'd just return
NULL.


> +
> +    /* We use exactly one 4K page, so the "Size" field is 0. */
> +    reg = attr | BIT(63) | (virt_to_maddr(buffer) & GENMASK(51, 12));

Shouldn't the mask be GENMASK(47, 12)? Maybe I have an old spec
version.


> +    writeq_relaxed(reg, cbasereg);
> +
> +    return buffer;
> +}
> +
>  static int gicv3_map_baser(void __iomem *basereg, uint64_t regc, int nr_items)
>  {
>      uint64_t attr;
> @@ -149,6 +170,10 @@ int gicv3_its_init(struct host_its *hw_its)
>          }
>      }
>  
> +    hw_its->cmd_buf = gicv3_map_cbaser(hw_its->its_base + GITS_CBASER);
> +    if ( IS_ERR(hw_its->cmd_buf) )
> +        return PTR_ERR(hw_its->cmd_buf);
> +
>      return 0;
>  }
>  
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 589b889..b2a003f 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -69,6 +69,7 @@ struct host_its {
>      paddr_t addr;
>      paddr_t size;
>      void __iomem *its_base;
> +    void *cmd_buf;
>  };
>  
>  extern struct list_head host_its_list;
> -- 
> 2.9.0
>
Julien Grall Nov. 2, 2016, 1:38 p.m. UTC | #3
Hello Andre,

On 28/09/16 19:24, Andre Przywara wrote:
> Instead of directly manipulating the tables in memory, an ITS driver
> sends commands via a ring buffer to the ITS h/w to create or alter the
> LPI mappings.
> Allocate memory for that buffer and tell the ITS about it to be able
> to send ITS commands.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/gic-its.c        | 25 +++++++++++++++++++++++++
>  xen/include/asm-arm/gic-its.h |  1 +
>  2 files changed, 26 insertions(+)
>
> diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c
> index 40238a2..c8a7a7e 100644
> --- a/xen/arch/arm/gic-its.c
> +++ b/xen/arch/arm/gic-its.c
> @@ -18,6 +18,7 @@
>
>  #include <xen/config.h>
>  #include <xen/lib.h>
> +#include <xen/err.h>
>  #include <xen/device_tree.h>
>  #include <xen/libfdt/libfdt.h>
>  #include <asm/p2m.h>
> @@ -56,6 +57,26 @@ static uint64_t encode_phys_addr(paddr_t addr, int page_bits)
>      return ret | (addr & GENMASK(51, 48)) >> (48 - 12);
>  }
>
> +static void *gicv3_map_cbaser(void __iomem *cbasereg)
> +{
> +    uint64_t attr, reg;
> +    void *buffer;
> +
> +    attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
> +    attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
> +    attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT;
> +

You could directly use 'reg' here rather than have a temporary variable.

Also what if the shareability/cacheability has been fixed by the hardware?

> +    buffer = alloc_xenheap_pages(0, 0);

Please document how you decide to use only a 4K page (is there 
potentially a drawback)? Also I would prefer if you add a define for the 
size of the command queue. This will be more readable.

> +    if ( !buffer )
> +        return ERR_PTR(-ENOMEM);
> +
> +    /* We use exactly one 4K page, so the "Size" field is 0. */
> +    reg = attr | BIT(63) | (virt_to_maddr(buffer) & GENMASK(51, 12));

Please introduce a define for bit 63. Also masking the address is not 
useful.

> +    writeq_relaxed(reg, cbasereg);

Should not we initialize GITS_CWRITER to 0? From the spec the field is 
reset to an UNKNOWN value (see 8.19.5)?

> +
> +    return buffer;
> +}
> +
>  static int gicv3_map_baser(void __iomem *basereg, uint64_t regc, int nr_items)
>  {
>      uint64_t attr;
> @@ -149,6 +170,10 @@ int gicv3_its_init(struct host_its *hw_its)
>          }
>      }
>
> +    hw_its->cmd_buf = gicv3_map_cbaser(hw_its->its_base + GITS_CBASER);
> +    if ( IS_ERR(hw_its->cmd_buf) )
> +        return PTR_ERR(hw_its->cmd_buf);
> +
>      return 0;
>  }
>
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 589b889..b2a003f 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -69,6 +69,7 @@ struct host_its {
>      paddr_t addr;
>      paddr_t size;
>      void __iomem *its_base;
> +    void *cmd_buf;
>  };
>
>  extern struct list_head host_its_list;
>

Regards,
Andre Przywara Nov. 10, 2016, 4:04 p.m. UTC | #4
Hi,

On 27/10/16 00:03, Stefano Stabellini wrote:
> On Wed, 28 Sep 2016, Andre Przywara wrote:
>> Instead of directly manipulating the tables in memory, an ITS driver
>> sends commands via a ring buffer to the ITS h/w to create or alter the
>> LPI mappings.
>> Allocate memory for that buffer and tell the ITS about it to be able
>> to send ITS commands.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  xen/arch/arm/gic-its.c        | 25 +++++++++++++++++++++++++
>>  xen/include/asm-arm/gic-its.h |  1 +
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c
>> index 40238a2..c8a7a7e 100644
>> --- a/xen/arch/arm/gic-its.c
>> +++ b/xen/arch/arm/gic-its.c
>> @@ -18,6 +18,7 @@
>>  
>>  #include <xen/config.h>
>>  #include <xen/lib.h>
>> +#include <xen/err.h>
>>  #include <xen/device_tree.h>
>>  #include <xen/libfdt/libfdt.h>
>>  #include <asm/p2m.h>
>> @@ -56,6 +57,26 @@ static uint64_t encode_phys_addr(paddr_t addr, int page_bits)
>>      return ret | (addr & GENMASK(51, 48)) >> (48 - 12);
>>  }
>>  
>> +static void *gicv3_map_cbaser(void __iomem *cbasereg)
> 
> Shouldn't it be its_map_cbaser?

Yes.

>> +{
>> +    uint64_t attr, reg;
>> +    void *buffer;
>> +
>> +    attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
>> +    attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
>> +    attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT;
>> +
>> +    buffer = alloc_xenheap_pages(0, 0);
>> +    if ( !buffer )
>> +        return ERR_PTR(-ENOMEM);
> 
> We haven't use much ERR_PTR on arm so far. In this case I'd just return
> NULL.

In this case I agree, though "we haven't uses it much" isn't really a
good argument ;-)

Cheers,
Andre.

>> +
>> +    /* We use exactly one 4K page, so the "Size" field is 0. */
>> +    reg = attr | BIT(63) | (virt_to_maddr(buffer) & GENMASK(51, 12));
> 
> Shouldn't the mask be GENMASK(47, 12)? Maybe I have an old spec
> version.
> 
> 
>> +    writeq_relaxed(reg, cbasereg);
>> +
>> +    return buffer;
>> +}
>> +
>>  static int gicv3_map_baser(void __iomem *basereg, uint64_t regc, int nr_items)
>>  {
>>      uint64_t attr;
>> @@ -149,6 +170,10 @@ int gicv3_its_init(struct host_its *hw_its)
>>          }
>>      }
>>  
>> +    hw_its->cmd_buf = gicv3_map_cbaser(hw_its->its_base + GITS_CBASER);
>> +    if ( IS_ERR(hw_its->cmd_buf) )
>> +        return PTR_ERR(hw_its->cmd_buf);
>> +
>>      return 0;
>>  }
>>  
>> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
>> index 589b889..b2a003f 100644
>> --- a/xen/include/asm-arm/gic-its.h
>> +++ b/xen/include/asm-arm/gic-its.h
>> @@ -69,6 +69,7 @@ struct host_its {
>>      paddr_t addr;
>>      paddr_t size;
>>      void __iomem *its_base;
>> +    void *cmd_buf;
>>  };
>>  
>>  extern struct list_head host_its_list;
>> -- 
>> 2.9.0
>>
>
diff mbox

Patch

diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c
index 40238a2..c8a7a7e 100644
--- a/xen/arch/arm/gic-its.c
+++ b/xen/arch/arm/gic-its.c
@@ -18,6 +18,7 @@ 
 
 #include <xen/config.h>
 #include <xen/lib.h>
+#include <xen/err.h>
 #include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.h>
 #include <asm/p2m.h>
@@ -56,6 +57,26 @@  static uint64_t encode_phys_addr(paddr_t addr, int page_bits)
     return ret | (addr & GENMASK(51, 48)) >> (48 - 12);
 }
 
+static void *gicv3_map_cbaser(void __iomem *cbasereg)
+{
+    uint64_t attr, reg;
+    void *buffer;
+
+    attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
+    attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
+    attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT;
+
+    buffer = alloc_xenheap_pages(0, 0);
+    if ( !buffer )
+        return ERR_PTR(-ENOMEM);
+
+    /* We use exactly one 4K page, so the "Size" field is 0. */
+    reg = attr | BIT(63) | (virt_to_maddr(buffer) & GENMASK(51, 12));
+    writeq_relaxed(reg, cbasereg);
+
+    return buffer;
+}
+
 static int gicv3_map_baser(void __iomem *basereg, uint64_t regc, int nr_items)
 {
     uint64_t attr;
@@ -149,6 +170,10 @@  int gicv3_its_init(struct host_its *hw_its)
         }
     }
 
+    hw_its->cmd_buf = gicv3_map_cbaser(hw_its->its_base + GITS_CBASER);
+    if ( IS_ERR(hw_its->cmd_buf) )
+        return PTR_ERR(hw_its->cmd_buf);
+
     return 0;
 }
 
diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
index 589b889..b2a003f 100644
--- a/xen/include/asm-arm/gic-its.h
+++ b/xen/include/asm-arm/gic-its.h
@@ -69,6 +69,7 @@  struct host_its {
     paddr_t addr;
     paddr_t size;
     void __iomem *its_base;
+    void *cmd_buf;
 };
 
 extern struct list_head host_its_list;