Hi,
On 21/10/2022 23:07, Xenia Ragiadakou wrote:
On 10/21/22 18:31, Ayan Kumar Halder wrote:
Hi Ayan
Refer Arm IHI 0069H ID020922,
The upper 32 bits of GICR_TYPER represent the affinity
whereas the lower 32 bits represent the other bits (eg processor
number, etc).
MPIDR_AFFINITY_LEVEL() returns a 32 bit number on aarch32. Thus, this
is appended to return GICR_TYPER register.
The last sentence doesn't seem to match your modification below.
Signed-off-by: Ayan Kumar Halder <ayank...@amd.com>
---
xen/arch/arm/vgic-v3.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index c31140eb20..d86b41a39f 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -190,14 +190,18 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct
vcpu *v, mmio_info_t *info,
case VREG64(GICR_TYPER):
{
- uint64_t typer, aff;
+ uint64_t typer;
+ uint32_t aff;
if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
- aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 |
- MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 |
- MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
- MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
+ aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 24 |
+ MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 16 |
+ MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 8 |
+ MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0));
typer = aff;
+
+ typer = typer << 32;
I find the "typer = aff; typer = typer << 32" quite confusing to read.
In fact, my first instinct would be to combine the two but this would do
the wrong thing. So I would prefer if we use a different approach (see
below).
+
/* We use the VCPU ID as the redistributor ID in bits[23:8] */
typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT;
I don't see an issue I just want to propose alternatives that I think
would reduce the changes, hopefully without breaking it.
So, other ways would be either to assign v->arch.vmpidr to a new
variable uint64_t vmpidr and operate on this (without changing the
shifts), or to leave the type of aff uint64_t, adjust the shifts and do
typer = aff << 32.
How about making MPIDR_AFFINITY_LEVEL returning a 64-bit value? The
other option would be to use what Xenia's last option. I.e:
" to level the type of aff uint64_t, adjust the shifts and do typer =
aff << 32". This would need a suitable comment though explain why the
shift can't be fold.
Cheers,
--
Julien Grall