[PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Marc Zyngier]

The GICv2 can have virtualization extension support, consisting
of an additional set of registers and interrupts. Add the necessary
binding to the GIC DT documentation.

Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/gic.txt |   35 +++++++++++++++++++++++-
 1 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
index 9b4b82a..62eb8df 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -11,7 +11,9 @@ have PPIs or SGIs.
 Main node required properties:
 
 - compatible : should be one of:
+	"arm,cortex-a15-gic"
 	"arm,cortex-a9-gic"
+	"arm,cortex-a7-gic"
 	"arm,arm11mp-gic"
 - interrupt-controller : Identifies the node as an interrupt controller
 - #interrupt-cells : Specifies the number of cells needed to encode an
@@ -39,8 +41,9 @@ Main node required properties:
   the GIC cpu interface register base and size.
 
 Optional
-- interrupts	: Interrupt source of the parent interrupt controller. Only
-  present on secondary GICs.
+- interrupts	: Interrupt source of the parent interrupt controller on
+  secondary GICs, or VGIC maintainance interrupt on primary GIC (see
+  below).
 
 - cpu-offset	: per-cpu offset within the distributor and cpu interface
   regions, used when the GIC doesn't have banked registers. The offset is
@@ -57,3 +60,31 @@ Example:
 		      <0xfff10100 0x100>;
 	};
 
+
+* GIC virtualization extensions (VGIC)
+
+For ARM cores that support the virtualization extensions, additional
+properties must be described (they only exist if the GIC is the
+primary interrupt controller).
+
+Required properties:
+
+- reg : Additional regions specifying the base physical address and
+  size of the VGIC registers. The first additional region is the GIC
+  virtual interface control register base and size. The 2nd additional
+  region is the GIC virtual cpu interface register base and size.
+
+- interrupts : VGIC maintainance interrupt.
+
+Example:
+
+	interrupt-controller@2c001000 {
+		compatible = "arm,cortex-a15-gic";
+		#interrupt-cells = <3>;
+		interrupt-controller;
+		reg = <0x2c001000 0x1000>,
+		      <0x2c002000 0x1000>,
+		      <0x2c004000 0x2000>,
+		      <0x2c006000 0x2000>;
+		interrupts = <1 9 0xf04>;
+	};
-- 
1.7.7.1

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Arnd Bergmann]

On Wednesday 09 May 2012, Marc Zyngier wrote:
> 
> The GICv2 can have virtualization extension support, consisting
> of an additional set of registers and interrupts. Add the necessary
> binding to the GIC DT documentation.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>

Would it make sense to add a way to detect whether a GIC is virtual
or real? Maybe an optional empty "virtual-gic" property or an additional
"compatible" value. Even if we don't need it now, it might come in handy
if we require it already.

	Arnd

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Marc Zyngier]

On Wed, 9 May 2012 19:21:42 +0000, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 09 May 2012, Marc Zyngier wrote:
>> 
>> The GICv2 can have virtualization extension support, consisting
>> of an additional set of registers and interrupts. Add the necessary
>> binding to the GIC DT documentation.
>> 
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> Would it make sense to add a way to detect whether a GIC is virtual
> or real? Maybe an optional empty "virtual-gic" property or an additional
> "compatible" value. Even if we don't need it now, it might come in handy
> if we require it already.

I don't really see a need for this. When running on a virtual machine, the
kernel cannot tell if this is the real thing or not (the virtual CPU
interface looks exactly like the normal one once mapped into the guest
address space).

Or maybe I just didn't get your use case?

        M.
-- 
Fast, cheap, reliable. Pick two.

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Arnd Bergmann]

