Re: [Xen-devel] [PATCH 07/28] ARM: GICv3 ITS: introduce device mapping

[Shanker Donthineni]

Hi Andre,


On 02/22/2017 01:06 AM, Vijay Kilari wrote:

Hi Andre,

On Tue, Jan 31, 2017 at 12:01 AM, Andre Przywara  wrote:

The ITS uses device IDs to map LPIs to a device. Dom0 will later use
those IDs, which we directly pass on to the host.
For this we have to map each device that Dom0 may request to a host
ITS device with the same identifier.
Allocate the respective memory and enter each device into an rbtree to
later be able to iterate over it or to easily teardown guests.

Signed-off-by: Andre Przywara 
---
  xen/arch/arm/gic-v3-its.c| 188 ++-
  xen/arch/arm/vgic-v3.c   |   3 +
  xen/include/asm-arm/domain.h |   3 +
  xen/include/asm-arm/gic_v3_its.h |  28 ++
  4 files changed, 221 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 6578e8a..4a3a394 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -21,8 +21,10 @@
  #include 
  #include 
  #include 
-#include 
+#include 
+#include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -94,6 +96,21 @@ static int its_send_cmd_mapc(struct host_its *its, int 
collection_id, int cpu)
  return its_send_command(its, cmd);
  }

+static int its_send_cmd_mapd(struct host_its *its, uint32_t deviceid,
+ int size, uint64_t itt_addr, bool valid)
+{
+uint64_t cmd[4];
+
+cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32);
+cmd[1] = size & GENMASK(4, 0);
+cmd[2] = itt_addr & GENMASK(51, 8);
+if ( valid )
+cmd[2] |= GITS_VALID_BIT;
+cmd[3] = 0x00;
+
+return its_send_command(its, cmd);
+}
+
  /* Set up the (1:1) collection mapping for the given host CPU. */
  int gicv3_its_setup_collection(int cpu)
  {
@@ -293,6 +310,7 @@ int gicv3_its_init(struct host_its *hw_its)
  reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
  if ( reg & GITS_TYPER_PTA )
  hw_its->flags |= HOST_ITS_USES_PTA;
+hw_its->itte_size = GITS_TYPER_ITT_SIZE(reg);

  for ( i = 0; i < GITS_BASER_NR_REGS; i++ )
  {
@@ -339,6 +357,173 @@ int gicv3_its_init(struct host_its *hw_its)
  return 0;
  }

+static void remove_mapped_guest_device(struct its_devices *dev)
+{
+if ( dev->hw_its )
+its_send_cmd_mapd(dev->hw_its, dev->host_devid, 0, 0, false);
+
+xfree(dev->itt_addr);
+xfree(dev);
+}
+
+int gicv3_its_map_guest_device(struct domain *d, int host_devid,
+   int guest_devid, int bits, bool valid)
+{
+void *itt_addr = NULL;
+struct its_devices *dev, *temp;
+struct rb_node **new = >arch.vgic.its_devices.rb_node, *parent = NULL;
+struct host_its *hw_its;
+int ret;
+
+/* check for already existing mappings */
+spin_lock(>arch.vgic.its_devices_lock);
+while (*new)
+{
+temp = rb_entry(*new, struct its_devices, rbnode);
+
+if ( temp->guest_devid == guest_devid )
+{
+if ( !valid )
+rb_erase(>rbnode, >arch.vgic.its_devices);
+
+spin_unlock(>arch.vgic.its_devices_lock);
+
+if ( valid )
+return -EBUSY;
+
+remove_mapped_guest_device(temp);
+
+return 0;
+}
+
+if ( guest_devid < temp->guest_devid )
+new = &((*new)->rb_right);

Shouldn't be rb_left here?

+else
+new = &((*new)->rb_left);

Shouldn't be rb_right here?



I found the same thing when I tested ITS feature on Qualcomm platform.


+}

I see duplicate code in find and inserting node into rb_tree.


+
+if ( !valid )
+{
+ret = -ENOENT;
+goto out_unlock;
+}
+
+/*
+ * TODO: Work out the correct hardware ITS to use here.
+ * Maybe a per-platform function: devid -> ITS?
+ * Or parsing the DT to find the msi_parent?
+ * Or get Dom0 to give us this information?
+ * For now just use the first ITS.
+ */
+hw_its = list_first_entry(_its_list, struct host_its, entry);
+
+ret = -ENOMEM;
+
+itt_addr = _xmalloc(BIT(bits) * hw_its->itte_size, 256);


Please use _xzalloc() to avoid potential issues with ITS hardware 
prefetching feature.


--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.


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

Re: [Xen-devel] [PATCH 07/28] ARM: GICv3 ITS: introduce device mapping

[Julien Grall]

Hi Andre,

On 30/01/17 18:31, Andre Przywara wrote:

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 6578e8a..4a3a394 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c


[...]



+
+int gicv3_its_map_guest_device(struct domain *d, int host_devid,
+   int guest_devid, int bits, bool valid)


Whilst looking at the IORT table it occurred to me that the DeviceID may 
not be uniq accross all the ITSes on the platform.


This means 2 ITS could use the same DeviceID for different devices. 
However, this function assume the DeviceID will always be uniq.


So we would need to know specify the pITS and vITS for all PCI devices.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 07/28] ARM: GICv3 ITS: introduce device mapping

[Vijay Kilari]

Hi Andre,

On Tue, Jan 31, 2017 at 12:01 AM, Andre Przywara  wrote:
> The ITS uses device IDs to map LPIs to a device. Dom0 will later use
> those IDs, which we directly pass on to the host.
> For this we have to map each device that Dom0 may request to a host
> ITS device with the same identifier.
> Allocate the respective memory and enter each device into an rbtree to
> later be able to iterate over it or to easily teardown guests.
>
> Signed-off-by: Andre Przywara 
> ---
>  xen/arch/arm/gic-v3-its.c| 188 
> ++-
>  xen/arch/arm/vgic-v3.c   |   3 +
>  xen/include/asm-arm/domain.h |   3 +
>  xen/include/asm-arm/gic_v3_its.h |  28 ++
>  4 files changed, 221 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 6578e8a..4a3a394 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -21,8 +21,10 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -94,6 +96,21 @@ static int its_send_cmd_mapc(struct host_its *its, int 
> collection_id, int cpu)
>  return its_send_command(its, cmd);
>  }
>
> +static int its_send_cmd_mapd(struct host_its *its, uint32_t deviceid,
> + int size, uint64_t itt_addr, bool valid)
> +{
> +uint64_t cmd[4];
> +
> +cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32);
> +cmd[1] = size & GENMASK(4, 0);
> +cmd[2] = itt_addr & GENMASK(51, 8);
> +if ( valid )
> +cmd[2] |= GITS_VALID_BIT;
> +cmd[3] = 0x00;
> +
> +return its_send_command(its, cmd);
> +}
> +
>  /* Set up the (1:1) collection mapping for the given host CPU. */
>  int gicv3_its_setup_collection(int cpu)
>  {
> @@ -293,6 +310,7 @@ int gicv3_its_init(struct host_its *hw_its)
>  reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
>  if ( reg & GITS_TYPER_PTA )
>  hw_its->flags |= HOST_ITS_USES_PTA;
> +hw_its->itte_size = GITS_TYPER_ITT_SIZE(reg);
>
>  for ( i = 0; i < GITS_BASER_NR_REGS; i++ )
>  {
> @@ -339,6 +357,173 @@ int gicv3_its_init(struct host_its *hw_its)
>  return 0;
>  }
>
> +static void remove_mapped_guest_device(struct its_devices *dev)
> +{
> +if ( dev->hw_its )
> +its_send_cmd_mapd(dev->hw_its, dev->host_devid, 0, 0, false);
> +
> +xfree(dev->itt_addr);
> +xfree(dev);
> +}
> +
> +int gicv3_its_map_guest_device(struct domain *d, int host_devid,
> +   int guest_devid, int bits, bool valid)
> +{
> +void *itt_addr = NULL;
> +struct its_devices *dev, *temp;
> +struct rb_node **new = >arch.vgic.its_devices.rb_node, *parent = NULL;
> +struct host_its *hw_its;
> +int ret;
> +
> +/* check for already existing mappings */
> +spin_lock(>arch.vgic.its_devices_lock);
> +while (*new)
> +{
> +temp = rb_entry(*new, struct its_devices, rbnode);
> +
> +if ( temp->guest_devid == guest_devid )
> +{
> +if ( !valid )
> +rb_erase(>rbnode, >arch.vgic.its_devices);
> +
> +spin_unlock(>arch.vgic.its_devices_lock);
> +
> +if ( valid )
> +return -EBUSY;
> +
> +remove_mapped_guest_device(temp);
> +
> +return 0;
> +}
> +
> +if ( guest_devid < temp->guest_devid )
> +new = &((*new)->rb_right);

Shouldn't be rb_left here?
> +else
> +new = &((*new)->rb_left);
Shouldn't be rb_right here?

> +}

I see duplicate code in find and inserting node into rb_tree.

> +
> +if ( !valid )
> +{
> +ret = -ENOENT;
> +goto out_unlock;
> +}
> +
> +/*
> + * TODO: Work out the correct hardware ITS to use here.
> + * Maybe a per-platform function: devid -> ITS?
> + * Or parsing the DT to find the msi_parent?
> + * Or get Dom0 to give us this information?
> + * For now just use the first ITS.
> + */
> +hw_its = list_first_entry(_its_list, struct host_its, entry);
> +
> +ret = -ENOMEM;
> +
> +itt_addr = _xmalloc(BIT(bits) * hw_its->itte_size, 256);
> +if ( !itt_addr )
> +goto out_unlock;
> +
> +dev = xmalloc(struct its_devices);
> +if ( !dev )
> +{
> +xfree(itt_addr);
> +goto out_unlock;
> +}
> +
> +ret = its_send_cmd_mapd(hw_its, host_devid, bits - 1,
> +virt_to_maddr(itt_addr), true);
> +if ( ret )
> +{
> +xfree(itt_addr);
> +xfree(dev);
> +goto out_unlock;
> +}
> +
> +dev->itt_addr = itt_addr;
> +dev->hw_its = hw_its;
> +dev->guest_devid = guest_devid;
> +dev->host_devid = host_devid;
> +dev->eventids = BIT(bits);
> +
> +rb_link_node(>rbnode, parent, new);

parent is not updated before inserting. So whole tree is not
managed properly.

> +

Re: [Xen-devel] [PATCH 07/28] ARM: GICv3 ITS: introduce device mapping

[Julien Grall]

Hi Andre,

On 30/01/17 18:31, Andre Przywara wrote:

+static int its_send_cmd_mapd(struct host_its *its, uint32_t deviceid,
+ int size, uint64_t itt_addr, bool valid)
+{
+uint64_t cmd[4];
+
+cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32);
+cmd[1] = size & GENMASK(4, 0);
+cmd[2] = itt_addr & GENMASK(51, 8);
+if ( valid )
+cmd[2] |= GITS_VALID_BIT;
+cmd[3] = 0x00;
+
+return its_send_command(its, cmd);
+}
+
 /* Set up the (1:1) collection mapping for the given host CPU. */
 int gicv3_its_setup_collection(int cpu)
 {
@@ -293,6 +310,7 @@ int gicv3_its_init(struct host_its *hw_its)
 reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
 if ( reg & GITS_TYPER_PTA )
 hw_its->flags |= HOST_ITS_USES_PTA;
+hw_its->itte_size = GITS_TYPER_ITT_SIZE(reg);


The GITS_TYPER.ITT_entry_size indicates the number of bytes minus one. 
So you would have to add a + 1.


I would add it in the GITS_TYPER_ITT_SIZE macro to simplify it.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 07/28] ARM: GICv3 ITS: introduce device mapping

[Julien Grall]

Hi Andre,

On 30/01/2017 18:31, Andre Przywara wrote:

The ITS uses device IDs to map LPIs to a device. Dom0 will later use
those IDs, which we directly pass on to the host.
For this we have to map each device that Dom0 may request to a host
ITS device with the same identifier.
Allocate the respective memory and enter each device into an rbtree to
later be able to iterate over it or to easily teardown guests.

Signed-off-by: Andre Przywara 
---
 xen/arch/arm/gic-v3-its.c| 188 ++-
 xen/arch/arm/vgic-v3.c   |   3 +
 xen/include/asm-arm/domain.h |   3 +
 xen/include/asm-arm/gic_v3_its.h |  28 ++
 4 files changed, 221 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 6578e8a..4a3a394 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -21,8 +21,10 @@
 #include 
 #include 
 #include 
-#include 


Why did you drop the include xen/mm.h?


+#include 
+#include 
 #include 
+#include 


All the header looks to be included in an alphabetical order except this 
one. Why?



 #include 
 #include 
 #include 
@@ -94,6 +96,21 @@ static int its_send_cmd_mapc(struct host_its *its, int 
collection_id, int cpu)
 return its_send_command(its, cmd);
 }

