On Fri, Mar 3, 2023 at 2:15 AM Philippe Mathieu-Daudé <phi...@linaro.org> wrote: > > On 3/3/23 03:26, Haitao Shan wrote: > > Implement the AEHD accelerator including the AEHD AccelClass, > > AccelCPUClass, AccelOpsClass. > > > > Signed-off-by: Haitao Shan <hs...@google.com> > > --- > > hw/i386/x86.c | 2 +- > > include/exec/ram_addr.h | 2 - > > include/sysemu/aehd.h | 87 ++ > > include/sysemu/hw_accel.h | 1 + > > target/i386/aehd/aehd-accel-ops.c | 119 ++ > > target/i386/aehd/aehd-accel-ops.h | 22 + > > target/i386/aehd/aehd-all.c | 1020 +++++++++++++++ > > target/i386/aehd/aehd-cpu.c | 150 +++ > > target/i386/aehd/aehd-cpu.h | 41 + > > target/i386/aehd/aehd-stub.c | 22 + > > target/i386/aehd/aehd.c | 1915 +++++++++++++++++++++++++++++ > > target/i386/aehd/aehd_i386.h | 26 + > > target/i386/aehd/aehd_int.h | 2 +- > > target/i386/aehd/meson.build | 4 + > > target/i386/cpu.c | 12 +- > > target/i386/cpu.h | 5 +- > > target/i386/helper.c | 3 + > > target/i386/meson.build | 1 + > > 18 files changed, 3428 insertions(+), 6 deletions(-) > > I'm not going to review a 3000+ LoC patch, sorry. > > 1/ it is too big for my brain and I'm going to miss things I fully understand. I also struggled quite a bit when preparing the patch set. I know it is good to keep patches as small as possible. At the same time, each patch is required to compile and function independently. As the user space codes for AEHD are kind of a whole, I don't know how to split them.
While I will continue finding a good strategy to split the big patch, do you happen to have some suggestions? > 2/ it is likely duplicating KVM identical code; if so we > should refactor to use common code. In particular because > we can fix bugs once. I agree. Hope the KVM maintainer can give me some guide or he might disagree completely. > > > create mode 100644 target/i386/aehd/aehd-accel-ops.c > > create mode 100644 target/i386/aehd/aehd-accel-ops.h > > create mode 100644 target/i386/aehd/aehd-cpu.c > > create mode 100644 target/i386/aehd/aehd-cpu.h > > create mode 100644 target/i386/aehd/aehd-stub.c > > create mode 100644 target/i386/aehd/aehd_i386.h > > > > --- /dev/null > > +++ b/target/i386/aehd/aehd-accel-ops.c > > @@ -0,0 +1,119 @@ > > +/* > > + * QEMU AEHD support > > + * > > + * Copyright IBM, Corp. 2008 > > + * Red Hat, Inc. 2008 > > + * > > + * Authors: > > + * Anthony Liguori <aligu...@us.ibm.com> > > + * Glauber Costa <gco...@redhat.com> > > In various files you add in this series you use incorrect > author / (c). While you copy/paste their implementation, > they didn't authored this AEHD implementation (which is based > on their work). This is a surprise to me. I thought the practice was to copy and keep the original license header. I should be able to add but probably not to delete. So just let me confirm, it is OK to remove them while keeping the license text. Or I can change something like: Originally Authored by: Original Authors list Modified by: Me -- Haitao @Google
