| Message ID | 1375048598-15637-2-git-send-email-robherring2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Rob, On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: > From: Rob Herring <rob.herring@calxeda.com> > > The PSCI spec from ARM has been updated to 0.2 version. Update the > binding document to reflect the spec changes. For the binding, the > major changes are addition of system reset and poweroff functions. > The recommended function id numbering has also changed. > > Signed-off-by: Rob Herring <rob.herring@calxeda.com> > Cc: devicetree@vger.kernel.org > --- > Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------ > 1 file changed, 19 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt > index 433afe9..b8b4d9f 100644 > --- a/Documentation/devicetree/bindings/arm/psci.txt > +++ b/Documentation/devicetree/bindings/arm/psci.txt > @@ -21,7 +21,7 @@ to #0. > > Main node required properties: > > - - compatible : Must be "arm,psci" > + - compatible : Must be "arm,psci-0.2" or "arm,psci" For the purposes of handling different firmware implementations (which may have different bugs to work around and may require different arguments to cpu_suspend), it may be better to state "must contain" rather than "must be". > > - method : The method of calling the PSCI firmware. Permitted > values are: > @@ -32,6 +32,9 @@ Main node required properties: > "hvc" : HVC #0, with the register assignments specified > in this binding. > > + - psci_version : Function ID for PSCI_VERSION operation. Required for > + "arm,psci-0.2" compatible version or later. > + > Main node optional properties: > > - cpu_suspend : Function ID for CPU_SUSPEND operation The low bits of CPU_SUSPEND's power_state argument are platform specific and we'll need to deal with them if an implementation actually uses them for something. We can probably associate this with a specific implementation's compatible string, as we'll almost certainly need special code to handle the differences between them. If we have a compatible string for the first platform that ignores the argument (or just uses zero), that can be shared by all those implementations that don't give any fine-grained control here. > @@ -42,14 +45,24 @@ Main node optional properties: > > - migrate : Function ID for MIGRATE operation > > + - system_reset : Function ID for SYSTEM_RESET operation > + > + - system_off : Function ID for SYSTEM_OFF operation > + > > Example: > > psci { > - compatible = "arm,psci"; > + compatible = "arm,psci-0.2"; > method = "smc"; > - cpu_suspend = <0x95c10000>; > - cpu_off = <0x95c10001>; > - cpu_on = <0x95c10002>; > - migrate = <0x95c10003>; > + psci_version = <0x84000000>; > + cpu_suspend = <0x84000001>; > + cpu_off = <0x84000002>; > + cpu_on = <0x84000003>; > + affinity_info = <0x84000004>; > + migrate = <0x84000005>; > + migrate_info_type = <0x84000006>; > + migrate_info_up_cpu = <0x84000007>; > + system_off = <0x84000008>; > + system_reset = <0x84000009>; > }; One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't. Thanks, Mark.
On 07/29/2013 05:13 AM, Mark Rutland wrote: > Hi Rob, > > On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: >> From: Rob Herring <rob.herring@calxeda.com> >> >> The PSCI spec from ARM has been updated to 0.2 version. Update the >> binding document to reflect the spec changes. For the binding, the >> major changes are addition of system reset and poweroff functions. >> The recommended function id numbering has also changed. >> >> Signed-off-by: Rob Herring <rob.herring@calxeda.com> >> Cc: devicetree@vger.kernel.org >> --- >> Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------ >> 1 file changed, 19 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt >> index 433afe9..b8b4d9f 100644 >> --- a/Documentation/devicetree/bindings/arm/psci.txt >> +++ b/Documentation/devicetree/bindings/arm/psci.txt >> @@ -21,7 +21,7 @@ to #0. >> >> Main node required properties: >> >> - - compatible : Must be "arm,psci" >> + - compatible : Must be "arm,psci-0.2" or "arm,psci" > > For the purposes of handling different firmware implementations (which > may have different bugs to work around and may require different > arguments to cpu_suspend), it may be better to state "must contain" > rather than "must be". > >> >> - method : The method of calling the PSCI firmware. Permitted >> values are: >> @@ -32,6 +32,9 @@ Main node required properties: >> "hvc" : HVC #0, with the register assignments specified >> in this binding. >> >> + - psci_version : Function ID for PSCI_VERSION operation. Required for >> + "arm,psci-0.2" compatible version or later. >> + >> Main node optional properties: >> >> - cpu_suspend : Function ID for CPU_SUSPEND operation > > The low bits of CPU_SUSPEND's power_state argument are platform specific > and we'll need to deal with them if an implementation actually uses them > for something. We can probably associate this with a specific > implementation's compatible string, as we'll almost certainly need > special code to handle the differences between them. I'm not a fan of > If we have a compatible string for the first platform that ignores the > argument (or just uses zero), that can be shared by all those > implementations that don't give any fine-grained control here. > >> @@ -42,14 +45,24 @@ Main node optional properties: >> >> - migrate : Function ID for MIGRATE operation >> >> + - system_reset : Function ID for SYSTEM_RESET operation >> + >> + - system_off : Function ID for SYSTEM_OFF operation >> + >> >> Example: >> >> psci { >> - compatible = "arm,psci"; >> + compatible = "arm,psci-0.2"; >> method = "smc"; >> - cpu_suspend = <0x95c10000>; >> - cpu_off = <0x95c10001>; >> - cpu_on = <0x95c10002>; >> - migrate = <0x95c10003>; >> + psci_version = <0x84000000>; >> + cpu_suspend = <0x84000001>; >> + cpu_off = <0x84000002>; >> + cpu_on = <0x84000003>; >> + affinity_info = <0x84000004>; >> + migrate = <0x84000005>; >> + migrate_info_type = <0x84000006>; >> + migrate_info_up_cpu = <0x84000007>; >> + system_off = <0x84000008>; >> + system_reset = <0x84000009>; >> }; > > One of the things changed in PSCI 0.2 was the SMC calling convention, > though this isn't clear in the PSCI document. The function IDs for 32bit > and 64bit callers may differ, and we need to support describing an > arbitrary configuration of the two (same ID for both, different across > 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). > > I'd like to ensure the binding can deal with that from the start. We > could do this by having -32 and -64 variants of each function id (e.g. > cpu_off-64) , if the IDs actually differ, and use the regular combined > ID if they don't. Uggg. I guess I should have read the SMC calling convention doc... I was simply documenting what is already in the PSCI doc, but obviously that is not fully flushed out. How about something like this (for the complicated case of both 32 and 64 bit): method = "smc", "smc64"; psci_version = <0x84000000 0xc4000000>; cpu_suspend = <0x84000001 0xc4000001>; cpu_off = <0x84000002 0xc4000002>; cpu_on = <0x84000003 0xc4000003>; "smc" is a synonym for smc32 for compatibility. The number and order of methods determines the number and order of function IDs. A variation on this would be keep method as is and add a "#psci-cells" property to specify the number of function IDs. You can determine the 64-bit vs. 32-bit support based on the function ID itself. Rob
On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote: > On 07/29/2013 05:13 AM, Mark Rutland wrote: > > Hi Rob, > > > > On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: > >> From: Rob Herring <rob.herring@calxeda.com> > >> > >> The PSCI spec from ARM has been updated to 0.2 version. Update the > >> binding document to reflect the spec changes. For the binding, the > >> major changes are addition of system reset and poweroff functions. > >> The recommended function id numbering has also changed. > >> > >> Signed-off-by: Rob Herring <rob.herring@calxeda.com> > >> Cc: devicetree@vger.kernel.org > >> --- > >> Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------ > >> 1 file changed, 19 insertions(+), 6 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt > >> index 433afe9..b8b4d9f 100644 > >> --- a/Documentation/devicetree/bindings/arm/psci.txt > >> +++ b/Documentation/devicetree/bindings/arm/psci.txt > >> @@ -21,7 +21,7 @@ to #0. > >> > >> Main node required properties: > >> > >> - - compatible : Must be "arm,psci" > >> + - compatible : Must be "arm,psci-0.2" or "arm,psci" > > > > For the purposes of handling different firmware implementations (which > > may have different bugs to work around and may require different > > arguments to cpu_suspend), it may be better to state "must contain" > > rather than "must be". > > > >> > >> - method : The method of calling the PSCI firmware. Permitted > >> values are: > >> @@ -32,6 +32,9 @@ Main node required properties: > >> "hvc" : HVC #0, with the register assignments specified > >> in this binding. > >> > >> + - psci_version : Function ID for PSCI_VERSION operation. Required for > >> + "arm,psci-0.2" compatible version or later. > >> + > >> Main node optional properties: > >> > >> - cpu_suspend : Function ID for CPU_SUSPEND operation > > > > The low bits of CPU_SUSPEND's power_state argument are platform specific > > and we'll need to deal with them if an implementation actually uses them > > for something. We can probably associate this with a specific > > implementation's compatible string, as we'll almost certainly need > > special code to handle the differences between them. > > I'm not a fan of Me neither. The only alternative I can think of would rely on platform information from elsewhere to let you know how to call cpu_suspend, (e.g. a board's compatible string). As far as I can see, without some knowledge of the platform you can't use CPU_SUSPEND safely. > > > If we have a compatible string for the first platform that ignores the > > argument (or just uses zero), that can be shared by all those > > implementations that don't give any fine-grained control here. > > > >> @@ -42,14 +45,24 @@ Main node optional properties: > >> > >> - migrate : Function ID for MIGRATE operation > >> > >> + - system_reset : Function ID for SYSTEM_RESET operation > >> + > >> + - system_off : Function ID for SYSTEM_OFF operation > >> + > >> > >> Example: > >> > >> psci { > >> - compatible = "arm,psci"; > >> + compatible = "arm,psci-0.2"; > >> method = "smc"; > >> - cpu_suspend = <0x95c10000>; > >> - cpu_off = <0x95c10001>; > >> - cpu_on = <0x95c10002>; > >> - migrate = <0x95c10003>; > >> + psci_version = <0x84000000>; > >> + cpu_suspend = <0x84000001>; > >> + cpu_off = <0x84000002>; > >> + cpu_on = <0x84000003>; > >> + affinity_info = <0x84000004>; > >> + migrate = <0x84000005>; > >> + migrate_info_type = <0x84000006>; > >> + migrate_info_up_cpu = <0x84000007>; > >> + system_off = <0x84000008>; > >> + system_reset = <0x84000009>; > >> }; > > > > One of the things changed in PSCI 0.2 was the SMC calling convention, > > though this isn't clear in the PSCI document. The function IDs for 32bit > > and 64bit callers may differ, and we need to support describing an > > arbitrary configuration of the two (same ID for both, different across > > 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). > > > > I'd like to ensure the binding can deal with that from the start. We > > could do this by having -32 and -64 variants of each function id (e.g. > > cpu_off-64) , if the IDs actually differ, and use the regular combined > > ID if they don't. > > Uggg. I guess I should have read the SMC calling convention doc... I was > simply documenting what is already in the PSCI doc, but obviously that > is not fully flushed out. > > How about something like this (for the complicated case of both 32 and > 64 bit): > > method = "smc", "smc64"; > psci_version = <0x84000000 0xc4000000>; > cpu_suspend = <0x84000001 0xc4000001>; > cpu_off = <0x84000002 0xc4000002>; > cpu_on = <0x84000003 0xc4000003>; > > "smc" is a synonym for smc32 for compatibility. The number and order of > methods determines the number and order of function IDs. While this may be compatible with the arm implementation, it won't be compatible with the arm64 implementation, which assumes smc64 by default. As far as I am aware, the implementations currently in use (KVM and Xen) use the same ID for both, so I think "smc" should cover an ID valid for a native register width calling convention, and "smc64" and "smc32" describing values only valid for 64-bit wide and 32-bit wide calling conventions respectively. I've added Christoffer, Marc, and Stefano to Cc in case they have any comments. > > A variation on this would be keep method as is and add a "#psci-cells" > property to specify the number of function IDs. You can determine the > 64-bit vs. 32-bit support based on the function ID itself. I don't think that's a good idea - part of the reasoning for specifying the IDs is to cater for those not aligned with the ID guidelines in the spec, so we can't assume their choice of ID value gives us any useful information as to how they may be used. Thanks, Mark.
On Mon, Jul 29, 2013 at 03:18:43PM -0500, Rob Herring wrote: > On 07/29/2013 05:13 AM, Mark Rutland wrote: > > Hi Rob, > > > > On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: > >> From: Rob Herring <rob.herring@calxeda.com> > >> > >> The PSCI spec from ARM has been updated to 0.2 version. Update the > >> binding document to reflect the spec changes. For the binding, the > >> major changes are addition of system reset and poweroff functions. > >> The recommended function id numbering has also changed. > >> > >> Signed-off-by: Rob Herring <rob.herring@calxeda.com> > >> Cc: devicetree@vger.kernel.org > >> --- > >> Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------ > >> 1 file changed, 19 insertions(+), 6 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt > >> index 433afe9..b8b4d9f 100644 > >> --- a/Documentation/devicetree/bindings/arm/psci.txt > >> +++ b/Documentation/devicetree/bindings/arm/psci.txt > >> @@ -21,7 +21,7 @@ to #0. > >> > >> Main node required properties: > >> > >> - - compatible : Must be "arm,psci" > >> + - compatible : Must be "arm,psci-0.2" or "arm,psci" > > > > For the purposes of handling different firmware implementations (which > > may have different bugs to work around and may require different > > arguments to cpu_suspend), it may be better to state "must contain" > > rather than "must be". > > > >> > >> - method : The method of calling the PSCI firmware. Permitted > >> values are: > >> @@ -32,6 +32,9 @@ Main node required properties: > >> "hvc" : HVC #0, with the register assignments specified > >> in this binding. > >> > >> + - psci_version : Function ID for PSCI_VERSION operation. Required for > >> + "arm,psci-0.2" compatible version or later. > >> + > >> Main node optional properties: > >> > >> - cpu_suspend : Function ID for CPU_SUSPEND operation > > > > The low bits of CPU_SUSPEND's power_state argument are platform specific > > and we'll need to deal with them if an implementation actually uses them > > for something. We can probably associate this with a specific > > implementation's compatible string, as we'll almost certainly need > > special code to handle the differences between them. > > I'm not a fan of ...what? > > > If we have a compatible string for the first platform that ignores the > > argument (or just uses zero), that can be shared by all those > > implementations that don't give any fine-grained control here. > > > >> @@ -42,14 +45,24 @@ Main node optional properties: > >> > >> - migrate : Function ID for MIGRATE operation > >> > >> + - system_reset : Function ID for SYSTEM_RESET operation > >> + > >> + - system_off : Function ID for SYSTEM_OFF operation > >> + > >> > >> Example: > >> > >> psci { > >> - compatible = "arm,psci"; > >> + compatible = "arm,psci-0.2"; > >> method = "smc"; > >> - cpu_suspend = <0x95c10000>; > >> - cpu_off = <0x95c10001>; > >> - cpu_on = <0x95c10002>; > >> - migrate = <0x95c10003>; > >> + psci_version = <0x84000000>; > >> + cpu_suspend = <0x84000001>; > >> + cpu_off = <0x84000002>; > >> + cpu_on = <0x84000003>; > >> + affinity_info = <0x84000004>; > >> + migrate = <0x84000005>; > >> + migrate_info_type = <0x84000006>; > >> + migrate_info_up_cpu = <0x84000007>; > >> + system_off = <0x84000008>; > >> + system_reset = <0x84000009>; > >> }; > > > > One of the things changed in PSCI 0.2 was the SMC calling convention, > > though this isn't clear in the PSCI document. The function IDs for 32bit > > and 64bit callers may differ, and we need to support describing an > > arbitrary configuration of the two (same ID for both, different across > > 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). > > > > I'd like to ensure the binding can deal with that from the start. We > > could do this by having -32 and -64 variants of each function id (e.g. > > cpu_off-64) , if the IDs actually differ, and use the regular combined > > ID if they don't. > > Uggg. I guess I should have read the SMC calling convention doc... I was > simply documenting what is already in the PSCI doc, but obviously that > is not fully flushed out. > > How about something like this (for the complicated case of both 32 and > 64 bit): > > method = "smc", "smc64"; > psci_version = <0x84000000 0xc4000000>; > cpu_suspend = <0x84000001 0xc4000001>; > cpu_off = <0x84000002 0xc4000002>; > cpu_on = <0x84000003 0xc4000003>; > > "smc" is a synonym for smc32 for compatibility. The number and order of > methods determines the number and order of function IDs. > > A variation on this would be keep method as is and add a "#psci-cells" > property to specify the number of function IDs. You can determine the > 64-bit vs. 32-bit support based on the function ID itself. Just to point out, these arguments all apply equally to hvc. So, if there is an "smc64" method, we should likewise define "hvc64" etc, with parallel meaning. Cheers ---Dave
On 07/30/2013 04:49 AM, Mark Rutland wrote: > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote: >> On 07/29/2013 05:13 AM, Mark Rutland wrote: >>> Hi Rob, >>> >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: >>>> From: Rob Herring <rob.herring@calxeda.com> [snip] >>> One of the things changed in PSCI 0.2 was the SMC calling convention, >>> though this isn't clear in the PSCI document. The function IDs for 32bit >>> and 64bit callers may differ, and we need to support describing an >>> arbitrary configuration of the two (same ID for both, different across >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). >>> >>> I'd like to ensure the binding can deal with that from the start. We >>> could do this by having -32 and -64 variants of each function id (e.g. >>> cpu_off-64) , if the IDs actually differ, and use the regular combined >>> ID if they don't. >> >> Uggg. I guess I should have read the SMC calling convention doc... I was >> simply documenting what is already in the PSCI doc, but obviously that >> is not fully flushed out. >> >> How about something like this (for the complicated case of both 32 and >> 64 bit): >> >> method = "smc", "smc64"; >> psci_version = <0x84000000 0xc4000000>; >> cpu_suspend = <0x84000001 0xc4000001>; >> cpu_off = <0x84000002 0xc4000002>; >> cpu_on = <0x84000003 0xc4000003>; >> >> "smc" is a synonym for smc32 for compatibility. The number and order of >> methods determines the number and order of function IDs. > > While this may be compatible with the arm implementation, it won't be > compatible with the arm64 implementation, which assumes smc64 by > default. > > As far as I am aware, the implementations currently in use (KVM and Xen) > use the same ID for both, so I think "smc" should cover an ID valid for > a native register width calling convention, and "smc64" and "smc32" > describing values only valid for 64-bit wide and 32-bit wide calling > conventions respectively. The problem is that does not work for a 32-bit kernel on 64-bit h/w as native from the dts perspective is smc64. Just like the cpu bindings, the binding cannot change based on 32 or 64 bit OS. I don't think we really have to deal with that here. We can simply say "smc" is only for "arm,psci" and deprecated for "arm,psci-0.2". > I've added Christoffer, Marc, and Stefano to Cc in case they have any > comments. > >> >> A variation on this would be keep method as is and add a "#psci-cells" >> property to specify the number of function IDs. You can determine the >> 64-bit vs. 32-bit support based on the function ID itself. > > I don't think that's a good idea - part of the reasoning for specifying > the IDs is to cater for those not aligned with the ID guidelines in the > spec, so we can't assume their choice of ID value gives us any useful > information as to how they may be used. Kind of pointless to encode information into the IDs if you cannot rely on that... Rob
On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote: > On 07/30/2013 04:49 AM, Mark Rutland wrote: > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote: > >> On 07/29/2013 05:13 AM, Mark Rutland wrote: > >>> Hi Rob, > >>> > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: > >>>> From: Rob Herring <rob.herring@calxeda.com> > > [snip] > > >>> One of the things changed in PSCI 0.2 was the SMC calling convention, > >>> though this isn't clear in the PSCI document. The function IDs for 32bit > >>> and 64bit callers may differ, and we need to support describing an > >>> arbitrary configuration of the two (same ID for both, different across > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). > >>> > >>> I'd like to ensure the binding can deal with that from the start. We > >>> could do this by having -32 and -64 variants of each function id (e.g. > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined > >>> ID if they don't. > >> > >> Uggg. I guess I should have read the SMC calling convention doc... I was > >> simply documenting what is already in the PSCI doc, but obviously that > >> is not fully flushed out. > >> > >> How about something like this (for the complicated case of both 32 and > >> 64 bit): > >> > >> method = "smc", "smc64"; > >> psci_version = <0x84000000 0xc4000000>; > >> cpu_suspend = <0x84000001 0xc4000001>; > >> cpu_off = <0x84000002 0xc4000002>; > >> cpu_on = <0x84000003 0xc4000003>; > >> > >> "smc" is a synonym for smc32 for compatibility. The number and order of > >> methods determines the number and order of function IDs. > > > > While this may be compatible with the arm implementation, it won't be > > compatible with the arm64 implementation, which assumes smc64 by > > default. > > > > As far as I am aware, the implementations currently in use (KVM and Xen) > > use the same ID for both, so I think "smc" should cover an ID valid for > > a native register width calling convention, and "smc64" and "smc32" > > describing values only valid for 64-bit wide and 32-bit wide calling > > conventions respectively. > > The problem is that does not work for a 32-bit kernel on 64-bit h/w as > native from the dts perspective is smc64. Just like the cpu bindings, > the binding cannot change based on 32 or 64 bit OS. I don't think we > really have to deal with that here. We can simply say "smc" is only for > "arm,psci" and deprecated for "arm,psci-0.2". Agreed. I'd be happy with only having "smc32" and "smc64" for "arm,psci-0.2". Thanks, Mark.
On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote: > On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote: > > On 07/30/2013 04:49 AM, Mark Rutland wrote: > > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote: > > >> On 07/29/2013 05:13 AM, Mark Rutland wrote: > > >>> Hi Rob, > > >>> > > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: > > >>>> From: Rob Herring <rob.herring@calxeda.com> > > > > [snip] > > > > >>> One of the things changed in PSCI 0.2 was the SMC calling convention, > > >>> though this isn't clear in the PSCI document. The function IDs for 32bit > > >>> and 64bit callers may differ, and we need to support describing an > > >>> arbitrary configuration of the two (same ID for both, different across > > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). > > >>> > > >>> I'd like to ensure the binding can deal with that from the start. We > > >>> could do this by having -32 and -64 variants of each function id (e.g. > > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined > > >>> ID if they don't. > > >> > > >> Uggg. I guess I should have read the SMC calling convention doc... I was > > >> simply documenting what is already in the PSCI doc, but obviously that > > >> is not fully flushed out. > > >> > > >> How about something like this (for the complicated case of both 32 and > > >> 64 bit): > > >> > > >> method = "smc", "smc64"; > > >> psci_version = <0x84000000 0xc4000000>; > > >> cpu_suspend = <0x84000001 0xc4000001>; > > >> cpu_off = <0x84000002 0xc4000002>; > > >> cpu_on = <0x84000003 0xc4000003>; > > >> > > >> "smc" is a synonym for smc32 for compatibility. The number and order of > > >> methods determines the number and order of function IDs. > > > > > > While this may be compatible with the arm implementation, it won't be > > > compatible with the arm64 implementation, which assumes smc64 by > > > default. > > > > > > As far as I am aware, the implementations currently in use (KVM and Xen) > > > use the same ID for both, so I think "smc" should cover an ID valid for > > > a native register width calling convention, and "smc64" and "smc32" > > > describing values only valid for 64-bit wide and 32-bit wide calling > > > conventions respectively. > > > > The problem is that does not work for a 32-bit kernel on 64-bit h/w as > > native from the dts perspective is smc64. Just like the cpu bindings, > > the binding cannot change based on 32 or 64 bit OS. I don't think we > > really have to deal with that here. We can simply say "smc" is only for > > "arm,psci" and deprecated for "arm,psci-0.2". > > Agreed. I'd be happy with only having "smc32" and "smc64" for > "arm,psci-0.2". Actually, from some quick discussion with Marc and Dave, I think we can handle this better, in a way that leaves this backwards compatible. Rather than relying on the method string to tell us the calling convention, I think we should rely on the function ID, as I proposed earlier. The existing function ids provided in the "arm,psci" binding are implicitly relying on the PSCI implementation to detect the register width and act accordingly. This is trivially true on 32bit hardware, KVM (where the same ID is used for 32-bit and 64-bit guests), and while I'm not entirely sure about Xen I believe it's true there. We can make this explicit as we extend the binding. Having a -64 and -32 variant of each ID (while not pretty) allows us to add additional IDs for functions that might only have a 32-bit or 64-bit interface implemented, in addition to functions with common IDs: psci { compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci"; cpu_off = <12345678>; cpu_on = <01234567>; system_reset-32 = <02222222>; system_reset-64 = <12222222>; affinity_info-64 = <15555555>; }; This means that hypervisors could update their PSCI implementation while keeping their DTS compatible with existing kernels. Thanks, Mark.
On Tue, 30 Jul 2013, Mark Rutland wrote: > On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote: > > On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote: > > > On 07/30/2013 04:49 AM, Mark Rutland wrote: > > > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote: > > > >> On 07/29/2013 05:13 AM, Mark Rutland wrote: > > > >>> Hi Rob, > > > >>> > > > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: > > > >>>> From: Rob Herring <rob.herring@calxeda.com> > > > > > > [snip] > > > > > > >>> One of the things changed in PSCI 0.2 was the SMC calling convention, > > > >>> though this isn't clear in the PSCI document. The function IDs for 32bit > > > >>> and 64bit callers may differ, and we need to support describing an > > > >>> arbitrary configuration of the two (same ID for both, different across > > > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). > > > >>> > > > >>> I'd like to ensure the binding can deal with that from the start. We > > > >>> could do this by having -32 and -64 variants of each function id (e.g. > > > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined > > > >>> ID if they don't. > > > >> > > > >> Uggg. I guess I should have read the SMC calling convention doc... I was > > > >> simply documenting what is already in the PSCI doc, but obviously that > > > >> is not fully flushed out. > > > >> > > > >> How about something like this (for the complicated case of both 32 and > > > >> 64 bit): > > > >> > > > >> method = "smc", "smc64"; > > > >> psci_version = <0x84000000 0xc4000000>; > > > >> cpu_suspend = <0x84000001 0xc4000001>; > > > >> cpu_off = <0x84000002 0xc4000002>; > > > >> cpu_on = <0x84000003 0xc4000003>; > > > >> > > > >> "smc" is a synonym for smc32 for compatibility. The number and order of > > > >> methods determines the number and order of function IDs. > > > > > > > > While this may be compatible with the arm implementation, it won't be > > > > compatible with the arm64 implementation, which assumes smc64 by > > > > default. > > > > > > > > As far as I am aware, the implementations currently in use (KVM and Xen) > > > > use the same ID for both, so I think "smc" should cover an ID valid for > > > > a native register width calling convention, and "smc64" and "smc32" > > > > describing values only valid for 64-bit wide and 32-bit wide calling > > > > conventions respectively. > > > > > > The problem is that does not work for a 32-bit kernel on 64-bit h/w as > > > native from the dts perspective is smc64. Just like the cpu bindings, > > > the binding cannot change based on 32 or 64 bit OS. I don't think we > > > really have to deal with that here. We can simply say "smc" is only for > > > "arm,psci" and deprecated for "arm,psci-0.2". > > > > Agreed. I'd be happy with only having "smc32" and "smc64" for > > "arm,psci-0.2". I would be happy with that too (Xen only uses HVC as "method"). > Actually, from some quick discussion with Marc and Dave, I think we can > handle this better, in a way that leaves this backwards compatible. > > Rather than relying on the method string to tell us the calling > convention, I think we should rely on the function ID, as I proposed > earlier. The existing function ids provided in the "arm,psci" binding > are implicitly relying on the PSCI implementation to detect the register > width and act accordingly. This is trivially true on 32bit hardware, KVM > (where the same ID is used for 32-bit and 64-bit guests), and while I'm > not entirely sure about Xen I believe it's true there. We can make this > explicit as we extend the binding. > > Having a -64 and -32 variant of each ID (while not pretty) allows us to > add additional IDs for functions that might only have a 32-bit or 64-bit > interface implemented, in addition to functions with common IDs: > > psci { > compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci"; > cpu_off = <12345678>; > cpu_on = <01234567>; > system_reset-32 = <02222222>; > system_reset-64 = <12222222>; > affinity_info-64 = <15555555>; > }; > > This means that hypervisors could update their PSCI implementation while > keeping their DTS compatible with existing kernels. Are you proposing of getting rid of "method" completely and therefore have 4 possible function IDs for each function: system_reset-32-HVC system_reset-64-SMC system_reset-32-HVC system_reset-64-SMC or are you proposing of choosing just the register width via function IDs? Honestly I think it would be cleaner to introduce a new field called "width" can that be 32 or 64 and represent the register width, rather than having an explosion of function IDs.
On Tue, 2013-07-30 at 15:33 +0100, Stefano Stabellini wrote: > On Tue, 30 Jul 2013, Mark Rutland wrote: > > On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote: > > > On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote: > > > > On 07/30/2013 04:49 AM, Mark Rutland wrote: > > > > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote: > > > > >> On 07/29/2013 05:13 AM, Mark Rutland wrote: > > > > >>> Hi Rob, > > > > >>> > > > > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: > > > > >>>> From: Rob Herring <rob.herring@calxeda.com> > > > > > > > > [snip] > > > > > > > > >>> One of the things changed in PSCI 0.2 was the SMC calling convention, > > > > >>> though this isn't clear in the PSCI document. The function IDs for 32bit > > > > >>> and 64bit callers may differ, and we need to support describing an > > > > >>> arbitrary configuration of the two (same ID for both, different across > > > > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). > > > > >>> > > > > >>> I'd like to ensure the binding can deal with that from the start. We > > > > >>> could do this by having -32 and -64 variants of each function id (e.g. > > > > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined > > > > >>> ID if they don't. > > > > >> > > > > >> Uggg. I guess I should have read the SMC calling convention doc... I was > > > > >> simply documenting what is already in the PSCI doc, but obviously that > > > > >> is not fully flushed out. > > > > >> > > > > >> How about something like this (for the complicated case of both 32 and > > > > >> 64 bit): > > > > >> > > > > >> method = "smc", "smc64"; > > > > >> psci_version = <0x84000000 0xc4000000>; > > > > >> cpu_suspend = <0x84000001 0xc4000001>; > > > > >> cpu_off = <0x84000002 0xc4000002>; > > > > >> cpu_on = <0x84000003 0xc4000003>; > > > > >> > > > > >> "smc" is a synonym for smc32 for compatibility. The number and order of > > > > >> methods determines the number and order of function IDs. > > > > > > > > > > While this may be compatible with the arm implementation, it won't be > > > > > compatible with the arm64 implementation, which assumes smc64 by > > > > > default. > > > > > > > > > > As far as I am aware, the implementations currently in use (KVM and Xen) > > > > > use the same ID for both, so I think "smc" should cover an ID valid for > > > > > a native register width calling convention, and "smc64" and "smc32" > > > > > describing values only valid for 64-bit wide and 32-bit wide calling > > > > > conventions respectively. > > > > > > > > The problem is that does not work for a 32-bit kernel on 64-bit h/w as > > > > native from the dts perspective is smc64. Just like the cpu bindings, > > > > the binding cannot change based on 32 or 64 bit OS. I don't think we > > > > really have to deal with that here. We can simply say "smc" is only for > > > > "arm,psci" and deprecated for "arm,psci-0.2". > > > > > > Agreed. I'd be happy with only having "smc32" and "smc64" for > > > "arm,psci-0.2". > > I would be happy with that too (Xen only uses HVC as "method"). Presumably we have the same issues with hvc vs hvc32 vs hvc64 though? And does smcX imply hvcX or does that also need to be documented here? > > > Actually, from some quick discussion with Marc and Dave, I think we can > > handle this better, in a way that leaves this backwards compatible. > > > > Rather than relying on the method string to tell us the calling > > convention, I think we should rely on the function ID, as I proposed > > earlier. The existing function ids provided in the "arm,psci" binding > > are implicitly relying on the PSCI implementation to detect the register > > width and act accordingly. This is trivially true on 32bit hardware, KVM > > (where the same ID is used for 32-bit and 64-bit guests), and while I'm > > not entirely sure about Xen I believe it's true there. We can make this > > explicit as we extend the binding. > > > > Having a -64 and -32 variant of each ID (while not pretty) allows us to > > add additional IDs for functions that might only have a 32-bit or 64-bit > > interface implemented, in addition to functions with common IDs: > > > > psci { > > compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci"; > > cpu_off = <12345678>; > > cpu_on = <01234567>; > > system_reset-32 = <02222222>; > > system_reset-64 = <12222222>; > > affinity_info-64 = <15555555>; > > }; > > > > This means that hypervisors could update their PSCI implementation while > > keeping their DTS compatible with existing kernels. > > Are you proposing of getting rid of "method" completely and therefore > have 4 possible function IDs for each function: > > system_reset-32-HVC > system_reset-64-SMC > system_reset-32-HVC > system_reset-64-SMC > > or are you proposing of choosing just the register width via function > IDs? > > Honestly I think it would be cleaner to introduce a new field called > "width" can that be 32 or 64 and represent the register width, rather > than having an explosion of function IDs.
Isn't this being overthought? "method" (I would have re-used model, but that's by-the-by) should just determine which call interface to use. Only one should end up in a device tree - it doesn't make a lot of sense to specify more than one, to me, given my read of the specs and the SMC calling convention. For AArch64 chips they should implement 64-bit SMC interface or 32-bit SMC interface. Either way you get there, the function number is encoded with the type of call anyway so AArch64 secure monitors or hypervisors will know which one was intended, given the SMC calling convention defines this "64-bit" bit pretty clearly. The "method" is an instruction to the "guest OS" in how to fill it's registers more than it is in function names. * An AArch64 guest under an AArch32 hypervisor/sm seems pretty weird and in any case, the device tree would use the 32-bit convention (any SMC64 call in 32-bit state should return "Unknown SMC Function Identifier" if that bit is set) * An AArch32 guest under an AArch32 hypervisor/sm would use the 32-bit convention * An AArch32 guest under an AArch64 hypervisor/sm would use the 32-bit convention * An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit convention or the 32-bit convention depending on how the sm is written, but it doesn't matter which is used if both can be supported.. but you'd only want to use one of them. TLDR; Supply your (EL1) guests with the preferred method, not a list of all possible methods, as above. If you're the vendor and you defined any part of EL3 or EL2 you're in complete control of which method you want subordinate levels to use by way of implementation, right? BTW according to the specs you can't have EL2 without EL3 on ARMv7. I would think the issues with hvc vs hvc64 need to be thought out; hypervisors can trap SMC from their guests and do the right thing without the guest ever knowing that a hypervisor is above it. If it is the case that every Xen DomU kernel needs to know that it is virtualized and to be told to use HVC instead of SMC? What's the benefit of that? -- Matt On Tue, Jul 30, 2013 at 9:42 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Tue, 2013-07-30 at 15:33 +0100, Stefano Stabellini wrote: >> On Tue, 30 Jul 2013, Mark Rutland wrote: >> > On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote: >> > > On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote: >> > > > On 07/30/2013 04:49 AM, Mark Rutland wrote: >> > > > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote: >> > > > >> On 07/29/2013 05:13 AM, Mark Rutland wrote: >> > > > >>> Hi Rob, >> > > > >>> >> > > > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: >> > > > >>>> From: Rob Herring <rob.herring@calxeda.com> >> > > > >> > > > [snip] >> > > > >> > > > >>> One of the things changed in PSCI 0.2 was the SMC calling convention, >> > > > >>> though this isn't clear in the PSCI document. The function IDs for 32bit >> > > > >>> and 64bit callers may differ, and we need to support describing an >> > > > >>> arbitrary configuration of the two (same ID for both, different across >> > > > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). >> > > > >>> >> > > > >>> I'd like to ensure the binding can deal with that from the start. We >> > > > >>> could do this by having -32 and -64 variants of each function id (e.g. >> > > > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined >> > > > >>> ID if they don't. >> > > > >> >> > > > >> Uggg. I guess I should have read the SMC calling convention doc... I was >> > > > >> simply documenting what is already in the PSCI doc, but obviously that >> > > > >> is not fully flushed out. >> > > > >> >> > > > >> How about something like this (for the complicated case of both 32 and >> > > > >> 64 bit): >> > > > >> >> > > > >> method = "smc", "smc64"; >> > > > >> psci_version = <0x84000000 0xc4000000>; >> > > > >> cpu_suspend = <0x84000001 0xc4000001>; >> > > > >> cpu_off = <0x84000002 0xc4000002>; >> > > > >> cpu_on = <0x84000003 0xc4000003>; >> > > > >> >> > > > >> "smc" is a synonym for smc32 for compatibility. The number and order of >> > > > >> methods determines the number and order of function IDs. >> > > > > >> > > > > While this may be compatible with the arm implementation, it won't be >> > > > > compatible with the arm64 implementation, which assumes smc64 by >> > > > > default. >> > > > > >> > > > > As far as I am aware, the implementations currently in use (KVM and Xen) >> > > > > use the same ID for both, so I think "smc" should cover an ID valid for >> > > > > a native register width calling convention, and "smc64" and "smc32" >> > > > > describing values only valid for 64-bit wide and 32-bit wide calling >> > > > > conventions respectively. >> > > > >> > > > The problem is that does not work for a 32-bit kernel on 64-bit h/w as >> > > > native from the dts perspective is smc64. Just like the cpu bindings, >> > > > the binding cannot change based on 32 or 64 bit OS. I don't think we >> > > > really have to deal with that here. We can simply say "smc" is only for >> > > > "arm,psci" and deprecated for "arm,psci-0.2". >> > > >> > > Agreed. I'd be happy with only having "smc32" and "smc64" for >> > > "arm,psci-0.2". >> >> I would be happy with that too (Xen only uses HVC as "method"). > > Presumably we have the same issues with hvc vs hvc32 vs hvc64 though? > > And does smcX imply hvcX or does that also need to be documented here? > >> >> > Actually, from some quick discussion with Marc and Dave, I think we can >> > handle this better, in a way that leaves this backwards compatible. >> > >> > Rather than relying on the method string to tell us the calling >> > convention, I think we should rely on the function ID, as I proposed >> > earlier. The existing function ids provided in the "arm,psci" binding >> > are implicitly relying on the PSCI implementation to detect the register >> > width and act accordingly. This is trivially true on 32bit hardware, KVM >> > (where the same ID is used for 32-bit and 64-bit guests), and while I'm >> > not entirely sure about Xen I believe it's true there. We can make this >> > explicit as we extend the binding. >> > >> > Having a -64 and -32 variant of each ID (while not pretty) allows us to >> > add additional IDs for functions that might only have a 32-bit or 64-bit >> > interface implemented, in addition to functions with common IDs: >> > >> > psci { >> > compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci"; >> > cpu_off = <12345678>; >> > cpu_on = <01234567>; >> > system_reset-32 = <02222222>; >> > system_reset-64 = <12222222>; >> > affinity_info-64 = <15555555>; >> > }; >> > >> > This means that hypervisors could update their PSCI implementation while >> > keeping their DTS compatible with existing kernels. >> >> Are you proposing of getting rid of "method" completely and therefore >> have 4 possible function IDs for each function: >> >> system_reset-32-HVC >> system_reset-64-SMC >> system_reset-32-HVC >> system_reset-64-SMC >> >> or are you proposing of choosing just the register width via function >> IDs? >> >> Honestly I think it would be cleaner to introduce a new field called >> "width" can that be 32 or 64 and represent the register width, rather >> than having an explosion of function IDs. > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 07/30/2013 09:33 AM, Stefano Stabellini wrote: > On Tue, 30 Jul 2013, Mark Rutland wrote: >> On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote: >>> On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote: >>>> On 07/30/2013 04:49 AM, Mark Rutland wrote: >>>>> On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote: >>>>>> On 07/29/2013 05:13 AM, Mark Rutland wrote: >>>>>>> Hi Rob, >>>>>>> >>>>>>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: >>>>>>>> From: Rob Herring <rob.herring@calxeda.com> >>>> >>>> [snip] >>>> >>>>>>> One of the things changed in PSCI 0.2 was the SMC calling convention, >>>>>>> though this isn't clear in the PSCI document. The function IDs for 32bit >>>>>>> and 64bit callers may differ, and we need to support describing an >>>>>>> arbitrary configuration of the two (same ID for both, different across >>>>>>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). >>>>>>> >>>>>>> I'd like to ensure the binding can deal with that from the start. We >>>>>>> could do this by having -32 and -64 variants of each function id (e.g. >>>>>>> cpu_off-64) , if the IDs actually differ, and use the regular combined >>>>>>> ID if they don't. >>>>>> >>>>>> Uggg. I guess I should have read the SMC calling convention doc... I was >>>>>> simply documenting what is already in the PSCI doc, but obviously that >>>>>> is not fully flushed out. >>>>>> >>>>>> How about something like this (for the complicated case of both 32 and >>>>>> 64 bit): >>>>>> >>>>>> method = "smc", "smc64"; >>>>>> psci_version = <0x84000000 0xc4000000>; >>>>>> cpu_suspend = <0x84000001 0xc4000001>; >>>>>> cpu_off = <0x84000002 0xc4000002>; >>>>>> cpu_on = <0x84000003 0xc4000003>; >>>>>> >>>>>> "smc" is a synonym for smc32 for compatibility. The number and order of >>>>>> methods determines the number and order of function IDs. >>>>> >>>>> While this may be compatible with the arm implementation, it won't be >>>>> compatible with the arm64 implementation, which assumes smc64 by >>>>> default. >>>>> >>>>> As far as I am aware, the implementations currently in use (KVM and Xen) >>>>> use the same ID for both, so I think "smc" should cover an ID valid for >>>>> a native register width calling convention, and "smc64" and "smc32" >>>>> describing values only valid for 64-bit wide and 32-bit wide calling >>>>> conventions respectively. >>>> >>>> The problem is that does not work for a 32-bit kernel on 64-bit h/w as >>>> native from the dts perspective is smc64. Just like the cpu bindings, >>>> the binding cannot change based on 32 or 64 bit OS. I don't think we >>>> really have to deal with that here. We can simply say "smc" is only for >>>> "arm,psci" and deprecated for "arm,psci-0.2". >>> >>> Agreed. I'd be happy with only having "smc32" and "smc64" for >>> "arm,psci-0.2". > > I would be happy with that too (Xen only uses HVC as "method"). > > >> Actually, from some quick discussion with Marc and Dave, I think we can >> handle this better, in a way that leaves this backwards compatible. >> >> Rather than relying on the method string to tell us the calling >> convention, I think we should rely on the function ID, as I proposed >> earlier. The existing function ids provided in the "arm,psci" binding >> are implicitly relying on the PSCI implementation to detect the register >> width and act accordingly. This is trivially true on 32bit hardware, KVM >> (where the same ID is used for 32-bit and 64-bit guests), and while I'm >> not entirely sure about Xen I believe it's true there. We can make this >> explicit as we extend the binding. >> >> Having a -64 and -32 variant of each ID (while not pretty) allows us to >> add additional IDs for functions that might only have a 32-bit or 64-bit >> interface implemented, in addition to functions with common IDs: >> >> psci { >> compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci"; >> cpu_off = <12345678>; >> cpu_on = <01234567>; >> system_reset-32 = <02222222>; >> system_reset-64 = <12222222>; >> affinity_info-64 = <15555555>; >> }; >> >> This means that hypervisors could update their PSCI implementation while >> keeping their DTS compatible with existing kernels. > > Are you proposing of getting rid of "method" completely and therefore > have 4 possible function IDs for each function: > > system_reset-32-HVC > system_reset-64-SMC > system_reset-32-HVC > system_reset-64-SMC No. you should never have a DTB with both hvc and smc. The h/w (firmware) to OS interface is smc. The hypervisor to guest interface is a separate ABI and would be hvc. They are independent of each other. > or are you proposing of choosing just the register width via function > IDs? > > Honestly I think it would be cleaner to introduce a new field called > "width" can that be 32 or 64 and represent the register width, rather > than having an explosion of function IDs. I can think of lots of ways it would be cleaner, but that is not what ARM defined. There are 3 cases to handle: 32-bit only calls, 64-bit only calls and both 32/64-bit calls. I think hypervisors have the same issue as firmware. Do you know the guest is 32 or 64 bit up front and can provide it a dtb based on that? If not, then you can never use the 64-bit only calls. So Xen has to use 32-bit only or provide both 32 and 64 bit interfaces. Rob
On Tue, 2013-07-30 at 12:48 -0500, Matt Sealey wrote: > * An AArch64 guest under an AArch32 hypervisor/sm seems pretty weird > and in any case, the device tree would use the 32-bit convention (any > SMC64 call in 32-bit state should return "Unknown SMC Function > Identifier" if that bit is set) I don't believe 64-bit EL1 under 32-bit EL2 is allowed by the architecture. Each EL must be at least the width of the one below it. > TLDR; Supply your (EL1) guests with the preferred method, not a list > of all possible methods, as above. If you're the vendor and you > defined any part of EL3 or EL2 you're in complete control of which > method you want subordinate levels to use by way of implementation, > right? I haven't seen the entirety of this discussion, but that seems logical to me. > > BTW according to the specs you can't have EL2 without EL3 on ARMv7. I > would think the issues with hvc vs hvc64 need to be thought out; > hypervisors can trap SMC from their guests and do the right thing > without the guest ever knowing that a hypervisor is above it. The issue is that SMC traps don't provide you with the #imm in the syndrome register so you need to fetch and decode the instruction in the hypervisor manually, which is possible but faffsome and relatively costly (mostly due to the need to map guest memory on demand). > If it is the case that every Xen DomU kernel needs to know that it is > virtualized and to be told to use HVC instead of SMC? What's the > benefit of that? Just the avoidance of a fetch and decode, I think. I'm also not sure if Secure world can't prevent NS EL2 from tapping SMC, would need to check the specs. Ian.
On Tue, 2013-07-30 at 14:34 -0500, Rob Herring wrote: > I can think of lots of ways it would be cleaner, but that is not what > ARM defined. There are 3 cases to handle: 32-bit only calls, 64-bit only > calls and both 32/64-bit calls. I think hypervisors have the same issue > as firmware. Do you know the guest is 32 or 64 bit up front and can > provide it a dtb based on that? Yes. An EL cannot change its own bit width and needs to rely on the EL above returning in the right mode. IOW a hypervisor needs to know if it is launching a 32- or 64-bit guest already. I suppose in theory we could supply a mode switch hypercall, but we don't and I think that is rather out of scope here... Ian.
On Tue, Jul 30, 2013 at 03:33:34PM +0100, Stefano Stabellini wrote: > On Tue, 30 Jul 2013, Mark Rutland wrote: > > On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote: > > > On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote: > > > > On 07/30/2013 04:49 AM, Mark Rutland wrote: > > > > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote: > > > > >> On 07/29/2013 05:13 AM, Mark Rutland wrote: > > > > >>> Hi Rob, > > > > >>> > > > > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: > > > > >>>> From: Rob Herring <rob.herring@calxeda.com> > > > > > > > > [snip] > > > > > > > > >>> One of the things changed in PSCI 0.2 was the SMC calling convention, > > > > >>> though this isn't clear in the PSCI document. The function IDs for 32bit > > > > >>> and 64bit callers may differ, and we need to support describing an > > > > >>> arbitrary configuration of the two (same ID for both, different across > > > > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). > > > > >>> > > > > >>> I'd like to ensure the binding can deal with that from the start. We > > > > >>> could do this by having -32 and -64 variants of each function id (e.g. > > > > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined > > > > >>> ID if they don't. > > > > >> > > > > >> Uggg. I guess I should have read the SMC calling convention doc... I was > > > > >> simply documenting what is already in the PSCI doc, but obviously that > > > > >> is not fully flushed out. > > > > >> > > > > >> How about something like this (for the complicated case of both 32 and > > > > >> 64 bit): > > > > >> > > > > >> method = "smc", "smc64"; > > > > >> psci_version = <0x84000000 0xc4000000>; > > > > >> cpu_suspend = <0x84000001 0xc4000001>; > > > > >> cpu_off = <0x84000002 0xc4000002>; > > > > >> cpu_on = <0x84000003 0xc4000003>; > > > > >> > > > > >> "smc" is a synonym for smc32 for compatibility. The number and order of > > > > >> methods determines the number and order of function IDs. > > > > > > > > > > While this may be compatible with the arm implementation, it won't be > > > > > compatible with the arm64 implementation, which assumes smc64 by > > > > > default. > > > > > > > > > > As far as I am aware, the implementations currently in use (KVM and Xen) > > > > > use the same ID for both, so I think "smc" should cover an ID valid for > > > > > a native register width calling convention, and "smc64" and "smc32" > > > > > describing values only valid for 64-bit wide and 32-bit wide calling > > > > > conventions respectively. > > > > > > > > The problem is that does not work for a 32-bit kernel on 64-bit h/w as > > > > native from the dts perspective is smc64. Just like the cpu bindings, > > > > the binding cannot change based on 32 or 64 bit OS. I don't think we > > > > really have to deal with that here. We can simply say "smc" is only for > > > > "arm,psci" and deprecated for "arm,psci-0.2". > > > > > > Agreed. I'd be happy with only having "smc32" and "smc64" for > > > "arm,psci-0.2". > > I would be happy with that too (Xen only uses HVC as "method"). > > > > Actually, from some quick discussion with Marc and Dave, I think we can > > handle this better, in a way that leaves this backwards compatible. > > > > Rather than relying on the method string to tell us the calling > > convention, I think we should rely on the function ID, as I proposed > > earlier. The existing function ids provided in the "arm,psci" binding > > are implicitly relying on the PSCI implementation to detect the register > > width and act accordingly. This is trivially true on 32bit hardware, KVM > > (where the same ID is used for 32-bit and 64-bit guests), and while I'm > > not entirely sure about Xen I believe it's true there. We can make this > > explicit as we extend the binding. > > > > Having a -64 and -32 variant of each ID (while not pretty) allows us to > > add additional IDs for functions that might only have a 32-bit or 64-bit > > interface implemented, in addition to functions with common IDs: > > > > psci { > > compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci"; > > cpu_off = <12345678>; > > cpu_on = <01234567>; > > system_reset-32 = <02222222>; > > system_reset-64 = <12222222>; > > affinity_info-64 = <15555555>; > > }; > > > > This means that hypervisors could update their PSCI implementation while > > keeping their DTS compatible with existing kernels. > > Are you proposing of getting rid of "method" completely and therefore > have 4 possible function IDs for each function: > > system_reset-32-HVC > system_reset-64-SMC > system_reset-32-HVC > system_reset-64-SMC > > or are you proposing of choosing just the register width via function > IDs? Just the register width via the function ID's property name. > > Honestly I think it would be cleaner to introduce a new field called > "width" can that be 32 or 64 and represent the register width, rather > than having an explosion of function IDs. > Possibly. I'm worried people may create a mixture of shared, 64-bit only, and 32-bit only IDs. Thanks, Mark.
On Tue, Jul 30, 2013 at 03:42:34PM +0100, Ian Campbell wrote: > On Tue, 2013-07-30 at 15:33 +0100, Stefano Stabellini wrote: > > On Tue, 30 Jul 2013, Mark Rutland wrote: > > > On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote: > > > > On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote: > > > > > On 07/30/2013 04:49 AM, Mark Rutland wrote: > > > > > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote: > > > > > >> On 07/29/2013 05:13 AM, Mark Rutland wrote: > > > > > >>> Hi Rob, > > > > > >>> > > > > > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote: > > > > > >>>> From: Rob Herring <rob.herring@calxeda.com> > > > > > > > > > > [snip] > > > > > > > > > > >>> One of the things changed in PSCI 0.2 was the SMC calling convention, > > > > > >>> though this isn't clear in the PSCI document. The function IDs for 32bit > > > > > >>> and 64bit callers may differ, and we need to support describing an > > > > > >>> arbitrary configuration of the two (same ID for both, different across > > > > > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit). > > > > > >>> > > > > > >>> I'd like to ensure the binding can deal with that from the start. We > > > > > >>> could do this by having -32 and -64 variants of each function id (e.g. > > > > > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined > > > > > >>> ID if they don't. > > > > > >> > > > > > >> Uggg. I guess I should have read the SMC calling convention doc... I was > > > > > >> simply documenting what is already in the PSCI doc, but obviously that > > > > > >> is not fully flushed out. > > > > > >> > > > > > >> How about something like this (for the complicated case of both 32 and > > > > > >> 64 bit): > > > > > >> > > > > > >> method = "smc", "smc64"; > > > > > >> psci_version = <0x84000000 0xc4000000>; > > > > > >> cpu_suspend = <0x84000001 0xc4000001>; > > > > > >> cpu_off = <0x84000002 0xc4000002>; > > > > > >> cpu_on = <0x84000003 0xc4000003>; > > > > > >> > > > > > >> "smc" is a synonym for smc32 for compatibility. The number and order of > > > > > >> methods determines the number and order of function IDs. > > > > > > > > > > > > While this may be compatible with the arm implementation, it won't be > > > > > > compatible with the arm64 implementation, which assumes smc64 by > > > > > > default. > > > > > > > > > > > > As far as I am aware, the implementations currently in use (KVM and Xen) > > > > > > use the same ID for both, so I think "smc" should cover an ID valid for > > > > > > a native register width calling convention, and "smc64" and "smc32" > > > > > > describing values only valid for 64-bit wide and 32-bit wide calling > > > > > > conventions respectively. > > > > > > > > > > The problem is that does not work for a 32-bit kernel on 64-bit h/w as > > > > > native from the dts perspective is smc64. Just like the cpu bindings, > > > > > the binding cannot change based on 32 or 64 bit OS. I don't think we > > > > > really have to deal with that here. We can simply say "smc" is only for > > > > > "arm,psci" and deprecated for "arm,psci-0.2". > > > > > > > > Agreed. I'd be happy with only having "smc32" and "smc64" for > > > > "arm,psci-0.2". > > > > I would be happy with that too (Xen only uses HVC as "method"). > > Presumably we have the same issues with hvc vs hvc32 vs hvc64 though? Yes. I'd not considered this with my original reply, but we have the exact same issue for hvc. > > And does smcX imply hvcX or does that also need to be documented here? The two are mutually exclusive, neither implies the other (KVM will barf if you use an SMC iirc). Thanks, Mark.
On Tue, Jul 30, 2013 at 06:48:59PM +0100, Matt Sealey wrote: > Isn't this being overthought? > > "method" (I would have re-used model, but that's by-the-by) should > just determine which call interface to use. Only one should end up in > a device tree - it doesn't make a lot of sense to specify more than > one, to me, given my read of the specs and the SMC calling convention. > > For AArch64 chips they should implement 64-bit SMC interface or 32-bit > SMC interface. Either way you get there, the function number is > encoded with the type of call anyway so AArch64 secure monitors or > hypervisors will know which one was intended, given the SMC calling > convention defines this "64-bit" bit pretty clearly. The "method" is > an instruction to the "guest OS" in how to fill it's registers more > than it is in function names. Implementations may not follow the recommended ID allocations, and thus we cannot rely on the function ID to provide us with any useful information. We certainly cannot rely on it for hvcs, where for an existing implementation (KVM) uses the same IDs for 32-bit and 64-bit guests. If we force the use of said bit, then new kernels cannot necessarily work on existing PSCI implementations (which may not handle the use of said bit as you expect). If we make the binding for psci-0.2 explicitly require the use of this bit, then future firmware/virtualisation code cannot provide new functionality withoot breaking backwards compatiblity with existing kernels, as an "arm,psci-0.2" node would be incompatible with an "arm,psci" node (which is all an existing kernel can handle). > > * An AArch64 guest under an AArch32 hypervisor/sm seems pretty weird > and in any case, the device tree would use the 32-bit convention (any > SMC64 call in 32-bit state should return "Unknown SMC Function > Identifier" if that bit is set) This is not allowed by the architecture. > * An AArch32 guest under an AArch32 hypervisor/sm would use the > 32-bit convention > * An AArch32 guest under an AArch64 hypervisor/sm would use the 32-bit > convention Both of these cases are trivially mandated by the architecture. > * An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit > convention or the 32-bit convention depending on how the sm is > written, but it doesn't matter which is used if both can be > supported.. but you'd only want to use one of them. Not all implementation will implement both, so there needs to be a way to describe that. Which is actually used is up to the OS. > > TLDR; Supply your (EL1) guests with the preferred method, not a list > of all possible methods, as above. If you're the vendor and you > defined any part of EL3 or EL2 you're in complete control of which > method you want subordinate levels to use by way of implementation, > right? As far as I can tell, no-one was arguing against this strategy. However, for those cases where functionality is implemented for only one register-width, or requires a different ID per register-width, this needs to be described somehow. Thanks, Mark.
On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <mark.rutland@arm.com> wrote: > > If we force the use of said bit, then new kernels cannot necessarily > work on existing PSCI implementations (which may not handle the use of > said bit as you expect). If we make the binding for psci-0.2 explicitly > require the use of this bit, then future firmware/virtualisation code > cannot provide new functionality withoot breaking backwards compatiblity > with existing kernels, as an "arm,psci-0.2" node would be incompatible > with an "arm,psci" node (which is all an existing kernel can handle). Okay so fighting backwards compatibility.. what I wasn''t suggesting was just giving bits and then knowing what the function type was from the bit - the SMC ABI in play calls that shot. But you don't need to (Rob's suggestion) supply two function ids for each in one property or provide two properties for each function variant in the same node. Doesn't that complicate parsing a little too much? >> * An AArch64 guest under an AArch32 hypervisor/sm seems pretty weird >> and in any case, the device tree would use the 32-bit convention (any >> SMC64 call in 32-bit state should return "Unknown SMC Function >> Identifier" if that bit is set) > > This is not allowed by the architecture. Understood.. >> * An AArch32 guest under an AArch32 hypervisor/sm would use the >> 32-bit convention >> * An AArch32 guest under an AArch64 hypervisor/sm would use the 32-bit >> convention > > Both of these cases are trivially mandated by the architecture. > >> * An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit >> convention or the 32-bit convention depending on how the sm is >> written, but it doesn't matter which is used if both can be >> supported.. but you'd only want to use one of them. > > Not all implementation will implement both, so there needs to be a way > to describe that. Which is actually used is up to the OS. Okay but putting both ways in a single node, and describing two function ids (per property or with two properties) is what I was getting at.. for the one case where you can use both at once, the device tree description is king here. I am a little concerned that the support for this is going down the route of telling the OS all possible ways to do the same thing instead of trying to get into using a single, preferred way in the common cases. Putting both call methods in the same node, doubling the function id property lengths, or suffixing function id properties to do it seems like putting information in the tree purely for the sake of it. In what cases would it (uncommon cases only being existing, pre-PCSI pre-SMC-conventions potentially for other things). In the case of PSCI there's an opportunity to be strict about it.. why would it be sane to allow a mixed implementation? Dictate that either support all the 64-bit versions or all the 32-bit versions or both? And if both are supported, dictate in the device tree which the OS should be using. What about having two nodes? There is nothing in device trees that says you can't have two psci { compatible="arm,psci-0.2" } nodes, one with method=smc and one with method=smc64 or hvc64 or what have you. Parsing and setup of the PSCI code can be quit early if it's not the "desirable" method for the OS (i.e. 64-bit on a 32-bit kernel). Then each node follows the exact same definition, and the differentiator is the call method and not the property names or complicating their contents. > As far as I can tell, no-one was arguing against this strategy. However, > for those cases where functionality is implemented for only one > register-width, or requires a different ID per register-width, this > needs to be described somehow. Do implementations like this really exist and who do we need to waterboard to make them stop doing it? :)
On 07/31/2013 12:24 PM, Matt Sealey wrote: > On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <mark.rutland@arm.com> wrote: [snip] >>> * An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit >>> convention or the 32-bit convention depending on how the sm is >>> written, but it doesn't matter which is used if both can be >>> supported.. but you'd only want to use one of them. >> >> Not all implementation will implement both, so there needs to be a way >> to describe that. Which is actually used is up to the OS. > > Okay but putting both ways in a single node, and describing two > function ids (per property or with two properties) is what I was > getting at.. for the one case where you can use both at once, the > device tree description is king here. > > I am a little concerned that the support for this is going down the > route of telling the OS all possible ways to do the same thing instead > of trying to get into using a single, preferred way in the common > cases. > > Putting both call methods in the same node, doubling the function id > property lengths, or suffixing function id properties to do it seems > like putting information in the tree purely for the sake of it. In > what cases would it (uncommon cases only being existing, pre-PCSI > pre-SMC-conventions potentially for other things). In the case of PSCI > there's an opportunity to be strict about it.. why would it be sane to > allow a mixed implementation? Dictate that either support all the > 64-bit versions or all the 32-bit versions or both? And if both are > supported, dictate in the device tree which the OS should be using. > > What about having two nodes? There is nothing in device trees that > says you can't have two psci { compatible="arm,psci-0.2" } nodes, one > with method=smc and one with method=smc64 or hvc64 or what have you. > Parsing and setup of the PSCI code can be quit early if it's not the > "desirable" method for the OS (i.e. 64-bit on a 32-bit kernel). Then > each node follows the exact same definition, and the differentiator is > the call method and not the property names or complicating their > contents. +1 This would certainly be easier for things like a hypervisor to parse and update. Rob
On Wed, Jul 31, 2013 at 12:49:20PM -0500, Rob Herring wrote: > On 07/31/2013 12:24 PM, Matt Sealey wrote: > > On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <mark.rutland@arm.com> wrote: > > [snip] > > >>> * An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit > >>> convention or the 32-bit convention depending on how the sm is > >>> written, but it doesn't matter which is used if both can be > >>> supported.. but you'd only want to use one of them. > >> > >> Not all implementation will implement both, so there needs to be a way > >> to describe that. Which is actually used is up to the OS. > > > > Okay but putting both ways in a single node, and describing two > > function ids (per property or with two properties) is what I was > > getting at.. for the one case where you can use both at once, the > > device tree description is king here. > > > > I am a little concerned that the support for this is going down the > > route of telling the OS all possible ways to do the same thing instead > > of trying to get into using a single, preferred way in the common > > cases. > > > > Putting both call methods in the same node, doubling the function id > > property lengths, or suffixing function id properties to do it seems > > like putting information in the tree purely for the sake of it. In > > what cases would it (uncommon cases only being existing, pre-PCSI > > pre-SMC-conventions potentially for other things). In the case of PSCI > > there's an opportunity to be strict about it.. why would it be sane to > > allow a mixed implementation? Dictate that either support all the > > 64-bit versions or all the 32-bit versions or both? And if both are > > supported, dictate in the device tree which the OS should be using. The DT is about description, not policy. If multiple usable flavours of the PSCI interface exist, then they exist, and there's no special reason to hide one of them from the DT that I can see. I take the point that we shouldn't describe useless or redundant information for no good reason, though. > > What about having two nodes? There is nothing in device trees that > > says you can't have two psci { compatible="arm,psci-0.2" } nodes, one > > with method=smc and one with method=smc64 or hvc64 or what have you. > > Parsing and setup of the PSCI code can be quit early if it's not the > > "desirable" method for the OS (i.e. 64-bit on a 32-bit kernel). Then > > each node follows the exact same definition, and the differentiator is > > the call method and not the property names or complicating their > > contents. > > +1 > > This would certainly be easier for things like a hypervisor to parse and > update. Can you elaborate on that? Wouldn't a hypervisor always rip out and replace PSCI node(s) with its own? Allowing the firmware's PSCI interface to "show through" to a guest VM sounds unwise. I'm not sure why having multiple nodes makes this easier. Having two nodes just moves information around. Does it really simplify anything? With the multiple nodes approach, you might actually need 3 nodes if you are providing backwards compatible support for psci-0.1 callers: one for psci-0.2 32-bit, one for psci-0.2 64-bit, and one for psci-0.1. Maybe not though ... I'd have to think about it a bit more. CHeers ---Dave
On Thu, Aug 1, 2013 at 12:51 PM, Dave Martin <Dave.Martin@arm.com> wrote: > On Wed, Jul 31, 2013 at 12:49:20PM -0500, Rob Herring wrote: >> On 07/31/2013 12:24 PM, Matt Sealey wrote: >> > On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <mark.rutland@arm.com> wrote: >> >> [snip] >> >> >>> * An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit >> >>> convention or the 32-bit convention depending on how the sm is >> >>> written, but it doesn't matter which is used if both can be >> >>> supported.. but you'd only want to use one of them. >> >> >> >> Not all implementation will implement both, so there needs to be a way >> >> to describe that. Which is actually used is up to the OS. >> > >> > Okay but putting both ways in a single node, and describing two >> > function ids (per property or with two properties) is what I was >> > getting at.. for the one case where you can use both at once, the >> > device tree description is king here. >> > >> > I am a little concerned that the support for this is going down the >> > route of telling the OS all possible ways to do the same thing instead >> > of trying to get into using a single, preferred way in the common >> > cases. >> > >> > Putting both call methods in the same node, doubling the function id >> > property lengths, or suffixing function id properties to do it seems >> > like putting information in the tree purely for the sake of it. In >> > what cases would it (uncommon cases only being existing, pre-PCSI >> > pre-SMC-conventions potentially for other things). In the case of PSCI >> > there's an opportunity to be strict about it.. why would it be sane to >> > allow a mixed implementation? Dictate that either support all the >> > 64-bit versions or all the 32-bit versions or both? And if both are >> > supported, dictate in the device tree which the OS should be using. > > The DT is about description, not policy. If multiple usable flavours of > the PSCI interface exist, then they exist, and there's no special reason > to hide one of them from the DT that I can see. > > I take the point that we shouldn't describe useless or redundant > information for no good reason, though. > >> > What about having two nodes? There is nothing in device trees that >> > says you can't have two psci { compatible="arm,psci-0.2" } nodes, one >> > with method=smc and one with method=smc64 or hvc64 or what have you. >> > Parsing and setup of the PSCI code can be quit early if it's not the >> > "desirable" method for the OS (i.e. 64-bit on a 32-bit kernel). Then >> > each node follows the exact same definition, and the differentiator is >> > the call method and not the property names or complicating their >> > contents. >> >> +1 >> >> This would certainly be easier for things like a hypervisor to parse and >> update. > > Can you elaborate on that? > > Wouldn't a hypervisor always rip out and replace PSCI node(s) with > its own? Allowing the firmware's PSCI interface to "show through" to a > guest VM sounds unwise. Perhaps. Minimally, the hypervisor could just replace smc with hvc for the method. > I'm not sure why having multiple nodes makes this easier. > > Having two nodes just moves information around. Does it really simplify > anything? You can add 'status = "disabled";' easily and have it apply to the whole node. Also, if you split-up by method, then it is clear which properties apply to which method. I don't really care for the <none>, -32 and -64 distinction on function ID properties. > With the multiple nodes approach, you might actually need 3 nodes if > you are providing backwards compatible support for psci-0.1 callers: one > for psci-0.2 32-bit, one for psci-0.2 64-bit, and one for psci-0.1. > Maybe not though ... I'd have to think about it a bit more. I'd hope we can transition away from psci-0.1. The things that rely on it are not really mature anyway. Rob
>On Thu, Aug 1, 2013 at 2:02 PM, Rob Herring <robherring2@gmail.com> wrote: > On Thu, Aug 1, 2013 at 12:51 PM, Dave Martin <Dave.Martin@arm.com> wrote: >> On Wed, Jul 31, 2013 at 12:49:20PM -0500, Rob Herring wrote: >>> On 07/31/2013 12:24 PM, Matt Sealey wrote: >>> > On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <mark.rutland@arm.com> wrote: >>> >>> [snip] >>> >> >> The DT is about description, not policy. If multiple usable flavours of >> the PSCI interface exist, then they exist, and there's no special reason >> to hide one of them from the DT that I can see. >> >> I take the point that we shouldn't describe useless or redundant >> information for no good reason, though. Good, that's the point I was trying to get across ;) Ideally though, the DT describes what is there to be used, not what is just there. If we had DT describing an entire SoC, on most of them half of it would be redundant, disabled devices which cannot be muxed out simultaneously with the active ones. There aren't many reasons to do so (and electrically, very few ways you can even do it at runtime anyway for the vast majority of things). What I am trying to figure out is if someone implements PSCI 0.1, 0.2 or some future version, then the device tree could explain those - one for each version. As Rob pointed out, you can status=disabled the ones you don't want people to use (or, leave them all in...). Maybe the idea here would be not to version the compatible property but use "model" (standard property!) for the version of PSCI, or an encoded version. If you parse PSCI nodes and find a 0.2 node, initialize it.. then stop looking. If your secure monitor guys didn't implement all of PSCI 0.2 you should never have defined a node for it in the first place. It's not useful to half-describe something in the device tree - device trees are meant to remove the guesswork of just having some chip and some firmware interface, and simplify finding and using the correct functional blocks and interfaces. I can't imagine a situation where someone would implement half of PSCI 0.2 and fall back to PSCI 0.1 in another node for other functions.. .. or implement half 32-bit and half 64-bit functions for a system, except at the point PSCI specifically falls down in that (PSCI 0.2 p5.4.1);. """The SMC calling convention [4], provides support for calls using only 32-bit parameters (SMC32), and for calls using 32 and 64-bit parameters (SMC64). Some of the PSCI functions defined above use only SMC32, some of the functions have both an SMC32 and an SMC64 version. For PSCI functions that use only 32-bit parameters, the arguments are passed in R0 to R3 (AArch32) or W0 to W3 (AArch64), with return values in R0 or W0. For versions using 64-bit parameters, the arguments are passed in X0 to X3, with return values in X0/W0 (depending on the return parameter size).""" Yes, some of them aren't defined as smc64 functions. This sooooo needs to be fixed for PSCI 0.3.. in essence the spec implies that 64-bit variants are a second cousin twice removed of the really important 32-bit ABI. The spec itself dictates that the function IDs are identical except for the magic 64-bit 'bit'. >> Wouldn't a hypervisor always rip out and replace PSCI node(s) with >> its own? Allowing the firmware's PSCI interface to "show through" to a >> guest VM sounds unwise. > > Perhaps. Minimally, the hypervisor could just replace smc with hvc for > the method. And that would be a special case for the hypervisor. Since the calling convention is *identical* apart from the instruction that causes that privilege level jump, this is quite easily afforded by the implementation. I still think that, as documented, hypervisors should just trap smc as the way in. It may be a pain in the backside to decode the immediate value, but this all got discussed and changed with syscalls and svc anyway a long while back - that immediate value is a monster and since there are some defined for very specific vendor interfaces (like semi-hosting) it is unwise to even use it in case some other magic firmware/debug/emulation process decides it is going to override your trap and go do something else than you intended. I don't think the smc calling convention really takes that into account, it just assumes you're trustworthy enough to not implement things like that and take it all into account. PSCI actually recommends you use 0 for that immediate value.. Same section as above: """In line with the SMC calling convention, the immediate value used with an SMC instruction must be 0. ARM recommends that a hypervisor providing a PSCI functions through an HVC conduit uses the same approach to the immediate value and parameter passing. This aids the generalization of client code.""" So, no need to decode smc #imm4, just do what it says in the registers. And no need for a Xen DomU to know it is even under a hypervisor, either, by having special code to cover the case. For what a particular hypervisor needs to do, if it intends to run on ARM, it should be tending to use a particular range of SMC function IDs (OEM services?) which are in any event going to be hypervisor specific anyway (Xen, QEMU, OKL4, whatever) and PSCI specifically keeps well away from them.. >> I'm not sure why having multiple nodes makes this easier. >> >> Having two nodes just moves information around. Does it really simplify >> anything? > > You can add 'status = "disabled";' easily and have it apply to the > whole node. Also, if you split-up by method, then it is clear which > properties apply to which method. I don't really care for the <none>, > -32 and -64 distinction on function ID properties. > >> With the multiple nodes approach, you might actually need 3 nodes if >> you are providing backwards compatible support for psci-0.1 callers: one >> for psci-0.2 32-bit, one for psci-0.2 64-bit, and one for psci-0.1. >> Maybe not though ... I'd have to think about it a bit more. > > I'd hope we can transition away from psci-0.1. The things that rely on > it are not really mature anyway. Right, but I have to agree with Mark that backwards compatibility is a key concept here too. It can be left in - if you supply psci-0.2 nodes, new kernels that support it can use those. Older kernels will find the older nodes and keep working. Even newer kernels with even newer nodes will use those.. in my opinion it shouldn't bind to psci-0.3 and psci-0.2 at the same time, though, and the trade-off is at the vendor level. If you want to have psci-0.2 ONLY, then they'd have to either provide patches for older kernels or just put a lower limit on the version you support. The whole "there are 64-bit functions but some of them aren't" thing is just a mess waiting to happen, though. Who's responsible for the spec? :D -- Matt
diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt index 433afe9..b8b4d9f 100644 --- a/Documentation/devicetree/bindings/arm/psci.txt +++ b/Documentation/devicetree/bindings/arm/psci.txt @@ -21,7 +21,7 @@ to #0. Main node required properties: - - compatible : Must be "arm,psci" + - compatible : Must be "arm,psci-0.2" or "arm,psci" - method : The method of calling the PSCI firmware. Permitted values are: @@ -32,6 +32,9 @@ Main node required properties: "hvc" : HVC #0, with the register assignments specified in this binding. + - psci_version : Function ID for PSCI_VERSION operation. Required for + "arm,psci-0.2" compatible version or later. + Main node optional properties: - cpu_suspend : Function ID for CPU_SUSPEND operation @@ -42,14 +45,24 @@ Main node optional properties: - migrate : Function ID for MIGRATE operation + - system_reset : Function ID for SYSTEM_RESET operation + + - system_off : Function ID for SYSTEM_OFF operation + Example: psci { - compatible = "arm,psci"; + compatible = "arm,psci-0.2"; method = "smc"; - cpu_suspend = <0x95c10000>; - cpu_off = <0x95c10001>; - cpu_on = <0x95c10002>; - migrate = <0x95c10003>; + psci_version = <0x84000000>; + cpu_suspend = <0x84000001>; + cpu_off = <0x84000002>; + cpu_on = <0x84000003>; + affinity_info = <0x84000004>; + migrate = <0x84000005>; + migrate_info_type = <0x84000006>; + migrate_info_up_cpu = <0x84000007>; + system_off = <0x84000008>; + system_reset = <0x84000009>; };