[Skiboot] [PATCH v4 1/4] Self Save: Introducing Support for SPR Self Save

[Pratik Sampat]

Hello Micheal,


On 17/03/20 8:43 am, Michael Ellerman wrote:
> Pratik Rajesh Sampat <psampat at linux.ibm.com> writes:
>> From: Prem Shanker Jha <premjha2 at in.ibm.com>
>>
>> The commit is a merger of commits that makes the following changes:
>> 1. Commit fixes some issues with code found during integration test
>>    -  replacement of addi with xor instruction during self save API.
>>    -  fixing instruction generation for MFMSR during self save
>>    -  data struct updates in STOP API
>>    -  error RC updates for hcode image build
>>    -  HOMER parser updates.
>>    -  removed self save support for URMOR and HRMOR
>>    -  code changes for compilation with OPAL
>>    -  populating CME Image header with unsecure HOMER address.
> Why did you merge those commits?
>
> The patch is basically unreviewable in its current form. If I was the
> maintainer I'd ask you to split it into smaller reviewable pieces. Or at
> the very least split the above changes out from the STOP API changes
> below.
>
> cheers

You're right. The patch is quite difficult in the state it is now.

The problem essentially is that these are two commits in the hostboot
repository which were commited by somebody else much earlier and to maintain
consistency I didn't split the commit.

The reason for squashing these commits is that the second commit changed the
abstractions that were established by the first commit which made it even more
difficult to comprehend what was going on.

I would be happy to split this if it helps in reviewing. Also if you have any
other suggestion of how I can front this patch better, I'd be glad.

Links for the commits in hostboot:
https://github.com/open-power/hostboot/commit/55a180dc08f9e9a837672ebae03a92dd9bba3ef9
https://github.com/open-power/hostboot/commit/b3996f56863f07cd6fa2f5a392c5c2497d177b35

Thanks!
Pratik

>> Key_Cronus_Test=PM_REGRESS
>>
>> Change-Id: I7cedcc466267c4245255d8d75c01ed695e316720
>> Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/66580
>> Tested-by: FSP CI Jenkins <fsp-CI-jenkins+hostboot at us.ibm.com>
>> Tested-by: HWSV CI <hwsv-ci+hostboot at us.ibm.com>
>> Tested-by: PPE CI <ppe-ci+hostboot at us.ibm.com>
>> Tested-by: Jenkins Server <pfd-jenkins+hostboot at us.ibm.com>
>> Tested-by: Cronus HW CI <cronushw-ci+hostboot at us.ibm.com>
>> Tested-by: Hostboot CI <hostboot-ci+hostboot at us.ibm.com>
>> Reviewed-by: Gregory S. Still <stillgs at us.ibm.com>
>> Reviewed-by: RAHUL BATRA <rbatra at us.ibm.com>
>> Reviewed-by: Jennifer A. Stofer <stofer at us.ibm.com>
>> Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/66587
>> Reviewed-by: Christian R. Geddes <crgeddes at us.ibm.com>
>> Signed-off-by: Prem Shanker Jha <premjha2 at in.ibm.com>
>> Signed-off-by: Akshay Adiga <akshay.adiga at linux.vnet.ibm.com>
>> Signed-off-by: Pratik Rajesh Sampat <psampat at linux.ibm.com>
>>
>> 2. The commit also incorporates changes that make STOP API project
>> agnostic changes include defining wrapper functions which call legacy
>> API. It also adds duplicate enum members which start with prefix PROC
>> instead of P9.
>>
>> Key_Cronus_Test=PM_REGRESS
>>
>> Change-Id: If87970f3e8cf9b507f33eb1be249e03eb3836a5e
>> RTC: 201128
>> Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/71307
>> Tested-by: FSP CI Jenkins <fsp-CI-jenkins+hostboot at us.ibm.com>
>> Tested-by: Jenkins Server <pfd-jenkins+hostboot at us.ibm.com>
>> Tested-by: Hostboot CI <hostboot-ci+hostboot at us.ibm.com>
>> Tested-by: Cronus HW CI <cronushw-ci+hostboot at us.ibm.com>
>> Reviewed-by: RANGANATHPRASAD G. BRAHMASAMUDRA <prasadbgr at in.ibm.com>
>> Reviewed-by: Gregory S. Still <stillgs at us.ibm.com>
>> Reviewed-by: Jennifer A Stofer <stofer at us.ibm.com>
>> Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/71314
>> Tested-by: Jenkins OP Build CI <op-jenkins+hostboot at us.ibm.com>
>> Tested-by: Jenkins OP HW <op-hw-jenkins+hostboot at us.ibm.com>
>> Reviewed-by: Daniel M. Crowell <dcrowell at us.ibm.com>
>> Signed-off-by: Prem Shanker Jha <premjha2 at in.ibm.com>
>> Signed-off-by: Pratik Rajesh Sampat <psampat at linux.ibm.com>
>> ---
>>   include/p9_stop_api.H                    |  79 +-
>>   libpore/p9_cpu_reg_restore_instruction.H |   4 +
>>   libpore/p9_stop_api.C                    | 954 +++++++++++++----------
>>   libpore/p9_stop_api.H                    | 115 ++-
>>   libpore/p9_stop_data_struct.H            |   4 +-
>>   libpore/p9_stop_util.H                   |   7 +-
>>   6 files changed, 721 insertions(+), 442 deletions(-)