linux-kernel - Re: [PATCH v7 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZkE+Fa4BS+LTxvgi@nvidia.com>
Date: Sun, 12 May 2024 15:09:25 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: <will@...nel.org>, <robin.murphy@....com>, <joro@...tes.org>,
	<thierry.reding@...il.com>, <vdumpa@...dia.com>, <jonathanh@...dia.com>,
	<linux-kernel@...r.kernel.org>, <iommu@...ts.linux.dev>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v7 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned
 VINTF

On Sun, May 12, 2024 at 01:06:13PM -0300, Jason Gunthorpe wrote:
> On Tue, May 07, 2024 at 10:56:54PM -0700, Nicolin Chen wrote:
> > When VCMDQs are assigned to a VINTF owned by a guest (HYP_OWN bit unset),
> > only TLB and ATC invalidation commands are supported by the VCMDQ HW. So,
> > add a new helper to scan the input cmd to make sure it is supported when
> > selecting a queue.
> > 
> > Note that the guest VM shouldn't have HYP_OWN bit being set regardless of
> > guest kernel driver writing it or not, i.e. the hypervisor running in the
> > host OS should wire this bit to zero when trapping a write access to this
> > VINTF_CONFIG register from a guest kernel.
> > 
> > Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
> > ---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 20 ++++++-----
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  5 +--
> >  .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 36 ++++++++++++++++++-
> >  3 files changed, 49 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index d1098991d64e..baf20e9976d3 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -332,10 +332,11 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
> >  	return 0;
> >  }
> >  
> > -static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu)
> > +static struct arm_smmu_cmdq *
> > +arm_smmu_get_cmdq(struct arm_smmu_device *smmu, u8 opcode)
> >  {
> >  	if (arm_smmu_has_tegra241_cmdqv(smmu))
> > -		return tegra241_cmdqv_get_cmdq(smmu);
> > +		return tegra241_cmdqv_get_cmdq(smmu, opcode);
> 
> It is worth a comment descrbing opcode here, I think.. At least the
> nesting invalidation will send mixed batches.

Right, this makes the "opcode" look bad, though we know that the
"opcode" in the nesting invalidation doesn't matter because VCMDQ
in that case supports all commands with HYP_OWN=1.

> opcode is sort of a handle for a group of related commands. But what
> is the group? Minimally it is opcode + SYNC, right?
>
>  The caller must only send opcode + SYNC commands to this queue.
>  The opcodes XX,YY,ZZ are interchangable and can be sent together.
> 
> ?

The "opcode" is intended to mean the opcode of a repeated commands
in an arm_smmu_cmdq_batch struct. And it is based on an assumption
that the driver doesn't and won't mix different commands into an
arm_smmu_cmdq_batch struct. Though it doesn't probably matter if a
batch mixes NH_ASID and ATC_INV..

A CMD_SYNC, on the other hand, is outside the batch struct. And
here is another assumption that CMD_SYNC is always supported by a
VCMDQ..

I could clarify the "opcode" here with these assumptions. Or maybe
we should think think of a better alternative?

> > -struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
> > +static bool tegra241_vintf_support_cmd(struct tegra241_vintf *vintf, u8 opcode)
> > +{
> > +       /* Hypervisor-owned VINTF can execute any command in its VCMDQs */
> > +	if (READ_ONCE(vintf->hyp_own))
> > +		return true;
> > +
> > +	/* Guest-owned VINTF must Check against the list of supported CMDs */
> > +	switch (opcode) {
> > +	case CMDQ_OP_TLBI_NH_ASID:
> > +	case CMDQ_OP_TLBI_NH_VA:
> > +	case CMDQ_OP_ATC_INV:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> 
> When I look at the nesting patch it also includes SYNC, NH_VAA, and
> NH_ALL. Are they supported here? VAA is not supported in the HW at all
> right? What about NH_ALL?

In a nesting case, the host-level VCMDQ runs those comands, i.e.
HYP_OWN=1 meaning no command limitation.

Thanks
Nicolin