Re: [Qemu-arm] [PATCH v3 14/20] intc/arm_gic: Wire the vCPU interface

[Luc Michel]

On 07/12/2018 03:37 PM, Peter Maydell wrote:
> On 29 June 2018 at 14:29, Luc Michel <address@hidden> wrote:
>> Add the read/write functions to handle accesses to the vCPU interface.
>> Those accesses are forwarded to the real CPU interface, with the CPU id
>> being converted to the corresponding vCPU id (vCPU id = CPU id +
>> GIC_NCPU).
>>
>> As for the CPU interface, we create a base region for the vCPU interface
>> that fetches the current vCPU id using the current_cpu global variable, and
>> one mirror region per vCPU which maps to that specific vCPU id. This is
>> required by the GIC architecture specification.
> 
> 
>>  static void arm_gic_realize(DeviceState *dev, Error **errp)
>>  {
>>      /* Device instance realize function for the GIC sysbus device */
>> @@ -1531,7 +1590,7 @@ static void arm_gic_realize(DeviceState *dev, Error 
>> **errp)
>>      }
>>
>>      /* This creates distributor and main CPU interface (s->cpuiomem[0]) */
> 
> Can we also update this comment to indicate what virt-related
> memory regions are being created?
> 
>> -    gic_init_irqs_and_mmio(s, gic_set_irq, gic_ops, NULL);
>> +    gic_init_irqs_and_mmio(s, gic_set_irq, gic_ops, gic_virt_ops);
>>
>>      /* Extra core-specific regions for the CPU interfaces. This is
>>       * necessary for "franken-GIC" implementations, for example on
>> @@ -1547,6 +1606,16 @@ static void arm_gic_realize(DeviceState *dev, Error 
>> **errp)
>>                                &s->backref[i], "gic_cpu", 0x100);
>>          sysbus_init_mmio(sbd, &s->cpuiomem[i+1]);
>>      }
>> +
>> +    if (s->virt_extn) {
> 
> ...and a comment about what these regions are for.
> 
> What requires these per-core regions anyway? There's no way to
> specify them in the device tree bindings for Linux, which AFAIK
> only cares about using the "vcpu i/f for this core" registers.
> I don't think the GIC-400 has these. (It does have per-cpu
> aliases of the GICH_* registers, but this patchset doesn't seem
> to implement those.)
My mistake. I misread the specifications. Those aliases should target
the virtual interfaces, and not the virtual CPU interfaces. I'll change
that.

> 
>> +        for (i = 0; i < s->num_cpu; i++) {
>> +            memory_region_init_io(&s->vcpuiomem[i + 1], OBJECT(s),
>> +                                  &gic_vcpu_ops, &s->backref[i],
>> +                                  "gic_vcpu", 0x2000);
>> +            sysbus_init_mmio(sbd, &s->vcpuiomem[i + 1]);
>> +        }
>> +    }
>> +
>>  }
> 
> thanks
> -- PMM
>

signature.asc
Description: OpenPGP digital signature