Hi,
Each adaptations are distinct, so I would prefer if they are in in
separate patch.
This will also make clear which components you touched because I would
be surprised if this is really the only place where we need adaptation.
Maybe that's because you didn't compile everything (which is fine).
On 15/12/2022 19:32, Ayan Kumar Halder wrote:
1. Supersections are supported only for paddr greater than 32 bits.
Two questions:
* Can you outline why we can't keep the code around?
* Can you give a pointer to the Arm Arm that says supersection is not
supported?
2. Use 0 wherever physical addresses are right shifted for 32 bit > 3. Use
PRIx64 to print u64
It would be good to explain that the current use was buggy because we
are printing a PTE and not a physical address.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.hal...@amd.com>
---
xen/arch/arm/guest_walk.c | 2 ++
xen/arch/arm/mm.c | 2 +-
xen/drivers/passthrough/arm/smmu.c | 5 +++++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 43d3215304..4384068285 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -149,6 +149,7 @@ static bool guest_walk_sd(const struct vcpu *v,
mask = (1ULL << L1DESC_SECTION_SHIFT) - 1;
*ipa = ((paddr_t)pte.sec.base << L1DESC_SECTION_SHIFT) | (gva &
mask);
}
+#ifndef CONFIG_ARM_PA_32
A "malicious" guest can still set that bit. So now, you will return from
guest_walk_sd() with 'ipa' not set at all.
If this is effectively not supported, then we should return 'false'
rather than claiming the translation was successful.
else /* Supersection */
{
mask = (1ULL << L1DESC_SUPERSECTION_SHIFT) - 1;
@@ -157,6 +158,7 @@ static bool guest_walk_sd(const struct vcpu *v,
*ipa |= (paddr_t)(pte.supersec.extbase1) <<
L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
*ipa |= (paddr_t)(pte.supersec.extbase2) <<
L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
}
+#endif
/* Set permissions so that the caller can check the flags by herself. */
if ( !pte.sec.ro )
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index be939fb106..3bc9894008 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -229,7 +229,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
pte = mapping[offsets[level]];
- printk("%s[0x%03x] = 0x%"PRIpaddr"\n",
+ printk("%s[0x%03x] = 0x%"PRIx64"\n",
level_strs[level], offsets[level], pte.bits);
if ( level == 3 || !pte.walk.valid || !pte.walk.table )
diff --git a/xen/drivers/passthrough/arm/smmu.c
b/xen/drivers/passthrough/arm/smmu.c
index 5ae180a4cc..522a478ccf 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1184,7 +1184,12 @@ static void arm_smmu_init_context_bank(struct
arm_smmu_domain *smmu_domain)
reg = (p2maddr & ((1ULL << 32) - 1));
writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
+
+#ifndef CONFIG_ARM_PA_32
reg = (p2maddr >> 32);
+#else
+ reg = 0;
+#endif
I think it would be better if we implement writeq(). This will also
avoid the now strange ((1ULL << 32) - 1) when p2maddr is a paddr_t.
if (stage1)
reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
Cheers,
--
Julien Grall
