Hi Jon,
On Tue, 06 Feb 2024 12:28:27 +0000,
Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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
These two are definitely part of the CPU architecture.
> drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
This is just a bug. Please don't copy this stuff.
> drivers/hwtracing/coresight/coresight-etm4x-core.c
> drivers/hwtracing/coresight/coresight-etm4x-core.c
> drivers/irqchip/irq-apple-aic.c
These are also part of the CPU architecture.
>
> 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
> }
This stuff should simply be ripped out and burned. Whoever wrote it
didn't understand how this works.
>
> > - 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?
It does. And that's the problem. On ARMv8.0, we run the Linux kernel
at EL1. Tegra186 is ARMv8.0 (Denver + A57). So as written, this change
breaks the very platform it intends to support.
>
> > - 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.
It's not about disabling it. It is about correctly supporting it
(providing full emulation for it), or advertising it as something
different so that SW can handle it differently.
Poking into the internals of how the kernel is booted for a driver
that isn't tied to the core architecture (because it would need to
access system registers, for example) is not an acceptable outcome.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
