Hi all,

This adds requester IDs to ARM CPUs and adds a "user-defined" memory
attribute.

The requester ID on ARM CPUs is there because I've seen some cases where
there's an IOMMU between a CPU and memory that uses the CPU's requester
ID to look up how it should translate, such as an SMMU TBU or some other
IOMMU-like device.
For a specific downstream example I've seen, Xilinx overrides CPU
attributes with ones passed in by an object property in order to have
their IOMMUs work:
https://github.com/Xilinx/qemu/blob/23b643ba1683a47ef49447a45643fe2172d6f8ca/accel/tcg/cputlb.c#L1127.
The object property with the memory attributes is declared here, for
reference: https://github.com/Xilinx/qemu/blob/23b643ba1683a47ef49447a45643fe2172d6f8ca/target/arm/cpu.c#L1310.

The user-defined attribute represents optional user signals that are a
part of AMBA-AXI. As the name suggests, these are defined
per-implementation and devices that receive these have their own
interpretation of what the user-defined attribute means.

We add them in CPUs and PCI transactions, because some of their
attributes are set in functions in ways that are not user-facing. DMAs
or other devices that set attributes (using address_space_rw or some
other means), can add them on a per-device basis.

RFC because it's possible we might want this implementated in some other
way, and it touches some pretty frequently used code that I'm somewhat
familiar with, but not 100% familiar with.

Thanks,
Joe

Joe Komlodi (5):
  target/arm: Add requester ID to memattrs
  memattrs: Fix target_tlb_bit whitespace
  memattrs: Add user-defined attribute
  target/arm: Add user-defined memattrs
  hw/pci: Add user-defined memattrs

 hw/pci/pci.c                | 3 +++
 include/exec/memattrs.h     | 8 +++++---
 include/hw/pci/pci_device.h | 1 +
 target/arm/cpu.c            | 6 ++++++
 target/arm/cpu.h            | 8 ++++++++
 target/arm/ptw.c            | 9 +++++++++
 6 files changed, 32 insertions(+), 3 deletions(-)

-- 
2.44.0.rc0.258.g7320e95886-goog

On Tue, 27 Feb 2024 at 22:24, Joe Komlodi <komlodi@google.com> wrote:
> This adds requester IDs to ARM CPUs and adds a "user-defined" memory
> attribute.
>
> The requester ID on ARM CPUs is there because I've seen some cases where
> there's an IOMMU between a CPU and memory that uses the CPU's requester
> ID to look up how it should translate, such as an SMMU TBU or some other
> IOMMU-like device.
> For a specific downstream example I've seen, Xilinx overrides CPU
> attributes with ones passed in by an object property in order to have
> their IOMMUs work:
> https://github.com/Xilinx/qemu/blob/23b643ba1683a47ef49447a45643fe2172d6f8ca/accel/tcg/cputlb.c#L1127.
> The object property with the memory attributes is declared here, for
> reference: https://github.com/Xilinx/qemu/blob/23b643ba1683a47ef49447a45643fe2172d6f8ca/target/arm/cpu.c#L1310.
>
> The user-defined attribute represents optional user signals that are a
> part of AMBA-AXI. As the name suggests, these are defined
> per-implementation and devices that receive these have their own
> interpretation of what the user-defined attribute means.
>
> We add them in CPUs and PCI transactions, because some of their
> attributes are set in functions in ways that are not user-facing. DMAs
> or other devices that set attributes (using address_space_rw or some
> other means), can add them on a per-device basis.

So as far as I can see, this patchset defines a bunch of mechanism,
but no actual users: no device looks at these new memattrs, no board
code sets the properties. I don't really want to add this without
an upstream usecase for it.

thanks
-- PMM

On Wed, Feb 28, 2024 at 6:21 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 27 Feb 2024 at 22:24, Joe Komlodi <komlodi@google.com> wrote:
> > This adds requester IDs to ARM CPUs and adds a "user-defined" memory
> > attribute.
> >
> > The requester ID on ARM CPUs is there because I've seen some cases where
> > there's an IOMMU between a CPU and memory that uses the CPU's requester
> > ID to look up how it should translate, such as an SMMU TBU or some other
> > IOMMU-like device.
> > For a specific downstream example I've seen, Xilinx overrides CPU
> > attributes with ones passed in by an object property in order to have
> > their IOMMUs work:
> > https://github.com/Xilinx/qemu/blob/23b643ba1683a47ef49447a45643fe2172d6f8ca/accel/tcg/cputlb.c#L1127.
> > The object property with the memory attributes is declared here, for
> > reference: https://github.com/Xilinx/qemu/blob/23b643ba1683a47ef49447a45643fe2172d6f8ca/target/arm/cpu.c#L1310.
> >
> > The user-defined attribute represents optional user signals that are a
> > part of AMBA-AXI. As the name suggests, these are defined
> > per-implementation and devices that receive these have their own
> > interpretation of what the user-defined attribute means.
> >
> > We add them in CPUs and PCI transactions, because some of their
> > attributes are set in functions in ways that are not user-facing. DMAs
> > or other devices that set attributes (using address_space_rw or some
> > other means), can add them on a per-device basis.
>
> So as far as I can see, this patchset defines a bunch of mechanism,
> but no actual users: no device looks at these new memattrs, no board
> code sets the properties. I don't really want to add this without
> an upstream usecase for it.

