[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5)
From: |
Ben Widawsky |
Subject: |
Re: [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5) |
Date: |
Thu, 28 Jan 2021 08:58:01 -0800 |
On 21-01-28 08:51:51, Ben Widawsky wrote:
> 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
>
Ignore this. It was a problem with my commandline.
I think I have something limping along now.
- [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5), (continued)
- [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5), Ben Widawsky, 2021/01/05
- Re: [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5), Ben Widawsky, 2021/01/27
- Re: [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5), Jonathan Cameron, 2021/01/28
- Re: [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5), Ben Widawsky, 2021/01/28
- Re: [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5), Ben Widawsky, 2021/01/28
- Re: [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5), Ben Widawsky, 2021/01/28
- Re: [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5),
Ben Widawsky <=
- Re: [RFC PATCH v2 24/32] hw/cxl/device: Add a memory device (8.2.8.5), Jonathan Cameron, 2021/01/28
[RFC PATCH v2 23/32] hw/cxl/rp: Add a root port, Ben Widawsky, 2021/01/05
[RFC PATCH v2 26/32] acpi/cxl: Add _OSC implementation (9.14.2), Ben Widawsky, 2021/01/05
[RFC PATCH v2 28/32] acpi/cxl: Create the CEDT (9.14.1), Ben Widawsky, 2021/01/05
[RFC PATCH v2 27/32] tests/acpi: allow CEDT table addition, Ben Widawsky, 2021/01/05
Re: [RFC PATCH v2 00/32] CXL 2.0 Support, Jonathan Cameron, 2021/01/08