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 4:41 PM, Jonathan Cameron wrote:
> On Tue, 1 Jul 2025 15:26:26 +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.)  
>> Doesn't it depend on the init RAM size setting.
>> if the init RAM top + REDIST2 aligns to a 256MB boundary (size of the
>> PCI ECAM) aren't you likely to have no hole?
> Hi Eric,
>
> Is that possible?  I think the device_memory_base being force to align
> to a 1 GiB means that never happens.  That seems to occur even
> if there is no device_memory.  
>
>     device_memory_base =
>         ROUND_UP(vms->memmap[VIRT_MEM].base + ms->ram_size, GiB);
>     device_memory_size = ms->maxram_size - ms->ram_size + ms->ram_slots * GiB;
>
>     /* Base address of the high IO region */
>     memtop = base = device_memory_base + ROUND_UP(device_memory_size, GiB);
> //So here we are GiB aligned.
Yes you are totally right, even without device memory, base is 1GiB
aligned when entering virt_set_high_memmap. I forgot this alignment
enforcement.

So we are good. Sorry for the noise

> ...
>
>     if (base < vms->memmap[VIRT_MEM].base + LEGACY_RAMLIMIT_BYTES) {
>         base = vms->memmap[VIRT_MEM].base + LEGACY_RAMLIMIT_BYTES;
>     }
>
> //That's 256 GiB in or leave it alone as more than that but GiB aligned.
>
>    /* We know for sure that at least the memory fits in the PA space */
>     vms->highest_gpa = memtop - 1;
>
>     virt_set_high_memmap(vms, base, pa_bits);
>
>
> So I think I'm fine. I should call out that REDIST2 is GiB
> aligned though in this patch description.
>>
>>> 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.  
>> what kind of improvement would you foresee wrt highest_gpa?
> This was mostly a response to Peter expressed that he was expecting
> highest_gpa to reflect the limit, not the highest yet seen.
>
> I'm not sure how to resolve that without having awkward naming
> like highest_gpa_sofar. There are existing comments where it is updated
> so I'm not thinking we need to change anything for this.

OK I understand now.

Eric
>
>
> Thanks for taking a look,
>
> Jonathan
>