On Wednesday 09 May 2012, Marc Zyngier wrote:
> On Wed, 9 May 2012 19:21:42 +0000, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Wednesday 09 May 2012, Marc Zyngier wrote:
> >> 
> >> The GICv2 can have virtualization extension support, consisting
> >> of an additional set of registers and interrupts. Add the necessary
> >> binding to the GIC DT documentation.
> >> 
> >> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
> > 
> > Would it make sense to add a way to detect whether a GIC is virtual
> > or real? Maybe an optional empty "virtual-gic" property or an additional
> > "compatible" value. Even if we don't need it now, it might come in handy
> > if we require it already.
> 
> I don't really see a need for this. When running on a virtual machine, the
> kernel cannot tell if this is the real thing or not (the virtual CPU
> interface looks exactly like the normal one once mapped into the guest
> address space).
> 
> Or maybe I just didn't get your use case?

Well, one difference seems to be the VGIC maintainance interrupt that may
or may not be present, and another one the additional registers. Of course
you can imply the type of GIC from the presence of this extra data,
but I think it would be better to make it explicit.

	Arnd

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Marc Zyngier]

On Wed, 9 May 2012 20:27:49 +0000, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Wednesday 09 May 2012, Marc Zyngier wrote:
>> On Wed, 9 May 2012 19:21:42 +0000, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> > On Wednesday 09 May 2012, Marc Zyngier wrote:
>> >> 
>> >> The GICv2 can have virtualization extension support, consisting
>> >> of an additional set of registers and interrupts. Add the necessary
>> >> binding to the GIC DT documentation.
>> >> 
>> >> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
>> > 
>> > Would it make sense to add a way to detect whether a GIC is virtual
>> > or real? Maybe an optional empty "virtual-gic" property or an
>> > additional
>> > "compatible" value. Even if we don't need it now, it might come in
>> > handy
>> > if we require it already.
>> 
>> I don't really see a need for this. When running on a virtual machine,
>> the
>> kernel cannot tell if this is the real thing or not (the virtual CPU
>> interface looks exactly like the normal one once mapped into the guest
>> address space).
>> 
>> Or maybe I just didn't get your use case?
> 
> Well, one difference seems to be the VGIC maintainance interrupt that
may
> or may not be present, and another one the additional registers. Of
course
> you can imply the type of GIC from the presence of this extra data,
> but I think it would be better to make it explicit.

Ah, I see what you mean. But these registers and interrupt are actually
only visible on the hypervizor side. The guest shouldn't see any of this.

I suppose we could have a "arm,has-virt-extensions" property in devices
that implement the virtualization extensions (GIC, timers...).

What do you think?

        M.
-- 
Fast, cheap, reliable. Pick two.

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Arnd Bergmann]

On Wednesday 09 May 2012, Marc Zyngier wrote:
> Ah, I see what you mean. But these registers and interrupt are actually
> only visible on the hypervizor side. The guest shouldn't see any of this.

Rihgt, that makes sense.

> I suppose we could have a "arm,has-virt-extensions" property in devices
> that implement the virtualization extensions (GIC, timers...).
> 
> What do you think?

Sounds good. I don't have a strong preference whether that should be a
property like you suggest or a separate "compatible" value, maybe Rob or
Grant can comment on what they prefer.

On the guest side, is it guaranteed that the virtual GIC looks like
a real GIC without those extensions? If the actual behavior depends on
the hypervisor, it might still make sense to add another flag in there
to tell that it's virtual, even if we don't require it for now.

	Arnd

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Marc Zyngier]

On Wed, 9 May 2012 20:47:06 +0000, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Wednesday 09 May 2012, Marc Zyngier wrote:
>> Ah, I see what you mean. But these registers and interrupt are actually
>> only visible on the hypervizor side. The guest shouldn't see any of
this.
> 
> Rihgt, that makes sense.
> 
>> I suppose we could have a "arm,has-virt-extensions" property in devices
>> that implement the virtualization extensions (GIC, timers...).
>> 
>> What do you think?
> 
> Sounds good. I don't have a strong preference whether that should be a
> property like you suggest or a separate "compatible" value, maybe Rob or
> Grant can comment on what they prefer.

At the moment, the compatible string should be pretty explicit, as it is
not possible to build an A7 or A15 based SoC without the virtualization
extensions.

> On the guest side, is it guaranteed that the virtual GIC looks like
> a real GIC without those extensions? If the actual behavior depends on
> the hypervisor, it might still make sense to add another flag in there
> to tell that it's virtual, even if we don't require it for now.

