* [PATCH 1/6] xen/arm: gic: Fix indentation in gic_update_one_lr
2018-03-09 16:35 [PATCH 0/6] xen/arm: Rework the way to store the LR julien.grall
@ 2018-03-09 16:35 ` julien.grall
2018-03-10 15:53 ` André Przywara
2018-03-09 16:35 ` [PATCH 2/6] xen/arm: vgic: Override the group in lr everytime julien.grall
` (4 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: julien.grall @ 2018-03-09 16:35 UTC (permalink / raw)
To: xen-devel; +Cc: andre.przywara, Julien Grall, sstabellini
From: Julien Grall <julien.grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/gic-vgic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 61f093db50..e3cb47e80e 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -197,8 +197,8 @@ static void gic_update_one_lr(struct vcpu *v, int i)
{
if ( p->desc == NULL )
{
- lr_val.state |= GICH_LR_PENDING;
- gic_hw_ops->write_lr(i, &lr_val);
+ lr_val.state |= GICH_LR_PENDING;
+ gic_hw_ops->write_lr(i, &lr_val);
}
else
gdprintk(XENLOG_WARNING, "unable to inject hw irq=%d into d%dv%d: already active in LR%d\n",
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/6] xen/arm: gic: Fix indentation in gic_update_one_lr
2018-03-09 16:35 ` [PATCH 1/6] xen/arm: gic: Fix indentation in gic_update_one_lr julien.grall
@ 2018-03-10 15:53 ` André Przywara
0 siblings, 0 replies; 16+ messages in thread
From: André Przywara @ 2018-03-10 15:53 UTC (permalink / raw)
To: julien.grall, xen-devel; +Cc: sstabellini
On 09/03/18 17:35, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre.
> ---
> xen/arch/arm/gic-vgic.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 61f093db50..e3cb47e80e 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -197,8 +197,8 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> {
> if ( p->desc == NULL )
> {
> - lr_val.state |= GICH_LR_PENDING;
> - gic_hw_ops->write_lr(i, &lr_val);
> + lr_val.state |= GICH_LR_PENDING;
> + gic_hw_ops->write_lr(i, &lr_val);
> }
> else
> gdprintk(XENLOG_WARNING, "unable to inject hw irq=%d into d%dv%d: already active in LR%d\n",
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/6] xen/arm: vgic: Override the group in lr everytime
2018-03-09 16:35 [PATCH 0/6] xen/arm: Rework the way to store the LR julien.grall
2018-03-09 16:35 ` [PATCH 1/6] xen/arm: gic: Fix indentation in gic_update_one_lr julien.grall
@ 2018-03-09 16:35 ` julien.grall
2018-03-14 18:02 ` Andre Przywara
2018-03-09 16:35 ` [PATCH 3/6] xen/arm: gic: Use bool instead of uint8_t for the hw_status in gic_lr julien.grall
` (3 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: julien.grall @ 2018-03-09 16:35 UTC (permalink / raw)
To: xen-devel; +Cc: andre.przywara, Julien Grall, sstabellini
From: Julien Grall <julien.grall@arm.com>
At the moment, write_lr is assuming the caller will set correctly the
group. However the group should always be 0 when the guest is using
vGICv2 and 1 for vGICv3. As the caller should not care about the group,
override it directly.
With that change, write_lr is now behaving like update_lr for the group.
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/gic-v2.c | 4 +---
xen/arch/arm/gic-v3.c | 11 ++++++++---
xen/include/asm-arm/gic.h | 1 -
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index f16e17c1a3..fc105c08b8 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -469,7 +469,6 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
lr_reg->state = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK;
lr_reg->hw_status = (lrv >> GICH_V2_LR_HW_SHIFT) & GICH_V2_LR_HW_MASK;
- lr_reg->grp = (lrv >> GICH_V2_LR_GRP_SHIFT) & GICH_V2_LR_GRP_MASK;
}
static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
@@ -483,8 +482,7 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
<< GICH_V2_LR_STATE_SHIFT) |
((uint32_t)(lr_reg->hw_status & GICH_V2_LR_HW_MASK)
- << GICH_V2_LR_HW_SHIFT) |
- ((uint32_t)(lr_reg->grp & GICH_V2_LR_GRP_MASK) << GICH_V2_LR_GRP_SHIFT) );
+ << GICH_V2_LR_HW_SHIFT));
writel_gich(lrv, GICH_LR + lr * 4);
}
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 09b49a07d5..0dfa1a1e08 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1012,7 +1012,6 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
lr_reg->priority = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
lr_reg->state = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
lr_reg->hw_status = (lrv >> ICH_LR_HW_SHIFT) & ICH_LR_HW_MASK;
- lr_reg->grp = (lrv >> ICH_LR_GRP_SHIFT) & ICH_LR_GRP_MASK;
}
static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
@@ -1023,8 +1022,14 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
((u64)(lr->virq & ICH_LR_VIRTUAL_MASK) << ICH_LR_VIRTUAL_SHIFT) |
((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)|
((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) |
- ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_SHIFT) |
- ((u64)(lr->grp & ICH_LR_GRP_MASK) << ICH_LR_GRP_SHIFT) );
+ ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_SHIFT) );
+
+ /*
+ * When the guest is using vGICv3, all the IRQs are Group 1. Group 0
+ * would result in a FIQ, which will not be expected by the guest OS.
+ */
+ if ( current->domain->arch.vgic.version == GIC_V3 )
+ lrv |= ICH_LR_GRP1;
gicv3_ich_write_lr(lr_reg, lrv);
}
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 49cb94f792..1eb08b856e 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -211,7 +211,6 @@ struct gic_lr {
uint8_t priority;
uint8_t state;
uint8_t hw_status;
- uint8_t grp;
};
enum gic_version {
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/6] xen/arm: vgic: Override the group in lr everytime
2018-03-09 16:35 ` [PATCH 2/6] xen/arm: vgic: Override the group in lr everytime julien.grall
@ 2018-03-14 18:02 ` Andre Przywara
2018-03-14 18:07 ` Julien Grall
0 siblings, 1 reply; 16+ messages in thread
From: Andre Przywara @ 2018-03-14 18:02 UTC (permalink / raw)
To: julien.grall, xen-devel; +Cc: sstabellini
On 09/03/18 17:35, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
>
> At the moment, write_lr is assuming the caller will set correctly the
> group. However the group should always be 0 when the guest is using
> vGICv2 and 1 for vGICv3. As the caller should not care about the group,
> override it directly.
I think that makes sense, mostly because this is what KVM does as well.
And it's a good idea to do this in write_lr().
I understand that this is effectively what I did in the new VGIC, but it
would be good to double check that this is the right thing to do
for the old VGIC as well. Did you test this on some GICv3 h/w or the
model? Or do we always call update_lr() anyway?
Cheers,
Andre.
> With that change, write_lr is now behaving like update_lr for the group.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/arm/gic-v2.c | 4 +---
> xen/arch/arm/gic-v3.c | 11 ++++++++---
> xen/include/asm-arm/gic.h | 1 -
> 3 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index f16e17c1a3..fc105c08b8 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -469,7 +469,6 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
> lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
> lr_reg->state = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK;
> lr_reg->hw_status = (lrv >> GICH_V2_LR_HW_SHIFT) & GICH_V2_LR_HW_MASK;
> - lr_reg->grp = (lrv >> GICH_V2_LR_GRP_SHIFT) & GICH_V2_LR_GRP_MASK;
> }
>
> static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
> @@ -483,8 +482,7 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
> ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
> << GICH_V2_LR_STATE_SHIFT) |
> ((uint32_t)(lr_reg->hw_status & GICH_V2_LR_HW_MASK)
> - << GICH_V2_LR_HW_SHIFT) |
> - ((uint32_t)(lr_reg->grp & GICH_V2_LR_GRP_MASK) << GICH_V2_LR_GRP_SHIFT) );
> + << GICH_V2_LR_HW_SHIFT));
>
> writel_gich(lrv, GICH_LR + lr * 4);
> }
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 09b49a07d5..0dfa1a1e08 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1012,7 +1012,6 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
> lr_reg->priority = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
> lr_reg->state = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
> lr_reg->hw_status = (lrv >> ICH_LR_HW_SHIFT) & ICH_LR_HW_MASK;
> - lr_reg->grp = (lrv >> ICH_LR_GRP_SHIFT) & ICH_LR_GRP_MASK;
> }
>
> static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> @@ -1023,8 +1022,14 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK) << ICH_LR_VIRTUAL_SHIFT) |
> ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)|
> ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) |
> - ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_SHIFT) |
> - ((u64)(lr->grp & ICH_LR_GRP_MASK) << ICH_LR_GRP_SHIFT) );
> + ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_SHIFT) );
> +
> + /*
> + * When the guest is using vGICv3, all the IRQs are Group 1. Group 0
> + * would result in a FIQ, which will not be expected by the guest OS.
> + */
> + if ( current->domain->arch.vgic.version == GIC_V3 )
> + lrv |= ICH_LR_GRP1;
>
> gicv3_ich_write_lr(lr_reg, lrv);
> }
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 49cb94f792..1eb08b856e 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -211,7 +211,6 @@ struct gic_lr {
> uint8_t priority;
> uint8_t state;
> uint8_t hw_status;
> - uint8_t grp;
> };
>
> enum gic_version {
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/6] xen/arm: vgic: Override the group in lr everytime
2018-03-14 18:02 ` Andre Przywara
@ 2018-03-14 18:07 ` Julien Grall
0 siblings, 0 replies; 16+ messages in thread
From: Julien Grall @ 2018-03-14 18:07 UTC (permalink / raw)
To: Andre Przywara, xen-devel; +Cc: sstabellini
Hi Andre,
On 03/14/2018 06:02 PM, Andre Przywara wrote:
> On 09/03/18 17:35, julien.grall@arm.com wrote:
>> From: Julien Grall <julien.grall@arm.com>
>>
>> At the moment, write_lr is assuming the caller will set correctly the
>> group. However the group should always be 0 when the guest is using
>> vGICv2 and 1 for vGICv3. As the caller should not care about the group,
>> override it directly.
>
> I think that makes sense, mostly because this is what KVM does as well.
> And it's a good idea to do this in write_lr().
>
> I understand that this is effectively what I did in the new VGIC, but it
> would be good to double check that this is the right thing to do
> for the old VGIC as well. Did you test this on some GICv3 h/w or the
> model? Or do we always call update_lr() anyway?
I had a patch to remove update_lr and use write_lr. I exercised the code
with it but didn't send it because of some nasty bug with the priority
in write_lr and the old vGIC.
On the old vGIC write_lr is only used when in gic_update_one_lr(...)
combine with read_lr before. In that context we don't care about the
group, so overwriting it is fine and the right thing to do.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/6] xen/arm: gic: Use bool instead of uint8_t for the hw_status in gic_lr
2018-03-09 16:35 [PATCH 0/6] xen/arm: Rework the way to store the LR julien.grall
2018-03-09 16:35 ` [PATCH 1/6] xen/arm: gic: Fix indentation in gic_update_one_lr julien.grall
2018-03-09 16:35 ` [PATCH 2/6] xen/arm: vgic: Override the group in lr everytime julien.grall
@ 2018-03-09 16:35 ` julien.grall
2018-03-14 18:06 ` Andre Przywara
2018-03-09 16:35 ` [PATCH 4/6] xen/arm: gic: Split the field state in gic_lr in 2 fields active and pending julien.grall
` (2 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: julien.grall @ 2018-03-09 16:35 UTC (permalink / raw)
To: xen-devel; +Cc: andre.przywara, Julien Grall, sstabellini
From: Julien Grall <julien.grall@arm.com>
hw_status can only be 1 or 0. So convert to a bool.
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/gic-v2.c | 9 +++++----
xen/arch/arm/gic-v3.c | 8 +++++---
xen/include/asm-arm/gic.h | 2 +-
3 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index fc105c08b8..23223575a2 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -468,7 +468,7 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
lr_reg->state = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK;
- lr_reg->hw_status = (lrv >> GICH_V2_LR_HW_SHIFT) & GICH_V2_LR_HW_MASK;
+ lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
}
static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
@@ -480,9 +480,10 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
<< GICH_V2_LR_PRIORITY_SHIFT) |
((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
- << GICH_V2_LR_STATE_SHIFT) |
- ((uint32_t)(lr_reg->hw_status & GICH_V2_LR_HW_MASK)
- << GICH_V2_LR_HW_SHIFT));
+ << GICH_V2_LR_STATE_SHIFT) );
+
+ if ( lr_reg->hw_status )
+ lrv |= GICH_V2_LR_HW;
writel_gich(lrv, GICH_LR + lr * 4);
}
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 0dfa1a1e08..0711e509a6 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1011,7 +1011,7 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
lr_reg->priority = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
lr_reg->state = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
- lr_reg->hw_status = (lrv >> ICH_LR_HW_SHIFT) & ICH_LR_HW_MASK;
+ lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
}
static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
@@ -1021,8 +1021,10 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)|
((u64)(lr->virq & ICH_LR_VIRTUAL_MASK) << ICH_LR_VIRTUAL_SHIFT) |
((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)|
- ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) |
- ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_SHIFT) );
+ ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) );
+
+ if ( lr->hw_status )
+ lrv |= ICH_LR_HW;
/*
* When the guest is using vGICv3, all the IRQs are Group 1. Group 0
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 1eb08b856e..daec51499c 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -210,7 +210,7 @@ struct gic_lr {
uint32_t virq;
uint8_t priority;
uint8_t state;
- uint8_t hw_status;
+ bool hw_status;
};
enum gic_version {
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 3/6] xen/arm: gic: Use bool instead of uint8_t for the hw_status in gic_lr
2018-03-09 16:35 ` [PATCH 3/6] xen/arm: gic: Use bool instead of uint8_t for the hw_status in gic_lr julien.grall
@ 2018-03-14 18:06 ` Andre Przywara
0 siblings, 0 replies; 16+ messages in thread
From: Andre Przywara @ 2018-03-14 18:06 UTC (permalink / raw)
To: julien.grall, xen-devel; +Cc: sstabellini
Hi,
On 09/03/18 16:35, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
>
> hw_status can only be 1 or 0. So convert to a bool.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre.
> ---
> xen/arch/arm/gic-v2.c | 9 +++++----
> xen/arch/arm/gic-v3.c | 8 +++++---
> xen/include/asm-arm/gic.h | 2 +-
> 3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index fc105c08b8..23223575a2 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -468,7 +468,7 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
> lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
> lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
> lr_reg->state = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK;
> - lr_reg->hw_status = (lrv >> GICH_V2_LR_HW_SHIFT) & GICH_V2_LR_HW_MASK;
> + lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
> }
>
> static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
> @@ -480,9 +480,10 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
> ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
> << GICH_V2_LR_PRIORITY_SHIFT) |
> ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
> - << GICH_V2_LR_STATE_SHIFT) |
> - ((uint32_t)(lr_reg->hw_status & GICH_V2_LR_HW_MASK)
> - << GICH_V2_LR_HW_SHIFT));
> + << GICH_V2_LR_STATE_SHIFT) );
> +
> + if ( lr_reg->hw_status )
> + lrv |= GICH_V2_LR_HW;
>
> writel_gich(lrv, GICH_LR + lr * 4);
> }
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0dfa1a1e08..0711e509a6 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1011,7 +1011,7 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
>
> lr_reg->priority = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
> lr_reg->state = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
> - lr_reg->hw_status = (lrv >> ICH_LR_HW_SHIFT) & ICH_LR_HW_MASK;
> + lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
> }
>
> static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> @@ -1021,8 +1021,10 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)|
> ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK) << ICH_LR_VIRTUAL_SHIFT) |
> ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)|
> - ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) |
> - ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_SHIFT) );
> + ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) );
> +
> + if ( lr->hw_status )
> + lrv |= ICH_LR_HW;
>
> /*
> * When the guest is using vGICv3, all the IRQs are Group 1. Group 0
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 1eb08b856e..daec51499c 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -210,7 +210,7 @@ struct gic_lr {
> uint32_t virq;
> uint8_t priority;
> uint8_t state;
> - uint8_t hw_status;
> + bool hw_status;
> };
>
> enum gic_version {
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/6] xen/arm: gic: Split the field state in gic_lr in 2 fields active and pending
2018-03-09 16:35 [PATCH 0/6] xen/arm: Rework the way to store the LR julien.grall
` (2 preceding siblings ...)
2018-03-09 16:35 ` [PATCH 3/6] xen/arm: gic: Use bool instead of uint8_t for the hw_status in gic_lr julien.grall
@ 2018-03-09 16:35 ` julien.grall
2018-03-14 18:09 ` Andre Przywara
2018-03-09 16:35 ` [PATCH 5/6] xen/arm: GIC: Only set pirq in the LR when hw_status is set julien.grall
2018-03-09 16:35 ` [PATCH 6/6] ARM: GIC: extend LR read/write functions to cover EOI and source julien.grall
5 siblings, 1 reply; 16+ messages in thread
From: julien.grall @ 2018-03-09 16:35 UTC (permalink / raw)
To: xen-devel; +Cc: andre.przywara, Julien Grall, sstabellini
From: Julien Grall <julien.grall@arm.com>
Mostly making the code nicer to read.
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/gic-v2.c | 15 +++++++++++----
xen/arch/arm/gic-v3.c | 12 +++++++++---
xen/arch/arm/gic-vgic.c | 6 +++---
xen/include/asm-arm/gic.h | 3 ++-
xen/include/asm-arm/gic_v3_defs.h | 2 ++
5 files changed, 27 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 23223575a2..90d8f652d3 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -51,6 +51,8 @@
#define GICH_V2_LR_PHYSICAL_SHIFT 10
#define GICH_V2_LR_STATE_MASK 0x3
#define GICH_V2_LR_STATE_SHIFT 28
+#define GICH_V2_LR_PENDING (1U << 28)
+#define GICH_V2_LR_ACTIVE (1U << 29)
#define GICH_V2_LR_PRIORITY_SHIFT 23
#define GICH_V2_LR_PRIORITY_MASK 0x1f
#define GICH_V2_LR_HW_SHIFT 31
@@ -467,7 +469,8 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK;
lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
- lr_reg->state = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK;
+ lr_reg->pending = (lrv & GICH_V2_LR_PENDING) == GICH_V2_LR_PENDING;
+ lr_reg->active = (lrv & GICH_V2_LR_ACTIVE) == GICH_V2_LR_ACTIVE;
lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
}
@@ -478,9 +481,13 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
lrv = ( ((lr_reg->pirq & GICH_V2_LR_PHYSICAL_MASK) << GICH_V2_LR_PHYSICAL_SHIFT) |
((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT) |
((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
- << GICH_V2_LR_PRIORITY_SHIFT) |
- ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
- << GICH_V2_LR_STATE_SHIFT) );
+ << GICH_V2_LR_PRIORITY_SHIFT) );
+
+ if ( lr_reg->active )
+ lrv |= GICH_V2_LR_ACTIVE;
+
+ if ( lr_reg->pending )
+ lrv |= GICH_V2_LR_PENDING;
if ( lr_reg->hw_status )
lrv |= GICH_V2_LR_HW;
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 0711e509a6..4dbbf0afd2 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1010,7 +1010,8 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
lr_reg->virq = (lrv >> ICH_LR_VIRTUAL_SHIFT) & ICH_LR_VIRTUAL_MASK;
lr_reg->priority = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
- lr_reg->state = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
+ lr_reg->pending = (lrv & ICH_LR_STATE_PENDING) == ICH_LR_STATE_PENDING;
+ lr_reg->active = (lrv & ICH_LR_STATE_ACTIVE) == ICH_LR_STATE_ACTIVE;
lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
}
@@ -1020,8 +1021,13 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)|
((u64)(lr->virq & ICH_LR_VIRTUAL_MASK) << ICH_LR_VIRTUAL_SHIFT) |
- ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)|
- ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) );
+ ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT) );
+
+ if ( lr->active )
+ lrv |= ICH_LR_STATE_ACTIVE;
+
+ if ( lr->pending )
+ lrv |= ICH_LR_STATE_PENDING;
if ( lr->hw_status )
lrv |= ICH_LR_HW;
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index e3cb47e80e..d831b35525 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -189,7 +189,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
return;
}
- if ( lr_val.state & GICH_LR_ACTIVE )
+ if ( lr_val.active )
{
set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
@@ -197,7 +197,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
{
if ( p->desc == NULL )
{
- lr_val.state |= GICH_LR_PENDING;
+ lr_val.pending = true;
gic_hw_ops->write_lr(i, &lr_val);
}
else
@@ -205,7 +205,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
irq, v->domain->domain_id, v->vcpu_id, i);
}
}
- else if ( lr_val.state & GICH_LR_PENDING )
+ else if ( lr_val.pending )
{
int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
#ifdef GIC_DEBUG
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index daec51499c..c32861d4fa 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -209,7 +209,8 @@ struct gic_lr {
/* Virtual IRQ */
uint32_t virq;
uint8_t priority;
- uint8_t state;
+ bool active;
+ bool pending;
bool hw_status;
};
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index ccb72cf0f1..817bb0d5c7 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -171,6 +171,8 @@
#define ICH_LR_PHYSICAL_SHIFT 32
#define ICH_LR_STATE_MASK 0x3
#define ICH_LR_STATE_SHIFT 62
+#define ICH_LR_STATE_PENDING (1UL << 62)
+#define ICH_LR_STATE_ACTIVE (1UL << 63)
#define ICH_LR_PRIORITY_MASK 0xff
#define ICH_LR_PRIORITY_SHIFT 48
#define ICH_LR_HW_MASK 0x1
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 4/6] xen/arm: gic: Split the field state in gic_lr in 2 fields active and pending
2018-03-09 16:35 ` [PATCH 4/6] xen/arm: gic: Split the field state in gic_lr in 2 fields active and pending julien.grall
@ 2018-03-14 18:09 ` Andre Przywara
2018-03-14 18:20 ` Julien Grall
0 siblings, 1 reply; 16+ messages in thread
From: Andre Przywara @ 2018-03-14 18:09 UTC (permalink / raw)
To: julien.grall, xen-devel; +Cc: sstabellini
Hi,
On 09/03/18 16:35, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
>
> Mostly making the code nicer to read.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/arm/gic-v2.c | 15 +++++++++++----
> xen/arch/arm/gic-v3.c | 12 +++++++++---
> xen/arch/arm/gic-vgic.c | 6 +++---
> xen/include/asm-arm/gic.h | 3 ++-
> xen/include/asm-arm/gic_v3_defs.h | 2 ++
> 5 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 23223575a2..90d8f652d3 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -51,6 +51,8 @@
> #define GICH_V2_LR_PHYSICAL_SHIFT 10
> #define GICH_V2_LR_STATE_MASK 0x3
> #define GICH_V2_LR_STATE_SHIFT 28
> +#define GICH_V2_LR_PENDING (1U << 28)
> +#define GICH_V2_LR_ACTIVE (1U << 29)
> #define GICH_V2_LR_PRIORITY_SHIFT 23
> #define GICH_V2_LR_PRIORITY_MASK 0x1f
> #define GICH_V2_LR_HW_SHIFT 31
> @@ -467,7 +469,8 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
> lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK;
> lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
> lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
> - lr_reg->state = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK;
> + lr_reg->pending = (lrv & GICH_V2_LR_PENDING) == GICH_V2_LR_PENDING;
> + lr_reg->active = (lrv & GICH_V2_LR_ACTIVE) == GICH_V2_LR_ACTIVE;
> lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
> }
>
> @@ -478,9 +481,13 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
> lrv = ( ((lr_reg->pirq & GICH_V2_LR_PHYSICAL_MASK) << GICH_V2_LR_PHYSICAL_SHIFT) |
> ((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT) |
> ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
> - << GICH_V2_LR_PRIORITY_SHIFT) |
> - ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
> - << GICH_V2_LR_STATE_SHIFT) );
> + << GICH_V2_LR_PRIORITY_SHIFT) );
> +
> + if ( lr_reg->active )
> + lrv |= GICH_V2_LR_ACTIVE;
> +
> + if ( lr_reg->pending )
> + lrv |= GICH_V2_LR_PENDING;
>
> if ( lr_reg->hw_status )
> lrv |= GICH_V2_LR_HW;
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0711e509a6..4dbbf0afd2 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1010,7 +1010,8 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
> lr_reg->virq = (lrv >> ICH_LR_VIRTUAL_SHIFT) & ICH_LR_VIRTUAL_MASK;
>
> lr_reg->priority = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
> - lr_reg->state = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
> + lr_reg->pending = (lrv & ICH_LR_STATE_PENDING) == ICH_LR_STATE_PENDING;
> + lr_reg->active = (lrv & ICH_LR_STATE_ACTIVE) == ICH_LR_STATE_ACTIVE;
> lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
> }
>
> @@ -1020,8 +1021,13 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>
> lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)|
> ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK) << ICH_LR_VIRTUAL_SHIFT) |
> - ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)|
> - ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) );
> + ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT) );
> +
> + if ( lr->active )
> + lrv |= ICH_LR_STATE_ACTIVE;
> +
> + if ( lr->pending )
> + lrv |= ICH_LR_STATE_PENDING;
>
> if ( lr->hw_status )
> lrv |= ICH_LR_HW;
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index e3cb47e80e..d831b35525 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -189,7 +189,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> return;
> }
>
> - if ( lr_val.state & GICH_LR_ACTIVE )
> + if ( lr_val.active )
> {
> set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> @@ -197,7 +197,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> {
> if ( p->desc == NULL )
> {
> - lr_val.state |= GICH_LR_PENDING;
> + lr_val.pending = true;
> gic_hw_ops->write_lr(i, &lr_val);
> }
> else
> @@ -205,7 +205,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> irq, v->domain->domain_id, v->vcpu_id, i);
> }
> }
> - else if ( lr_val.state & GICH_LR_PENDING )
> + else if ( lr_val.pending )
> {
> int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> #ifdef GIC_DEBUG
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index daec51499c..c32861d4fa 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -209,7 +209,8 @@ struct gic_lr {
> /* Virtual IRQ */
> uint32_t virq;
> uint8_t priority;
> - uint8_t state;
> + bool active;
> + bool pending;
> bool hw_status;
> };
>
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index ccb72cf0f1..817bb0d5c7 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -171,6 +171,8 @@
> #define ICH_LR_PHYSICAL_SHIFT 32
> #define ICH_LR_STATE_MASK 0x3
> #define ICH_LR_STATE_SHIFT 62
> +#define ICH_LR_STATE_PENDING (1UL << 62)
> +#define ICH_LR_STATE_ACTIVE (1UL << 63)
Should that be 1ULL, just in case we ever get 32-bit support for GICv3?
Regardless of that:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre.
> #define ICH_LR_PRIORITY_MASK 0xff
> #define ICH_LR_PRIORITY_SHIFT 48
> #define ICH_LR_HW_MASK 0x1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 4/6] xen/arm: gic: Split the field state in gic_lr in 2 fields active and pending
2018-03-14 18:09 ` Andre Przywara
@ 2018-03-14 18:20 ` Julien Grall
0 siblings, 0 replies; 16+ messages in thread
From: Julien Grall @ 2018-03-14 18:20 UTC (permalink / raw)
To: Andre Przywara, xen-devel; +Cc: sstabellini
On 03/14/2018 06:09 PM, Andre Przywara wrote:
> On 09/03/18 16:35, julien.grall@arm.com wrote:
>> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
>> index ccb72cf0f1..817bb0d5c7 100644
>> --- a/xen/include/asm-arm/gic_v3_defs.h
>> +++ b/xen/include/asm-arm/gic_v3_defs.h
>> @@ -171,6 +171,8 @@
>> #define ICH_LR_PHYSICAL_SHIFT 32
>> #define ICH_LR_STATE_MASK 0x3
>> #define ICH_LR_STATE_SHIFT 62
>> +#define ICH_LR_STATE_PENDING (1UL << 62)
>> +#define ICH_LR_STATE_ACTIVE (1UL << 63)
>
> Should that be 1ULL, just in case we ever get 32-bit support for GICv3?
Yes, good point. I will fix that.
>
> Regardless of that:
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 5/6] xen/arm: GIC: Only set pirq in the LR when hw_status is set
2018-03-09 16:35 [PATCH 0/6] xen/arm: Rework the way to store the LR julien.grall
` (3 preceding siblings ...)
2018-03-09 16:35 ` [PATCH 4/6] xen/arm: gic: Split the field state in gic_lr in 2 fields active and pending julien.grall
@ 2018-03-09 16:35 ` julien.grall
2018-03-14 18:14 ` Andre Przywara
2018-03-09 16:35 ` [PATCH 6/6] ARM: GIC: extend LR read/write functions to cover EOI and source julien.grall
5 siblings, 1 reply; 16+ messages in thread
From: julien.grall @ 2018-03-09 16:35 UTC (permalink / raw)
To: xen-devel; +Cc: andre.przywara, Julien Grall, sstabellini
From: Julien Grall <julien.grall@arm.com>
The field pirq should only be valid when the virtual interrupt
is associated to a physical interrupt.
This change will help to extend gic_lr for supporting specific virtual
interrupt field (e.g eoi, source) that clashes with the PIRQ field.
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/gic-v2.c | 13 ++++++++++---
xen/arch/arm/gic-v3.c | 10 +++++++---
xen/include/asm-arm/gic.h | 2 +-
3 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 90d8f652d3..daf8c61258 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -466,20 +466,24 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
uint32_t lrv;
lrv = readl_gich(GICH_LR + lr * 4);
- lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK;
lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
lr_reg->pending = (lrv & GICH_V2_LR_PENDING) == GICH_V2_LR_PENDING;
lr_reg->active = (lrv & GICH_V2_LR_ACTIVE) == GICH_V2_LR_ACTIVE;
lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
+
+ if ( lr_reg->hw_status )
+ {
+ lr_reg->pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
+ lr_reg->pirq &= GICH_V2_LR_PHYSICAL_MASK;
+ }
}
static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
{
uint32_t lrv = 0;
- lrv = ( ((lr_reg->pirq & GICH_V2_LR_PHYSICAL_MASK) << GICH_V2_LR_PHYSICAL_SHIFT) |
- ((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT) |
+ lrv = (((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT) |
((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
<< GICH_V2_LR_PRIORITY_SHIFT) );
@@ -490,7 +494,10 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
lrv |= GICH_V2_LR_PENDING;
if ( lr_reg->hw_status )
+ {
lrv |= GICH_V2_LR_HW;
+ lrv |= lr_reg->pirq << GICH_V2_LR_PHYSICAL_SHIFT;
+ }
writel_gich(lrv, GICH_LR + lr * 4);
}
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 4dbbf0afd2..f73d386df1 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1006,21 +1006,22 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
lrv = gicv3_ich_read_lr(lr);
- lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
lr_reg->virq = (lrv >> ICH_LR_VIRTUAL_SHIFT) & ICH_LR_VIRTUAL_MASK;
lr_reg->priority = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
lr_reg->pending = (lrv & ICH_LR_STATE_PENDING) == ICH_LR_STATE_PENDING;
lr_reg->active = (lrv & ICH_LR_STATE_ACTIVE) == ICH_LR_STATE_ACTIVE;
lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
+
+ if ( lr_reg->hw_status )
+ lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
}
static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
{
uint64_t lrv = 0;
- lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)|
- ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK) << ICH_LR_VIRTUAL_SHIFT) |
+ lrv = ( ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK) << ICH_LR_VIRTUAL_SHIFT) |
((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT) );
if ( lr->active )
@@ -1030,7 +1031,10 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
lrv |= ICH_LR_STATE_PENDING;
if ( lr->hw_status )
+ {
lrv |= ICH_LR_HW;
+ lrv |= (uint64_t)lr->pirq << ICH_LR_PHYSICAL_SHIFT;
+ }
/*
* When the guest is using vGICv3, all the IRQs are Group 1. Group 0
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index c32861d4fa..545901b120 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -204,7 +204,7 @@ union gic_state_data {
* The LR register format is different for GIC HW version
*/
struct gic_lr {
- /* Physical IRQ */
+ /* Physical IRQ -> Only set when hw_status is set. */
uint32_t pirq;
/* Virtual IRQ */
uint32_t virq;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 5/6] xen/arm: GIC: Only set pirq in the LR when hw_status is set
2018-03-09 16:35 ` [PATCH 5/6] xen/arm: GIC: Only set pirq in the LR when hw_status is set julien.grall
@ 2018-03-14 18:14 ` Andre Przywara
0 siblings, 0 replies; 16+ messages in thread
From: Andre Przywara @ 2018-03-14 18:14 UTC (permalink / raw)
To: julien.grall, xen-devel; +Cc: sstabellini
Hi,
On 09/03/18 16:35, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
>
> The field pirq should only be valid when the virtual interrupt
> is associated to a physical interrupt.
>
> This change will help to extend gic_lr for supporting specific virtual
> interrupt field (e.g eoi, source) that clashes with the PIRQ field.
Makes some sense, yes.
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre.
> ---
> xen/arch/arm/gic-v2.c | 13 ++++++++++---
> xen/arch/arm/gic-v3.c | 10 +++++++---
> xen/include/asm-arm/gic.h | 2 +-
> 3 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 90d8f652d3..daf8c61258 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -466,20 +466,24 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
> uint32_t lrv;
>
> lrv = readl_gich(GICH_LR + lr * 4);
> - lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK;
> lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
> lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
> lr_reg->pending = (lrv & GICH_V2_LR_PENDING) == GICH_V2_LR_PENDING;
> lr_reg->active = (lrv & GICH_V2_LR_ACTIVE) == GICH_V2_LR_ACTIVE;
> lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
> +
> + if ( lr_reg->hw_status )
> + {
> + lr_reg->pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
> + lr_reg->pirq &= GICH_V2_LR_PHYSICAL_MASK;
> + }
> }
>
> static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
> {
> uint32_t lrv = 0;
>
> - lrv = ( ((lr_reg->pirq & GICH_V2_LR_PHYSICAL_MASK) << GICH_V2_LR_PHYSICAL_SHIFT) |
> - ((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT) |
> + lrv = (((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT) |
> ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
> << GICH_V2_LR_PRIORITY_SHIFT) );
>
> @@ -490,7 +494,10 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
> lrv |= GICH_V2_LR_PENDING;
>
> if ( lr_reg->hw_status )
> + {
> lrv |= GICH_V2_LR_HW;
> + lrv |= lr_reg->pirq << GICH_V2_LR_PHYSICAL_SHIFT;
> + }
>
> writel_gich(lrv, GICH_LR + lr * 4);
> }
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 4dbbf0afd2..f73d386df1 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1006,21 +1006,22 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
>
> lrv = gicv3_ich_read_lr(lr);
>
> - lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
> lr_reg->virq = (lrv >> ICH_LR_VIRTUAL_SHIFT) & ICH_LR_VIRTUAL_MASK;
>
> lr_reg->priority = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
> lr_reg->pending = (lrv & ICH_LR_STATE_PENDING) == ICH_LR_STATE_PENDING;
> lr_reg->active = (lrv & ICH_LR_STATE_ACTIVE) == ICH_LR_STATE_ACTIVE;
> lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
> +
> + if ( lr_reg->hw_status )
> + lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
> }
>
> static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> {
> uint64_t lrv = 0;
>
> - lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)|
> - ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK) << ICH_LR_VIRTUAL_SHIFT) |
> + lrv = ( ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK) << ICH_LR_VIRTUAL_SHIFT) |
> ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT) );
>
> if ( lr->active )
> @@ -1030,7 +1031,10 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> lrv |= ICH_LR_STATE_PENDING;
>
> if ( lr->hw_status )
> + {
> lrv |= ICH_LR_HW;
> + lrv |= (uint64_t)lr->pirq << ICH_LR_PHYSICAL_SHIFT;
> + }
>
> /*
> * When the guest is using vGICv3, all the IRQs are Group 1. Group 0
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index c32861d4fa..545901b120 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -204,7 +204,7 @@ union gic_state_data {
> * The LR register format is different for GIC HW version
> */
> struct gic_lr {
> - /* Physical IRQ */
> + /* Physical IRQ -> Only set when hw_status is set. */
> uint32_t pirq;
> /* Virtual IRQ */
> uint32_t virq;
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 6/6] ARM: GIC: extend LR read/write functions to cover EOI and source
2018-03-09 16:35 [PATCH 0/6] xen/arm: Rework the way to store the LR julien.grall
` (4 preceding siblings ...)
2018-03-09 16:35 ` [PATCH 5/6] xen/arm: GIC: Only set pirq in the LR when hw_status is set julien.grall
@ 2018-03-09 16:35 ` julien.grall
2018-03-14 18:19 ` Andre Przywara
5 siblings, 1 reply; 16+ messages in thread
From: julien.grall @ 2018-03-09 16:35 UTC (permalink / raw)
To: xen-devel; +Cc: andre.przywara, Julien Grall, sstabellini, Andre Przywara
From: Andre Przywara <andre.przywara@linaro.org>
So far our LR read/write functions do not handle the EOI bit and the
source CPUID bits in an LR, because the current VGIC implementation does
not use them.
Extend the gic_lr data structure to hold these bits of information by
using a union to differentiate field used depending on whether the vIRQ
has a corresponding pIRQ.
Note that source is not covered by GICv3 LR.
This allows the new VGIC to use this information.
Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/gic-v2.c | 22 +++++++++++++++++++---
xen/arch/arm/gic-v3.c | 11 +++++++++--
xen/include/asm-arm/gic.h | 16 ++++++++++++++--
3 files changed, 42 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index daf8c61258..69f8d6044a 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -474,8 +474,17 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
if ( lr_reg->hw_status )
{
- lr_reg->pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
- lr_reg->pirq &= GICH_V2_LR_PHYSICAL_MASK;
+ lr_reg->h.pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
+ lr_reg->h.pirq &= GICH_V2_LR_PHYSICAL_MASK;
+ }
+ else
+ {
+ lr_reg->v.eoi = (lrv & GICH_V2_LR_MAINTENANCE_IRQ) == GICH_V2_LR_MAINTENANCE_IRQ;
+ /*
+ * This is only valid for SGI, but it does not matter to always
+ * read it as it should be 0 by default.
+ */
+ lr_reg->v.source = (lrv >> GICH_V2_LR_CPUID_SHIFT) & GICH_V2_LR_CPUID_MASK;
}
}
@@ -496,7 +505,14 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
if ( lr_reg->hw_status )
{
lrv |= GICH_V2_LR_HW;
- lrv |= lr_reg->pirq << GICH_V2_LR_PHYSICAL_SHIFT;
+ lrv |= lr_reg->h.pirq << GICH_V2_LR_PHYSICAL_SHIFT;
+ }
+ else
+ {
+ if ( lr_reg->v.eoi )
+ lrv |= GICH_V2_LR_MAINTENANCE_IRQ;
+ if ( lr_reg->virq < NR_GIC_SGI )
+ lrv |= (uint32_t)lr_reg->v.source << GICH_V2_LR_CPUID_SHIFT;
}
writel_gich(lrv, GICH_LR + lr * 4);
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index f73d386df1..a855069111 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1014,7 +1014,9 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
if ( lr_reg->hw_status )
- lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
+ lr_reg->h.pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
+ else
+ lr_reg->v.eoi = (lrv & ICH_LR_MAINTENANCE_IRQ) == ICH_LR_MAINTENANCE_IRQ;
}
static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
@@ -1033,7 +1035,12 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
if ( lr->hw_status )
{
lrv |= ICH_LR_HW;
- lrv |= (uint64_t)lr->pirq << ICH_LR_PHYSICAL_SHIFT;
+ lrv |= (uint64_t)lr->h.pirq << ICH_LR_PHYSICAL_SHIFT;
+ }
+ else
+ {
+ if ( lr->v.eoi )
+ lrv |= ICH_LR_MAINTENANCE_IRQ;
}
/*
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 545901b120..4cf5bb385d 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -204,14 +204,26 @@ union gic_state_data {
* The LR register format is different for GIC HW version
*/
struct gic_lr {
- /* Physical IRQ -> Only set when hw_status is set. */
- uint32_t pirq;
/* Virtual IRQ */
uint32_t virq;
uint8_t priority;
bool active;
bool pending;
bool hw_status;
+ union
+ {
+ /* Only filled when there are a corresponding pIRQ (hw_state = true) */
+ struct
+ {
+ uint32_t pirq;
+ } h;
+ /* Only filled when there are no corresponding pIRQ (hw_state = false) */
+ struct
+ {
+ bool eoi;
+ uint8_t source; /* GICv2 only */
+ } v;
+ };
};
enum gic_version {
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 6/6] ARM: GIC: extend LR read/write functions to cover EOI and source
2018-03-09 16:35 ` [PATCH 6/6] ARM: GIC: extend LR read/write functions to cover EOI and source julien.grall
@ 2018-03-14 18:19 ` Andre Przywara
2018-03-14 18:32 ` Julien Grall
0 siblings, 1 reply; 16+ messages in thread
From: Andre Przywara @ 2018-03-14 18:19 UTC (permalink / raw)
To: julien.grall, xen-devel; +Cc: sstabellini, Andre Przywara
Hi,
On 09/03/18 16:35, julien.grall@arm.com wrote:
> From: Andre Przywara <andre.przywara@linaro.org>
I think this is quite different from what I ever wrote, so please drop
my authorship here.
> So far our LR read/write functions do not handle the EOI bit and the
> source CPUID bits in an LR, because the current VGIC implementation does
> not use them.
> Extend the gic_lr data structure to hold these bits of information by
> using a union to differentiate field used depending on whether the vIRQ
> has a corresponding pIRQ.
As mentioned before, I am not sure this is really necessary. The idea of
struct gic_lr is to provide a hardware agnostic view of an LR. So the
actual read_lr/write_lr function take care of reading/populating pINTID,
iff the HW bit is set (as done in your patch 5/6).
Given that, I don't think we need to change the current code in this
respect, as this is just a small internal interface.
But then again I don't care enough, so if that makes you happy ....
> Note that source is not covered by GICv3 LR.
I was thinking the same, but Marc pointed me to that inconspicuous note
on page 8-299 in ARM IHI 0069D (at the end of "vINTID", chapter 8.4.6).
> This allows the new VGIC to use this information.
>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/arm/gic-v2.c | 22 +++++++++++++++++++---
> xen/arch/arm/gic-v3.c | 11 +++++++++--
> xen/include/asm-arm/gic.h | 16 ++++++++++++++--
> 3 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index daf8c61258..69f8d6044a 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -474,8 +474,17 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>
> if ( lr_reg->hw_status )
> {
> - lr_reg->pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
> - lr_reg->pirq &= GICH_V2_LR_PHYSICAL_MASK;
> + lr_reg->h.pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
> + lr_reg->h.pirq &= GICH_V2_LR_PHYSICAL_MASK;
> + }
> + else
> + {
> + lr_reg->v.eoi = (lrv & GICH_V2_LR_MAINTENANCE_IRQ) == GICH_V2_LR_MAINTENANCE_IRQ;
I think you can drop the " == GICH_V2_LR_MAINTENANCE_IRQ", as .eoi is a
bool. Avoids the linebreak.
> + /*
> + * This is only valid for SGI, but it does not matter to always
> + * read it as it should be 0 by default.
> + */
> + lr_reg->v.source = (lrv >> GICH_V2_LR_CPUID_SHIFT) & GICH_V2_LR_CPUID_MASK;
> }
> }
>
> @@ -496,7 +505,14 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
> if ( lr_reg->hw_status )
> {
> lrv |= GICH_V2_LR_HW;
> - lrv |= lr_reg->pirq << GICH_V2_LR_PHYSICAL_SHIFT;
> + lrv |= lr_reg->h.pirq << GICH_V2_LR_PHYSICAL_SHIFT;
> + }
> + else
> + {
> + if ( lr_reg->v.eoi )
> + lrv |= GICH_V2_LR_MAINTENANCE_IRQ;
> + if ( lr_reg->virq < NR_GIC_SGI )
> + lrv |= (uint32_t)lr_reg->v.source << GICH_V2_LR_CPUID_SHIFT;
> }
>
> writel_gich(lrv, GICH_LR + lr * 4);
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index f73d386df1..a855069111 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1014,7 +1014,9 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
> lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
>
> if ( lr_reg->hw_status )
> - lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
> + lr_reg->h.pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
> + else
> + lr_reg->v.eoi = (lrv & ICH_LR_MAINTENANCE_IRQ) == ICH_LR_MAINTENANCE_IRQ;
Same here.
> }
>
> static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> @@ -1033,7 +1035,12 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> if ( lr->hw_status )
> {
> lrv |= ICH_LR_HW;
> - lrv |= (uint64_t)lr->pirq << ICH_LR_PHYSICAL_SHIFT;
> + lrv |= (uint64_t)lr->h.pirq << ICH_LR_PHYSICAL_SHIFT;
> + }
> + else
> + {
> + if ( lr->v.eoi )
> + lrv |= ICH_LR_MAINTENANCE_IRQ;
> }
>
> /*
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 545901b120..4cf5bb385d 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -204,14 +204,26 @@ union gic_state_data {
> * The LR register format is different for GIC HW version
> */
> struct gic_lr {
> - /* Physical IRQ -> Only set when hw_status is set. */
> - uint32_t pirq;
> /* Virtual IRQ */
> uint32_t virq;
> uint8_t priority;
> bool active;
> bool pending;
> bool hw_status;
> + union
> + {
> + /* Only filled when there are a corresponding pIRQ (hw_state = true) */
> + struct
> + {
> + uint32_t pirq;
> + } h;
> + /* Only filled when there are no corresponding pIRQ (hw_state = false) */
> + struct
> + {
> + bool eoi;
> + uint8_t source; /* GICv2 only */
> + } v;
That looks somewhat confusing to me. So either you use "hw" and "virt"
for the struct identifiers, or, preferably, just drop them altogether:
union {
uint32_t pirq; /* Contains the associated hardware IRQ. */
struct /* Only used for virtual interrupts. */
{
bool eoi;
uint8_t source;
};
};
Cheers,
Andre.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 6/6] ARM: GIC: extend LR read/write functions to cover EOI and source
2018-03-14 18:19 ` Andre Przywara
@ 2018-03-14 18:32 ` Julien Grall
0 siblings, 0 replies; 16+ messages in thread
From: Julien Grall @ 2018-03-14 18:32 UTC (permalink / raw)
To: Andre Przywara, xen-devel; +Cc: sstabellini, Andre Przywara
On 03/14/2018 06:19 PM, Andre Przywara wrote:
> Hi,
Hi Andre,
Thank you for the review.
> On 09/03/18 16:35, julien.grall@arm.com wrote:
>> From: Andre Przywara <andre.przywara@linaro.org>
>
> I think this is quite different from what I ever wrote, so please drop
> my authorship here.
Fine, I wasn't sure given that you were the original author or extending
the LR. I can pointing that in the commit message :).
>> So far our LR read/write functions do not handle the EOI bit and the
>> source CPUID bits in an LR, because the current VGIC implementation does
>> not use them.
>> Extend the gic_lr data structure to hold these bits of information by
>> using a union to differentiate field used depending on whether the vIRQ
>> has a corresponding pIRQ.
>
> As mentioned before, I am not sure this is really necessary. The idea of
> struct gic_lr is to provide a hardware agnostic view of an LR. So the
> actual read_lr/write_lr function take care of reading/populating pINTID,
> iff the HW bit is set (as done in your patch 5/6).
> Given that, I don't think we need to change the current code in this
> respect, as this is just a small internal interface.
Even if I know the vGIC, I find easier with this solution to
differentiate what is for the HW IRQ or not.
The size of Xen Arm is becoming quite significant for me to remember
every single line/structure. So the main goal here is to help the
reviewer to spend less time on patch review as it helps to spot directly
misusage.
>
> But then again I don't care enough, so if that makes you happy ....
>
>> Note that source is not covered by GICv3 LR.
>
> I was thinking the same, but Marc pointed me to that inconspicuous note
> on page 8-299 in ARM IHI 0069D (at the end of "vINTID", chapter 8.4.6).
Doh :/. I will drop the comment and update the GICv3 code then.
>
>> This allows the new VGIC to use this information.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>> xen/arch/arm/gic-v2.c | 22 +++++++++++++++++++---
>> xen/arch/arm/gic-v3.c | 11 +++++++++--
>> xen/include/asm-arm/gic.h | 16 ++++++++++++++--
>> 3 files changed, 42 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index daf8c61258..69f8d6044a 100644
>> --- a/xen/arch/arm/gic-v2.c
>> +++ b/xen/arch/arm/gic-v2.c
>> @@ -474,8 +474,17 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>>
>> if ( lr_reg->hw_status )
>> {
>> - lr_reg->pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
>> - lr_reg->pirq &= GICH_V2_LR_PHYSICAL_MASK;
>> + lr_reg->h.pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
>> + lr_reg->h.pirq &= GICH_V2_LR_PHYSICAL_MASK;
>> + }
>> + else
>> + {
>> + lr_reg->v.eoi = (lrv & GICH_V2_LR_MAINTENANCE_IRQ) == GICH_V2_LR_MAINTENANCE_IRQ;
>
> I think you can drop the " == GICH_V2_LR_MAINTENANCE_IRQ", as .eoi is a
> bool. Avoids the linebreak.
The '==' was more to avoid assuming GIC_V2_LR_MAINTENANCE_IRQ is a
single bit. But I was probably too cautious, I will drop it.
>> writel_gich(lrv, GICH_LR + lr * 4);
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index f73d386df1..a855069111 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1014,7 +1014,9 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
>> lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
>>
>> if ( lr_reg->hw_status )
>> - lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
>> + lr_reg->h.pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
>> + else
>> + lr_reg->v.eoi = (lrv & ICH_LR_MAINTENANCE_IRQ) == ICH_LR_MAINTENANCE_IRQ;
>
> Same here.
Ditto.
>
>> }
>>
>> static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>> @@ -1033,7 +1035,12 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>> if ( lr->hw_status )
>> {
>> lrv |= ICH_LR_HW;
>> - lrv |= (uint64_t)lr->pirq << ICH_LR_PHYSICAL_SHIFT;
>> + lrv |= (uint64_t)lr->h.pirq << ICH_LR_PHYSICAL_SHIFT;
>> + }
>> + else
>> + {
>> + if ( lr->v.eoi )
>> + lrv |= ICH_LR_MAINTENANCE_IRQ;
>> }
>>
>> /*
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index 545901b120..4cf5bb385d 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -204,14 +204,26 @@ union gic_state_data {
>> * The LR register format is different for GIC HW version
>> */
>> struct gic_lr {
>> - /* Physical IRQ -> Only set when hw_status is set. */
>> - uint32_t pirq;
>> /* Virtual IRQ */
>> uint32_t virq;
>> uint8_t priority;
>> bool active;
>> bool pending;
>> bool hw_status;
>> + union
>> + {
>> + /* Only filled when there are a corresponding pIRQ (hw_state = true) */
>> + struct
>> + {
>> + uint32_t pirq;
>> + } h;
>> + /* Only filled when there are no corresponding pIRQ (hw_state = false) */
>> + struct
>> + {
>> + bool eoi;
>> + uint8_t source; /* GICv2 only */
>> + } v;
>
> That looks somewhat confusing to me. So either you use "hw" and "virt"
> for the struct identifiers, or, preferably, just drop them altogether:
>
> union {
> uint32_t pirq; /* Contains the associated hardware IRQ. */
> struct /* Only used for virtual interrupts. */
> {
> bool eoi;
> uint8_t source;
> };
> };
I would prefer to keep a name for each structure. It is not obvious for
me that eoi is only for virtual IRQ. I mostly want to help code review
in the future.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread