On 2018/3/20 16:42, Auger Eric wrote:
> Hi Shannon,
> On 20/03/18 08:26, Shannon Zhao wrote:
>> While we skip the GIC_INTERNAL irqs, we don't change the register offset
>> accordingly. This will overlap the GICR registers value and leave the
>> last GIC_INTERNAL irq's registers out of update.
>>
>> Fix this by skipping the registers banked by GICR.
>>
>> Signed-off-by: Shannon Zhao <zhaoshengl...@huawei.com>
>> ---
>> hw/intc/arm_gicv3_kvm.c | 38 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>>
>> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
>> index 3536795..d423cba 100644
>> --- a/hw/intc/arm_gicv3_kvm.c
>> +++ b/hw/intc/arm_gicv3_kvm.c
>> @@ -136,6 +136,12 @@ static void kvm_dist_get_priority(GICv3State *s,
>> uint32_t offset, uint8_t *bmp)
>> int irq;
>>
>> field = (uint32_t *)bmp;
>> + /* For the KVM GICv3, affinity routing is always enabled, and the first
>> 8
>> + * GICD_IPRIORITYR<n> registers are always RAZ/WI. The corresponding
>> + * functionality is replaced by GICR_IPRIORITYR<n>. So it doesn't need
>> to
>> + * sync them.
>> + */
>> + offset += (8 * sizeof(uint32_t));
>> for_each_dist_irq_reg(irq, s->num_irq, 8) {
>> kvm_gicd_access(s, offset, ®, false);
>> *field = reg;
>> @@ -150,6 +156,12 @@ static void kvm_dist_put_priority(GICv3State *s,
>> uint32_t offset, uint8_t *bmp)
>> int irq;
>>
>> field = (uint32_t *)bmp;
>> + /* For the KVM GICv3, affinity routing is always enabled, and the first
>> 8
>> + * GICD_IPRIORITYR<n> registers are always RAZ/WI. The corresponding
>> + * functionality is replaced by GICR_IPRIORITYR<n>. So it doesn't need
>> to
>> + * sync them.
>> + */
>> + offset += (8 * sizeof(uint32_t));
>> for_each_dist_irq_reg(irq, s->num_irq, 8) {
>> reg = *field;
>> kvm_gicd_access(s, offset, ®, true);
>> @@ -164,6 +176,12 @@ static void kvm_dist_get_edge_trigger(GICv3State *s,
>> uint32_t offset,
>> uint32_t reg;
>> int irq;
>>
>> + /* For the KVM GICv3, affinity routing is always enabled, and the first
>> 2
>> + * GICD_ICFGR<n> registers are always RAZ/WI. The corresponding
>> + * functionality is replaced by GICR_ICFGR<n>. So it doesn't need to
>> sync
>> + * them.
>> + */
>> + offset += (2 * sizeof(uint32_t));
>> for_each_dist_irq_reg(irq, s->num_irq, 2) {
>> kvm_gicd_access(s, offset, ®, false);
>> reg = half_unshuffle32(reg >> 1);
>> @@ -181,6 +199,12 @@ static void kvm_dist_put_edge_trigger(GICv3State *s,
>> uint32_t offset,
>> uint32_t reg;
>> int irq;
>>
>> + /* For the KVM GICv3, affinity routing is always enabled, and the first
>> 2
>> + * GICD_ICFGR<n> registers are always RAZ/WI. The corresponding
>> + * functionality is replaced by GICR_ICFGR<n>. So it doesn't need to
>> sync
>> + * them.
>> + */
>> + offset += (2 * sizeof(uint32_t));
>> for_each_dist_irq_reg(irq, s->num_irq, 2) {
>> reg = *gic_bmp_ptr32(bmp, irq);
>> if (irq % 32 != 0) {
>> @@ -222,6 +246,12 @@ static void kvm_dist_getbmp(GICv3State *s, uint32_t
>> offset, uint32_t *bmp)
>> uint32_t reg;
>> int irq;
>>
>> + /* For the KVM GICv3, affinity routing is always enabled, and the
>> + * GICD_IGROUPR0/GICD_ISENABLER0/GICD_ISPENDR0/GICD_ISACTIVER0 registers
>> + * are always RAZ/WI. The corresponding functionality is replaced by the
>> + * GICR registers. So it doesn't need to sync them.
>> + */
>> + offset += (1 * sizeof(uint32_t));
>> for_each_dist_irq_reg(irq, s->num_irq, 1) {
>> kvm_gicd_access(s, offset, ®, false);
>> *gic_bmp_ptr32(bmp, irq) = reg;
>> @@ -235,6 +265,14 @@ static void kvm_dist_putbmp(GICv3State *s, uint32_t
>> offset,
>> uint32_t reg;
>> int irq;
>>
>> + /* For the KVM GICv3, affinity routing is always enabled, and the
>> + * GICD_IGROUPR0/GICD_ISENABLER0/GICD_ISPENDR0/GICD_ISACTIVER0 registers
>> + * are always RAZ/WI. The corresponding functionality is replaced by the
>> + * GICR registers. So it doesn't need to sync them.
>> + */
>> + offset += (1 * sizeof(uint32_t));
> I wonder we couldn't create a new for_each_dist_irq_reg() macro taking
> the offset and clroffset and integrating that shift inside?
>
Peter, what's your opinion?
>> + if (clroffset != 0)
> nit style issue: brace needed here
>
OK
> Besides Reviewed-by: Eric Auger <eric.au...@redhat.com>
>
Thanks!
> Thanks
>
> Eric
>
>> + clroffset += (1 * sizeof(uint32_t));
>> for_each_dist_irq_reg(irq, s->num_irq, 1) {
>> /* If this bitmap is a set/clear register pair, first write to the
>> * clear-reg to clear all bits before using the set-reg to write
>>
>
> .
>
--
Shannon