Nothing is really guaranteed, as the VGIC only exposes a GIC CPU interface
to the guest, and the distributor has to be modeled by the hypervisor. But
if the difference is observable by the guest, is it still a GIC? And how to
differentiate between behavior A and behavior B with a single flag?

My take would be that if the guest can tell the difference, than it
shouldn't be called a GIC, and have separate bindings. Or at least a
different compatible string that we could use to enable quirks in the
driver.

        M.
-- 
Fast, cheap, reliable. Pick two.

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Arnd Bergmann]

On Wednesday 09 May 2012, Marc Zyngier wrote:
> > 
> > Sounds good. I don't have a strong preference whether that should be a
> > property like you suggest or a separate "compatible" value, maybe Rob or
> > Grant can comment on what they prefer.
> 
> At the moment, the compatible string should be pretty explicit, as it is
> not possible to build an A7 or A15 based SoC without the virtualization
> extensions.

Ok, makes sense. Then again, if you run an A15 guests on an A15 host,
the virtual GIC would not have those extensions, right?

> > On the guest side, is it guaranteed that the virtual GIC looks like
> > a real GIC without those extensions? If the actual behavior depends on
> > the hypervisor, it might still make sense to add another flag in there
> > to tell that it's virtual, even if we don't require it for now.
> 
> Nothing is really guaranteed, as the VGIC only exposes a GIC CPU interface
> to the guest, and the distributor has to be modeled by the hypervisor. But
> if the difference is observable by the guest, is it still a GIC? And how to
> differentiate between behavior A and behavior B with a single flag?

Right, in that case, we would probably want a hypervisor specific compatible
property to identify it.
 
> My take would be that if the guest can tell the difference, than it
> shouldn't be called a GIC, and have separate bindings. Or at least a
> different compatible string that we could use to enable quirks in the
> driver.

Ok, good.

	Arnd

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Marc Zyngier]

On 10/05/12 11:53, Arnd Bergmann wrote:
> On Wednesday 09 May 2012, Marc Zyngier wrote:
>>>
>>> Sounds good. I don't have a strong preference whether that should be a
>>> property like you suggest or a separate "compatible" value, maybe Rob or
>>> Grant can comment on what they prefer.
>>
>> At the moment, the compatible string should be pretty explicit, as it is
>> not possible to build an A7 or A15 based SoC without the virtualization
>> extensions.
> 
> Ok, makes sense. Then again, if you run an A15 guests on an A15 host,
> the virtual GIC would not have those extensions, right?

Indeed, they would be inaccessible (the hypervisor should not map them
into the guest address space). I'd expect the guest kernel not to try to
use them anyway, as it wouldn't be able to use the HYP mode.

But again, this is implementation specific.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Grant Likely]

On Wed, May 9, 2012 at 3:34 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On Wed, 9 May 2012 20:47:06 +0000, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> On Wednesday 09 May 2012, Marc Zyngier wrote:
>>> Ah, I see what you mean. But these registers and interrupt are actually
>>> only visible on the hypervizor side. The guest shouldn't see any of
> this.
>>
>> Rihgt, that makes sense.
>>
>>> I suppose we could have a "arm,has-virt-extensions" property in devices
>>> that implement the virtualization extensions (GIC, timers...).
>>>
>>> What do you think?
>>
>> Sounds good. I don't have a strong preference whether that should be a
>> property like you suggest or a separate "compatible" value, maybe Rob or
>> Grant can comment on what they prefer.
>
> At the moment, the compatible string should be pretty explicit, as it is
> not possible to build an A7 or A15 based SoC without the virtualization
> extensions.
>
>> On the guest side, is it guaranteed that the virtual GIC looks like
>> a real GIC without those extensions? If the actual behavior depends on
>> the hypervisor, it might still make sense to add another flag in there
>> to tell that it's virtual, even if we don't require it for now.
>
> Nothing is really guaranteed, as the VGIC only exposes a GIC CPU interface
> to the guest, and the distributor has to be modeled by the hypervisor. But
> if the difference is observable by the guest, is it still a GIC? And how to
> differentiate between behavior A and behavior B with a single flag?
>
> My take would be that if the guest can tell the difference, than it
> shouldn't be called a GIC, and have separate bindings. Or at least a
> different compatible string that we could use to enable quirks in the
> driver.

