On Fri, 14 Jun 2024 15:08:35 +0100
Jonathan Cameron <jonathan.came...@huawei.com> wrote:
> On Fri, 14 Jun 2024 12:57:25 +0200
> Igor Mammedov <imamm...@redhat.com> wrote:
>
> > On Thu, 6 Jun 2024 18:47:16 +0100
> > Jonathan Cameron <jonathan.came...@huawei.com> wrote:
> >
> > > On Thu, 6 Jun 2024 16:06:53 +0200
> > > Igor Mammedov <imamm...@redhat.com> wrote:
> > >
> > > > On Wed, 5 Jun 2024 19:04:55 +0100
> > > > Jonathan Cameron <jonathan.came...@huawei.com> wrote:
> > > >
> > > > > Treating the HID as an integer caused it to get bit reversed
> > > > > on big endian hosts running little endian guests. Treat it
> > > > > as a character array instead.
> > > > >
> > > > > Fixes hw/acpi: Generic Port Affinity Structure Support
> > > > > Tested-by: Richard Henderson <richard.hender...@linaro.org>
> > > > > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
> > > > >
> > > > > ---
> > > > > Richard ran the version posted in the thread on an s390 instance.
> > > > > Thanks for the help!
> > > > >
> > > > > Difference from version in thread:
> > > > > - Instantiate i in the for loop.
> > > > >
> > > > > Sending out now so Michael can decide whether to fold this in, or
> > > > > drop the GP series for now from his pull request (in which case
> > > > > I'll do an updated version with this and Markus' docs feedback
> > > > > folded in.)
> > > > >
> > > > > ---
> > > > > include/hw/acpi/acpi_generic_initiator.h | 2 +-
> > > > > hw/acpi/acpi_generic_initiator.c | 4 +++-
> > > > > 2 files changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/include/hw/acpi/acpi_generic_initiator.h
> > > > > b/include/hw/acpi/acpi_generic_initiator.h
> > > > > index 1a899af30f..5baefda33a 100644
> > > > > --- a/include/hw/acpi/acpi_generic_initiator.h
> > > > > +++ b/include/hw/acpi/acpi_generic_initiator.h
> > > > > @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle {
> > > > > uint16_t bdf;
> > > > > };
> > > > > struct {
> > > > > - uint64_t hid;
> > > > > + char hid[8];
> > > > > uint32_t uid;
> > > > > };
> > > > > };
> > > >
> > > > not sure on top of what this patch applies but I have some generic
> > > > comments wrt it
> > >
> > > https://lore.kernel.org/qemu-devel/20240524100507.32106-1-jonathan.came...@huawei.com/
> > >
> > > Comments are all on elements of the existing upstream code, but I'm
> > > touching it
> > > anyway so will look at making the improvements you suggest as new
> > > precursors
> > > to v3 given we are going around again anyway.
> > >
> > > >
> > > > why PCIDeviceHandle is in header file? is there plan for it
> > > > being used outside of acpi_generic_initiator.c?
> > >
> > > I'll add a precursor patch to my series that moves
> > > it and anything else that should be more local. May well move
> > > to being local in aml_build.c given your later comments with the
> > > various fields passed in as parameters.
> > >
> > > >
> > > >
> > > > > diff --git a/hw/acpi/acpi_generic_initiator.c
> > > > > b/hw/acpi/acpi_generic_initiator.c
> > > > > index 78b80dcf08..f064753b67 100644
> > > > > --- a/hw/acpi/acpi_generic_initiator.c
> > > > > +++ b/hw/acpi/acpi_generic_initiator.c
> > > > > @@ -151,7 +151,9 @@ build_srat_generic_node_affinity(GArray
> > > > > *table_data, int node,
> > > > > build_append_int_noprefix(table_data, 0, 12);
> > > > > } else {
> > > > > /* Device Handle - ACPI */
> > > > > - build_append_int_noprefix(table_data, handle->hid, 8);
> > > > > + for (int i = 0; i < sizeof(handle->hid); i++) {
> > > > > + build_append_int_noprefix(table_data, handle->hid[i], 1);
> > > > > + }
> > > > > build_append_int_noprefix(table_data, handle->uid, 4);
> > > > > build_append_int_noprefix(table_data, 0, 4);
> > > >
> > > > instead of open codding structure
> > > >
> > > > it might be better to introduce helper in aml_build.c
> > > > something like
> > > > /* proper reference to spec as we do for other ACPI primitives */
> > > > build_append_srat_acpi_device_handle(GArray *table_data, char* hid,
> > > > unit32_t uid)
> > > > assert(strlen(hid) ...
> > > > for() {
> > > > build_append_byte()
> > > > }
> > > > ...
> > > >
> > > > the same applies to "Device Handle - PCI" structure
> > >
> > > I'll look at moving that stuff and the affinity structure creation
> > > code themselves in there. I think they ended up in this file because
> > > of the other infrastructure needed to create these nodes and it
> > > will have felt natural to keep this together.
> > >
> > > Putting it in aml_build.c will put it with similar code though
> > > which makes sense to me.
> >
> > the point of moving handle packing to aml-build.c,
> > is to isolate primitives that likely could be reused later on elsewhere
> > and hide little endiannes from API user.
> > So shuch errors as you are fixing wouldn't be easy to introduce
> > (as long as API does it right)
> >
> >
> > Also this API probably should take not packed BDF, i.e. something like this:
> > build_append_srat_pci_device_handle(GArray *table_data, bus, dev, func)
> >
> > Or a packed BDF as you suggest in the later email, but then API function
> > wold have
> > to 'decode' that before putting numbers into table, which complicates things
> > and likely would pull in PCI deps to unpack BDF, which I'd rather avoid in
> > generic aml-build.c
>
> Ok. I can split it up. My motivation for the encoded version was that
> the spec field is defined as a 2 byte field, but it is also broken out
> in the description into byte 2 then various bits of byte 3 so we can construct
> it that way instead. (were it defined as bits in the 16 bit field this would
> make less sense). Should still be obvious enough to anyone trying to
> correlate the two.
>
> Splitting the devfn bit up though is tricky as that doesn't really
> have a separate meaning in PCI any more given ARI where it becomes an 8 bit
> function ID. So probably makes more sense to keep that as devfn as it's
> coming from pci->devfn in that form anyway and they both forms get encoded
> into a byte anyway.
ack,
as long as acpi API user doesn't have to worry about byte order it should be
fine
>
> Jonathan
>
>
> >
> > >
> > > >
> > > > Also get rid of PCI deps in acpi_generic_initiator.c
> > > > move build_all_acpi_generic_initiators/build_srat_generic_pci_initiator
> > > > into
> > > > hw/acpi/pci.c
> > >
> > > Today it's used only for PCI devices, but that's partly an artifact
> > > of how we get to the root complex via the bus below it.
> > >
> > > Spec wise, it's just as applicable to platform devices etc, but maybe
> > > we can move it to pci.c for now and move it out again if it gains other
> > > users. Or leave it in acpi_generic_initiator.c but have all the aml
> > > stuff in aml_build.c as you suggest.
> > >
> > > > file if it has to access PCI code/structures directly
> > > > (which I'm not convinced it should, can we get/expose what it needs as
> > > > QOM properties?)
> > >
> > > Maybe. I'll see what I can come up with. This feels involved
> > > however so I'm more doubtful about this as a precursor.
> > >
> > > >
> > > > btw:
> > > > build_all_acpi_generic_initiators() name doesn't match what it's doing.
> > > > it composes only one initiator entry.
> > >
> > > I'll look at tidying up all the relevant naming.
> > >
> > > Jonathan
> > >
> > > >
> > > > > }
> > > >
> > > >
> > >
> >
> >
>
