On Fri, 16 May 2025 05:44:34 +0000
"Zhijian Li (Fujitsu)" <[email protected]> wrote:
> On 13/05/2025 19:14, Jonathan Cameron via wrote:
> > Previously these somewhat device like structures were tracked using a list
> > in the CXLState in each machine. This is proving restrictive in a few
> > cases where we need to iterate through these without being aware of the
> > machine type. Just make them sysbus devices.
> Make sense.
>
> Minor comments inline
> > diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
> > index 9cd7905ea2..20806e5dd4 100644
> > --- a/hw/acpi/cxl.c
> > +++ b/hw/acpi/cxl.c
> > @@ -22,6 +22,7 @@
> > #include "hw/pci/pci_bridge.h"
> > #include "hw/pci/pci_host.h"
> > #include "hw/cxl/cxl.h"
> > +#include "hw/cxl/cxl_host.h"
> > #include "hw/mem/memory-device.h"
> > #include "hw/acpi/acpi.h"
> > #include "hw/acpi/aml-build.h"
> > @@ -135,56 +136,62 @@ static void cedt_build_chbs(GArray *table_data,
> > PXBCXLDev *cxl)
> > * Interleave ways encoding in CXL 2.0 ECN: 3, 6, 12 and 16-way memory
> > * interleaving.
> > */
> > -static void cedt_build_cfmws(GArray *table_data, CXLState *cxls)
> > +static int cedt_build_cfmws(Object *obj, void *opaque)
>
> Too much unrelated indent updates in this function
Fan addressed this. It's due to the loop now being external to this
call.
>
>
> > {
> > - GList *it;
> > + struct CXLFixedWindow *fw;
> > + Aml *cedt = opaque;
> > + GArray *table_data = cedt->buf;
> > + int i;
> >
> > - for (it = cxls->fixed_windows; it; it = it->next) {
> > - CXLFixedWindow *fw = it->data;
> > - int i;
> > + if (!object_dynamic_cast(obj, TYPE_CXL_FMW)) {
> > + return 0;
>
> Never reach here? It seems the caller has ensured passing TYPE_CXL_FMW obj.
Excellent point. Further than that, now the iterator is gone
I can just pass in correctly typed pointers from the caller
which is a nice to have as well!
>
>
>
>
> > + }
> > + fw = CXL_FMW(obj);
> >
> > - /* Type */
> > - build_append_int_noprefix(table_data, 1, 1);
> > + /* Type */
> > + build_append_int_noprefix(table_data, 1, 1);
> >
> > - /* Reserved */
> > - build_append_int_noprefix(table_data, 0, 1);
> > + /* Reserved */
> > + build_append_int_noprefix(table_data, 0, 1);
> >
> > - /* Record Length */
> > - build_append_int_noprefix(table_data, 36 + 4 * fw->num_targets, 2);
> > + /* Record Length */
> > + build_append_int_noprefix(table_data, 36 + 4 * fw->num_targets, 2);
> >
> > - /* Reserved */
> > - build_append_int_noprefix(table_data, 0, 4);
> > + /* Reserved */
> > + build_append_int_noprefix(table_data, 0, 4);
> >
> > - /* Base HPA */
> > - build_append_int_noprefix(table_data, fw->mr.addr, 8);
> > + /* Base HPA */
> > + build_append_int_noprefix(table_data, fw->mr.addr, 8);
> >
> > - /* Window Size */
> > - build_append_int_noprefix(table_data, fw->size, 8);
> > + /* Window Size */
> > + build_append_int_noprefix(table_data, fw->size, 8);
> >
> > - /* Host Bridge Interleave Ways */
> > - build_append_int_noprefix(table_data, fw->enc_int_ways, 1);
> > + /* Host Bridge Interleave Ways */
> > + build_append_int_noprefix(table_data, fw->enc_int_ways, 1);
> >
> > - /* Host Bridge Interleave Arithmetic */
> > - build_append_int_noprefix(table_data, 0, 1);
> > + /* Host Bridge Interleave Arithmetic */
> > + build_append_int_noprefix(table_data, 0, 1);
> >
> > - /* Reserved */
> > - build_append_int_noprefix(table_data, 0, 2);
> > + /* Reserved */
> > + build_append_int_noprefix(table_data, 0, 2);
> >
> > - /* Host Bridge Interleave Granularity */
> > - build_append_int_noprefix(table_data, fw->enc_int_gran, 4);
> > + /* Host Bridge Interleave Granularity */
> > + build_append_int_noprefix(table_data, fw->enc_int_gran, 4);
> >
> > - /* Window Restrictions */
> > - build_append_int_noprefix(table_data, 0x0f, 2); /* No restrictions
> > */
> > + /* Window Restrictions */
> > + build_append_int_noprefix(table_data, 0x0f, 2);
> >
> > - /* QTG ID */
> > - build_append_int_noprefix(table_data, 0, 2);
> > + /* QTG ID */
> > + build_append_int_noprefix(table_data, 0, 2);
> >
> > - /* Host Bridge List (list of UIDs - currently bus_nr) */
> > - for (i = 0; i < fw->num_targets; i++) {
> > - g_assert(fw->target_hbs[i]);
> > - build_append_int_noprefix(table_data,
> > PXB_DEV(fw->target_hbs[i])->bus_nr, 4);
> > - }
> > + /* Host Bridge List (list of UIDs - currently bus_nr) */
> > + for (i = 0; i < fw->num_targets; i++) {
> > + g_assert(fw->target_hbs[i]);
> > + build_append_int_noprefix(table_data,
> > + PXB_DEV(fw->target_hbs[i])->bus_nr, 4);
> > }
> > +
> > + return 0;
> > }
> > index cae4afcdde..13eb6bf6a4 100644
> > --- a/hw/cxl/cxl-host-stubs.c
> > +++ b/hw/cxl/cxl-host-stubs.c
> > @@ -8,8 +8,12 @@
> > +
> > +struct cfmw_update_state {
> > + hwaddr base;
> > + hwaddr maxaddr;
> > +};
> > +
> > +static void cxl_fmws_update(Object *obj, void *opaque)
> > +{
> > + struct CXLFixedWindow *fw;
> > + struct cfmw_update_state *s = opaque;
> > +
> > + if (!object_dynamic_cast(obj, TYPE_CXL_FMW)) {
>
>
> Never reach here? It seems the caller has ensured passing TYPE_CXL_FMW obj.
Also a good point. Here as well I can simply pass the
correct type of pointer in for this and hwaddr *base, hwaddr max_addr
removing the need for cfmw_update_state()
That is all stale infrastructure from before I changed this handling
to force the device order.
Thanks,
Jonathan
