Re: [PATCH qemu v16 3/5] hw/arm/virt: Basic CXL enablement on pci_expander_bridge instances pxb-cxl

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Tue, 1 Jul 2025 18:12:39 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> Hi Jonathan,
> On 7/1/25 5:52 PM, Jonathan Cameron wrote:
> > On Tue, 1 Jul 2025 17:34:36 +0200
> > Eric Auger <eric.auger@redhat.com> wrote:
> >  
> >> Hi Jonathan,
> >>
> >> On 6/25/25 6:19 PM, Jonathan Cameron via wrote:  
> >>> Code based on i386/pc enablement.
> >>> The memory layout places space for 16 host bridge register regions after
> >>> the GIC_REDIST2 in the extended memmap. This is a hole in the current
> >>> map so adding them here has no impact on placement of other memory regions
> >>> (tested with enough CPUs for GIC_REDIST2 to be in use.)
> >>>
> >>> The CFMWs are placed above the extended memmap.  Note the confusing
> >>> existing variable highest_gpa is the highest_gpa that has been allocated
> >>> at a particular point in setting up the memory map.
> >>>
> >>> The cxl_devices_state.host_mr is provides a small space in which to place    
> >> s/is//  
> > Fixed. Thanks.  
> >>> the individual host bridge register regions for whatever host bridges are
> >>> allocated via -device pxb-cxl on the command line. The existing dynamic
> >>> sysbus infrastructure is not reused because pxb-cxl is a PCI device not
> >>> a sysbus one but these registers are directly in the main memory map,
> >>> not the PCI address space.
> >>>
> >>> Only create the CEDT table if cxl=on set for the machine. Default to off.
> >>>
> >>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>> ---
> >>> @@ -1895,6 +1917,9 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
> >>>      if (device_memory_size > 0) {
> >>>          machine_memory_devices_init(ms, device_memory_base, device_memory_size);
> >>>      }
> >>> +    vms->highest_gpa = cxl_fmws_set_memmap(ROUND_UP(vms->highest_gpa + 1,
> >>> +                                                    256 * MiB),
> >>> +                                           BIT_ULL(pa_bits)) - 1;    
> >> in hw/cxl/cxl-host.c, there seems to be a loop on fw windows? I guess
> >> those windows only exist if cxl option is set. In the positive,
> >> highest_gpa will be changed only if the option is set, which is fine.
> >> Indeed we have requested_ipa_size = 64 - clz64(vms->highest_gpa). So we
> >> shall not modify this if cxl is not set.  
> so do you confirm highest_gpa is unchanged in case cxl/fmw option is not
> set ?
> >>
> >> What I am a bit concerned with is that it"consumes" some high memory
> >> without making it explicit in extended_memmap. Shouldn't we book some
> >> dedicated space there? Sorry I am jumping very late in the review, maybe
> >> turning things worse & noisy :-( Eric  
> > No problem with late review - whilst it looks late we had a several year
> > gap at one point in updating this series!
> >
> > How much to book?  It's effectively infinite much like device memory.
> > Would be odd to book the minimum which is 256MiB given any useful system
> > is going to have a lot more than that (they are usually a few TiB but
> > may be much larger than that).
> >
> > Would a comment after 
> >    [VIRT_HIGH_PCIE_MMIO] =     { 0x0, DEFAULT_HIGH_PCIE_MMIO_SIZE }, 
> > such as
> >    /* Any CXL Fixed memory windows come here */
> > be enough?  
> yes at least it deserves a comment I think. Then it must be understood
> that it may prevent new regions from being added in the high mem range.
> I am definitively not the most knowledgeable guy to decide whether it is
> critical. I have not checked CCA impact on the layout for instance.

Current CXL isn't migratable but when we fix that I guess this will
mean that we need to version any new features that need space
in the high memory map to only be enabled on newer machines if
CXL is also enabled.  For now migration isn't relevant to CXL/qemu
usecases but it soon will be.

Jonathan

> 
> Thanks
> 
> Eric
> >
> >  
> >  
> 
>