Re: [Minios-devel] [PATCH 00/47] MINI-OS: enable the arm64 support

[Huang Shijie <shijie.huang@xxxxxxx>]

On Fri, Mar 16, 2018 at 10:08:50AM +0000, Julien Grall wrote:
Hi Julien,
> Hi,
> 
> On 16/03/18 02:15, Huang Shijie wrote:
> > On Thu, Mar 15, 2018 at 10:51:56AM +0000, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 15/03/18 04:48, Huang Shijie wrote:
> > > > On Wed, Mar 14, 2018 at 10:21:52AM +0000, Julien Grall wrote:
> > > > Hi Julien,
> > > >      I feel sorry that the patch set was not sent outsides.
> > > > 
> > > >      I checked the archive for minios, and I did not find the email.
> > > >      It seems there is something wrong with my git config, I will check 
> > > > it,
> > > >      and fix it, and send it again.
> > > 
> > > Are you registered on the minios mailing list?
> > I did not registered on the minios mailing list, I check it by the archive.
> > > 
> > > > > Few generic comments on this series.
> > > > > 
> > > > > On 03/14/2018 09:39 AM, Huang Shijie wrote:
> > > > > >   2.) Tests
> > > > > >     I tested this patch set on Softiron(arm64) and x86_64 platform.
> > > > > 
> > > > > How about arm32? What is the state after this series?
> > > > 
> > > > I did not test the arm32, since it even can not pass the compiler for 
> > > > arm32.
> > > > I suggest we do not care about the arm32, and fix it after the arm64 
> > > > code is merged
> > > > in future.
> > > 
> > > Well, we already had a discussion on this on the previous version and 
> > > agreed
> > > on a plan. I would like to understand why this was not followed?
> > I think I have followed the plan:
> >     1.) change the DTC as a folder, not the submodule.
> >     2.) refactor the arm32 code the separate folders.
> > 
> > Which is missing from the plan?
> 
> Dropping arm32 code, this is what I meant by "clean slate" and clarified by
> various e-mail privately and on the mailing list. See Wei's answer [1]:

Thanks a lot.
I will remove the arm32 code in the next version.

Thanks
Huang Shijie
> 
> "If you're sure arm32 can't work, #2 is probably easier.  Please stick a
> patch at the beginning to rip out the old port. That can easily be
> applied."
> > > 
> > > > 
> > > > > 
> > > > > On the previous version, I clearly suggested 2 paths to add support 
> > > > > for
> > > > > arm64:
> > > > > 
> > > > > "I can see two solutions going forward:
> > > > >          1) The arm directory is first reshaped to welcome arm64. 
> > > > > This means:
> > > > >                  * moving out arm32 specific code
> > > > >                  * switch to LPAE page-table
> > > > >                  * introducing helpers for common code to call 
> > > > > arch-specific
> > > > > code
> > > > >             On the code is reshaped, the arm64 series is added on top.
> > > > > 
> > > > >          2) Start the arm64 port from a clean slate and then port 
> > > > > arm32 over.
> > > > > 
> > > > > Knowing the state of the arm32 port, I would lean towards 2). This 
> > > > > would
> > > > > allow more flexibility and make easier to review. At the moment, I 
> > > > > have to
> > > > > hunt down the code to see what is missing."
> > > > > 
> > > > > This series does not follow any of them and end up to have #if
> > > > > defined(__aarch64__) in the common code. This really defeating the 
> > > > > purpose
> > > > > of the refactoring below.
> > > > > 
> > > > > To be clear, I am not suggesting to add arm32 port, I am just asking 
> > > > > to not
> > > > > make things worst than the current state.
> > > > The current state is already very worst for arm32 now. :)
> > > > 
> > > > Without this patch set, the arm32 is not work; with this patch set, the 
> > > > arm32
> > > > still cannot work...
> > > 
> > > So what's the point to keep that code around? This making this series 
> > > nearly
> > I moved the arm32 code to the separate folder, and do not change it.
> 
> You didn't change that code but change quite heavily the common code by
> spreading #ifdef aarch64 in it. This design looks completely wrong and
> clearly show that the two port are currently not that compatible. Indeed the
> memory management is quite different (LPAE vs short page-table).
> 
> > I thought I have done it from a clear slate.
> > Now, I found I feel confused about the "clean slate"..
> 
> Clean slate means:
> 
> "a state in which you are starting an activity or process again, not
> considering what has happened in the past at all"
> 
> In that particular context it means removing the arm32 port and start from
> scratch.
> 
okay.


> There are clearly no point to keep code around that are completely wrong and
> where I have no way to verify whether this is valid change without having to
> spend a significant amount of time to hunt down what are missing.
> 
> Cheers,
> 
> [1]
> https://lists.xenproject.org/archives/html/minios-devel/2017-11/msg00136.html
> 
> -- 
> Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel