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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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.
> 
> 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?