Re: [PATCH 5/6] hw/acpi/viot: sort VIOT ACPI table entries by PCI host bus min_bus

[Mark Cave-Ayland]

On 19/05/2022 08:50, Ani Sinha wrote:

> On Wed, May 18, 2022 at 4:39 PM Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
> > This ensures that the VIOT ACPI table output is always stable for a given PCI
> > topology by ensuring that entries are ordered according to min_bus.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> other than the nit below,
> Reviewed-by: Ani Sinha <ani@anisinha.ca>
>
> > ---
> >   hw/acpi/viot.c | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> >
> > diff --git a/hw/acpi/viot.c b/hw/acpi/viot.c
> > index ce3b7b8c75..f5714b95bd 100644
> > --- a/hw/acpi/viot.c
> > +++ b/hw/acpi/viot.c
> > @@ -64,6 +64,20 @@ static int pci_host_bridges(Object *obj, void *opaque)
> >       return 0;
> >   }
> >
> > +static int pci_host_range_compare(gconstpointer a, gconstpointer b)
>
> nit: shouldn't this have a gint return type since we use gconstpointer
> as arguments anyway?
> https://docs.gtk.org/glib/callback.CompareFunc.html

I guess so - int/gint seem to be fairly interchangeable, and there are examples of 
both in the QEMU codebase. The only reason it appears in the patch as int is because 
that was how it was in the reference code I used from iort_idmap_compare().

I'll change this to gint for v2.

> > +{
> > +    struct viot_pci_host_range *range_a = (struct viot_pci_host_range *)a;
> > +    struct viot_pci_host_range *range_b = (struct viot_pci_host_range *)b;
> > +
> > +    if (range_a->min_bus < range_b->min_bus) {
> > +        return -1;
> > +    } else if (range_a->min_bus > range_b->min_bus) {
> > +        return 1;
> > +    } else {
> > +        return 0;
> > +    }
> > +}
> > +
> >   /*
> >    * Generate a VIOT table with one PCI-based virtio-iommu that manages PCI
> >    * endpoints.
> > @@ -87,6 +101,9 @@ void build_viot(MachineState *ms, GArray *table_data, 
> > BIOSLinker *linker,
> >       object_child_foreach_recursive(OBJECT(ms), pci_host_bridges,
> >                                      pci_host_ranges);
> >
> > +    /* Sort the pci host ranges by min_bus */
> > +    g_array_sort(pci_host_ranges, pci_host_range_compare);
> > +
> >       /* ACPI table header */
> >       acpi_table_begin(&table, table_data);
> >       /* Node count */
> > --
> > 2.20.1


ATB,

Mark.