Re: [Xen-devel] [PATCH 12/12] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq

2017-10-26 Thread Julien Grall 

Hi Andre,

On 10/19/2017 01:48 PM, Andre Przywara wrote:

The functions to actually populate a list register were accessing
the VGIC internal pending_irq struct, although they should be abstracting
from that.
Break the needed information down to remove the reference to pending_irq
from gic-v[23].c.

Signed-off-by: Andre Przywara 
---
  xen/arch/arm/gic-v2.c | 14 +++---
  xen/arch/arm/gic-v3.c | 12 ++--
  xen/arch/arm/gic-vgic.c   |  3 ++-
  xen/include/asm-arm/gic.h |  4 ++--
  4 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 511c8d7294..e5acff8900 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -428,8 +428,8 @@ static void gicv2_disable_interface(void)
  spin_unlock();
  }
  
-static void gicv2_update_lr(int lr, const struct pending_irq *p,

-unsigned int state)
+static void gicv2_update_lr(int lr, unsigned int virq, uint8_t priority,
+unsigned int hw_irq, unsigned int state)
  {
  uint32_t lr_reg;
  
@@ -437,12 +437,12 @@ static void gicv2_update_lr(int lr, const struct pending_irq *p,

  BUG_ON(lr < 0);
  
  lr_reg = (((state & GICH_V2_LR_STATE_MASK) << GICH_V2_LR_STATE_SHIFT)  |

-  ((GIC_PRI_TO_GUEST(p->priority) & GICH_V2_LR_PRIORITY_MASK)
- << GICH_V2_LR_PRIORITY_SHIFT) |
-  ((p->irq & GICH_V2_LR_VIRTUAL_MASK) << 
GICH_V2_LR_VIRTUAL_SHIFT));
+  ((GIC_PRI_TO_GUEST(priority) & GICH_V2_LR_PRIORITY_MASK)
+  << GICH_V2_LR_PRIORITY_SHIFT) |
+  ((virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT));
  
-if ( p->desc != NULL )

-lr_reg |= GICH_V2_LR_HW | ((p->desc->irq & GICH_V2_LR_PHYSICAL_MASK )
+if ( hw_irq != -1 )
+lr_reg |= GICH_V2_LR_HW | ((hw_irq & GICH_V2_LR_PHYSICAL_MASK )
 << GICH_V2_LR_PHYSICAL_SHIFT);
  
  writel_gich(lr_reg, GICH_LR + lr * 4);

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 74d00e0c54..3dec407a02 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -944,8 +944,8 @@ static void gicv3_disable_interface(void)
  spin_unlock();
  }
  
-static void gicv3_update_lr(int lr, const struct pending_irq *p,

-unsigned int state)
+static void gicv3_update_lr(int lr, unsigned int virq, uint8_t priority,
+unsigned int hw_irq, unsigned int state)
  {
  uint64_t val = 0;
  
@@ -961,11 +961,11 @@ static void gicv3_update_lr(int lr, const struct pending_irq *p,

  if ( current->domain->arch.vgic.version == GIC_V3 )
  val |= GICH_LR_GRP1;
  
-val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT;

-val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
+val |= (uint64_t)priority << GICH_LR_PRIORITY_SHIFT;
+val |= ((uint64_t)virq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
  
-   if ( p->desc != NULL )

-   val |= GICH_LR_HW | (((uint64_t)p->desc->irq & GICH_LR_PHYSICAL_MASK)
+   if ( hw_irq != -1 )


hw_irq is unsigned to technically it should be ~0. Also, I would prefer 
if you introduce a define making clear where the -1 comes from.


Lastly, I guess IRQ ~0 will never exist?

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 12/12] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq

2017-10-25 Thread Stefano Stabellini 
On Thu, 19 Oct 2017, Andre Przywara wrote:
> The functions to actually populate a list register were accessing
> the VGIC internal pending_irq struct, although they should be abstracting
> from that.
> Break the needed information down to remove the reference to pending_irq
> from gic-v[23].c.
> 
> Signed-off-by: Andre Przywara 

Reviewed-by: Stefano Stabellini 


> ---
>  xen/arch/arm/gic-v2.c | 14 +++---
>  xen/arch/arm/gic-v3.c | 12 ++--
>  xen/arch/arm/gic-vgic.c   |  3 ++-
>  xen/include/asm-arm/gic.h |  4 ++--
>  4 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 511c8d7294..e5acff8900 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -428,8 +428,8 @@ static void gicv2_disable_interface(void)
>  spin_unlock();
>  }
>  
> -static void gicv2_update_lr(int lr, const struct pending_irq *p,
> -unsigned int state)
> +static void gicv2_update_lr(int lr, unsigned int virq, uint8_t priority,
> +unsigned int hw_irq, unsigned int state)
>  {
>  uint32_t lr_reg;
>  
> @@ -437,12 +437,12 @@ static void gicv2_update_lr(int lr, const struct 
> pending_irq *p,
>  BUG_ON(lr < 0);
>  
>  lr_reg = (((state & GICH_V2_LR_STATE_MASK) << GICH_V2_LR_STATE_SHIFT)  |
> -  ((GIC_PRI_TO_GUEST(p->priority) & GICH_V2_LR_PRIORITY_MASK)
> - << GICH_V2_LR_PRIORITY_SHIFT) |
> -  ((p->irq & GICH_V2_LR_VIRTUAL_MASK) << 
> GICH_V2_LR_VIRTUAL_SHIFT));
> +  ((GIC_PRI_TO_GUEST(priority) & GICH_V2_LR_PRIORITY_MASK)
> +  << GICH_V2_LR_PRIORITY_SHIFT) |
> +  ((virq & GICH_V2_LR_VIRTUAL_MASK) << 
> GICH_V2_LR_VIRTUAL_SHIFT));
>  
> -if ( p->desc != NULL )
> -lr_reg |= GICH_V2_LR_HW | ((p->desc->irq & GICH_V2_LR_PHYSICAL_MASK )
> +if ( hw_irq != -1 )
> +lr_reg |= GICH_V2_LR_HW | ((hw_irq & GICH_V2_LR_PHYSICAL_MASK )
> << GICH_V2_LR_PHYSICAL_SHIFT);
>  
>  writel_gich(lr_reg, GICH_LR + lr * 4);
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 74d00e0c54..3dec407a02 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -944,8 +944,8 @@ static void gicv3_disable_interface(void)
>  spin_unlock();
>  }
>  
> -static void gicv3_update_lr(int lr, const struct pending_irq *p,
> -unsigned int state)
> +static void gicv3_update_lr(int lr, unsigned int virq, uint8_t priority,
> +unsigned int hw_irq, unsigned int state)
>  {
>  uint64_t val = 0;
>  
> @@ -961,11 +961,11 @@ static void gicv3_update_lr(int lr, const struct 
> pending_irq *p,
>  if ( current->domain->arch.vgic.version == GIC_V3 )
>  val |= GICH_LR_GRP1;
>  
> -val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT;
> -val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << 
> GICH_LR_VIRTUAL_SHIFT;
> +val |= (uint64_t)priority << GICH_LR_PRIORITY_SHIFT;
> +val |= ((uint64_t)virq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
>  
> -   if ( p->desc != NULL )
> -   val |= GICH_LR_HW | (((uint64_t)p->desc->irq & GICH_LR_PHYSICAL_MASK)
> +   if ( hw_irq != -1 )
> +   val |= GICH_LR_HW | (((uint64_t)hw_irq & GICH_LR_PHYSICAL_MASK)
> << GICH_LR_PHYSICAL_SHIFT);
>  
>  gicv3_ich_write_lr(lr, val);
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 7765d83432..e783f3b54b 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -52,7 +52,8 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
>  
>  clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, >status);
>  
> -gic_hw_ops->update_lr(lr, p, state);
> +gic_hw_ops->update_lr(lr, p->irq, p->priority,
> +  p->desc ? p->desc->irq : -1, state);
>  
>  set_bit(GIC_IRQ_GUEST_VISIBLE, >status);
>  clear_bit(GIC_IRQ_GUEST_QUEUED, >status);
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index fe14094c0f..66f0957fab 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -339,8 +339,8 @@ struct gic_hw_operations {
>  /* Disable CPU physical and virtual interfaces */
>  void (*disable_interface)(void);
>  /* Update LR register with state and priority */
> -void (*update_lr)(int lr, const struct pending_irq *pending_irq,
> -  unsigned int state);
> +void (*update_lr)(int lr, unsigned int virq, uint8_t priority,
> +  unsigned int hw_irq, unsigned int state);
>  /* Update HCR status register */
>  void (*update_hcr_status)(uint32_t flag, bool set);
>  /* Clear LR register */
> -- 
> 2.14.1
> 

___
Xen-devel mailing list

[Xen-devel] [PATCH 12/12] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq

2017-10-19 Thread Andre Przywara 
The functions to actually populate a list register were accessing
the VGIC internal pending_irq struct, although they should be abstracting
from that.
Break the needed information down to remove the reference to pending_irq
from gic-v[23].c.

Signed-off-by: Andre Przywara 
---
 xen/arch/arm/gic-v2.c | 14 +++---
 xen/arch/arm/gic-v3.c | 12 ++--
 xen/arch/arm/gic-vgic.c   |  3 ++-
 xen/include/asm-arm/gic.h |  4 ++--
 4 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 511c8d7294..e5acff8900 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -428,8 +428,8 @@ static void gicv2_disable_interface(void)
 spin_unlock();
 }
 
-static void gicv2_update_lr(int lr, const struct pending_irq *p,
-unsigned int state)
+static void gicv2_update_lr(int lr, unsigned int virq, uint8_t priority,
+unsigned int hw_irq, unsigned int state)
 {
 uint32_t lr_reg;
 
@@ -437,12 +437,12 @@ static void gicv2_update_lr(int lr, const struct 
pending_irq *p,
 BUG_ON(lr < 0);
 
 lr_reg = (((state & GICH_V2_LR_STATE_MASK) << GICH_V2_LR_STATE_SHIFT)  |
-  ((GIC_PRI_TO_GUEST(p->priority) & GICH_V2_LR_PRIORITY_MASK)
- << GICH_V2_LR_PRIORITY_SHIFT) |
-  ((p->irq & GICH_V2_LR_VIRTUAL_MASK) << 
GICH_V2_LR_VIRTUAL_SHIFT));
+  ((GIC_PRI_TO_GUEST(priority) & GICH_V2_LR_PRIORITY_MASK)
+  << GICH_V2_LR_PRIORITY_SHIFT) |
+  ((virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT));
 
-if ( p->desc != NULL )
-lr_reg |= GICH_V2_LR_HW | ((p->desc->irq & GICH_V2_LR_PHYSICAL_MASK )
+if ( hw_irq != -1 )
+lr_reg |= GICH_V2_LR_HW | ((hw_irq & GICH_V2_LR_PHYSICAL_MASK )
<< GICH_V2_LR_PHYSICAL_SHIFT);
 
 writel_gich(lr_reg, GICH_LR + lr * 4);
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 74d00e0c54..3dec407a02 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -944,8 +944,8 @@ static void gicv3_disable_interface(void)
 spin_unlock();
 }
 
-static void gicv3_update_lr(int lr, const struct pending_irq *p,
-unsigned int state)
+static void gicv3_update_lr(int lr, unsigned int virq, uint8_t priority,
+unsigned int hw_irq, unsigned int state)
 {
 uint64_t val = 0;
 
@@ -961,11 +961,11 @@ static void gicv3_update_lr(int lr, const struct 
pending_irq *p,
 if ( current->domain->arch.vgic.version == GIC_V3 )
 val |= GICH_LR_GRP1;
 
-val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT;
-val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
+val |= (uint64_t)priority << GICH_LR_PRIORITY_SHIFT;
+val |= ((uint64_t)virq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
 
-   if ( p->desc != NULL )
-   val |= GICH_LR_HW | (((uint64_t)p->desc->irq & GICH_LR_PHYSICAL_MASK)
+   if ( hw_irq != -1 )
+   val |= GICH_LR_HW | (((uint64_t)hw_irq & GICH_LR_PHYSICAL_MASK)
<< GICH_LR_PHYSICAL_SHIFT);
 
 gicv3_ich_write_lr(lr, val);
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 7765d83432..e783f3b54b 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -52,7 +52,8 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
 
 clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, >status);
 
-gic_hw_ops->update_lr(lr, p, state);
+gic_hw_ops->update_lr(lr, p->irq, p->priority,
+  p->desc ? p->desc->irq : -1, state);
 
 set_bit(GIC_IRQ_GUEST_VISIBLE, >status);
 clear_bit(GIC_IRQ_GUEST_QUEUED, >status);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index fe14094c0f..66f0957fab 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -339,8 +339,8 @@ struct gic_hw_operations {
 /* Disable CPU physical and virtual interfaces */
 void (*disable_interface)(void);
 /* Update LR register with state and priority */
-void (*update_lr)(int lr, const struct pending_irq *pending_irq,
-  unsigned int state);
+void (*update_lr)(int lr, unsigned int virq, uint8_t priority,
+  unsigned int hw_irq, unsigned int state);
 /* Update HCR status register */
 void (*update_hcr_status)(uint32_t flag, bool set);
 /* Clear LR register */
-- 
2.14.1


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