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

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

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.

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

¯\_(ツ)_/¯ 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.

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

~Gregory