On 07/25/2014 09:08 AM, Will Deacon wrote:
> Hi Joel,
>
> On Fri, Jul 25, 2014 at 03:02:58PM +0100, Joel Schopp wrote:
>>>>>> I can't think of any way of determining whether a particular
>>>>>> system gets this right or wrong automatically, which suggests
>>>>>> perhaps we need to allow the device tree to specify that the
>>>>>> GICV is 64k-page-safe...
>>>>> When we support such systems, I also think we'll need a device-tree change.
>>>>> My main concern right now is stopping the ability to hose the entire machine
>>>>> by trying to instantiate a virtual GIC.
>>>> ...I don't see how your patch prevents instantiating a VGIC
>>>> and hosing the machine on a system where the 64K
>>>> with the GICV registers in it goes
>>>> [GICV registers] [machine blows up if you read this]
>>>> 0K 8K 64K
>>> True, if such a machine existed, then this patch wouldn't detect it. I don't
>>> think we support anything like that in mainline at the moment, but the
>>> following additional diff should solve the problem, no?
>>>
>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>> index fa9a95b3ed19..476d3bf540a8 100644
>>> --- a/virt/kvm/arm/vgic.c
>>> +++ b/virt/kvm/arm/vgic.c
>>> @@ -1539,6 +1539,14 @@ int kvm_vgic_hyp_init(void)
>>> goto out_unmap;
>>> }
>>>
>>> + if (!PAGE_ALIGNED(resource_size(&vcpu_res))) {
>>> + kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n",
>>> + (unsigned long long)resource_size(&vcpu_res),
>>> + PAGE_SIZE);
>>> + ret = -ENXIO;
>>> + goto out_unmap;
>>> + }
>>> +
>>> vgic_vcpu_base = vcpu_res.start;
>>>
>>> kvm_info("%s@%llx IRQ%d\n", vgic_node->name,
>> This would break with my SOC device tree which looks like this. Note
>> this device tree works just fine without checks.
>>
>> gic: interrupt-controller@e1101000 {
>> compatible = "arm,gic-400-v2m";
>> #interrupt-cells = <3>;
>> #address-cells = <0>;
>> interrupt-controller;
>> msi-controller;
>> reg = <0x0 0xe1110000 0 0x1000>, /* gic dist */
>> <0x0 0xe112f000 0 0x2000>, /* gic cpu */
>> <0x0 0xe114f000 0 0x2000>, /* gic virtual ic*/
>> <0x0 0xe116f000 0 0x2000>, /* gic virtual cpu*/
>> <0x0 0xe1180000 0 0x1000>; /* gic msi */
>> interrupts = <1 8 0xf04>;
>> };
> I appreciate it may work, but that's only because the kernel is actually
> using an alias of GICV at 0xe1160000 by accident. I would say that you're
> getting away with passing an incorrect description.
The problem is that by the spec the size is 0x2000 and was never
properly rearchitected from 4K to variable page size support. The
workaround is that each of the 4K in the 64K page (16 of them) are
really mapped to the same location in hardware. So the contents of
0xe1160000 is the same as the contents of 0xe116f000. See “Appendix F:
GIC-400 and 64KB Translation Granule” in v2.1 of the ARM _Server Base
System Architecture_ specification.
Now if we were to change the entry to <0x0 0xe1160000 0 0x2000> it would
be obviously broken because the second half would map to a duplicate of
the first half.
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html