mbox series

[v3,0/2] memory: tegra: Skip restricted register access from Guest

Message ID 20240412130540.28447-1-sumitg@nvidia.com
Headers show
Series memory: tegra: Skip restricted register access from Guest | expand

Message

Sumit Gupta April 12, 2024, 1:05 p.m. UTC
MC SID and Broadbast channel register access is restricted for Guest VM.
But the Tegra MC driver is considering both these regions as mandatory
and causes error on access. Change that to consider both the regions as
optional in SoC's from Tegra186 onwards. Then skip access to the
restricted registers from Guest if its region is not present in Guest DT
and hence not mapped.
Previously, the solution in [1] was not accepted. Now, handled the
problem with this alternate solution.

---
v2[2] -> v3:
- fix the yaml error.
- add null check for ch_regs in mc_ch_readl() causing error in old SoC's
- remove unused populate label reported by kernel test robot.
- fix existing compile warning about length of name in tegra186_mc_probe.

v1[1] -> v2:
- consider broadcast channel registers also as restricted along with sid
- make sid and broadcast regions optional and access if present in dt.
- update the yaml file.

Sumit Gupta (2):
  dt-bindings: make sid and broadcast reg optional
  memory: tegra: make sid and broadcast regions optional

 .../nvidia,tegra186-mc.yaml                   | 95 ++++++++++---------
 drivers/memory/tegra/mc.c                     |  9 +-
 drivers/memory/tegra/mc.h                     | 22 +++--
 drivers/memory/tegra/tegra186.c               | 25 ++---
 4 files changed, 82 insertions(+), 69 deletions(-)

[1]https://lore.kernel.org/lkml/20240206114852.8472-1-sumitg@nvidia.com/
[2]https://lore.kernel.org/lkml/20240402132626.24693-1-sumitg@nvidia.com/

Comments

Krzysztof Kozlowski April 22, 2024, 7:12 a.m. UTC | #1
On 12/04/2024 15:05, Sumit Gupta wrote:
> MC SID and Broadbast channel register access is restricted for Guest VM.

Same typo

> In Tegra MC driver, consider both the regions as optional and skip
> access to restricted registers from Guest if a region is not present
> in Guest DT.
> 

...

