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

[Vaidyanathan Srinivasan]

* Pratik Sampat <psampat at linux.ibm.com> [2020-03-17 15:11:36]:

> 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.

As Pratik mentioned, this is coming from hostboot/hcode component and
splitting them makes it difficult to review.  We should keep
appropriate squashed commit to OPAL tree that makes it easy to review
from OPAL's interface view.

> 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

Hostboot/hcode development model, release criteria are different and
independent of OPAL.  Syncing up commits more than required with these
components may not help us anyway.  I would say these rough edges are
limitation in the way we integration different firmware components.

OPAL and Linux maintainers do a pretty good job of keeping OPAL to
Linux interface compatible and consistent. OPAL and below layers have
rough edges. Atleast now we have good amount of checks in OPAL so as
to not crash and burn :)

--Vaidy