Yeah, I believe the current use-cases for this series are mostly downstream.
It's possible that there's an upstream device that might benefit from
it, but I'm not aware of one.

Is the concern the usefulness of the series, or the worry about it bit-rotting?
If it's the latter, would a qtest be alright to make sure it doesn't rot?

Thanks,
Joe
>
> thanks
> -- PMM

On Thu, 29 Feb 2024 at 04:52, Joe Komlodi <komlodi@google.com> wrote:
> On Wed, Feb 28, 2024 at 6:21 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > So as far as I can see, this patchset defines a bunch of mechanism,
> > but no actual users: no device looks at these new memattrs, no board
> > code sets the properties. I don't really want to add this without
> > an upstream usecase for it.
>
> Yeah, I believe the current use-cases for this series are mostly downstream.
> It's possible that there's an upstream device that might benefit from
> it, but I'm not aware of one.
>
> Is the concern the usefulness of the series, or the worry about it bit-rotting?
> If it's the latter, would a qtest be alright to make sure it doesn't rot?

My main issues are:
 * it's hard to review design without the uses of the code
 * it's extra complexity and maintenance burden that we don't
   need (upstream): accepting the patches gives upstream extra
   work to deal with into the future with no benefit to us
 * dead code is code that typically we would remove
 * we end up with something we can't refactor or clean up
   or change because the only reason we have it is for code
   that we don't have any visibility into: effectively it
   becomes an API for us that we can't change, which is not
   something QEMU does except for specific well defined API
   surfaces (QMP, plugins, etc)

Our usual approach is "submit the patches that add the new core
feature/mechanism along with the patches that add the new
device/board/etc that uses it". Compare the recent patches
also from Google for the ITS and SMMU that try to add hooks
that aren't needed by anything in upstream QEMU:
https://patchew.org/QEMU/20240221171716.1260192-1-nabihestefan@google.com/20240221171716.1260192-3-nabihestefan@google.com/
https://patchew.org/QEMU/20240221173325.1494895-1-nabihestefan@google.com/20240221173325.1494895-3-nabihestefan@google.com/
-- we rejected those for the same reason.

thanks
-- PMM

On Thu, Feb 29, 2024 at 1:57 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 29 Feb 2024 at 04:52, Joe Komlodi <komlodi@google.com> wrote:
> > On Wed, Feb 28, 2024 at 6:21 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > > So as far as I can see, this patchset defines a bunch of mechanism,
> > > but no actual users: no device looks at these new memattrs, no board
> > > code sets the properties. I don't really want to add this without
> > > an upstream usecase for it.
> >
> > Yeah, I believe the current use-cases for this series are mostly downstream.
> > It's possible that there's an upstream device that might benefit from
> > it, but I'm not aware of one.
> >
> > Is the concern the usefulness of the series, or the worry about it bit-rotting?
> > If it's the latter, would a qtest be alright to make sure it doesn't rot?
>
> My main issues are:
>  * it's hard to review design without the uses of the code
>  * it's extra complexity and maintenance burden that we don't
>    need (upstream): accepting the patches gives upstream extra
>    work to deal with into the future with no benefit to us
>  * dead code is code that typically we would remove
>  * we end up with something we can't refactor or clean up
>    or change because the only reason we have it is for code
>    that we don't have any visibility into: effectively it
>    becomes an API for us that we can't change, which is not
>    something QEMU does except for specific well defined API
>    surfaces (QMP, plugins, etc)
>
> Our usual approach is "submit the patches that add the new core
> feature/mechanism along with the patches that add the new
> device/board/etc that uses it". Compare the recent patches
> also from Google for the ITS and SMMU that try to add hooks
> that aren't needed by anything in upstream QEMU:
> https://patchew.org/QEMU/20240221171716.1260192-1-nabihestefan@google.com/20240221171716.1260192-3-nabihestefan@google.com/
> https://patchew.org/QEMU/20240221173325.1494895-1-nabihestefan@google.com/20240221173325.1494895-3-nabihestefan@google.com/
> -- we rejected those for the same reason.

Makes sense!
Thanks for taking the time to look over them.

- Joe
>
> thanks
> -- PMM