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

[Mark Cave-Ayland]

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

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.

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


ATB,

Mark.