Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards

[Aleksandar Markovic <aleksandar.m.mail@gmail.com>]

On Thu, Nov 21, 2019 at 8:52 PM Taylor Simpson <tsimpson@quicinc.com> wrote:
>
> They are imported from the existing Hexagon simulator.  Please understand that this patch is the first in a series.  Later patches will contain more elaborate contents in that directory.  The reason I don't want to reformat them is to stay in sync with the other simulator in the future.  When the other team makes changes to the code (either to fix bugs or add features), it will be easier to identify the changes and bring them into qemu.
>
> Taylor
>

Taylor,

Please understand that this patch can't remain a single patch. It
can't remain even a set of 2 or 3 patches as others suggested. A patch
is a logically connected unit of code whose typical size is less than
200 lines. There are lots of such logical units in this single path
that you sent, and you should not have sent it in its present form,
even if you wanted just comments to it. You should have submitted a
series rather than a single patch. And you should have said this is v1
of my series that I will expand later on. Guidelines for submissions
are here:

https://wiki.qemu.org/Contribute/SubmitAPatch

As far as "imported" files, frankly, I dislike the fact that you are
willing to sacrifice our coding style guidelines in favor to your
convenience. But, more than this, I also find very problematic that
you practically create a dependency between QEMU and another
simulator. QEMU implementation should rely on specifications, and only
on specifications, and certainly should not depend on another
simulator. Currently, in QEMU, there are some cases of imported
disassemblers or similar relatively unimportant tools, but those
imports change very rarely, and are modified to comply to QEMU coding
style. I am not aware on dependency of QEMU on another simulator in
the form you want to do for Hexagon. My strong impression is that you
will create more problems than benefits with such dependency, both for
you and for QEMU in general.

Once a CPU or any other device is specified though documentation,
these specs don't change. Consequently, their emulation does not
change too, in functional sense. The fact that you anticipate changes
in these files imported from another simulator, leaves me with a
(possibly wrong) perception that neither Hexagon internal simulator
nor QEMU implementation you are trying to integrate are complete. If
that is not true, can you explain what exactly you expect to be
changing in imported files?

Yours,
Aleksandar


> -----Original Message-----
> From: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
> Sent: Thursday, November 21, 2019 1:20 PM
> To: Taylor Simpson <tsimpson@quicinc.com>
> Cc: Laurent Vivier <laurent@vivier.eu>; Riku Voipio <riku.voipio@iki.fi>; QEMU Developers <qemu-devel@nongnu.org>
> Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards
>
>
> >  create mode 100644 target/hexagon/imported/global_types.h
> >  create mode 100644 target/hexagon/imported/iss_ver_registers.h
> >  create mode 100644 target/hexagon/imported/max.h  create mode 100644
> > target/hexagon/imported/regs.h
>
> Taylor, if I understood you well, these files don't confirm to QEMU coding standard, because they are imported. But, from where? And what is the reason they need to be imported (and not created independently by you or somebody else, but within QEMU code style guidelines) ?
> Their content looks fairly simple to me.
>
> Thanks,
> Aleksandar