Re: [PATCH 6/6] Add the AEHD implementation.

[Haitao Shan]

On Fri, Mar 3, 2023 at 2:15 AM Philippe Mathieu-Daudé <philmd@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 <hshan@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   <aliguori@us.ibm.com>
> > + *  Glauber Costa     <gcosta@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