Re: [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs
On Thu, Feb 29, 2024 at 1:57 AM Peter Maydell wrote: > > On Thu, 29 Feb 2024 at 04:52, Joe Komlodi wrote: > > On Wed, Feb 28, 2024 at 6:21 AM Peter Maydell > > 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-nabiheste...@google.com/20240221171716.1260192-3-nabiheste...@google.com/ > https://patchew.org/QEMU/20240221173325.1494895-1-nabiheste...@google.com/20240221173325.1494895-3-nabiheste...@google.com/ > -- we rejected those for the same reason. Makes sense! Thanks for taking the time to look over them. - Joe > > thanks > -- PMM
Re: [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs
On Thu, 29 Feb 2024 at 04:52, Joe Komlodi wrote: > On Wed, Feb 28, 2024 at 6:21 AM Peter Maydell > 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-nabiheste...@google.com/20240221171716.1260192-3-nabiheste...@google.com/ https://patchew.org/QEMU/20240221173325.1494895-1-nabiheste...@google.com/20240221173325.1494895-3-nabiheste...@google.com/ -- we rejected those for the same reason. thanks -- PMM
Re: [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs
On Wed, Feb 28, 2024 at 6:21 AM Peter Maydell wrote: > > On Tue, 27 Feb 2024 at 22:24, Joe Komlodi 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
Re: [RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs
On Tue, 27 Feb 2024 at 22:24, Joe Komlodi 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
[RFC PATCH 0/5] memattrs: target/arm: add user-defined and requester ID memattrs
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