>  
>  static inline u32 mc_readl(const struct tegra_mc *mc, unsigned long offset)
> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
> index 1b3183951bfe..716582255eeb 100644
> --- a/drivers/memory/tegra/tegra186.c
> +++ b/drivers/memory/tegra/tegra186.c
> @@ -26,20 +26,16 @@
>  static int tegra186_mc_probe(struct tegra_mc *mc)
>  {
>  	struct platform_device *pdev = to_platform_device(mc->dev);
> +	struct resource *res;
>  	unsigned int i;
> -	char name[8];
> +	char name[14];

How is it relevant? I don't see this being used in your diff.


Best regards,
Krzysztof
Sumit Gupta April 22, 2024, 2:36 p.m. UTC | #2
> On 12/04/2024 15:05, Sumit Gupta wrote:
>> MC SID and Broadbast channel register access is restricted for Guest VM.
> 
> Same typo
> 
Thank you for catching. Will correct in v4.

>> In Tegra MC driver, consider both the regions as optional and skip
>> access to restricted registers from Guest if a region is not present
>> in Guest DT.
>>
> 
> ...
> 
>>
>>   static inline u32 mc_readl(const struct tegra_mc *mc, unsigned long offset)
>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>> index 1b3183951bfe..716582255eeb 100644
>> --- a/drivers/memory/tegra/tegra186.c
>> +++ b/drivers/memory/tegra/tegra186.c
>> @@ -26,20 +26,16 @@
>>   static int tegra186_mc_probe(struct tegra_mc *mc)
>>   {
>>        struct platform_device *pdev = to_platform_device(mc->dev);
>> +     struct resource *res;
>>        unsigned int i;
>> -     char name[8];
>> +     char name[14];
> 
> How is it relevant? I don't see this being used in your diff.
> 
> 
> Best regards,
> Krzysztof
> 

Did this change for below warning coming with 'W=1'.

../drivers/memory/tegra/tegra186.c: In function tegra186_mc_probe:
../drivers/memory/tegra/tegra186.c:51:49: warning: %u directive output 
may be truncated writing between 1 and 10 bytes into a region of size 6 
[8;;https://gc
c.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-truncation=-Wformat-truncation=8;;]
    51 |                 snprintf(name, sizeof(name), "ch%u", i);
       |                                                 ^~
../drivers/memory/tegra/tegra186.c:51:46: note: directive argument in 
the range [0, 4294967294]
    51 |                 snprintf(name, sizeof(name), "ch%u", i);
       |                                              ^~~~~~
../drivers/memory/tegra/tegra186.c:51:17: note: snprintf output between 
4 and 13 bytes into a destination of size 8
    51 |                 snprintf(name, sizeof(name), "ch%u", i);
       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Thank you,
Sumit Gupta
Krzysztof Kozlowski April 23, 2024, 2:41 p.m. UTC | #3
On 22/04/2024 16:36, Sumit Gupta wrote:
> 
>> On 12/04/2024 15:05, Sumit Gupta wrote:
>>> MC SID and Broadbast channel register access is restricted for Guest VM.
>>
>> Same typo
>>
> Thank you for catching. Will correct in v4.
> 
>>> In Tegra MC driver, consider both the regions as optional and skip
>>> access to restricted registers from Guest if a region is not present
>>> in Guest DT.
>>>
>>
>> ...
>>
>>>
>>>   static inline u32 mc_readl(const struct tegra_mc *mc, unsigned long offset)
>>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>>> index 1b3183951bfe..716582255eeb 100644
>>> --- a/drivers/memory/tegra/tegra186.c
>>> +++ b/drivers/memory/tegra/tegra186.c
>>> @@ -26,20 +26,16 @@
>>>   static int tegra186_mc_probe(struct tegra_mc *mc)
>>>   {
>>>        struct platform_device *pdev = to_platform_device(mc->dev);
>>> +     struct resource *res;
>>>        unsigned int i;
>>> -     char name[8];
>>> +     char name[14];
>>
>> How is it relevant? I don't see this being used in your diff.
>>
>>
>> Best regards,
>> Krzysztof
>>
> 
> Did this change for below warning coming with 'W=1'.
> 
> ../drivers/memory/tegra/tegra186.c: In function tegra186_mc_probe:
> ../drivers/memory/tegra/tegra186.c:51:49: warning: %u directive output 
> may be truncated writing between 1 and 10 bytes into a region of size 6 
> [8;;https://gc
> c.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-truncation=-Wformat-truncation=8;;]
>     51 |                 snprintf(name, sizeof(name), "ch%u", i);
>        |                                                 ^~
> ../drivers/memory/tegra/tegra186.c:51:46: note: directive argument in 
> the range [0, 4294967294]
>     51 |                 snprintf(name, sizeof(name), "ch%u", i);
>        |                                              ^~~~~~
> ../drivers/memory/tegra/tegra186.c:51:17: note: snprintf output between 
> 4 and 13 bytes into a destination of size 8
>     51 |                 snprintf(name, sizeof(name), "ch%u", i);
>        |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I asked how this is relevant to this change and you answer there is a
warning. If the warning was there, your answer is really just deflecting
the topic, so obviously this is new warning. Which part of code uses
longer name?

BTW, really, such answers do not make review of your code smoother.

Best regards,
Krzysztof
Sumit Gupta April 23, 2024, 7:46 p.m. UTC | #4
>>>>
>>>>    static inline u32 mc_readl(const struct tegra_mc *mc, unsigned long offset)
>>>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>>>> index 1b3183951bfe..716582255eeb 100644
>>>> --- a/drivers/memory/tegra/tegra186.c
>>>> +++ b/drivers/memory/tegra/tegra186.c
>>>> @@ -26,20 +26,16 @@
>>>>    static int tegra186_mc_probe(struct tegra_mc *mc)
>>>>    {
>>>>         struct platform_device *pdev = to_platform_device(mc->dev);
>>>> +     struct resource *res;
>>>>         unsigned int i;
>>>> -     char name[8];
>>>> +     char name[14];
>>>
>>> How is it relevant? I don't see this being used in your diff.
>>>
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> Did this change for below warning coming with 'W=1'.
>>
>> ../drivers/memory/tegra/tegra186.c: In function tegra186_mc_probe:
>> ../drivers/memory/tegra/tegra186.c:51:49: warning: %u directive output
>> may be truncated writing between 1 and 10 bytes into a region of size 6
>> [8;;https://gc
>> c.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-truncation=-Wformat-truncation=8;;]
>>      51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>         |                                                 ^~
>> ../drivers/memory/tegra/tegra186.c:51:46: note: directive argument in
>> the range [0, 4294967294]
>>      51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>         |                                              ^~~~~~
>> ../drivers/memory/tegra/tegra186.c:51:17: note: snprintf output between
>> 4 and 13 bytes into a destination of size 8
>>      51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I asked how this is relevant to this change and you answer there is a
> warning. If the warning was there, your answer is really just deflecting
> the topic, so obviously this is new warning. Which part of code uses
> longer name?
> 
> BTW, really, such answers do not make review of your code smoother.
> 
> Best regards,
> Krzysztof
> 

Apologies for not explaining it earlier.

I increased the buffer size to suppress a static check warning in the
existing code due to big range of 'unsigned int i', if copied to small
name buffer.

Seems like the warning is harmless as the maximum value of num_channels
is 16. I will remove it and keep the buffer size as 8 in the next
version.

Thank you,
Sumit Gupta
Krzysztof Kozlowski April 24, 2024, 4:09 a.m. UTC | #5
On 23/04/2024 21:46, Sumit Gupta wrote:
> 
> 
> 
>>>>>
>>>>>    static inline u32 mc_readl(const struct tegra_mc *mc, unsigned long offset)
>>>>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>>>>> index 1b3183951bfe..716582255eeb 100644
>>>>> --- a/drivers/memory/tegra/tegra186.c
>>>>> +++ b/drivers/memory/tegra/tegra186.c
>>>>> @@ -26,20 +26,16 @@
>>>>>    static int tegra186_mc_probe(struct tegra_mc *mc)
>>>>>    {
>>>>>         struct platform_device *pdev = to_platform_device(mc->dev);
>>>>> +     struct resource *res;
>>>>>         unsigned int i;
>>>>> -     char name[8];
>>>>> +     char name[14];
>>>>
>>>> How is it relevant? I don't see this being used in your diff.
>>>>
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Did this change for below warning coming with 'W=1'.
>>>
>>> ../drivers/memory/tegra/tegra186.c: In function tegra186_mc_probe:
>>> ../drivers/memory/tegra/tegra186.c:51:49: warning: %u directive output
>>> may be truncated writing between 1 and 10 bytes into a region of size 6
>>> [8;;https://gc
>>> c.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-truncation=-Wformat-truncation=8;;]
>>>      51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>>         |                                                 ^~
>>> ../drivers/memory/tegra/tegra186.c:51:46: note: directive argument in
>>> the range [0, 4294967294]
>>>      51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>>         |                                              ^~~~~~
>>> ../drivers/memory/tegra/tegra186.c:51:17: note: snprintf output between
>>> 4 and 13 bytes into a destination of size 8
>>>      51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>>         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> I asked how this is relevant to this change and you answer there is a
>> warning. If the warning was there, your answer is really just deflecting
>> the topic, so obviously this is new warning. Which part of code uses
>> longer name?
>>
>> BTW, really, such answers do not make review of your code smoother.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Apologies for not explaining it earlier.
> 
> I increased the buffer size to suppress a static check warning in the
> existing code due to big range of 'unsigned int i', if copied to small
> name buffer.
> 
> Seems like the warning is harmless as the maximum value of num_channels
> is 16. I will remove it and keep the buffer size as 8 in the next
> version.
> 

That's not the point. For the third time: how is it relevant to this
change here? Was or was not the warning before?

Best regards,
Krzysztof
Sumit Gupta April 24, 2024, 5:27 a.m. UTC | #6
>>
>>>>>>
>>>>>>     static inline u32 mc_readl(const struct tegra_mc *mc, unsigned long offset)
>>>>>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>>>>>> index 1b3183951bfe..716582255eeb 100644
>>>>>> --- a/drivers/memory/tegra/tegra186.c
>>>>>> +++ b/drivers/memory/tegra/tegra186.c
>>>>>> @@ -26,20 +26,16 @@
>>>>>>     static int tegra186_mc_probe(struct tegra_mc *mc)
>>>>>>     {
>>>>>>          struct platform_device *pdev = to_platform_device(mc->dev);
>>>>>> +     struct resource *res;
>>>>>>          unsigned int i;
>>>>>> -     char name[8];
>>>>>> +     char name[14];
>>>>>
>>>>> How is it relevant? I don't see this being used in your diff.
>>>>>
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>>
>>>> Did this change for below warning coming with 'W=1'.
>>>>
>>>> ../drivers/memory/tegra/tegra186.c: In function tegra186_mc_probe:
>>>> ../drivers/memory/tegra/tegra186.c:51:49: warning: %u directive output
>>>> may be truncated writing between 1 and 10 bytes into a region of size 6
>>>> [8;;https://gc
>>>> c.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-truncation=-Wformat-truncation=8;;]
>>>>       51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>>>          |                                                 ^~
>>>> ../drivers/memory/tegra/tegra186.c:51:46: note: directive argument in
>>>> the range [0, 4294967294]
>>>>       51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>>>          |                                              ^~~~~~
>>>> ../drivers/memory/tegra/tegra186.c:51:17: note: snprintf output between
>>>> 4 and 13 bytes into a destination of size 8
>>>>       51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>>>          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> I asked how this is relevant to this change and you answer there is a
>>> warning. If the warning was there, your answer is really just deflecting
>>> the topic, so obviously this is new warning. Which part of code uses
>>> longer name?
>>>
>>> BTW, really, such answers do not make review of your code smoother.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> Apologies for not explaining it earlier.
>>
>> I increased the buffer size to suppress a static check warning in the
>> existing code due to big range of 'unsigned int i', if copied to small
>> name buffer.
>>
>> Seems like the warning is harmless as the maximum value of num_channels
>> is 16. I will remove it and keep the buffer size as 8 in the next
>> version.
>>
> 
> That's not the point. For the third time: how is it relevant to this
> change here? Was or was not the warning before?
> 

This is not relevant to the change here. The warning was before as well.

Thank you,
Sumit Gupta

> Best regards,
> Krzysztof
>
Krzysztof Kozlowski April 24, 2024, 5:44 a.m. UTC | #7
On 24/04/2024 07:27, Sumit Gupta wrote:
> 
> 
>>>
>>>>>>>
>>>>>>>     static inline u32 mc_readl(const struct tegra_mc *mc, unsigned long offset)
>>>>>>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>>>>>>> index 1b3183951bfe..716582255eeb 100644
>>>>>>> --- a/drivers/memory/tegra/tegra186.c
>>>>>>> +++ b/drivers/memory/tegra/tegra186.c
>>>>>>> @@ -26,20 +26,16 @@
>>>>>>>     static int tegra186_mc_probe(struct tegra_mc *mc)
>>>>>>>     {
>>>>>>>          struct platform_device *pdev = to_platform_device(mc->dev);
>>>>>>> +     struct resource *res;
>>>>>>>          unsigned int i;
>>>>>>> -     char name[8];
>>>>>>> +     char name[14];
>>>>>>
>>>>>> How is it relevant? I don't see this being used in your diff.
>>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>> Krzysztof
>>>>>>
>>>>>
>>>>> Did this change for below warning coming with 'W=1'.
>>>>>
>>>>> ../drivers/memory/tegra/tegra186.c: In function tegra186_mc_probe:
>>>>> ../drivers/memory/tegra/tegra186.c:51:49: warning: %u directive output
>>>>> may be truncated writing between 1 and 10 bytes into a region of size 6
>>>>> [8;;https://gc
>>>>> c.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-truncation=-Wformat-truncation=8;;]
>>>>>       51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>>>>          |                                                 ^~
>>>>> ../drivers/memory/tegra/tegra186.c:51:46: note: directive argument in
>>>>> the range [0, 4294967294]
>>>>>       51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>>>>          |                                              ^~~~~~
>>>>> ../drivers/memory/tegra/tegra186.c:51:17: note: snprintf output between
>>>>> 4 and 13 bytes into a destination of size 8
>>>>>       51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>>>>          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> I asked how this is relevant to this change and you answer there is a
>>>> warning. If the warning was there, your answer is really just deflecting
>>>> the topic, so obviously this is new warning. Which part of code uses
>>>> longer name?
>>>>
>>>> BTW, really, such answers do not make review of your code smoother.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Apologies for not explaining it earlier.
>>>
>>> I increased the buffer size to suppress a static check warning in the
>>> existing code due to big range of 'unsigned int i', if copied to small
>>> name buffer.
>>>
>>> Seems like the warning is harmless as the maximum value of num_channels
>>> is 16. I will remove it and keep the buffer size as 8 in the next
>>> version.
>>>
>>
>> That's not the point. For the third time: how is it relevant to this
>> change here? Was or was not the warning before?
>>
> 
> This is not relevant to the change here. The warning was before as well.

OK, fixing the warning is always a good idea, but this *must* be always
separate patch, with its own explanation and rationale, and warning message.

Best regards,
Krzysztof
Sumit Gupta April 24, 2024, 6:27 a.m. UTC | #8
>>>>>>>>
>>>>>>>>      static inline u32 mc_readl(const struct tegra_mc *mc, unsigned long offset)
>>>>>>>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>>>>>>>> index 1b3183951bfe..716582255eeb 100644
>>>>>>>> --- a/drivers/memory/tegra/tegra186.c
>>>>>>>> +++ b/drivers/memory/tegra/tegra186.c
>>>>>>>> @@ -26,20 +26,16 @@
>>>>>>>>      static int tegra186_mc_probe(struct tegra_mc *mc)
>>>>>>>>      {
>>>>>>>>           struct platform_device *pdev = to_platform_device(mc->dev);
>>>>>>>> +     struct resource *res;
>>>>>>>>           unsigned int i;
>>>>>>>> -     char name[8];
>>>>>>>> +     char name[14];
>>>>>>>
>>>>>>> How is it relevant? I don't see this being used in your diff.
>>>>>>>
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Krzysztof
>>>>>>>
>>>>>>
>>>>>> Did this change for below warning coming with 'W=1'.
>>>>>>
>>>>>> ../drivers/memory/tegra/tegra186.c: In function tegra186_mc_probe:
>>>>>> ../drivers/memory/tegra/tegra186.c:51:49: warning: %u directive output
>>>>>> may be truncated writing between 1 and 10 bytes into a region of size 6
>>>>>> [8;;https://gc
>>>>>> c.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-truncation=-Wformat-truncation=8;;]
>>>>>>        51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>>>>>           |                                                 ^~
>>>>>> ../drivers/memory/tegra/tegra186.c:51:46: note: directive argument in
>>>>>> the range [0, 4294967294]
>>>>>>        51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>>>>>           |                                              ^~~~~~
>>>>>> ../drivers/memory/tegra/tegra186.c:51:17: note: snprintf output between
>>>>>> 4 and 13 bytes into a destination of size 8
>>>>>>        51 |                 snprintf(name, sizeof(name), "ch%u", i);
>>>>>>           |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>
>>>>> I asked how this is relevant to this change and you answer there is a
>>>>> warning. If the warning was there, your answer is really just deflecting
>>>>> the topic, so obviously this is new warning. Which part of code uses
>>>>> longer name?
>>>>>
>>>>> BTW, really, such answers do not make review of your code smoother.
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>>
>>>> Apologies for not explaining it earlier.
>>>>
>>>> I increased the buffer size to suppress a static check warning in the
>>>> existing code due to big range of 'unsigned int i', if copied to small
>>>> name buffer.
>>>>
>>>> Seems like the warning is harmless as the maximum value of num_channels
>>>> is 16. I will remove it and keep the buffer size as 8 in the next
>>>> version.
>>>>
>>>
>>> That's not the point. For the third time: how is it relevant to this
>>> change here? Was or was not the warning before?
>>>
>>
>> This is not relevant to the change here. The warning was before as well.
> 
> OK, fixing the warning is always a good idea, but this *must* be always
> separate patch, with its own explanation and rationale, and warning message.
> 

Sure, will submit a separate patch for the warning and spin a v4 for 
this patch series after incorporating all review comments.

Thank you,
Sumit Gupta

> Best regards,
> Krzysztof
>