Re: [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5)

[Ben Widawsky]

On 21-01-28 07:14:44, Ben Widawsky wrote:
> On 21-01-28 07:03:18, Ben Widawsky wrote:
> > On 21-01-28 10:25:38, Jonathan Cameron wrote:
> > > On Wed, 27 Jan 2021 13:26:45 -0800
> > > Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > 
> > > > On 21-01-27 22:03:12, Igor Mammedov wrote:
> > > > > On Tue,  5 Jan 2021 08:53:15 -0800
> > > > > Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > > >   
> > > > > > A CXL memory device (AKA Type 3) is a CXL component that contains 
> > > > > > some
> > > > > > combination of volatile and persistent memory. It also implements 
> > > > > > the
> > > > > > previously defined mailbox interface as well as the memory device
> > > > > > firmware interface.
> > > > > > 
> > > > > > The following example will create a 256M device in a 512M window:
> > > > > > 
> > > > > > -object 
> > > > > > "memory-backend-file,id=cxl-mem1,share,mem-path=cxl-type3,size=512M"
> > > > > > -device "cxl-type3,bus=rp0,memdev=cxl-mem1,id=cxl-pmem0,size=256M"  
> > > > > 
> > > > > I'd expect whole backend used by frontend, so one would not need 
> > > > > "size" property
> > > > > on frontend (like we do with memory devices).
> > > > > So question is why it partially uses memdev?  
> > > > 
> > > > Answered in a separate thread...
> > > 
> > > One possible suggestion inline.
> > > 
> > > > > > +
> > > > > > +static void cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
> > > > > > +{
> > > > > > +    MemoryRegionSection mrs;
> > > > > > +    MemoryRegion *mr;
> > > > > > +    uint64_t offset = 0;
> > > > > > +    size_t remaining_size;
> > > > > > +
> > > > > > +    if (!ct3d->hostmem) {
> > > > > > +        error_setg(errp, "memdev property must be set");
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    /* FIXME: need to check mr is the host bridge's MR */
> > > > > > +    mr = host_memory_backend_get_memory(ct3d->hostmem);
> > > > > > +
> > > > > > +    /* Create our new subregion */
> > > > > > +    ct3d->cxl_dstate.pmem = g_new(MemoryRegion, 1);
> > > > > > +
> > > > > > +    /* Find the first free space in the window */
> > > > > > +    WITH_RCU_READ_LOCK_GUARD()
> > > > > > +    {
> > > > > > +        mrs = memory_region_find(mr, offset, 1);
> > > > > > +        while (mrs.mr && mrs.mr != mr) {
> > > > > > +            offset += memory_region_size(mrs.mr);
> > > > > > +            mrs = memory_region_find(mr, offset, 1);
> > > > > > +        }
> > > > > > +    }
> > > > > > +
> > > > > > +    remaining_size = memory_region_size(mr) - offset;
> > > > > > +    if (remaining_size < ct3d->size) {
> > > > > > +        g_free(ct3d->cxl_dstate.pmem);
> > > > > > +        error_setg(errp,
> > > > > > +                   "Not enough free space (%zd) required for 
> > > > > > device (%" PRId64  ")",
> > > > > > +                   remaining_size, ct3d->size);
> > > > > > +    }
> > > > > > +
> > > > > > +    /* Register our subregion as non-volatile */
> > > > > > +    memory_region_init_ram(ct3d->cxl_dstate.pmem, OBJECT(ct3d),
> > > > > > +                           "cxl_type3-memory", ct3d->size, errp);  
> > > > > this allocates ct3d->size of anon RAM, was this an intention?
> > > > > If yes, can you clarify why extra RAM is used instead of using what
> > > > > backend provides?  
> > > > 
> > > > It sounds like I'm doing the wrong thing then. There should be one 
> > > > chunk of
> > > > memory which is a subset of the full memory backend object. Could you 
> > > > please
> > > > advise on what I should be doing instead? Is add_subregion() sufficient?
> > > 
> > > Taking inspiration from nvdimm I'm carrying a patch that uses
> > > memory_region_init_alias(ct3d->cxl_dstate.pmem, OBJECT(qct3d)q,
> > >                    "cxl_type3-memory", mr, offset, ct3d->size);
> > > 
> > > I 'think' that's doing the right thing, but haven't fully tested it yet
> > > so may be completely wrong :)
> > > 
> > > Then for the pmem addr, call memory_region_set_address() to put it
> > > in a particular location.
> > > 
> > 
> > Yes - this is what I'd like to do and what I initially tried, and I also 
> > believe
> > it's right, but it doesn't work.
> > 
> > range_invariant: Assertion `range->lob <= range->upb || range->lob == 
> > range->upb + 1' failed.
> > 
> > I was digging into this yesterday, but opted to start a new thread on the
> > matter.
> > 
> 
> Hmm. I think I need to figure out the right add_subregion after this and it
> might work. I'll keep digging, but if you have ideas, let me know.

[snip]

I managed to get a bit further. With the following, I start getting complaints
about fragmented memory when adding devices later.

     memory_region_init_alias(ct3d->cxl_dstate.pmem, OBJECT(ct3d),
             "cxl_type3-memory", mr, mr->addr + offset, ct3d->size);
     memory_region_set_nonvolatile(ct3d->cxl_dstate.pmem, true);
     memory_region_add_subregion(mr, offset, ct3d->cxl_dstate.pmem);

-device nvdimm,memdev=nvmem1,id=nv1,label-size=2M,node=5: could not find 
position in guest address space for memory device - memory fragmented due to 
alignments