On 20.05.20 13:48, Alice Guo wrote:
>
>
>> -----Original Message-----
>> From: Alice Guo
>> Sent: 2020年5月20日 19:43
>> To: Jan Kiszka <[email protected]>
>> Subject: RE: [EXT] Re: [PATCH V1 1/2] arm64: introduce smmu-v2 support
>>
>>
>>
>>> -----Original Message-----
>>> From: [email protected] <[email protected]>
>>> On Behalf Of Jan Kiszka
>>> Sent: 2020年5月8日 23:51
>>> To: Peng Fan <[email protected]>; Alice Guo <[email protected]>;
>>> [email protected]
>>> Subject: [EXT] Re: [PATCH V1 1/2] arm64: introduce smmu-v2 support
>>>
>>> Caution: EXT Email
>>>
>>> On 29.04.20 12:02, [email protected] wrote:
>>>> From: Alice Guo <[email protected]>
>>>>
>>>> Support smmu-v2 mmu500, add sid master support, only support stage2
>>>> translation.
>>>>
>>>> Signed-off-by: Alice Guo <[email protected]>
>>>> ---
>>>> hypervisor/arch/arm64/Kbuild | 1 +
>>>> hypervisor/arch/arm64/arm-smmu-regs.h | 220 ++++++
>>>> hypervisor/arch/arm64/smmu.c | 926
>>> ++++++++++++++++++++++++++
>>>> include/jailhouse/cell-config.h | 15 +
>>>> include/jailhouse/sizes.h | 47 ++
>>>> 5 files changed, 1209 insertions(+)
>>>> create mode 100644 hypervisor/arch/arm64/arm-smmu-regs.h
>>>> create mode 100644 hypervisor/arch/arm64/smmu.c
>>>> create mode 100644 include/jailhouse/sizes.h
>>>>
>>>> diff --git a/hypervisor/arch/arm64/Kbuild b/hypervisor/arch/arm64/Kbuild
>>>> index c34b0f32..e87c6e53 100644
>>>> --- a/hypervisor/arch/arm64/Kbuild
>>>> +++ b/hypervisor/arch/arm64/Kbuild
>>>> @@ -22,3 +22,4 @@ always := lib.a
>>>> lib-y := $(common-objs-y)
>>>> lib-y += entry.o setup.o control.o mmio.o paging.o caches.o traps.o
>>>> lib-y += iommu.o smmu-v3.o ti-pvu.o
>>>> +lib-y += smmu.o
>>>> diff --git a/hypervisor/arch/arm64/arm-smmu-regs.h
>>> b/hypervisor/arch/arm64/arm-smmu-regs.h
>>>> new file mode 100644
>>>> index 00000000..a1226e4a
>>>> --- /dev/null
>>>> +++ b/hypervisor/arch/arm64/arm-smmu-regs.h
>>>> @@ -0,0 +1,220 @@
>>>> +/*
>>>> + * IOMMU API for ARM architected SMMU implementations.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>> + * GNU General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License
>>>> + * along with this program; if not, write to the Free Software
>>>> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307,
>>> USA.
>>>> + *
>>>> + * Copyright (C) 2013 ARM Limited
>>>> + *
>>>> + * Author: Will Deacon <[email protected]>
>>>
>>> Is this file a 1:1 copy of the kernel header? Or is it also adopted?
>>>
>>
>> yes, copied from 4.14 kernel
>>
>>>> + */
>>>> +
>>>> +#ifndef _ARM_SMMU_REGS_H
>>>> +#define _ARM_SMMU_REGS_H
>>>> +
>>>> +/* Configuration registers */
>>>> +#define ARM_SMMU_GR0_sCR0 0x0
>>>> +#define sCR0_CLIENTPD (1 << 0)
>>>> +#define sCR0_GFRE (1 << 1)
>>>> +#define sCR0_GFIE (1 << 2)
>>>> +#define sCR0_EXIDENABLE (1 << 3)
>>>> +#define sCR0_GCFGFRE (1 << 4)
>>>> +#define sCR0_GCFGFIE (1 << 5)
>>>> +#define sCR0_USFCFG (1 << 10)
>>>> +#define sCR0_VMIDPNE (1 << 11)
>>>> +#define sCR0_PTM (1 << 12)
>>>> +#define sCR0_FB (1 << 13)
>>>> +#define sCR0_VMID16EN (1 << 31)
>>>> +#define sCR0_BSU_SHIFT 14
>>>> +#define sCR0_BSU_MASK 0x3
>>>> +
>>>> +/* Auxiliary Configuration register */
>>>> +#define ARM_SMMU_GR0_sACR 0x10
>>>> +
>>>> +/* Identification registers */
>>>> +#define ARM_SMMU_GR0_ID0 0x20
>>>> +#define ARM_SMMU_GR0_ID1 0x24
>>>> +#define ARM_SMMU_GR0_ID2 0x28
>>>> +#define ARM_SMMU_GR0_ID3 0x2c
>>>> +#define ARM_SMMU_GR0_ID4 0x30
>>>> +#define ARM_SMMU_GR0_ID5 0x34
>>>> +#define ARM_SMMU_GR0_ID6 0x38
>>>> +#define ARM_SMMU_GR0_ID7 0x3c
>>>> +#define ARM_SMMU_GR0_sGFSR 0x48
>>>> +#define ARM_SMMU_GR0_sGFSYNR0 0x50
>>>> +#define ARM_SMMU_GR0_sGFSYNR1 0x54
>>>> +#define ARM_SMMU_GR0_sGFSYNR2 0x58
>>>> +
>>>> +#define ID0_S1TS (1 << 30)
>>>> +#define ID0_S2TS (1 << 29)
>>>> +#define ID0_NTS (1 << 28)
>>>> +#define ID0_SMS (1 << 27)
>>>> +#define ID0_ATOSNS (1 << 26)
>>>> +#define ID0_PTFS_NO_AARCH32 (1 << 25)
>>>> +#define ID0_PTFS_NO_AARCH32S (1 << 24)
>>>> +#define ID0_CTTW (1 << 14)
>>>> +#define ID0_NUMIRPT_SHIFT 16
>>>> +#define ID0_NUMIRPT_MASK 0xff
>>>> +#define ID0_NUMSIDB_SHIFT 9
>>>> +#define ID0_NUMSIDB_MASK 0xf
>>>> +#define ID0_EXIDS (1 << 8)
>>>> +#define ID0_NUMSMRG_SHIFT 0
>>>> +#define ID0_NUMSMRG_MASK 0xff
>>>> +
>>>> +#define ID1_PAGESIZE (1 << 31)
>>>> +#define ID1_NUMPAGENDXB_SHIFT 28
>>>> +#define ID1_NUMPAGENDXB_MASK 7
>>>> +#define ID1_NUMS2CB_SHIFT 16
>>>> +#define ID1_NUMS2CB_MASK 0xff
>>>> +#define ID1_NUMCB_SHIFT 0
>>>> +#define ID1_NUMCB_MASK 0xff
>>>> +
>>>> +#define ID2_OAS_SHIFT 4
>>>> +#define ID2_OAS_MASK 0xf
>>>> +#define ID2_IAS_SHIFT 0
>>>> +#define ID2_IAS_MASK 0xf
>>>> +#define ID2_UBS_SHIFT 8
>>>> +#define ID2_UBS_MASK 0xf
>>>> +#define ID2_PTFS_4K (1 << 12)
>>>> +#define ID2_PTFS_16K (1 << 13)
>>>> +#define ID2_PTFS_64K (1 << 14)
>>>> +#define ID2_VMID16 (1 << 15)
>>>> +
>>>> +#define ID7_MAJOR_SHIFT 4
>>>> +#define ID7_MAJOR_MASK 0xf
>>>> +
>>>> +/* Global TLB invalidation */
>>>> +#define ARM_SMMU_GR0_TLBIVMID 0x64
>>>> +#define ARM_SMMU_GR0_TLBIALLNSNH 0x68
>>>> +#define ARM_SMMU_GR0_TLBIALLH 0x6c
>>>> +#define ARM_SMMU_GR0_sTLBGSYNC 0x70
>>>> +#define ARM_SMMU_GR0_sTLBGSTATUS 0x74
>>>> +#define sTLBGSTATUS_GSACTIVE (1 << 0)
>>>> +
>>>> +/* Stream mapping registers */
>>>> +#define ARM_SMMU_GR0_SMR(n) (0x800 + ((n) << 2))
>>>> +#define SMR_VALID (1 << 31)
>>>> +#define SMR_MASK_SHIFT 16
>>>> +#define SMR_ID_SHIFT 0
>>>> +
>>>> +#define ARM_SMMU_GR0_S2CR(n) (0xc00 + ((n) << 2))
>>>> +#define S2CR_CBNDX_SHIFT 0
>>>> +#define S2CR_CBNDX_MASK 0xff
>>>> +#define S2CR_EXIDVALID (1 << 10)
>>>> +#define S2CR_TYPE_SHIFT 16
>>>> +#define S2CR_TYPE_MASK 0x3
>>>> +enum arm_smmu_s2cr_type {
>>>> + S2CR_TYPE_TRANS,
>>>> + S2CR_TYPE_BYPASS,
>>>> + S2CR_TYPE_FAULT,
>>>> +};
>>>
>>> What is the advantage of modelling these as enum? And are we anywhere
>>> relying on their exact values for the width if those types? If so, then
>>> better use u* types and constant #defines.
>>>
>>
>> copied from 4.14 kernel. Should I rebased to 5.7? or any suggestions?
>>
Well, if it's just copied, my question would actually have to go to
Will. Let's keep it like Linux does for now.
>>>> +
>>>> +#define S2CR_PRIVCFG_SHIFT 24
>>>> +#define S2CR_PRIVCFG_MASK 0x3
>>>> +enum arm_smmu_s2cr_privcfg {
>>>> + S2CR_PRIVCFG_DEFAULT,
>>>> + S2CR_PRIVCFG_DIPAN,
>>>> + S2CR_PRIVCFG_UNPRIV,
>>>> + S2CR_PRIVCFG_PRIV,
>>>> +};
>>>
>>> Same as above.
>>>
>>>> +
>>>> +/* Context bank attribute registers */
>>>> +#define ARM_SMMU_GR1_CBAR(n) (0x0 + ((n) << 2))
>>>> +#define CBAR_VMID_SHIFT 0
>>>> +#define CBAR_VMID_MASK 0xff
>>>> +#define CBAR_S1_BPSHCFG_SHIFT 8
>>>> +#define CBAR_S1_BPSHCFG_MASK 3
>>>> +#define CBAR_S1_BPSHCFG_NSH 3
>>>> +#define CBAR_S1_MEMATTR_SHIFT 12
>>>> +#define CBAR_S1_MEMATTR_MASK 0xf
>>>> +#define CBAR_S1_MEMATTR_WB 0xf
>>>> +#define CBAR_TYPE_SHIFT 16
>>>> +#define CBAR_TYPE_MASK 0x3
>>>> +#define CBAR_TYPE_S2_TRANS (0 << CBAR_TYPE_SHIFT)
>>>> +#define CBAR_TYPE_S1_TRANS_S2_BYPASS (1 << CBAR_TYPE_SHIFT)
>>>> +#define CBAR_TYPE_S1_TRANS_S2_FAULT (2 << CBAR_TYPE_SHIFT)
>>>> +#define CBAR_TYPE_S1_TRANS_S2_TRANS (3 << CBAR_TYPE_SHIFT)
>>>> +#define CBAR_IRPTNDX_SHIFT 24
>>>> +#define CBAR_IRPTNDX_MASK 0xff
>>>> +
>>>> +#define ARM_SMMU_GR1_CBA2R(n) (0x800 + ((n) <<
>> 2))
>>>> +#define CBA2R_RW64_32BIT (0 << 0)
>>>> +#define CBA2R_RW64_64BIT (1 << 0)
>>>> +#define CBA2R_VMID_SHIFT 16
>>>> +#define CBA2R_VMID_MASK 0xffff
>>>> +
>>>> +#define ARM_SMMU_CB_SCTLR 0x0
>>>> +#define ARM_SMMU_CB_ACTLR 0x4
>>>> +#define ARM_SMMU_CB_RESUME 0x8
>>>> +#define ARM_SMMU_CB_TTBCR2 0x10
>>>> +#define ARM_SMMU_CB_TTBR0 0x20
>>>> +#define ARM_SMMU_CB_TTBR1 0x28
>>>> +#define ARM_SMMU_CB_TTBCR 0x30
>>>> +#define ARM_SMMU_CB_CONTEXTIDR 0x34
>>>> +#define ARM_SMMU_CB_S1_MAIR0 0x38
>>>> +#define ARM_SMMU_CB_S1_MAIR1 0x3c
>>>> +#define ARM_SMMU_CB_PAR 0x50
>>>> +#define ARM_SMMU_CB_FSR 0x58
>>>> +#define ARM_SMMU_CB_FAR 0x60
>>>> +#define ARM_SMMU_CB_FSYNR0 0x68
>>>> +#define ARM_SMMU_CB_S1_TLBIVA 0x600
>>>> +#define ARM_SMMU_CB_S1_TLBIASID 0x610
>>>> +#define ARM_SMMU_CB_S1_TLBIVAL 0x620
>>>> +#define ARM_SMMU_CB_S2_TLBIIPAS2 0x630
>>>> +#define ARM_SMMU_CB_S2_TLBIIPAS2L 0x638
>>>> +#define ARM_SMMU_CB_TLBSYNC 0x7f0
>>>> +#define ARM_SMMU_CB_TLBSTATUS 0x7f4
>>>> +#define ARM_SMMU_CB_ATS1PR 0x800
>>>> +#define ARM_SMMU_CB_ATSR 0x8f0
>>>> +
>>>> +#define SCTLR_S1_ASIDPNE (1 << 12)
>>>> +#define SCTLR_CFCFG (1 << 7)
>>>> +#define SCTLR_CFIE (1 << 6)
>>>> +#define SCTLR_CFRE (1 << 5)
>>>> +#define SCTLR_E (1 << 4)
>>>> +#define SCTLR_AFE (1 << 2)
>>>> +#define SCTLR_TRE (1 << 1)
>>>> +#define SCTLR_M (1 << 0)
>>>> +
>>>> +#define CB_PAR_F (1 << 0)
>>>> +
>>>> +#define ATSR_ACTIVE (1 << 0)
>>>> +
>>>> +#define RESUME_RETRY (0 << 0)
>>>> +#define RESUME_TERMINATE (1 << 0)
>>>> +
>>>> +#define TTBCR2_SEP_SHIFT 15
>>>> +#define TTBCR2_SEP_UPSTREAM (0x7 << TTBCR2_SEP_SHIFT)
>>>> +#define TTBCR2_AS (1 << 4)
>>>> +
>>>> +#define TTBRn_ASID_SHIFT 48
>>>> +
>>>> +#define FSR_MULTI (1 << 31)
>>>> +#define FSR_SS (1 << 30)
>>>> +#define FSR_UUT (1 << 8)
>>>> +#define FSR_ASF (1 << 7)
>>>> +#define FSR_TLBLKF (1 << 6)
>>>> +#define FSR_TLBMCF (1 << 5)
>>>> +#define FSR_EF (1 << 4)
>>>> +#define FSR_PF (1 << 3)
>>>> +#define FSR_AFF (1 << 2)
>>>> +#define FSR_TF (1 << 1)
>>>> +
>>>> +#define FSR_IGN (FSR_AFF | FSR_ASF
>> | \
>>>> + FSR_TLBMCF | FSR_TLBLKF)
>>>> +#define FSR_FAULT (FSR_MULTI | FSR_SS |
>> FSR_UUT |
>>> \
>>>> + FSR_EF | FSR_PF | FSR_TF |
>>> FSR_IGN)
>>>> +
>>>> +#define FSYNR0_WNR (1 << 4)
>>>> +
>>>> +#endif /* _ARM_SMMU_REGS_H */
>>>> diff --git a/hypervisor/arch/arm64/smmu.c
>>> b/hypervisor/arch/arm64/smmu.c
>>>> new file mode 100644
>>>> index 00000000..ea1b4c1e
>>>> --- /dev/null
>>>> +++ b/hypervisor/arch/arm64/smmu.c
>>>> @@ -0,0 +1,926 @@
>>>> +/*
>>>> + * Jailhouse, a Linux-based partitioning hypervisor
>>>> + *
>>>> + * Copyright 2018-2020 NXP
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>>>> + * the COPYING file in the top-level directory.
>>>> + *
>>>> + * Modified from Linux smmu.c
>>>> + */
>>>> +
>>>> +#include <jailhouse/control.h>
>>>> +#include <jailhouse/ivshmem.h>
>>>> +#include <jailhouse/mmio.h>
>>>> +#include <jailhouse/paging.h>
>>>> +#include <jailhouse/pci.h>
>>>> +#include <jailhouse/printk.h>
>>>> +#include <jailhouse/sizes.h>
>>>> +#include <jailhouse/string.h>
>>>> +#include <jailhouse/unit.h>
>>>> +#include <asm/spinlock.h>
>>>> +#include <jailhouse/cell-config.h>
>>>> +
>>>> +#include "arm-smmu-regs.h"
>>>
>>> Are those regs common to anyone? SMMUv3? If not, better fold in. If
>>> there is something to share, then move to include/asm/,
>>>
>>
>> The regs are used by current smmu-v2 driver. not smmuv3. I could check
>> whether smmuv3 could reuse, but I guess no, because the hardware design is
>> different.
Then just move the required defines from arm-smmu-regs.h into this c
file. Just like smmu-v3.c does as well.
>>
>>>> +
>>>> +#define ARM_32_LPAE_TCR_EAE (1 << 31)
>>>> +#define ARM_64_LPAE_S2_TCR_RES1 (1 << 31)
>>>> +
>>>> +#define ARM_LPAE_TCR_EPD1 (1 << 23)
>>>> +
>>>> +#define ARM_LPAE_TCR_TG0_4K (0 << 14)
>>>> +#define ARM_LPAE_TCR_TG0_64K (1 << 14)
>>>> +#define ARM_LPAE_TCR_TG0_16K (2 << 14)
>>>> +
>>>> +#define ARM_LPAE_TCR_SH0_SHIFT 12
>>>> +#define ARM_LPAE_TCR_SH0_MASK 0x3
>>>> +#define ARM_LPAE_TCR_SH_NS 0
>>>> +#define ARM_LPAE_TCR_SH_OS 2
>>>> +#define ARM_LPAE_TCR_SH_IS 3
>>>> +
>>>> +#define ARM_LPAE_TCR_ORGN0_SHIFT 10
>>>> +#define ARM_LPAE_TCR_IRGN0_SHIFT 8
>>>> +#define ARM_LPAE_TCR_RGN_MASK 0x3
>>>> +#define ARM_LPAE_TCR_RGN_NC 0
>>>> +#define ARM_LPAE_TCR_RGN_WBWA 1
>>>> +#define ARM_LPAE_TCR_RGN_WT 2
>>>> +#define ARM_LPAE_TCR_RGN_WB 3
>>>> +
>>>> +#define ARM_LPAE_TCR_SL0_SHIFT 6
>>>> +#define ARM_LPAE_TCR_SL0_MASK 0x3
>>>> +#define ARM_LPAE_TCR_SL0_LVL_2 0
>>>> +#define ARM_LPAE_TCR_SL0_LVL_1 1
>>>> +
>>>> +#define ARM_LPAE_TCR_T0SZ_SHIFT 0
>>>> +#define ARM_LPAE_TCR_SZ_MASK 0xf
>>>> +
>>>> +#define ARM_LPAE_TCR_PS_SHIFT 16
>>>> +#define ARM_LPAE_TCR_PS_MASK 0x7
>>>> +
>>>> +#define ARM_LPAE_TCR_IPS_SHIFT 32
>>>> +#define ARM_LPAE_TCR_IPS_MASK 0x7
>>>> +
>>>> +#define ARM_LPAE_TCR_PS_32_BIT 0x0ULL
>>>> +#define ARM_LPAE_TCR_PS_36_BIT 0x1ULL
>>>> +#define ARM_LPAE_TCR_PS_40_BIT 0x2ULL
>>>> +#define ARM_LPAE_TCR_PS_42_BIT 0x3ULL
>>>> +#define ARM_LPAE_TCR_PS_44_BIT 0x4ULL
>>>> +#define ARM_LPAE_TCR_PS_48_BIT 0x5ULL
>>>> +#define ARM_LPAE_TCR_PS_52_BIT 0x6ULL
>>>> +#define ARM_MMU500_ACTLR_CPRE (1 << 1)
>>>> +
>>>> +#define ARM_MMU500_ACR_CACHE_LOCK (1 << 26)
>>>> +#define ARM_MMU500_ACR_S2CRB_TLBEN (1 << 10)
>>>> +#define ARM_MMU500_ACR_SMTNMB_TLBEN (1 << 8)
>>>> +
>>>> +#define TLB_LOOP_TIMEOUT 1000000 /* 1s! */
>>>> +#define TLB_SPIN_COUNT 10
>>>
>>> How do those numbers map to real-time? Are they truly CPU(-frequency)
>>> independent?
>>>
>>
>> The numbers are copied from Linux kernel. Actually the function using the
>> macros are only called by arm_smmu_init in root cell initialization stage,
>> so i
>> suppose it not affect runtime realtime.
With "real-time" I mean that the actual runtime is. Look, upstream has a
udelay in the loop. You don't have anything. So the comment "1s!" is
very likley wrong, and it is the question how to ensure that we wait
sufficiently long on faster CPUs.
>>
>>>> +
>>>> +/* Maximum number of context banks per SMMU */
>>>> +#define ARM_SMMU_MAX_CBS 128
>>>> +
>>>> +/* SMMU global address space */
>>>> +#define ARM_SMMU_GR0(smmu) ((smmu)->base)
>>>> +#define ARM_SMMU_GR1(smmu) ((smmu)->base + (1 <<
>>> (smmu)->pgshift))
>>>> +
>>>> +/*
>>>> + * SMMU global address space with conditional offset to access secure
>>>> + * aliases of non-secure registers (e.g. nsCR0: 0x400, nsGFSR: 0x448,
>>>> + * nsGFSYNR0: 0x450)
>>>> + */
>>>> +#define ARM_SMMU_GR0_NS(smmu)
>>> \
>>>> + ((smmu)->base +
>>> \
>>>> + ((smmu->options &
>> ARM_SMMU_OPT_SECURE_CFG_ACCESS)
>>> \
>>>> + ? 0x400 : 0))
>>>> +
>>>> +/* Translation context bank */
>>>> +#define ARM_SMMU_CB(smmu, n) ((smmu)->cb_base + ((n) <<
>>> (smmu)->pgshift))
>>>> +
>>>> +#define MSI_IOVA_BASE 0x8000000
>>>> +#define MSI_IOVA_LENGTH 0x100000
>>>> +
>>>> +struct arm_smmu_s2cr {
>>>> + enum arm_smmu_s2cr_type type;
>>>> + enum arm_smmu_s2cr_privcfg privcfg;
>>>> + u8 cbndx;
>>>> +};
>>>> +
>>>> +struct arm_smmu_smr {
>>>> + u16 mask;
>>>> + u16 id;
>>>> + bool valid;
>>>> +};
>>>> +
>>>> +struct arm_smmu_cb {
>>>> + u64 ttbr[2];
>>>> + u32 tcr[2];
>>>> + u32 mair[2];
>>>> + struct arm_smmu_cfg *cfg;
>>>> +};
>>>
>>> Is any of those data structures shared with the hardware? Or are they
>>> just for internal book-keeping? I'm asking because of aligments, packing
>>> etc. Applies to almost all structs you define.
>>>
>>
>> adopted from linux kernel. For internal booking. Should I keep linux coding
>> style
>> when I adopt linux code, or adapt to jailhouse one?
This was not a style question but a was checking the correctness of the
structure. Style is ok here also for Jailhouse.
>>
>>>> +
>>>> +struct arm_smmu_device {
>>>> + void *base;
>>>> + void *cb_base;
>>>> + u32 num_masters;
>>>> + unsigned long pgshift;
>>>> +
>>>> +#define ARM_SMMU_FEAT_COHERENT_WALK (1 << 0)
>>>> +#define ARM_SMMU_FEAT_STREAM_MATCH (1 << 1)
>>>> +#define ARM_SMMU_FEAT_TRANS_S1 (1 << 2)
>>>> +#define ARM_SMMU_FEAT_TRANS_S2 (1 << 3)
>>>> +#define ARM_SMMU_FEAT_TRANS_NESTED (1 << 4)
>>>> +#define ARM_SMMU_FEAT_TRANS_OPS (1 << 5)
>>>> +#define ARM_SMMU_FEAT_VMID16 (1 << 6)
>>>> +#define ARM_SMMU_FEAT_FMT_AARCH64_4K (1 << 7)
>>>> +#define ARM_SMMU_FEAT_FMT_AARCH64_16K (1 << 8)
>>>> +#define ARM_SMMU_FEAT_FMT_AARCH64_64K (1 << 9)
>>>> +#define ARM_SMMU_FEAT_FMT_AARCH32_L (1 << 10)
>>>> +#define ARM_SMMU_FEAT_FMT_AARCH32_S (1 << 11)
>>>> +#define ARM_SMMU_FEAT_EXIDS (1 << 12)
>>>
>>> I would prefer those defines outside of the struct. Also below.
>>>
>>>> + u32 features;
>>>> +
>>>> +#define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
>>>> + u32 options;
>>>> + enum arm_smmu_arch_version version;
>>>> + enum arm_smmu_implementation model;
>>>> +
>>>> + u32 num_context_banks;
>>>> + u32 num_s2_context_banks;
>>>> + struct arm_smmu_cb *cbs;
>>>> +
>>>> + u32 num_mapping_groups;
>>>> + u16 streamid_mask;
>>>> + u16 smr_mask_mask;
>>>> + struct arm_smmu_smr *smrs;
>>>> + struct arm_smmu_s2cr *s2crs;
>>>> + struct arm_smmu_cfg *cfgs;
>>>> +
>>>> + unsigned long va_size;
>>>> + unsigned long ipa_size;
>>>> + unsigned long pa_size;
>>>> + unsigned long pgsize_bitmap;
>>>> +
>>>> + u32 num_global_irqs;
>>>> + u32 num_context_irqs;
>>>> + unsigned int *irqs;
>>>> +
>>>> + spinlock_t global_sync_lock;
>>>> +};
>>>> +
>>>> +enum arm_smmu_context_fmt {
>>>> + ARM_SMMU_CTX_FMT_NONE,
>>>> + ARM_SMMU_CTX_FMT_AARCH64,
>>>> + ARM_SMMU_CTX_FMT_AARCH32_L,
>>>> + ARM_SMMU_CTX_FMT_AARCH32_S,
>>>> +};
>>>> +
>>>> +struct arm_smmu_cfg {
>>>> + u8 cbndx;
>>>> + u8 irptndx;
>>>> + union {
>>>> + u16 asid;
>>>> + u16 vmid;
>>>> + };
>>>> + u32 cbar;
>>>> + enum arm_smmu_context_fmt fmt;
>>>> +};
>>>> +#define INVALID_IRPTNDX 0xff
>>>> +
>>>> +enum arm_smmu_domain_stage {
>>>> + ARM_SMMU_DOMAIN_S1 = 0,
>>>> + ARM_SMMU_DOMAIN_S2,
>>>> + ARM_SMMU_DOMAIN_NESTED,
>>>> + ARM_SMMU_DOMAIN_BYPASS,
>>>> +};
>>>
>>> This enum is even not uses for typechecking. So, simple #defines suffice.
>>>
>>>> +
>>>> +#define s2cr_init_val (struct arm_smmu_s2cr){ \
>>>> + .type = S2CR_TYPE_BYPASS, \
>>>> +}
>>>> +
>>>> +
>>>> +static struct arm_smmu_device smmu_device;
>>>> +static unsigned long pgsize_bitmap = -1;
>>>> +static u16 arm_sid_mask;
>>>
>>> Why a global arm_sid_mask when it is configured per SMMU instance?
>>>
>>
>> I need to rethink about this case, I only took one smmu into consideration
>> when
>> doing i.MX8QM.
If there can be more, we should at least plan for it, specifically when
that is simple enough. And if you assume n == 1 for now, at least leave
a comment.
>>
>>>> +
>>>> +static void arm_smmu_write_smr(struct arm_smmu_device *smmu, int
>> idx)
>>>> +{
>>>> + struct arm_smmu_smr *smr = smmu->smrs + idx;
>>>> + u32 reg = smr->id << SMR_ID_SHIFT | smr->mask <<
>>> SMR_MASK_SHIFT;
>>>> +
>>>> + if (!(smmu->features & ARM_SMMU_FEAT_EXIDS) && smr->valid)
>>>> + reg |= SMR_VALID;
>>>> + mmio_write32(ARM_SMMU_GR0(smmu) +
>>> ARM_SMMU_GR0_SMR(idx), reg);
>>>> +}
>>>> +
>>>> +static void arm_smmu_write_s2cr(struct arm_smmu_device *smmu, int
>> idx)
>>>> +{
>>>> + struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
>>>> + u32 reg = (s2cr->type & S2CR_TYPE_MASK) << S2CR_TYPE_SHIFT |
>>>> + (s2cr->cbndx & S2CR_CBNDX_MASK) <<
>>> S2CR_CBNDX_SHIFT |
>>>> + (s2cr->privcfg & S2CR_PRIVCFG_MASK) <<
>>> S2CR_PRIVCFG_SHIFT;
>>>
>>> We already have the GET_FIELD macro. Maybe some SET_FIELD would be
>>> useful. It would allow to encode the fields in-place, without #defines.
>>>
>>
>> ok, I will adopt jailhouse coding style.
>>
>>>> +
>>>> + if (smmu->features & ARM_SMMU_FEAT_EXIDS && smmu->smrs
>> &&
>>>> + smmu->smrs[idx].valid)
>>>> + reg |= S2CR_EXIDVALID;
>>>> + mmio_write32(ARM_SMMU_GR0(smmu) +
>>> ARM_SMMU_GR0_S2CR(idx), reg);
>>>> +}
>>>> +
>>>> +static void arm_smmu_write_sme(struct arm_smmu_device *smmu, int
>> idx)
>>>> +{
>>>> + if (smmu->smrs)
>>>> + arm_smmu_write_smr(smmu, idx);
>>>> +
>>>> + arm_smmu_write_s2cr(smmu, idx);
>>>> +}
>>>> +
>>>> +/* Wait for any pending TLB invalidations to complete */
>>>> +static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu,
>>>> + void *sync, void *status)
>>>> +{
>>>> + unsigned int spin_cnt, delay;
>>>> +
>>>> + mmio_write32(sync, 0);
>>>> + for (delay = 1; delay < TLB_LOOP_TIMEOUT; delay *= 2) {
>>>> + for (spin_cnt = TLB_SPIN_COUNT; spin_cnt > 0; spin_cnt--) {
>>>> + if (!(mmio_read32(status) &
>>> sTLBGSTATUS_GSACTIVE))
>>>> + return;
>>>> + cpu_relax();
>>>> + }
>>>> + }
>>>> + printk("TLB sync timed out -- SMMU may be deadlocked\n");
>>>
>>> Hmm, end then? Contiue with eyes shut? Shouldn't this error be
>>> propagated? Seems doable, at least for the cases I checked.
>>>
>>
>> 4.14 kernel does this. I need check 5.7 kernel whether there have improvment.
Even if not: No reason for us to ignore errors.
Jan
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/jailhouse-dev/517307ea-5500-de76-53e1-eafb21118417%40web.de.
