Re: [PATCH V2 4/8] xen/arm: Use the new mapping relations between vCPUID and vMPIDR

[Julien Grall <julien.grall@citrix.com>]

Hi Chen,

On 25/05/2015 04:34, Chen Baozi wrote:
> On Sun, May 24, 2015 at 01:51:21PM +0100, Julien Grall wrote:
>>> @@ -80,9 +72,7 @@ static struct vcpu *vgic_v3_get_target_vcpu(struct vcpu *v, unsigned int irq)
>>>
>>>       ASSERT(spin_is_locked(&rank->lock));
>>>
>>> -    target = rank->v3.irouter[irq % 32];
>>> -    target &= ~(GICD_IROUTER_SPI_MODE_ANY);
>>> -    target &= MPIDR_AFF0_MASK;
>>> +    target = vaffinity_to_vcpuid(rank->v3.irouter[irq % 32]);
>>
>> When irouter.IRM  = 1 (i.e any processor can be used for SPIs), the affinity
>> may be unknown.
>>
>> Although, when this register is saved we make sure to have AFF0 and AFF1 set
>> to 0.
>>
>> This change, as the current wasn't clear about it. I would be tempt to add a
>> specific case for irouter.IRM = 1. But I don't mind if you only add a
>> comment.
>
> If we add a specific case for iroute.IRM == 1, then which vcpu it will return?

You can choose any vCPU. Although, vCPU0 is the best one because it is 
always allocated and running.

> Will it better to check this bit before calling this function?

No, this function is called in generic code to get the VCPU target for a 
given interrupt.

>>
>>>       ASSERT(target >= 0 && target < v->domain->max_vcpus);
>>>
>>>       return v->domain->vcpu[target];
>>> @@ -751,7 +741,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>>>               vgic_unlock_rank(v, rank, flags);
>>>               return 1;
>>>           }
>>> -        vcpu_id = irouter;
>>> +        vcpu_id = vaffinity_to_vcpuid(irouter);
>>>           *r = vgic_v3_vcpu_to_irouter(v, vcpu_id);
>>
>> The current code is very pointless, irouter contains the value to return.
>> vgic_v3_vcpu_to_irouter is just an identity function.
>>
>> The read emulation for IROUTER can be simplify a lot to only returns the
>> value irouter which is already valid.
>>
>> I can send a patch to apply before your series to clean up this IROUTER
>> code. I would make unnecessary some of your changes.
>
> That will be fine.

I will give a try to send one by tomorrow.

>>
>>>           vgic_unlock_rank(v, rank, flags);
>>>           return 1;
>>> @@ -841,6 +831,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>>>       uint64_t new_irouter, new_target, old_target;
>>>       struct vcpu *old_vcpu, *new_vcpu;
>>>       int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
>>> +    uint32_t vcpu_id;
>>>
>>>       perfc_incr(vgicd_writes);
>>>
>>> @@ -925,8 +916,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>>>           }
>>>           else
>>>           {
>>> -            new_target = new_irouter & MPIDR_AFF0_MASK;
>>> -            if ( new_target >= v->domain->max_vcpus )
>>> +            new_target = new_irouter & MPIDR_HWID_MASK;
>>> +            vcpu_id = vaffinity_to_vcpuid(new_irouter);
>>> +            if ( vcpu_id >= v->domain->max_vcpus )
>>>               {
>>>                   printk(XENLOG_G_DEBUG
>>>                          "%pv: vGICD: wrong irouter at offset %#08x\n val 0x%lx vcpu %x",
>>> @@ -934,7 +926,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>>>                   vgic_unlock_rank(v, rank, flags);
>>>                   return 0;
>>>               }
>>> -            new_vcpu = vgic_v3_irouter_to_vcpu(v, new_irouter);
>>
>> I would prefer to keep vgic_v3_irouter_to_vcpu and return NULL if the VCPU
>> ID is too high.
>>
>> The emulation code would be:
>>
>>       new_vcpu = vgic_v3_irouter_to_vcpu(v, new_irouter);
>>       if ( !new_vcpu )
>>       {
>>          printk(.....);
>>       }
>>
>> Although the current emulation is wrong, if the guest is passing a wrong
>> MPIDR, we should just ignore the setting and let the interrupt going
>> pending. Anyway, I think it would require more work in Xen so I'm okay with
>> the current behavior.
>>
>>> +            new_vcpu = v->domain->vcpu[vcpu_id];
>>>           }
>>>
>>>           rank->v3.irouter[REG_RANK_INDEX(64, (gicd_reg - GICD_IROUTER),
>>> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
>>> index 5d899be..1c1e7de 100644
>>> --- a/xen/arch/arm/vpsci.c
>>> +++ b/xen/arch/arm/vpsci.c
>>> @@ -33,7 +33,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
>>>       register_t vcpuid;
>>>
>>>       if( ver == XEN_PSCI_V_0_2 )
>>> -        vcpuid = (target_cpu & MPIDR_HWID_MASK);
>>> +        vcpuid = vaffinity_to_vcpuid(target_cpu);
>>>       else
>>>           vcpuid = target_cpu;
>>
>> AFAICT in PSCI 0.1, target_cpu is a CPUID which is a MPIDR-like value. If
>> so, I think we may need to call vaffinity_to_vcpuid.
>>
>> But, I wasn't able to confirm with the spec. I guessed it from the Linux
>> usage. Maybe there is limit of number of CPU used with PSCI 0.1?
>
> I didn't read the spec, just change the code according the current
> '& MPIDR_HWID_MASK' code.

I wasn't able to find any difference between MPDIR vs CPUID in the spec.

(CC Parth the author of the code).

Regards,


-- 
Julien Grall