Re: [PATCH 5/5] cxl/vendor: SK hynix Niagara Multi-Headed SLD Device

[Gregory Price <gregory.price@memverge.com>]

On Wed, Sep 06, 2023 at 02:04:45PM +0100, Jonathan Cameron wrote:
> On Thu, 31 Aug 2023 21:29:14 -0400
> Gregory Price <gourry.memverge@gmail.com> wrote:
> 
> Hi Gregory,
> 
> Some comments inline, but I'm happy to add this to my staging tree in the meantime
> as it stands (might be a few days until I push a new branch though).
> 

I'm going to do one a quick v3 today with the feedback and some cleanup
in spots i noticed.

> > Signed-off-by: Gregory Price <gregory.price@memverge.com>
> > Signed-off-by: Junhee Ryu <junhee.ryu@sk.com>
> > Signed-off-by: Kwangjin Ko <kwangjin.ko@sk.com>
> 
> The SoB chain needs cleaning up.  Is this a co-developed situation?
> If it is use the rules in the kernel documentation as I don't think those
> are yet clearly stated in QEMU docs (and they are confusing so I won't try
> to restate them here).
> 

TL;DR: They gave me the command list, I wrote the model.  We got
approval to release the model, but I wasn't sure how to capture the
copyright/SoB list.  I suppose the copyright covers SKh, but since I
authored the model, it only requires my SoB?

After reading, I'm still not sure how to capture this lol.

Should I just switch the skh folks to Co-developed-by?

> 
> > diff --git a/hw/cxl/vendor/skhynix/meson.build b/hw/cxl/vendor/skhynix/meson.build
> > new file mode 100644
> > index 0000000000..4e57db65f1
> > --- /dev/null
> > +++ b/hw/cxl/vendor/skhynix/meson.build
> > @@ -0,0 +1 @@
> > +system_ss.add(when: 'CONFIG_CXL_VENDOR', if_true: files('skhynix_niagara.c',))
> > diff --git a/hw/cxl/vendor/skhynix/skhynix_niagara.c b/hw/cxl/vendor/skhynix/skhynix_niagara.c
> > new file mode 100644
> > index 0000000000..88e53cc6cc
> > --- /dev/null
> > +++ b/hw/cxl/vendor/skhynix/skhynix_niagara.c
> > @@ -0,0 +1,516 @@
> > +/*
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + *
> > + * Copyright (c) 2023 MemVerge Inc.
> > + * Copyright (c) 2023 SK hynix Inc.
> > + */
> > +
> > +#include <sys/shm.h>
> 
> This will need some osdep.h magic.  There is some there
> already but it will need relaxing (unless you want to run only on sparc ;)
> and we may need to make this device linux host only.
> 
> 

Good point, I had not considered osdep issues.  Do you know of any
examples of linux-only devices I can use to do a quick patch-up? I 
can come back around on this issue later.

> 
> > +
> > +enum {
> > +    NIAGARA_MHD = 0x55,
> > +        #define GET_MHD_INFO 0x0
> 
> Is this standard as it's in the normal space?
> If it is then I'd like the implementation pushed down to the
> type3 implementation (with some callbacks or similar.)
> 

:thinking_face:

maybe a similar pattern to the callback from before? I suppose I could
push this down into type3 and add an mhd callback in the class and have
niagara fill that in with the callback.

That *feels* right, so i'll go ahead with it.



If I misunderstood anything, let me know

~Gregory