[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <252d6094-b2d6-496d-b28f-93507a193ede@nvidia.com>
Date: Tue, 6 Feb 2024 12:28:27 +0000
From: Jon Hunter <jonathanh@...dia.com>
To: Marc Zyngier <maz@...nel.org>, Sumit Gupta <sumitg@...dia.com>
Cc: treding@...dia.com, krzysztof.kozlowski@...aro.org, mark.rutland@....com,
linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
amhetre@...dia.com, bbasu@...dia.com
Subject: Re: [Patch] memory: tegra: Skip SID override from Guest VM
Hi Marc,
On 06/02/2024 12:17, Marc Zyngier wrote:
> Hi Sumit,
>
> On Tue, 06 Feb 2024 11:48:52 +0000,
> Sumit Gupta <sumitg@...dia.com> wrote:
>>
>> MC SID register access is restricted for Guest VM.
>> So, skip the SID override programming from the Guest VM.
>>
>> Signed-off-by: Sumit Gupta <sumitg@...dia.com>
>> ---
>> drivers/memory/tegra/tegra186.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>> index 1b3183951bfe..df441896b69d 100644
>> --- a/drivers/memory/tegra/tegra186.c
>> +++ b/drivers/memory/tegra/tegra186.c
>> @@ -10,6 +10,7 @@
>> #include <linux/of.h>
>> #include <linux/of_platform.h>
>> #include <linux/platform_device.h>
>> +#include <asm/virt.h>
>>
>> #include <soc/tegra/mc.h>
>>
>> @@ -118,6 +119,11 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
>> unsigned int i, index = 0;
>> u32 sid;
>>
>> + if (!is_kernel_in_hyp_mode()) {
>> + dev_dbg(mc->dev, "Register access not allowed\n");
>> + return 0;
>> + }
>> +
>> if (!tegra_dev_iommu_get_stream_id(dev, &sid))
>> return 0;
>>
>> @@ -146,6 +152,11 @@ static int tegra186_mc_resume(struct tegra_mc *mc)
>> #if IS_ENABLED(CONFIG_IOMMU_API)
>> unsigned int i;
>>
>> + if (!is_kernel_in_hyp_mode()) {
>> + dev_dbg(mc->dev, "Register access not allowed\n");
>> + return 0;
>> + }
>> +
>> for (i = 0; i < mc->soc->num_clients; i++) {
>> const struct tegra_mc_client *client = &mc->soc->clients[i];
>>
>
> This doesn't look right. Multiple reasons:
>
> - This helper really has nothing to do in a driver. This is
> architectural stuff that is not intended for use outside of arch
> core code.
We see a few other drivers using this ...
drivers/perf/arm_pmuv3.c
drivers/clocksource/arm_arch_timer.c
drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
drivers/hwtracing/coresight/coresight-etm4x-core.c
drivers/hwtracing/coresight/coresight-etm4x-core.c
drivers/irqchip/irq-apple-aic.c
We were looking for a way to determine if the OS is a guest OS or not.
However, I can see that this is a ARM64 specific API and so probably the
above are only compiled for ARM64. Interestingly, the AMD driver
implements the following ...
static inline bool is_virtual_machine(void)
{
#if defined(CONFIG_X86)
return boot_cpu_has(X86_FEATURE_HYPERVISOR);
#elif defined(CONFIG_ARM64)
return !is_kernel_in_hyp_mode();
#else
return false;
#endif
}
> - My own tegra186 HW doesn't have VHE, since it is ARMv8.0, and this
> helper will always return 'false'. How could this result in
> something that still works? Can I get a free CPU upgrade?
I thought this API just checks to see if we are in EL2?
> - If you assign this device to a VM and that the hypervisor doesn't
> correctly virtualise it, then it is a different device and you
> should simply advertise it something else. Or even better, fix your
> hypervisor.
Sumit can add some more details on why we don't completely disable the
device for guest OSs.
Jon
--
nvpublic
Powered by blists - more mailing lists