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@xxxxxxxxxx>
[...]
@@ -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...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
