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 <ayankuma@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