Re: [PATCH v7 00/46] CXl 2.0 emulation Support

[Jonathan Cameron]

On Fri, 18 Mar 2022 08:14:58 +0000
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> On 17/03/2022 16:47, Jonathan Cameron via wrote:
> 
> >> Ah great! As you've already noticed my particular case was performing 
> >> partial
> >> decoding on a memory region, but there are no issues if you need to 
> >> dispatch to
> >> another existing address space such as PCI/IOMMU. Creating a separate 
> >> address space
> >> per device shouldn't be an issue either, as that's effectively how the PCI 
> >> bus master
> >> requests are handled.
> >>
> >> The address spaces are visible in "info mtree" so if you haven't already, 
> >> I would
> >> recommend generating a dynamic name for the address space based upon the 
> >> device
> >> name/address to make it easier for development and debugging.  
> > info mtree already provides the following with a static name
> > address-space: cxl-type3-dpa-space
> >    0000000000000000-000000000fffffff (prio 0, nv-ram): cxl-mem2
> > 
> > So the device association is there anyway.  Hence I'm not sure a dynamic 
> > name adds
> > a lot on this occasion and code is simpler without making it dynamic.  
> 
> Is this using a single address space for multiple memory devices, or one per 
> device 
> as you were suggesting in the thread? If it is one per device and cxl-mem2 is 
> the 
> value of the -device id parameter, I still think it is worth adding the same 
> device 
> id into the address space name for the sake of a g_strdup_printf() and 
> corresponding 
> g_free().

One per device.  Ultimately when I add volatile memory support we'll end up 
with possibly
having to add an mr as a container for the two hostmem mr.   Looking again, the 
name
above is actually the id of the mr, not the type3 device. Probably better to 
optionally
use the type3 device name if available.

I'll make the name something like cxl-type3-dpa-space-cxl-pmem3 if id available
and fall back to cxl-type3-dpa-space as before if not.

> 
> Alas I don't currently have the time (and enough knowledge of CXL!) to do a 
> more 
> comprehensive review of the patches, but a quick skim of the series suggests 
> it seems 
> quite mature. The only thing that I noticed was that there doesn't seem to be 
> any 
> trace-events added, which I think may be useful to aid driver developers if 
> they need 
> to debug some of the memory access routing.

Good suggestion.  I'm inclined to add them in a follow up patch though because
this patch set is already somewhat unmanageable from point of view of review.
I already have a number of other patches queued up for a second series adding
more functionality.

> 
> Finally I should point out that there are a number of more experienced PCI 
> developers 
> on the CC list than me, and they should have the final say on patch review. 
> So please 
> consider these comments as recommendations based upon my development work on 
> QEMU, 
> and not as a NAK for proceeding with the series :)

No problem and thanks for your help as (I think) you've solved the biggest open 
issue :)

Jonathan

> 
> 
> ATB,
> 
> Mark.