Message-Id: <D1NPLD5J2YE2.3H93K6QYATI49@gmail.com>
Date: Fri, 31 May 2024 11:06:29 +0200
From: "Thierry Reding" <thierry.reding@...il.com>
To: "Krzysztof Kozlowski" <krzk@...nel.org>, "Sameer Pujar"
 <spujar@...dia.com>, <vkoul@...nel.org>, <robh@...nel.org>,
 <krzk+dt@...nel.org>, <conor+dt@...nel.org>, <jonathanh@...dia.com>,
 <dmaengine@...r.kernel.org>, <devicetree@...r.kernel.org>
Cc: <linux-tegra@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <ldewangan@...dia.com>, <mkumard@...dia.com>
Subject: Re: [RESEND PATCH 1/2] dt-bindings: dma: Add reg-names to
 nvidia,tegra210-adma

On Fri May 31, 2024 at 9:43 AM CEST, Krzysztof Kozlowski wrote:
> On 30/05/2024 14:48, Thierry Reding wrote:
> > On Tue May 28, 2024 at 8:48 AM CEST, Krzysztof Kozlowski wrote:
> >> On 24/05/2024 09:36, Thierry Reding wrote:
> >>> On Wed May 22, 2024 at 1:29 PM CEST, Krzysztof Kozlowski wrote:
> >>>> On 22/05/2024 09:43, Sameer Pujar wrote:
> >>>>>
> >>>>>
> >>>>> On 22-05-2024 12:17, Krzysztof Kozlowski wrote:
> >>>>>> On 22/05/2024 07:35, Sameer Pujar wrote:
> >>>>>>> On 21-05-2024 17:23, Krzysztof Kozlowski wrote:
> >>>>>>>> On 21/05/2024 13:08, Sameer Pujar wrote:
> >>>>>>>>> From: Mohan Kumar <mkumard@...dia.com>
> >>>>>>>>>
> >>>>>>>>> For Non-Hypervisor mode, Tegra ADMA driver requires the register
> >>>>>>>>> resource range to include both global and channel page in the reg
> >>>>>>>>> entry. For Hypervisor more, Tegra ADMA driver requires only the
> >>>>>>>>> channel page and global page range is not allowed for access.
> >>>>>>>>>
> >>>>>>>>> Add reg-names DT binding for Hypervisor mode to help driver to
> >>>>>>>>> differentiate the config between Hypervisor and Non-Hypervisor
> >>>>>>>>> mode of execution.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Mohan Kumar <mkumard@...dia.com>
> >>>>>>>>> Signed-off-by: Sameer Pujar <spujar@...dia.com>
> >>>>>>>>> ---
> >>>>>>>>>    .../devicetree/bindings/dma/nvidia,tegra210-adma.yaml  | 10 ++++++++++
> >>>>>>>>>    1 file changed, 10 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra210-adma.yaml b/Documentation/devicetree/bindings/dma/nvidia,tegra210-adma.yaml
> >>>>>>>>> index 877147e95ecc..ede47f4a3eec 100644
> >>>>>>>>> --- a/Documentation/devicetree/bindings/dma/nvidia,tegra210-adma.yaml
> >>>>>>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra210-adma.yaml
> >>>>>>>>> @@ -29,8 +29,18 @@ properties:
> >>>>>>>>>              - const: nvidia,tegra186-adma
> >>>>>>>>>
> >>>>>>>>>      reg:
> >>>>>>>>> +    description: |
> >>>>>>>>> +      For hypervisor mode, the address range should include a
> >>>>>>>>> +      ADMA channel page address range, for non-hypervisor mode
> >>>>>>>>> +      it starts with ADMA base address covering Global and Channel
> >>>>>>>>> +      page address range.
> >>>>>>>>>        maxItems: 1
> >>>>>>>>>
> >>>>>>>>> +  reg-names:
> >>>>>>>>> +    description: only required for Hypervisor mode.
> >>>>>>>> This does not work like that. I provide vm entry for non-hypervisor mode
> >>>>>>>> and what? You claim it is virtualized?
> >>>>>>>>
> >>>>>>>> Drop property.
> >>>>>>> With 'vm' entry added for hypervisor mode, the 'reg' address range needs
> >>>>>>> to be updated to use channel specific region only. This is used to
> >>>>>>> inform driver to skip global regions which is taken care by hypervisor.
> >>>>>>> This is expected to be used in the scenario where Linux acts as a
> >>>>>>> virtual machine (VM). May be the hypervisor mode gives a different
> >>>>>>> impression here? Sorry, I did not understand what dropping the property
> >>>>>>> exactly means here.
> >>>>>> It was imperative. Drop it. Remove it. I provided explanation why.
> >>>>>
> >>>>> The driver doesn't know if it is operated in a native config or in the 
> >>>>> hypervisor config based on the 'reg' address range alone. So 'vm' entry 
> >>>>> with restricted 'reg' range is used to differentiate here for the 
> >>>>> hypervisor config. Just adding 'vm' entry won't be enough, the 'reg' 
> >>>>> region must be updated as well to have expected behavior. Not sure how 
> >>>>> this dependency can be enforced in the schema.
> >>>>
> >>>> That's not a unusual problem, so please come with a solution for your
> >>>> entire subarch. We've been discussing similar topic in terms of SCMI
> >>>> controlled resources (see talk on Linaro Connect a week ago:
> >>>> https://www.kitefor.events/events/linaro-connect-24/submissions/161 I
> >>>> don't know where is recording or slides, see also discussions on mailing
> >>>> lists about it), which is not that far away from the problem here. Other
> >>>> platforms and maybe nvidia had as well changes in IO space for
> >>>> virtualized configuration.
> >>>>
> >>>> Come with unified approach FOR ALL your devices, not only this one
> >>>> (that's kind of basic thing we keep repeating... don't solve only one
> >>>> your problem), do not abuse the regular property, because as I said:
> >>>> reg-names will be provided as well in non-vm case and then your entire
> >>>> logic is wrong. The purpose of reg-names is not to tell whether you have
> >>>> or have not virtualized environment.
> >>>
> >>> This isn't strictly about telling whether this is a virtualized
> >>> environment or not. Unfortunately the bindings don't make that very
> >>> clear, so let me try to give a bit more background.
> >>>
> >>> On Tegra devices the register regions associated with a device are
> >>> usually split up into 64 KiB chunks.
> >>
> >> So describing it as one IO region was incorrect from the start and you
> >> want to fix it by adding one more incorrect description: making first
> >> item meaning two different things. Sorry, that's not a correct way to
> >> fix things.
> > 
> > Yes, describing this as one I/O region was incorrect, and in hindsight
> > it should have been done differently.
> > 
> > However, I don't think it's correct to describe this as adding one more
> > incorrect description. Instead, what this does is add reg-names to
> > provide additional context so that the operating system can make the
> > necessary decisions as to what is allowed and what isn't.
> > 
> > In the absence of a reg-names property the current definition of the DT
> > bindings applies, so it means the region represents the entirety of the
> > device's I/O register space. That's one particular use-case for this
> > device.
> > 
> > For additional use-cases we can then use reg-names to differentiate
> > between what separate regions are and use them accordingly.
> > 
> >> Items are defined, thus first item is always expected to be what the
> >> binding already said. Adding reg-names changes nothing, because (as
> >> repeated many times) xxx-names is just a helper. Items are already defined.
> > 
> > I don't understand what you're trying to say here. I suppose adding
> > reg-names alone indeed doesn't change anything. But the point is that
> > once added we can now use these properties, at which point of course
> > things change.
> > 
> >>> One of these chunks, usually the first one, is a global region that
> >>> contains registers that configure the device as a whole. This is usually
> >>> privileged and accessible only to the hypervisor.
> >>>
> >>> Subsequent regions are meant to be assigned to individual VMs. Often the
> >>> regions take the form of "channels", so they are instances of the same
> >>> register block and control that separate slice of the hardware.
> >>>
> >>> What makes this a bit confusing is that for the sake of simplicity (and,
> >>> I guess, lack of foresight) the original bindings were written in a way
> >>> to encompass all registers without making that distinction. This worked
> >>> fine because we've only ever run Linux as host OS where it has access to
> >>> all those registers.
> >>>
> >>> However, when we move to virtualized environments that no longer works.
> >>>
> >>> Given the above, we can't read any registers in order to probe whether
> >>> we run as a guest or not. Trying to access any of the global registers
> >>> from a VM simply won't work and may crash the system. None of the
> >>> "channel" registers contain information indicating host vs. guest
> >>> either.
> >>
> >> I don't understand how it differs from what I said - you want to
> >> indicate that you run in virtualized environment and not all resources
> >> are accessible.
> >>
> >> The device still has the first (global) address, just it is not
> >> available due to hypervisor.
> > 
> > Yes, and that's a bad thing because there's no way for the device to
> > know that it can't access the registers. So it will just assume that it
> > can and try to access them, which would then result in a crash/error.
>
> Different compatible could note that or the global address would be
> removed from IO space, although then you need to rely on names and order
> is not fixed. I think Rob already proposed different compatible.
>
> This is also the way new Qcom platforms are going (older were using
> properties).
>
> However my earlier comment stays on: you will have for sure more cases
> like this, so please think upfront and pick unified approach for all
> future devices.

We already have. In fact we already have a few devices (host1x[0] and
MGBE[1]) where a similar path was chosen. Unification with those is why
we're proposing this.

This also applies to the memory controller SID bindings update that we
proposed a little while ago.

Thierry

[0]:
Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
[1]: Documentation/devicetree/bindings/net/nvidia,tegra234-mgbe.yaml

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists