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

Marc Zyngier
2019-11-13T09:47:10Z

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...