On Fri, May 24, 2024 at 09:36:08AM +0200, 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@xxxxxxxxxx> > > >>>>> > > >>>>> 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@xxxxxxxxxx> > > >>>>> Signed-off-by: Sameer Pujar <spujar@xxxxxxxxxx> > > >>>>> --- > > >>>>> .../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. > > 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. > > In order to make this work we need to more fine-grainedly specify the > register layout. I think the binding changes here aren't sufficient to > do that, though. > > Currently we have this for the ADMA controller: > > dma-controller@2930000 { > reg = <0x0 0x02930000 0x0 0x20000>; > }; > > This contains the global registers (0x2930000-0x293ffff) and the first > page/channel registers (0x2940000-0x294ffff) in one "reg" entry. Instead > I think what we need is this: > > dma-controller@2930000 { > reg = <0x0 0x02930000 0x0 0x10000>, > <0x0 0x02940000 0x0 0x10000>, > <0x0 0x02950000 0x0 0x10000>, > <0x0 0x02960000 0x0 0x10000>, > <0x0 0x02970000 0x0 0x10000>; > reg-names = "global", "page0", "page1", "page2", > "page3"; > }; > > That describes the device fully, but each of these entries is optional. > If "global" is present it means we are a hypervisor (or host OS). If an > additional "page" entry is present, we can also use those resources to > stream audio data. > > If "global" is not present, we know we are not a hypervisor and those > registers cannot be accessed. This would be the typical case for a guest > OS which has access only to the listed "page" entries. > > For backwards-compatibility with the existing bindings we should be able > to fallback to the singular register region and partition it up in the > driver as necessary. > > This is an approach that we've already implemented for certain devices > such as host1x and Ethernet where a similar split exists. I suspect that > we'll need to do this kind of split in a number of other bindings as > well. In a VM is a different (being a subset) programming model, so why not just a new compatible for virtualized case. That's what we'd do if actual h/w registers changed from one device to the next. Rob