I would agree with that.

g.

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Grant Likely]

On Wed, May 9, 2012 at 2:47 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Wednesday 09 May 2012, Marc Zyngier wrote:
>> Ah, I see what you mean. But these registers and interrupt are actually
>> only visible on the hypervizor side. The guest shouldn't see any of this.
>
> Rihgt, that makes sense.
>
>> I suppose we could have a "arm,has-virt-extensions" property in devices
>> that implement the virtualization extensions (GIC, timers...).
>>
>> What do you think?
>
> Sounds good. I don't have a strong preference whether that should be a
> property like you suggest or a separate "compatible" value, maybe Rob or
> Grant can comment on what they prefer.

I don't have a strong opinion either way.  This approach looks fine.

g.

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[David Vrabel]

On 09/05/12 18:57, Marc Zyngier wrote:
> The GICv2 can have virtualization extension support, consisting
> of an additional set of registers and interrupts. Add the necessary
> binding to the GIC DT documentation.

This looks fine for Xen.

David


> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  Documentation/devicetree/bindings/arm/gic.txt |   35 +++++++++++++++++++++++-
>  1 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> index 9b4b82a..62eb8df 100644
> --- a/Documentation/devicetree/bindings/arm/gic.txt
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -11,7 +11,9 @@ have PPIs or SGIs.
>  Main node required properties:
>  
>  - compatible : should be one of:
> +	"arm,cortex-a15-gic"
>  	"arm,cortex-a9-gic"
> +	"arm,cortex-a7-gic"
>  	"arm,arm11mp-gic"
>  - interrupt-controller : Identifies the node as an interrupt controller
>  - #interrupt-cells : Specifies the number of cells needed to encode an
> @@ -39,8 +41,9 @@ Main node required properties:
>    the GIC cpu interface register base and size.
>  
>  Optional
> -- interrupts	: Interrupt source of the parent interrupt controller. Only
> -  present on secondary GICs.
> +- interrupts	: Interrupt source of the parent interrupt controller on
> +  secondary GICs, or VGIC maintainance interrupt on primary GIC (see
> +  below).
>  
>  - cpu-offset	: per-cpu offset within the distributor and cpu interface
>    regions, used when the GIC doesn't have banked registers. The offset is
> @@ -57,3 +60,31 @@ Example:
>  		      <0xfff10100 0x100>;
>  	};
>  
> +
> +* GIC virtualization extensions (VGIC)
> +
> +For ARM cores that support the virtualization extensions, additional
> +properties must be described (they only exist if the GIC is the
> +primary interrupt controller).
> +
> +Required properties:
> +
> +- reg : Additional regions specifying the base physical address and
> +  size of the VGIC registers. The first additional region is the GIC
> +  virtual interface control register base and size. The 2nd additional
> +  region is the GIC virtual cpu interface register base and size.
> +
> +- interrupts : VGIC maintainance interrupt.
> +
> +Example:
> +
> +	interrupt-controller@2c001000 {
> +		compatible = "arm,cortex-a15-gic";
> +		#interrupt-cells = <3>;
> +		interrupt-controller;
> +		reg = <0x2c001000 0x1000>,
> +		      <0x2c002000 0x1000>,
> +		      <0x2c004000 0x2000>,
> +		      <0x2c006000 0x2000>;
> +		interrupts = <1 9 0xf04>;
> +	};

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Marc Zyngier]

On 11/05/12 15:14, David Vrabel wrote:
> On 09/05/12 18:57, Marc Zyngier wrote:
>> The GICv2 can have virtualization extension support, consisting
>> of an additional set of registers and interrupts. Add the necessary
>> binding to the GIC DT documentation.
> 
> This looks fine for Xen.

Thanks David. Can I add your Acked-by:?

	M.

> 
> David
> 
> 
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  Documentation/devicetree/bindings/arm/gic.txt |   35 +++++++++++++++++++++++-
>>  1 files changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
>> index 9b4b82a..62eb8df 100644
>> --- a/Documentation/devicetree/bindings/arm/gic.txt
>> +++ b/Documentation/devicetree/bindings/arm/gic.txt
>> @@ -11,7 +11,9 @@ have PPIs or SGIs.
>>  Main node required properties:
>>  
>>  - compatible : should be one of:
>> +	"arm,cortex-a15-gic"
>>  	"arm,cortex-a9-gic"
>> +	"arm,cortex-a7-gic"
>>  	"arm,arm11mp-gic"
>>  - interrupt-controller : Identifies the node as an interrupt controller
>>  - #interrupt-cells : Specifies the number of cells needed to encode an
>> @@ -39,8 +41,9 @@ Main node required properties:
>>    the GIC cpu interface register base and size.
>>  
>>  Optional
>> -- interrupts	: Interrupt source of the parent interrupt controller. Only
>> -  present on secondary GICs.
>> +- interrupts	: Interrupt source of the parent interrupt controller on
>> +  secondary GICs, or VGIC maintainance interrupt on primary GIC (see
>> +  below).
>>  
>>  - cpu-offset	: per-cpu offset within the distributor and cpu interface
>>    regions, used when the GIC doesn't have banked registers. The offset is
>> @@ -57,3 +60,31 @@ Example:
>>  		      <0xfff10100 0x100>;
>>  	};
>>  
>> +
>> +* GIC virtualization extensions (VGIC)
>> +
>> +For ARM cores that support the virtualization extensions, additional
>> +properties must be described (they only exist if the GIC is the
>> +primary interrupt controller).
>> +
>> +Required properties:
>> +
>> +- reg : Additional regions specifying the base physical address and
>> +  size of the VGIC registers. The first additional region is the GIC
>> +  virtual interface control register base and size. The 2nd additional
>> +  region is the GIC virtual cpu interface register base and size.
>> +
>> +- interrupts : VGIC maintainance interrupt.
>> +
>> +Example:
>> +
>> +	interrupt-controller@2c001000 {
>> +		compatible = "arm,cortex-a15-gic";
>> +		#interrupt-cells = <3>;
>> +		interrupt-controller;
>> +		reg = <0x2c001000 0x1000>,
>> +		      <0x2c002000 0x1000>,
>> +		      <0x2c004000 0x2000>,
>> +		      <0x2c006000 0x2000>;
>> +		interrupts = <1 9 0xf04>;
>> +	};
> 
> 


-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[David Vrabel]

On 11/05/12 15:42, Marc Zyngier wrote:
> On 11/05/12 15:14, David Vrabel wrote:
>> On 09/05/12 18:57, Marc Zyngier wrote:
>>> The GICv2 can have virtualization extension support, consisting
>>> of an additional set of registers and interrupts. Add the necessary
>>> binding to the GIC DT documentation.
>>
>> This looks fine for Xen.
> 
> Thanks David. Can I add your Acked-by:?

Sure.

Acked-by: David Vrabel <david.vrabel-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>

David

Re: [PATCH v2] ARM: DT: Add binding for GIC virtualization extentions (VGIC)

[Grant Likely]

On Fri, 11 May 2012 15:43:45 +0100, David Vrabel <david.vrabel-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org> wrote:
> On 11/05/12 15:42, Marc Zyngier wrote:
> > On 11/05/12 15:14, David Vrabel wrote:
> >> On 09/05/12 18:57, Marc Zyngier wrote:
> >>> The GICv2 can have virtualization extension support, consisting
> >>> of an additional set of registers and interrupts. Add the necessary
> >>> binding to the GIC DT documentation.
> >>
> >> This looks fine for Xen.
> > 
> > Thanks David. Can I add your Acked-by:?
> 
> Sure.
> 
> Acked-by: David Vrabel <david.vrabel-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>

Applied, thanks.

g.