| Message ID | OF5E6E62D3.16BC1A1C-ON8625802D.005940E3-8625802D.005A4857@notes.na.collabserv.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Dan, > /** > * @brief Query the HBRT host for a list of fixes > * > * There are times when workarounds need to be put into place to handle > * issues with the hosting layer (e.g. opal-prd) while fixes are not yet > * released. This is especially true because of the disconnected > release > * streams for the firmware and the hosting environment. > * > * @param i_set Indicates which set of fixes we're checking > * see HBRT_FIX_LIST_SET... > * > * @return a bitmask containing the relevant flags for the current > * implementation, see HBRT_FIX_FLAGS_... > */ > uint64_t (*get_fix_list)( uint64_t i_set ); > > /* OPAL fixes */ > #define HBRT_FIX_LIST_SET0_OPAL 0 > #define HBRT_FIX_FLAGS_OPAL_HAS_XSCOM_RC (1ul << 0) > > /* PHYP fixes */ > #define HBRT_FIX_LIST_SET1_PHYP 1 > > If we run over 64 flags, we'd just make a HBRT_FIX_LIST_SET2_OPAL2 2, etc. Yep, that makes sense. We'd just need to define the case where the 'set' is unknown to opal-prd or pHyp. I'd propose just returning zero, as long as we can ensure that zero-bit defaults will be acceptable in future. Regards, Jeremy
On Wed, 2016-09-14 at 08:47 +0800, Jeremy Kerr wrote: > Yep, that makes sense. We'd just need to define the case where the 'set' > is unknown to opal-prd or pHyp. I'd propose just returning zero, as long > as we can ensure that zero-bit defaults will be acceptable in future. Does HBRT knows whether it's running under pHyp or OPAL ? Or do we need to use this as a way to inform it ? Cheers, Ben.
Hi Dan, >> /** >> * @brief Query the HBRT host for a list of fixes >> * >> * There are times when workarounds need to be put into place to handle >> * issues with the hosting layer (e.g. opal-prd) while fixes are not yet >> * released. This is especially true because of the disconnected >> release >> * streams for the firmware and the hosting environment. >> * >> * @param i_set Indicates which set of fixes we're checking >> * see HBRT_FIX_LIST_SET... >> * >> * @return a bitmask containing the relevant flags for the current >> * implementation, see HBRT_FIX_FLAGS_... >> */ >> uint64_t (*get_fix_list)( uint64_t i_set ); >> >> /* OPAL fixes */ >> #define HBRT_FIX_LIST_SET0_OPAL 0 >> #define HBRT_FIX_FLAGS_OPAL_HAS_XSCOM_RC (1ul << 0) >> >> /* PHYP fixes */ >> #define HBRT_FIX_LIST_SET1_PHYP 1 >> >> If we run over 64 flags, we'd just make a HBRT_FIX_LIST_SET2_OPAL2 2, etc. > > Yep, that makes sense. We'd just need to define the case where the 'set' > is unknown to opal-prd or pHyp. I'd propose just returning zero, as long > as we can ensure that zero-bit defaults will be acceptable in future. On further thoughts, this could makes the interface a little difficult to implement, mostly on the HBRT side. If you're running under phyp, would you ever request the flags for SET0_OPAL? If so, it would indicate that the XSCOM_RC flags is *not* set (since it's returning 0 for an unknown set)? Or would the phyp implementation need to know about the OPAL flags? Or should we make the sets based on functionality, rather than implementation? Cheers, Jeremy
> Does HBRT knows whether it's running under pHyp or OPAL ? Yes, we know that since we have all of the information from the original HB boot where we need to know. -- Dan Crowell Senior Software Engineer - Power Systems Enablement Firmware IBM Rochester: t/l 553-2987 dcrowell@us.ibm.com From: Benjamin Herrenschmidt <benh@kernel.crashing.org> To: Jeremy Kerr <jk@ozlabs.org>, Daniel M Crowell/Rochester/IBM@IBMUS Cc: skiboot@lists.ozlabs.org, Corey Swenson/Rochester/IBM@IBMUS, Patrick Barrett/Rochester/IBM@IBMUS Date: 09/13/2016 08:06 PM Subject: Re: [PATCH 1/2] opal-prd: Add get_prd_flags to host interfaces On Wed, 2016-09-14 at 08:47 +0800, Jeremy Kerr wrote: > Yep, that makes sense. We'd just need to define the case where the 'set' > is unknown to opal-prd or pHyp. I'd propose just returning zero, as long > as we can ensure that zero-bit defaults will be acceptable in future. Does HBRT knows whether it's running under pHyp or OPAL ? Or do we need to use this as a way to inform it ? Cheers, Ben.
My intention was that the flags for OPAL and PHYP would be distinct. That
would handle the specific case we're dealing with right now. I don't want
to force a change into PHYP to set a bit to indicate they don't have a bug
we found in OPAL/opal-prd. Adding features into the mix does make the
idea of a comon set of flags more enticing though. I'll have to search
around for the notes I sent out months ago related to a 'get_capabilities'
function to see if there are more ideas we forgot.
I still like the idea of multiple sets, so here's attempt #3.
/* Common features */
#define HBRT_CAPABILITIES_SET0_COMMON 0
#define HBRT_CAPABILITIES_COMMON_AWESOME_FEATURE (1ul << 0)
#define HBRT_CAPABILITIES_COMMON_NEW_INTERFACE_PARMS (2ul << 0)
/* OPAL fixes */
#define HBRT_CAPABILITIES_SET1_OPAL 1
#define HBRT_CAPABILITIES_OPAL_HAS_XSCOM_RC (1ul << 0)
/* PHYP fixes */
#define HBRT_CAPABILITIES_SET2_PHYP 2
#define HBRT_CAPABILITIES_PHYP_FIXED_BROKEN_THING (1ul << 0)
> Or should we make the sets based on functionality, rather than
> implementation?
Not completely sure what you are suggesting here.
--
Dan Crowell
Senior Software Engineer - Power Systems Enablement Firmware
IBM Rochester: t/l 553-2987
dcrowell@us.ibm.com
From: Jeremy Kerr <jk@ozlabs.org>
To: Daniel M Crowell/Rochester/IBM@IBMUS
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
skiboot@lists.ozlabs.org, Corey Swenson/Rochester/IBM@IBMUS, Patrick
Barrett/Rochester/IBM@IBMUS
Date: 09/13/2016 08:36 PM
Subject: Re: [PATCH 1/2] opal-prd: Add get_prd_flags to host
interfaces
Hi Dan,
>> /**
>> * @brief Query the HBRT host for a list of fixes
>> *
>> * There are times when workarounds need to be put into place to
handle
>> * issues with the hosting layer (e.g. opal-prd) while fixes are
not yet
>> * released. This is especially true because of the disconnected
>> release
>> * streams for the firmware and the hosting environment.
>> *
>> * @param i_set Indicates which set of fixes we're checking
>> * see HBRT_FIX_LIST_SET...
>> *
>> * @return a bitmask containing the relevant flags for the current
>> * implementation, see HBRT_FIX_FLAGS_...
>> */
>> uint64_t (*get_fix_list)( uint64_t i_set );
>>
>> /* OPAL fixes */
>> #define HBRT_FIX_LIST_SET0_OPAL 0
>> #define HBRT_FIX_FLAGS_OPAL_HAS_XSCOM_RC (1ul << 0)
>>
>> /* PHYP fixes */
>> #define HBRT_FIX_LIST_SET1_PHYP 1
>>
>> If we run over 64 flags, we'd just make a HBRT_FIX_LIST_SET2_OPAL2 2,
etc.
>
> Yep, that makes sense. We'd just need to define the case where the 'set'
> is unknown to opal-prd or pHyp. I'd propose just returning zero, as long
> as we can ensure that zero-bit defaults will be acceptable in future.
On further thoughts, this could makes the interface a little difficult
to implement, mostly on the HBRT side.
If you're running under phyp, would you ever request the flags for
SET0_OPAL? If so, it would indicate that the XSCOM_RC flags is *not* set
(since it's returning 0 for an unknown set)? Or would the phyp
implementation need to know about the OPAL flags?
Or should we make the sets based on functionality, rather than
implementation?
Cheers,
Jeremy
Hi Dan, > My intention was that the flags for OPAL and PHYP would be distinct. > That would handle the specific case we're dealing with right now. I > don't want to force a change into PHYP to set a bit to indicate they > don't have a bug we found in OPAL/opal-prd. Adding features into the > mix does make the idea of a comon set of flags more enticing though. > I'll have to search around for the notes I sent out months ago related > to a 'get_capabilities' function to see if there are more ideas we forgot. > > I still like the idea of multiple sets, so here's attempt #3. > > /* Common features */ > #define HBRT_CAPABILITIES_SET0_COMMON 0 > #define HBRT_CAPABILITIES_COMMON_AWESOME_FEATURE (1ul << 0) > #define HBRT_CAPABILITIES_COMMON_NEW_INTERFACE_PARMS (2ul << 0) > > /* OPAL fixes */ > #define HBRT_CAPABILITIES_SET1_OPAL 1 > #define HBRT_CAPABILITIES_OPAL_HAS_XSCOM_RC (1ul << 0) > > /* PHYP fixes */ > #define HBRT_CAPABILITIES_SET2_PHYP 2 > #define HBRT_CAPABILITIES_PHYP_FIXED_BROKEN_THING (1ul << 0) OK, that almost exactly lines up with the new revision that I'm working on :) >> Or should we make the sets based on functionality, rather than >> implementation? > Not completely sure what you are suggesting here. This was more grouping the sets along the lines of the functionality that they affect, rather than the implementer of the interface (ie, OPAL vs PHYP). However - since HBRT knows whether it's running on OPAL vs. PHYP, it does makes sense to have fixes grouped into OPAL / PHYP categories. As long as we do use the COMMON set(s) when appropriate, and coordinate on changes to that, we should be fine. Are you okay with an unknown set returning 0? Cheers, Jeremy
> Are you okay with an unknown set returning 0? Yes, I think that is the only way it can work. We assume the bugs are un-fixed and the features are not present unless the bits are explicitly set. Do we need to plumb in the reverse (set_capabilities) as well? e.g. If HBRT is still using a version of a function that the host (still haven't found a good generic name for opal-prd/phyp-adjunct) has deprecated, how would you guys know (assuming the ABI isn't smart enough to hide it)? Perhaps we save this for a different day (or even the P9 interface update) regardless though... -- Dan Crowell Senior Software Engineer - Power Systems Enablement Firmware IBM Rochester: t/l 553-2987 dcrowell@us.ibm.com From: Jeremy Kerr <jk@ozlabs.org> To: Daniel M Crowell/Rochester/IBM@IBMUS Cc: Benjamin Herrenschmidt <benh@au1.ibm.com>, Corey Swenson/Rochester/IBM@IBMUS, Patrick Barrett/Rochester/IBM@IBMUS, skiboot list <skiboot@lists.ozlabs.org> Date: 09/13/2016 10:56 PM Subject: Re: [PATCH 1/2] opal-prd: Add get_prd_flags to host interfaces Hi Dan, > My intention was that the flags for OPAL and PHYP would be distinct. > That would handle the specific case we're dealing with right now. I > don't want to force a change into PHYP to set a bit to indicate they > don't have a bug we found in OPAL/opal-prd. Adding features into the > mix does make the idea of a comon set of flags more enticing though. > I'll have to search around for the notes I sent out months ago related > to a 'get_capabilities' function to see if there are more ideas we forgot. > > I still like the idea of multiple sets, so here's attempt #3. > > /* Common features */ > #define HBRT_CAPABILITIES_SET0_COMMON 0 > #define HBRT_CAPABILITIES_COMMON_AWESOME_FEATURE (1ul << 0) > #define HBRT_CAPABILITIES_COMMON_NEW_INTERFACE_PARMS (2ul << 0) > > /* OPAL fixes */ > #define HBRT_CAPABILITIES_SET1_OPAL 1 > #define HBRT_CAPABILITIES_OPAL_HAS_XSCOM_RC (1ul << 0) > > /* PHYP fixes */ > #define HBRT_CAPABILITIES_SET2_PHYP 2 > #define HBRT_CAPABILITIES_PHYP_FIXED_BROKEN_THING (1ul << 0) OK, that almost exactly lines up with the new revision that I'm working on :) >> Or should we make the sets based on functionality, rather than >> implementation? > Not completely sure what you are suggesting here. This was more grouping the sets along the lines of the functionality that they affect, rather than the implementer of the interface (ie, OPAL vs PHYP). However - since HBRT knows whether it's running on OPAL vs. PHYP, it does makes sense to have fixes grouped into OPAL / PHYP categories. As long as we do use the COMMON set(s) when appropriate, and coordinate on changes to that, we should be fine. Are you okay with an unknown set returning 0? Cheers, Jeremy
Hi Dan, >> Are you okay with an unknown set returning 0? > Yes, I think that is the only way it can work. We assume the bugs are > un-fixed and the features are not present unless the bits are explicitly > set. OK, I agree. > Do we need to plumb in the reverse (set_capabilities) as well? Could we just do this with a corresponding call in struct runtime_interfaces? > Perhaps we save this for a different day (or even the P9 interface > update) regardless though... Yep, I think it'd be better to wait until we have a concrete example of usage here. Cheers, Jeremy
diff --git a/external/opal-prd/hostboot-interface.h b/external/opal-prd/hostboot-interface.h index 05fe052..7729a5b 100644 --- a/external/opal-prd/hostboot-interface.h +++ b/external/opal-prd/hostboot-interface.h @@ -277,8 +277,16 @@ struct host_interfaces { int (*memory_error)( uint64_t i_startAddr, uint64_t i_endAddr, enum MemoryError_t i_errorType ); + /** + * @brief Query the prd infrastructure for interface capabilities. + * + * @return a bitmask containing the relevant OPAL_PRD_FLAGS_* for + * this implementation. + */ + uint64_t (*get_prd_flags)(void); + /* Reserve some space for future growth. */ - void (*reserved[32])(void); + void (*reserved[31])(void); }; struct runtime_interfaces { diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c index 18c6e49..775d9f1 100644 --- a/external/opal-prd/opal-prd.c +++ b/external/opal-prd/opal-prd.c @@ -649,6 +649,11 @@ int hservice_memory_error(uint64_t i_start_addr, uint64_t i_endAddr, return 0; } +uint64_t hservice_get_prd_flags(void) +{ + return 0; +} + int hservices_init(struct opal_prd_ctx *ctx, void *code) { uint64_t *s, *d; diff --git a/external/opal-prd/thunk.S b/external/opal-prd/thunk.S index f25afde..58e98d7 100644 --- a/external/opal-prd/thunk.S +++ b/external/opal-prd/thunk.S @@ -186,10 +186,11 @@ hinterface: CALLBACK_THUNK(hservice_i2c_write) CALLBACK_THUNK(hservice_ipmi_msg) CALLBACK_THUNK(hservice_memory_error) + CALLBACK_THUNK(hservice_get_prd_flags) .globl __hinterface_pad __hinterface_pad: /* Reserved space for future growth */ - .space 32*8,0 + .space 31*8,0 .globl __hinterface_end __hinterface_end: /* Eye catcher for debugging */
Another suggestion just to avoid running out of space, let's add a parm to give us essentially an infinite number of values. This also lets us separate out different implementations (i.e. OPAL vs PHYP) to avoid changes in one to indicate a fix in the other. /** * @brief Query the HBRT host for a list of fixes * * There are times when workarounds need to be put into place to handle * issues with the hosting layer (e.g. opal-prd) while fixes are not yet * released. This is especially true because of the disconnected release * streams for the firmware and the hosting environment. * * @param i_set Indicates which set of fixes we're checking * see HBRT_FIX_LIST_SET... * * @return a bitmask containing the relevant flags for the current * implementation, see HBRT_FIX_FLAGS_... */ uint64_t (*get_fix_list)( uint64_t i_set ); /* OPAL fixes */ #define HBRT_FIX_LIST_SET0_OPAL 0 #define HBRT_FIX_FLAGS_OPAL_HAS_XSCOM_RC (1ul << 0) /* PHYP fixes */ #define HBRT_FIX_LIST_SET1_PHYP 1 If we run over 64 flags, we'd just make a HBRT_FIX_LIST_SET2_OPAL2 2, etc. -- Dan Crowell Senior Software Engineer - Power Systems Enablement Firmware IBM Rochester: t/l 553-2987 dcrowell@us.ibm.com