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

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Tue, 5 Sep 2023 12:04:29 -0400
Gregory Price <gregory.price@memverge.com> wrote:

> 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?

Yes - I think only your SoB is fine.  Maybe some text to explain the other
parts.

> 
> 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.

No idea.

> 
> >   
> > > +
> > > +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