On Friday, November 22, 2019, Taylor Simpson <tsimpson@quicinc.com> wrote:

Hi Aleksandar,

The complete qemu-hexagon implementation can be found here
https://github.com/quic/qemu

From the beginning, I knew I would have to divide it up into smaller patches for review. The discussion on how divide it was here
https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg01971.html

I'll summarize the proposal
1) linux-user changes + linux-user/hexagon + skeleton of target/hexagon
2) Add the code this imported from the Hexagon simulator and the qemu helper generator
3) Add support for packet semantics
4) Add support for wide vector extensions
5) Add the helper overrides for performance optimization

I think you, for some reason, mix and keep mixing two really different things:

- outline of your changes (as seen as points 1-5 above)

- a patch

None of points 1-5 could be a patch. All of them should be patch series, or part of patch series.

I am surprised that it seems as if you did not take a look at the previous cases of platform introductions to QEMU. Or you did, but decided you don't need to bother creating patch series, you don't need to follow QEMU submission guidelines - you apparently think you can simply send the changes as several huge patches.

My assumption in the proposal was that each phase of the submission should compile and execute, and each phase should expand the capabilities of the Hexagon target. I apologize that it wasn't clear how large each of the patches would be.

I greatly appreciate all of the feedback on the proposal and the patch, and I am making the revisions. In several cases, the feedback also applies to the remainder of the code. So, I am making the changes there as well. From that perspective, I think it will reduce the overall burden on the reviewers.

As far as what might change in the imported files goes, the current simulator (hexagon-sim) has been used in production as part of the toolchain for over a decade and has been used to verify every version of the core that we have shipped. Since it is used in verification, it is considered the gold standard - more so than the PDF manual. Any changes to that code, including reformatting, would put qemu at risk of not accurately emulating the processor.

We in MIPS also have ISA verification software that can be viewed as "gold standard", but we don't burden QEMU with any imported files - and we certainly could, using the same rationale as you do.

I am opposed to considering the output (or a part) of an internally developed simulator more important than (or on the same level as) documentation, and calling it a "gold standard".

A gold-standard-related question arises here:

If something is called a "gold standard", should it be a constant, by its very definition?

Also, you did not answer my question from my previous resoonse, which was:

What do you expect to be changing in imported-from-another-simulator code?

And I add another one:

What exactly components of QEMU for Hexagon are contained in imported-from-another-simulator code?

Thanks,

Aleksandar

Taylor

-----Original Message-----
From: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
Sent: Thursday, November 21, 2019 2:45 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

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