| Message ID | 1610080146-14968-6-git-send-email-tsimpson@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,01/35] Hexagon Update MAINTAINERS file | expand |
Hi Taylor, On 1/8/21 5:28 AM, Taylor Simpson wrote: > Add hexagon to disas/meson.build > Add disas/hexagon.c > Add hexagon to include/disas/dis-asm.h > > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> > --- > include/disas/dis-asm.h | 1 + > disas/hexagon.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++ > disas/meson.build | 1 + > 3 files changed, 70 insertions(+) > create mode 100644 disas/hexagon.c > > diff --git a/include/disas/dis-asm.h b/include/disas/dis-asm.h > index 2164762..1f54fc0 100644 > --- a/include/disas/dis-asm.h > +++ b/include/disas/dis-asm.h > @@ -459,6 +459,7 @@ int print_insn_xtensa (bfd_vma, disassemble_info*); > int print_insn_riscv32 (bfd_vma, disassemble_info*); > int print_insn_riscv64 (bfd_vma, disassemble_info*); > int print_insn_rx(bfd_vma, disassemble_info *); > +int print_insn_hexagon(bfd_vma, disassemble_info *); > > #ifdef CONFIG_CAPSTONE > bool cap_disas_target(disassemble_info *info, uint64_t pc, size_t size); > diff --git a/disas/hexagon.c b/disas/hexagon.c > new file mode 100644 > index 0000000..968f11c > --- /dev/null > +++ b/disas/hexagon.c > @@ -0,0 +1,68 @@ > +/* > + * Copyright(c) 2019-2020 Qualcomm Innovation Center, Inc. All Rights Reserved. 2019-2021 :) > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see <http://www.gnu.org/licenses/>. If possible please also include the SPDX identifier (or simply it): SPDX-License-Identifier: GPL-2.0-or-later > + */ > + > +/* > + * QEMU Hexagon Disassembler > + */ > + > +#include "qemu/osdep.h" > +#include "disas/dis-asm.h" > +#include "target/hexagon/cpu_bits.h" > + > +/* > + * We will disassemble a packet with up to 4 instructions, so we need > + * a hefty size buffer. > + */ > +#define PACKET_BUFFER_LEN 1028 > + > +int print_insn_hexagon(bfd_vma memaddr, struct disassemble_info *info) > +{ > + uint32_t words[PACKET_WORDS_MAX]; > + bool found_end = false; > + char buf[PACKET_BUFFER_LEN]; > + int i; > + > + for (i = 0; i < PACKET_WORDS_MAX && !found_end; i++) { > + int status = (*info->read_memory_func)(memaddr + i * sizeof(uint32_t), > + (bfd_byte *)&words[i], > + sizeof(uint32_t), info); > + if (status) { > + if (i > 0) { > + break; > + } > + (*info->memory_error_func)(status, memaddr, info); > + return status; > + } > + if (is_packet_end(words[i])) { > + found_end = true; > + } > + } > + > + if (!found_end) { > + (*info->fprintf_func)(info->stream, "<invalid>"); > + return PACKET_WORDS_MAX * 4; I suppose 4 is sizeof(uint32_t), right? > + } > + > + int len = disassemble_hexagon(words, i, memaddr, buf, PACKET_BUFFER_LEN); > + int slen = strlen(buf); Per QEMU's CODING_STYLE.rst: Declarations ============ Mixed declarations (interleaving statements and declarations within blocks) are generally not allowed; declarations should be at the beginning of blocks. > + if (buf[slen - 1] == '\n') { > + buf[slen - 1] = '\0'; > + } > + (*info->fprintf_func)(info->stream, "%s", buf); > + > + return len; > +} > diff --git a/disas/meson.build b/disas/meson.build > index 09a8527..b7b659b 100644 > --- a/disas/meson.build > +++ b/disas/meson.build > @@ -6,6 +6,7 @@ common_ss.add(when: 'CONFIG_ARM_A64_DIS', if_true: files('arm-a64.cc')) > common_ss.add_all(when: 'CONFIG_ARM_A64_DIS', if_true: libvixl_ss) > common_ss.add(when: 'CONFIG_ARM_DIS', if_true: files('arm.c')) > common_ss.add(when: 'CONFIG_CRIS_DIS', if_true: files('cris.c')) > +common_ss.add(when: 'CONFIG_HEXAGON_DIS', if_true: files('hexagon.c')) > common_ss.add(when: 'CONFIG_HPPA_DIS', if_true: files('hppa.c')) > common_ss.add(when: 'CONFIG_I386_DIS', if_true: files('i386.c')) > common_ss.add(when: 'CONFIG_LM32_DIS', if_true: files('lm32.c')) > Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> With comments addressed: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> -----Original Message----- > From: Qemu-devel <qemu-devel- > bounces+tsimpson=quicinc.com@nongnu.org> On Behalf Of Philippe > Mathieu-Daudé > Sent: Saturday, January 9, 2021 3:38 PM > To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org > Cc: ale@rev.ng; Brian Cain <bcain@quicinc.com>; > richard.henderson@linaro.org; laurent@vivier.eu > Subject: Re: [PATCH v6 05/35] Hexagon (disas) disassembler > > Hi Taylor, > > On 1/8/21 5:28 AM, Taylor Simpson wrote: > > +/* > > + * Copyright(c) 2019-2020 Qualcomm Innovation Center, Inc. All Rights > Reserved. > > 2019-2021 :) > > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > > If possible please also include the SPDX identifier (or simply it): > > SPDX-License-Identifier: GPL-2.0-or-later I'll confirm with our legal department. Which is preferred - the identifier alone or the text and the identifier?
+Thomas/Daniel/Peter. On 1/11/21 10:14 PM, Taylor Simpson wrote: >> -----Original Message----- >> From: Qemu-devel <qemu-devel- >> bounces+tsimpson=quicinc.com@nongnu.org> On Behalf Of Philippe >> Mathieu-Daudé >> Sent: Saturday, January 9, 2021 3:38 PM >> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org >> Cc: ale@rev.ng; Brian Cain <bcain@quicinc.com>; >> richard.henderson@linaro.org; laurent@vivier.eu >> Subject: Re: [PATCH v6 05/35] Hexagon (disas) disassembler >> >> Hi Taylor, >> >> On 1/8/21 5:28 AM, Taylor Simpson wrote: >>> +/* >>> + * Copyright(c) 2019-2020 Qualcomm Innovation Center, Inc. All Rights >> Reserved. >> >> 2019-2021 :) >> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License as published by >>> + * the Free Software Foundation; either version 2 of the License, or >>> + * (at your option) any later version. >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> + * GNU General Public License for more details. >>> + * >>> + * You should have received a copy of the GNU General Public License >>> + * along with this program; if not, see <http://www.gnu.org/licenses/>. >> >> If possible please also include the SPDX identifier (or simply it): >> >> SPDX-License-Identifier: GPL-2.0-or-later > > I'll confirm with our legal department. Which is preferred - the identifier alone or the text and the identifier? Obviously IANAL, but my understanding from explanations from Thomas and Daniel is -- if one day QEMU switches to using SPDX -- when both are provided, it is very hard to remove a text license (which often is copy/pasted with mistakes). So the identifier alone is better (assuming your legal department confirms it has the same value). Now if you ask if there is any plan QEMU switch to SPDX, I'd say this is a gray zone. Peter expressively said he prefers a full switch or nothing. We don't have the resources for it. Meanwhile some companies prefer their employees to send new contributions with SPDX as it eases their compliance audit tools. I guess Thomas spend 1 week on this topic. TBH I spent more than 2 weeks and barely added the SPDX tag to a bit more than 30% of the codebase then had to give up because my time was over (this was when I understood I couldn't remove the text and had to redo the work). Not sure how this can be coordinated. As long as there is no strong corporate interest, this might stay in this gray zone... Regards, Phil.
On Mon, Jan 11, 2021 at 11:20:27PM +0100, Philippe Mathieu-Daudé wrote: > +Thomas/Daniel/Peter. > > On 1/11/21 10:14 PM, Taylor Simpson wrote: > >> -----Original Message----- > >> From: Qemu-devel <qemu-devel- > >> bounces+tsimpson=quicinc.com@nongnu.org> On Behalf Of Philippe > >> Mathieu-Daudé > >> Sent: Saturday, January 9, 2021 3:38 PM > >> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org > >> Cc: ale@rev.ng; Brian Cain <bcain@quicinc.com>; > >> richard.henderson@linaro.org; laurent@vivier.eu > >> Subject: Re: [PATCH v6 05/35] Hexagon (disas) disassembler > >> > >> Hi Taylor, > >> > >> On 1/8/21 5:28 AM, Taylor Simpson wrote: > >>> +/* > >>> + * Copyright(c) 2019-2020 Qualcomm Innovation Center, Inc. All Rights > >> Reserved. > >> > >> 2019-2021 :) > >> > >>> + * > >>> + * This program is free software; you can redistribute it and/or modify > >>> + * it under the terms of the GNU General Public License as published by > >>> + * the Free Software Foundation; either version 2 of the License, or > >>> + * (at your option) any later version. > >>> + * > >>> + * This program is distributed in the hope that it will be useful, > >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >>> + * GNU General Public License for more details. > >>> + * > >>> + * You should have received a copy of the GNU General Public License > >>> + * along with this program; if not, see <http://www.gnu.org/licenses/>. > >> > >> If possible please also include the SPDX identifier (or simply it): > >> > >> SPDX-License-Identifier: GPL-2.0-or-later > > > > I'll confirm with our legal department. Which is preferred - the identifier alone or the text and the identifier? > > Obviously IANAL, but my understanding from explanations from Thomas and > Daniel is -- if one day QEMU switches to using SPDX -- when both are > provided, it is very hard to remove a text license (which often is > copy/pasted with mistakes). So the identifier alone is better (assuming > your legal department confirms it has the same value). > > Now if you ask if there is any plan QEMU switch to SPDX, I'd say this > is a gray zone. Peter expressively said he prefers a full switch or > nothing. We don't have the resources for it. Meanwhile some companies > prefer their employees to send new contributions with SPDX as it eases > their compliance audit tools. > > I guess Thomas spend 1 week on this topic. TBH I spent more than 2 weeks > and barely added the SPDX tag to a bit more than 30% of the codebase > then had to give up because my time was over (this was when I understood > I couldn't remove the text and had to redo the work). > > Not sure how this can be coordinated. As long as there is no strong > corporate interest, this might stay in this gray zone... I can't see us removing all existing license boilerplate because of the legal challenges that involves. The amount of work does not justify the potential payoff, as compared to all the other useful things that could be worked on in QEMU. My expectation is that all files should have license header for the sake of consistency across the codebase, and may optionally have SPDX. With all this in mind, IMHO reviewers should NOT be raising with contributions to QEMU, as it creates a burden for contributors to talk to their legal teams. The amount of new files being added to QEMU are negligible compared to what's already in tree, so any new files we accept aren't making the situation measurably worse. Just accept files with normal license headers as we've always done. Anything involving SPDX in future will just need to bulk updates everything. Regards, Daniel
diff --git a/include/disas/dis-asm.h b/include/disas/dis-asm.h index 2164762..1f54fc0 100644 --- a/include/disas/dis-asm.h +++ b/include/disas/dis-asm.h @@ -459,6 +459,7 @@ int print_insn_xtensa (bfd_vma, disassemble_info*); int print_insn_riscv32 (bfd_vma, disassemble_info*); int print_insn_riscv64 (bfd_vma, disassemble_info*); int print_insn_rx(bfd_vma, disassemble_info *); +int print_insn_hexagon(bfd_vma, disassemble_info *); #ifdef CONFIG_CAPSTONE bool cap_disas_target(disassemble_info *info, uint64_t pc, size_t size); diff --git a/disas/hexagon.c b/disas/hexagon.c new file mode 100644 index 0000000..968f11c --- /dev/null +++ b/disas/hexagon.c @@ -0,0 +1,68 @@ +/* + * Copyright(c) 2019-2020 Qualcomm Innovation Center, Inc. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +/* + * QEMU Hexagon Disassembler + */ + +#include "qemu/osdep.h" +#include "disas/dis-asm.h" +#include "target/hexagon/cpu_bits.h" + +/* + * We will disassemble a packet with up to 4 instructions, so we need + * a hefty size buffer. + */ +#define PACKET_BUFFER_LEN 1028 + +int print_insn_hexagon(bfd_vma memaddr, struct disassemble_info *info) +{ + uint32_t words[PACKET_WORDS_MAX]; + bool found_end = false; + char buf[PACKET_BUFFER_LEN]; + int i; + + for (i = 0; i < PACKET_WORDS_MAX && !found_end; i++) { + int status = (*info->read_memory_func)(memaddr + i * sizeof(uint32_t), + (bfd_byte *)&words[i], + sizeof(uint32_t), info); + if (status) { + if (i > 0) { + break; + } + (*info->memory_error_func)(status, memaddr, info); + return status; + } + if (is_packet_end(words[i])) { + found_end = true; + } + } + + if (!found_end) { + (*info->fprintf_func)(info->stream, "<invalid>"); + return PACKET_WORDS_MAX * 4; + } + + int len = disassemble_hexagon(words, i, memaddr, buf, PACKET_BUFFER_LEN); + int slen = strlen(buf); + if (buf[slen - 1] == '\n') { + buf[slen - 1] = '\0'; + } + (*info->fprintf_func)(info->stream, "%s", buf); + + return len; +} diff --git a/disas/meson.build b/disas/meson.build index 09a8527..b7b659b 100644 --- a/disas/meson.build +++ b/disas/meson.build @@ -6,6 +6,7 @@ common_ss.add(when: 'CONFIG_ARM_A64_DIS', if_true: files('arm-a64.cc')) common_ss.add_all(when: 'CONFIG_ARM_A64_DIS', if_true: libvixl_ss) common_ss.add(when: 'CONFIG_ARM_DIS', if_true: files('arm.c')) common_ss.add(when: 'CONFIG_CRIS_DIS', if_true: files('cris.c')) +common_ss.add(when: 'CONFIG_HEXAGON_DIS', if_true: files('hexagon.c')) common_ss.add(when: 'CONFIG_HPPA_DIS', if_true: files('hppa.c')) common_ss.add(when: 'CONFIG_I386_DIS', if_true: files('i386.c')) common_ss.add(when: 'CONFIG_LM32_DIS', if_true: files('lm32.c'))
Add hexagon to disas/meson.build Add disas/hexagon.c Add hexagon to include/disas/dis-asm.h Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> --- include/disas/dis-asm.h | 1 + disas/hexagon.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++ disas/meson.build | 1 + 3 files changed, 70 insertions(+) create mode 100644 disas/hexagon.c