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

[Eric Auger <eric.auger@redhat.com>]

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.

Thanks

Eric
>
>  
>