+static int its_send_cmd_mapd(struct host_its *its, uint32_t deviceid,
+ int size, uint64_t itt_addr, bool valid)


Please use unsigned for the size. Also it could be uint8_t.

Also, itt_addr should technically be a paddr_t.


+{
+uint64_t cmd[4];
+
+cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32);
+cmd[1] = size & GENMASK(4, 0);


The mask here and the one below are not necessary and could hide a 
programming error.


It would make much sense to have an ASSERT(size < GITS_TYPER.IDbits) 
here and someone checking that the value is correct.



+cmd[2] = itt_addr & GENMASK(51, 8);


For this one, we only want to make sure the bits 7:0 are all zeroes as 
the address is provided by the caller. So I would replace the mask by an 
ASSERT(!(its_addr & GENMASK(7, 0)).



+if ( valid )
+cmd[2] |= GITS_VALID_BIT;
+cmd[3] = 0x00;
+
+return its_send_command(its, cmd);
+}
+
 /* Set up the (1:1) collection mapping for the given host CPU. */
 int gicv3_its_setup_collection(int cpu)
 {
@@ -293,6 +310,7 @@ int gicv3_its_init(struct host_its *hw_its)
 reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
 if ( reg & GITS_TYPER_PTA )
 hw_its->flags |= HOST_ITS_USES_PTA;
+hw_its->itte_size = GITS_TYPER_ITT_SIZE(reg);

 for ( i = 0; i < GITS_BASER_NR_REGS; i++ )
 {
@@ -339,6 +357,173 @@ int gicv3_its_init(struct host_its *hw_its)
 return 0;
 }

+static void remove_mapped_guest_device(struct its_devices *dev)
+{
+if ( dev->hw_its )
+its_send_cmd_mapd(dev->hw_its, dev->host_devid, 0, 0, false);


I would propagate the return of the ITS command as it may have failed 
because, for instance, the command queue is full.



+
+xfree(dev->itt_addr);
+xfree(dev);
+}
+
+int gicv3_its_map_guest_device(struct domain *d, int host_devid,
+   int guest_devid, int bits, bool valid)



Please use uint32_t for the host_devid and guest_devid.

Also, what is bits? It looks like to me this is the number of bit 
supported for the event. It think it would be clearer to pass the number 
of events and compute the number of bits within the function.


Furthermore, looking at the code, I think it would be better to have two 
separate functions: one to add a device, the other to remove. Overall 
the code looks quite different for both and some parameter are not useful.


Lastly, I don't see any code here which prevent a device to be assigned 
to another domain or even wrong host/guest DeviceID and wrong number of 
events bits. Who will do that?


For checking if the device has been assigned to another, I think this 
will be done could be done outside.


In any case, as requested by Stefano on the previous version, a TODO in 
the code will be needed to avoid forgetting.



+{
+void *itt_addr = NULL;
+struct its_devices *dev, *temp;
+struct rb_node **new = >arch.vgic.its_devices.rb_node, *parent = NULL;
+struct host_its *hw_its;
+int ret;
+
+/* check for already existing mappings */
+spin_lock(>arch.vgic.its_devices_lock);
+while (*new)


Coding style: while ( *new ).


+{
+temp = rb_entry(*new, struct its_devices, rbnode);
+
+if ( temp->guest_devid == guest_devid )
+{
+if ( !valid )
+rb_erase(>rbnode, >arch.vgic.its_devices);
+
+spin_unlock(>arch.vgic.its_devices_lock);
+
+if ( valid )


A printk(XENLOG_GUEST...) here would be useful to know which host 
DeviceID was associated to the guest deviceID.



+return -EBUSY;
+
+remove_mapped_guest_device(temp);


See my comment on the function 

[Xen-devel] [PATCH 07/28] ARM: GICv3 ITS: introduce device mapping

[Andre Przywara]

The ITS uses device IDs to map LPIs to a device. Dom0 will later use
those IDs, which we directly pass on to the host.
For this we have to map each device that Dom0 may request to a host
ITS device with the same identifier.
Allocate the respective memory and enter each device into an rbtree to
later be able to iterate over it or to easily teardown guests.

Signed-off-by: Andre Przywara 
---
 xen/arch/arm/gic-v3-its.c| 188 ++-
 xen/arch/arm/vgic-v3.c   |   3 +
 xen/include/asm-arm/domain.h |   3 +
 xen/include/asm-arm/gic_v3_its.h |  28 ++
 4 files changed, 221 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 6578e8a..4a3a394 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -21,8 +21,10 @@
 #include 
 #include 
 #include 
-#include 
+#include 
+#include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -94,6 +96,21 @@ static int its_send_cmd_mapc(struct host_its *its, int 
collection_id, int cpu)
 return its_send_command(its, cmd);
 }
 
+static int its_send_cmd_mapd(struct host_its *its, uint32_t deviceid,
+ int size, uint64_t itt_addr, bool valid)
+{
+uint64_t cmd[4];
+
+cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32);
+cmd[1] = size & GENMASK(4, 0);
+cmd[2] = itt_addr & GENMASK(51, 8);
+if ( valid )
+cmd[2] |= GITS_VALID_BIT;
+cmd[3] = 0x00;
+
+return its_send_command(its, cmd);
+}
+
 /* Set up the (1:1) collection mapping for the given host CPU. */
 int gicv3_its_setup_collection(int cpu)
 {
@@ -293,6 +310,7 @@ int gicv3_its_init(struct host_its *hw_its)
 reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
 if ( reg & GITS_TYPER_PTA )
 hw_its->flags |= HOST_ITS_USES_PTA;
+hw_its->itte_size = GITS_TYPER_ITT_SIZE(reg);
 
 for ( i = 0; i < GITS_BASER_NR_REGS; i++ )
 {
@@ -339,6 +357,173 @@ int gicv3_its_init(struct host_its *hw_its)
 return 0;
 }
 
+static void remove_mapped_guest_device(struct its_devices *dev)
+{
+if ( dev->hw_its )
+its_send_cmd_mapd(dev->hw_its, dev->host_devid, 0, 0, false);
+
+xfree(dev->itt_addr);
+xfree(dev);
+}
+
+int gicv3_its_map_guest_device(struct domain *d, int host_devid,
+   int guest_devid, int bits, bool valid)
+{
+void *itt_addr = NULL;
+struct its_devices *dev, *temp;
+struct rb_node **new = >arch.vgic.its_devices.rb_node, *parent = NULL;
+struct host_its *hw_its;
+int ret;
+
+/* check for already existing mappings */
+spin_lock(>arch.vgic.its_devices_lock);
+while (*new)
+{
+temp = rb_entry(*new, struct its_devices, rbnode);
+
+if ( temp->guest_devid == guest_devid )
+{
+if ( !valid )
+rb_erase(>rbnode, >arch.vgic.its_devices);
+
+spin_unlock(>arch.vgic.its_devices_lock);
+
+if ( valid )
+return -EBUSY;
+
+remove_mapped_guest_device(temp);
+
+return 0;
+}
+
+if ( guest_devid < temp->guest_devid )
+new = &((*new)->rb_right);
+else
+new = &((*new)->rb_left);
+}
+
+if ( !valid )
+{
+ret = -ENOENT;
+goto out_unlock;
+}
+
+/*
+ * TODO: Work out the correct hardware ITS to use here.
+ * Maybe a per-platform function: devid -> ITS?
+ * Or parsing the DT to find the msi_parent?
+ * Or get Dom0 to give us this information?
+ * For now just use the first ITS.
+ */
+hw_its = list_first_entry(_its_list, struct host_its, entry);
+
+ret = -ENOMEM;
+
+itt_addr = _xmalloc(BIT(bits) * hw_its->itte_size, 256);
+if ( !itt_addr )
+goto out_unlock;
+
+dev = xmalloc(struct its_devices);
+if ( !dev )
+{
+xfree(itt_addr);
+goto out_unlock;
+}
+
+ret = its_send_cmd_mapd(hw_its, host_devid, bits - 1,
+virt_to_maddr(itt_addr), true);
+if ( ret )
+{
+xfree(itt_addr);
+xfree(dev);
+goto out_unlock;
+}
+
+dev->itt_addr = itt_addr;
+dev->hw_its = hw_its;
+dev->guest_devid = guest_devid;
+dev->host_devid = host_devid;
+dev->eventids = BIT(bits);
+
+rb_link_node(>rbnode, parent, new);
+rb_insert_color(>rbnode, >arch.vgic.its_devices);
+
+spin_unlock(>arch.vgic.its_devices_lock);
+
+return 0;
+
+out_unlock:
+spin_unlock(>arch.vgic.its_devices_lock);
+return ret;
+}
+
+/* Removing any connections a domain had to any ITS in the system. */
+void gicv3_its_unmap_all_devices(struct domain *d)
+{
+struct rb_node *victim;
+struct its_devices *dev;
+
+/*
+ * This is an easily readable, yet inefficient implementation.
+ * It uses the provided iteration wrapper and erases each node, which
+ * possibly triggers rebalancing.
+ * This seems overkill