Re: [PATCH 3/4] cxl/type3: minimum MHD cci support

[Jonathan Cameron]

On Thu, 31 Aug 2023 12:59:24 -0400
Gregory Price <gregory.price@memverge.com> wrote:

> On Mon, Aug 07, 2023 at 03:56:09PM +0100, Jonathan Cameron wrote:
> > On Fri, 21 Jul 2023 12:35:08 -0400
> > Gregory Price <gourry.memverge@gmail.com> wrote:
> >   
> > > Implement the MHD GET_INFO cci command and add a shared memory
> > > region to the type3 device to host the information.
> > > 
> > > Add a helper program to initialize this shared memory region.
> > > 
> > > Add a function pointer to type3 devices for future work that
> > > will allow an mhd device to provide a hook to validate whether
> > > a memory access is valid or not.
> > > 
> > > For now, limit the number of LD's to the number of heads. Later,
> > > this limitation will need to be lifted for MH-MLDs.
> > > 
> > > Intended use case:
> > > 
> > > 1. Create the shared memory region
> > > 2. Format the shared memory region
> > > 3. Launch QEMU with `is_mhd=true,mhd_head=N,mhd_shmid=$shmid`  
> > 
> > I'd definitely like some feedback from experienced QEMU folk on
> > how to model this sort of cross QEMU instance sharing.
> > 
> > My instinct is a socket would be more flexible as it lets us
> > potentially emulate the machines on multiple hosts (assuming they
> > can see some shared backend storage).
> >   
> 
> I'd considered a socket, but after mulling it over, I realized the
> operations I was trying to implement amounted to an atomic compare
> and swap.  I wanted to avoid locks and serialization if at all
> possible to avoid introducing that into the hot-path of memory
> accesses.  Maybe there is a known pattern for this kind of
> multi-instance QEMU coherency stuff, but that seemed the simplest path.

I'd definitely like some input from others on this if possible as I've
no idea if this problem has really been tackled in the past.
> 
> This obviously has the downside of limiting emulation of MHD system to
> a local machine, but the use of common files/memory to back CXL memory
> already does that (i think).

Files should be fine I think on a suitable sharing aware file system.

> 
> ¯\_(ツ)_/¯ feedback welcome.  I'll leave it for now unless there are
> strong opinions.
> 
> > else not needed if we returned in previous leg.
> > 
> >     if (ct3d->is_mhd && ...
> >   
> > > +               (!ct3d->mhd_shmid || (ct3d->mhd_head == ~(0)))) {  
> > 
> > How does mhd_head equal that? Default is 0. I'm not sure there is a reason
> > to require it.
> >   
> 
> Good catch, the default should be ~(0).  Updated in the field
> initialization section.
> 
> > > +    /* For now, limit the number of heads to the number of LD's (SLD) */ 
> > >  
> > 
> > Feels backwards.  Number of heads never going to be bigger than numbre of
> > LDs.  Other way around is possible of course.
> >   
> > > +    if (ct3d->mhd_state->nr_heads <= ct3d->mhd_head) {  
> > 
> > mhd head needs to be out of range?  Confused.
> >   
> 
> Woops, yes this should be mhd_head >= nr_heads. Wording is backwards and
> so is the code.  fixed.
> 
> > > +    if (ct3d->mhd_state) {
> > > +        shmdt(ct3d->mhd_state);
> > > +    }  
> > 
> > Reverse order of realize - so I think this wants to be earlier.
> >   
> > >  }  
> 
> Just to be clear you *don't* want the reverse order, correct?  If so
> i'll swap it.

I do want reverse order.   Generally speaking it's much easier to
review precisely matching reverse order as then mostly the ordering is
fine in a way that it isn't in any other ordering.

> 
> > >  
> > > +    if (ct3d->is_mhd && ct3d->mhd_access_valid) {
> > > +        if (!ct3d->mhd_access_valid(ct3d, dpa_offset, size))
> > > +            return MEMTX_ERROR;  
> > 
> > Brackets for inner block.
> > Arguably could just add the ct3d->is_mhd check in the place where
> > mhd_access_valid is set and hence only need to check that here.
> > Maybe that makes it slightly harder to follow though.
> > 
> > Also could just unset is_mhd if mhd_access_valid not present..
> >  
> 
> I'm going to change the next patch to fully inherit CXLType3Dev into
> Niagara as you suggested.  I will have to see what falls out when i do
> that, but my feeling was being more explicit when checking pointers is
> better.  If you prefer to to drop the is_mhd check i'm ok with that
> simply because mhd_access_valid should always be NULL when is_mhd is
> false.  Either way the result is the same.

Minimizing redundant checks is always good as long as a reviewer can
easily see they are redundant. I think that's true here.

Jonathan

> 
> ~Gregory