[PATCH v10 15/17] KVM: arm64: implement ITS command queue command handlers

[André Przywara]

Hi Marc,

On 16/07/16 11:08, Marc Zyngier wrote:
> On Fri, 15 Jul 2016 12:43:36 +0100
>> +/*
>> + * Check whether a device ID can be stored into the guest device tables.
>> + * For a direct table this is pretty easy, but gets a bit nasty for
>> + * indirect tables. We check whether the resulting guest physical address
>> + * is actually valid (covered by a memslot and guest accessbible).
>> + * For this we have to read the respective first level entry.
>> + */
>> +static bool vgic_its_check_device_id(struct kvm *kvm, struct vgic_its *its,
>> +				     int device_id)
>> +{
>> +	u64 r = its->baser_device_table;
>> +	int nr_entries = GITS_BASER_NR_PAGES(r) * SZ_64K;
>> +	int index;
>> +	u64 indirect_ptr;
>> +	gfn_t gfn;
>> +
>> +
>> +	if (!(r & GITS_BASER_INDIRECT))
>> +		return device_id < (nr_entries / GITS_BASER_ENTRY_SIZE(r));
>> +
>> +	/* calculate and check the index into the 1st level */
>> +	index = device_id / (SZ_64K / GITS_BASER_ENTRY_SIZE(r));
>> +	if (index >= (nr_entries / sizeof(u64)))
>> +		return false;
>> +
>> +	/* Each 1st level entry is represented by a 64-bit value. */
>> +	if (!kvm_read_guest(kvm,
>> +			    BASER_ADDRESS(r) + index *
>> sizeof(indirect_ptr),
>> +			    &indirect_ptr, sizeof(indirect_ptr)))
>> +		return false;
> 
> Careful. The ITS tables are written in LE format, so you need a
> 
> 	indirect_ptr = le64_to_cpu(indirect_ptr);
> 
> to cater for the LE-on-BE case.

Oh right, endianness. Thanks for pointing that out!

>> +
>> +	/* check the valid bit of the first level entry */
>> +	if (!(indirect_ptr & BIT_ULL(63)))
>> +		return false;
>> +
>> +	/*
>> +	 * Mask the guest physical address and calculate the frame number.
>> +	 * Any address beyond our supported 48 bits of PA will be caught
>> +	 * by the actual check in the final step.
>> +	 */
>> +	gfn = (indirect_ptr & GENMASK_ULL(51, 16)) >> PAGE_SHIFT;
> 
> Another gotcha: Here, you're only checking for the CPU page that covers
> the beginning of the ITS page. If the CPU page size is smaller, you may
> end-up being out of bounds. You need something like:
> 
> 	l2_index = device_id % (SZ_64K / GITS_BASER_ENTRY_SIZE(r));
> 	gfn = ((indirect_ptr & GENMASK_ULL(51, 16)) +
> 	       l2_index * GITS_BASER_ENTRY_SIZE(r)) >> PAGE_SHIFT;
> 
> so that you check the presence of the actual location you're virtually
> touching.
>

Right, that nasty case came to me as well after sending the patches.
I had something like "gfn += ...." plus a comment in mind, but that's
purely cosmetical.

So do you want me to send out another version with those fixes (and
possibly the usage of vgic_get_irq_kref() above)?

Are there more comments?

Cheers,
Andre.