Re: [PATCH v2 12/36] irqchip/gic-v4.1: Implement the v4.1 flavour of VMAPP

[Marc Zyngier <maz@kernel.org>]

Hi Zenghui,

On 2019-11-13 09:11, Zenghui Yu wrote:
> Hi Marc,
>
> On 2019/10/27 22:42, Marc Zyngier wrote:
>> The ITS VMAPP command gains some new fields with GICv4.1:
>> - a default doorbell, which allows a single doorbell to be used for
>>    all the VLPIs routed to a given VPE
>> - a pointer to the configuration table (instead of having it in a 
>> register
>>    that gets context switched)
>> - a flag indicating whether this is the first map or the last unmap 
>> for
>>    this particulat VPE
>> - a flag indicating whether the pending table is known to be zeroed, 
>> or not
>> Plumb in the new fields in the VMAPP builder, and add the map/unmap
>> refcounting so that the ITS can do the right thing.
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>
> [...]
>
>> @@ -605,19 +626,45 @@ static struct its_vpe 
>> *its_build_vmapp_cmd(struct its_node *its,
>>   					   struct its_cmd_block *cmd,
>>   					   struct its_cmd_desc *desc)
>>   {
>> -	unsigned long vpt_addr;
>> +	unsigned long vpt_addr, vconf_addr;
>>   	u64 target;
>> -
>> -	vpt_addr = 
>> virt_to_phys(page_address(desc->its_vmapp_cmd.vpe->vpt_page));
>> -	target = desc->its_vmapp_cmd.col->target_address + 
>> its->vlpi_redist_offset;
>> +	bool alloc;
>>
>>   	its_encode_cmd(cmd, GITS_CMD_VMAPP);
>>   	its_encode_vpeid(cmd, desc->its_vmapp_cmd.vpe->vpe_id);
>>   	its_encode_valid(cmd, desc->its_vmapp_cmd.valid);
>> +
>> +	if (!desc->its_vmapp_cmd.valid) {
>> +		if (is_v4_1(its)) {
>> +			alloc = 
>> !atomic_dec_return(&desc->its_vmapp_cmd.vpe->vmapp_count);
>> +			its_encode_alloc(cmd, alloc);
>> +		}
>> +
>> +		goto out;
>> +	}
>> +
>> +	vpt_addr = 
>> virt_to_phys(page_address(desc->its_vmapp_cmd.vpe->vpt_page));
>> +	target = desc->its_vmapp_cmd.col->target_address + 
>> its->vlpi_redist_offset;
>> +
>>   	its_encode_target(cmd, target);
>>   	its_encode_vpt_addr(cmd, vpt_addr);
>>   	its_encode_vpt_size(cmd, LPI_NRBITS - 1);
>>   +	if (!is_v4_1(its))
>> +		goto out;
>> +
>> +	vconf_addr = 
>> virt_to_phys(page_address(desc->its_vmapp_cmd.vpe->its_vm->vprop_page));
>> +
>> +	alloc = 
>> atomic_inc_and_test(&desc->its_vmapp_cmd.vpe->vmapp_count);
>
> As the comment block on top of atomic_inc_and_test(atomic *v) says,
>
>  * Atomically increments @v by 1
>  * and returns true if the result is zero, or false for all
>  * other cases.
>  */
>
> We will always get the 'alloc' as false here, even if this is the
> first mapping of this vPE.  This is not as expected, I think.

As usual, a very good observation!

Indeed, I cocked up the logic here, as we need to test the value before
the increment (and not after). What we want is probably something like:

   alloc = !atomic_fetch_inc(&desc->its_vmapp_cmd.vpe->vmapp_count);

> And on the other hand, I wonder what is the reason for 'vmapp_count'
> to be atomic_t?

The rational is that we could end-up with multiple VMAPP commands 
emitted
in parallel, for example. That's probably not strictly necessary right 
now,
but I'm trying to be cautious.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...