On Mon, 17 Jun 2024 13:05:15 +0100
Jonathan Cameron <jonathan.came...@huawei.com> wrote:
> 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.
>
> I guess it was inevitable (and perhaps you had this thought). QEMU ordering
> of BDF has the bytes swapped wrt to the ACPI ordering. With them split the
> test was failing. I'll also tweak the test to ensure we have a non 0 bus
> number (by inserting a root port rather than having the generic initiator
> on the root bridge bus 0).
that's why I asked for split BDF (into parts) for acpi API to provide endian
agonstic interface. Internally acpi functions can convert/pack arguments as
necessary. And it would be easier to validate/review by just comparing
with spec description.
>
> Ankit. Can you confirm if you were seeing these reversed?
>
> https://elixir.bootlin.com/qemu/v9.0.1/source/hw/acpi/acpi_generic_initiator.c#L134
> uses PCI_BUIILD_BDF() which has bus << 8
> The ACPI write is little endian, so bus ends up in byte 3 but should be in
> byte 2.
>
> Jonathan
>
> >
> > 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
> > > >
> > > > >
> > > > > > }
> > > > >
> > > > >
> > > >
> > >
> > >
> >
> >
>
