Re: [PATCH v2 3/4] hw/riscv/virt, gpex: Provide 32-bit MMIO window for CXL host bridges

Hi Junjie,

Thanks for the thorough review.

> cfg->cxl_mmio32 is a single range on the main GPEX host, but this block
> runs once per CXL host bridge in the QLIST_FOREACH(bus, &bus->child)
> loop, so with more than one pxb-cxl every ACPI0016 gets a _CRS that
> declares the *same* [cxl_base, cxl_base + 256 MiB) window.

You're right. The static path synthesises the bridge window from the
machine-global cfg->cxl_mmio32 rather than per-bridge state, so it
does not scale past one pxb-cxl.

> is supporting more than one pxb-cxl in scope for this series, or is a
> single CXL host bridge the intended target for now? If the latter, a
> note to that effect in the commit message would help.

For this series the intended target is a single CXL host bridge,
which matches the bios-tables test case (one pxb-cxl at bus_nr=12).
I'll add a note to the commit message in v3.

> One smaller thing about the hand-built template: the bus range is
> hard-coded to [bus_num, bus_num + 15], so with cxl.1 at bus_nr=12 and
> cxl.2 at bus_nr=16 the two _CRS come out as [0x0C,0x1B] and [0x10,0x1F],
> which overlap on buses 0x10-0x1B.

This is a related symptom of the same root cause: pxb-cxl only carries
a bus_nr and the actual subordinate bus count is not known at
ACPI-build time, so neither build_crs() nor a static constant can
produce a correct span for the multi-bridge case.  The +15 heuristic
is borrowed from the ARM virt machine's gpex handling where a single
pxb is the norm.

> Both really trace back to the same spot: the static branch synthesises
> the bridge's window and bus range from machine-global constants
> (cfg->cxl_mmio32 and the fixed +15) rather than anything per-bridge, so
> neither scales past one pxb-cxl.

Agreed.  I'll add a TODO comment at the static branch in gpex-acpi.c
to flag this for future work, and document the single-bridge limitation
in the commit message as noted above.

Thanks,
Pei