On 16/12/2022 10:23, Julien Grall wrote:
Hi,
Hi Julien,
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?
For PA_32, the following bitoperation will overflow :-
*ipa |= (paddr_t)(pte.supersec.extbase1) <<
L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
*ipa |= (paddr_t)(pte.supersec.extbase2) <<
L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
Also, pte.supersec.extbase1 and pte.supersec.extbase2 are not valid for
PA_32. Refer xen/arch/arm/include/asm/short-desc.h
unsigned int extbase2:4; /* Extended base address, PA[39:36] */
unsigned int extbase1:4; /* Extended base address, PA[35:32] */
* Can you give a pointer to the Arm Arm that says supersection is not
supported?
I could not find any sentence in Arm Arm which says supersection is
**not** supported on 32 bit PA.
However,
Refer"ARM DDI 0487I.a ID081822", G5.4 "The VMSAv8-32 Short-descriptor
translation table format", G5-9163
"Support for Supersections is **optional**, except that an
implementation that supports more than 32 bits of PA must also support
Supersections to provide access to the entire PA space."
Also, G5.1.3 "Address spaces in VMSAv8-32", G5-9149
"AArch32 defines two translation table formats. The Long-descriptor
format gives access to the full 40-bit IPA or PA space at a granularity
of 4KB. The Short-descriptor format:
Gives access to a 32-bit PA space at 4KB granularity.
Gives access to a 40-bit PA space, but only at 16MB granularity, by
the use of Supersections."
from the above 2 snippets, I inferred that supersections are only
supported for PAs greater than 32 bits.
I could not find any evidence of supersections supported for 32 bit PA.
- Ayan
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,
