Re: [PATCH v4 01/11] KVM: arm/arm64: vgic-its: fix return value for device table restore

[Auger Eric <eric.auger@redhat.com>]

Hi Christoffer,

On 17/10/2017 23:40, Christoffer Dall wrote:
> On Tue, Oct 17, 2017 at 09:09:59AM +0200, Eric Auger wrote:
>> AT the moment if ITT only contains invalid entries,
>> vgic_its_restore_itt returns 1 and this is considered as
>> an an error in vgic_its_restore_dte.
>>
>> Also in case the device table only contains invalid entries,
>> the table restore fails and this is not correct.
>>
>> This patch fully revisits the errror handling while fixing those
>> 2 bugs.
>>
>> - entry_fn_t now takes a valid output paraleter
> 
>                                         parameter
> 
>> - scan_its_table() now returns <= 0 values and output 2 booleans,
>                                                  outputs
>>   valid and last.
>> - vgic_its_restore_itt() now returns <= 0 values.
>> - vgic_its_restore_device_tables() also returns <= 0 values.
>>
>> With that patch we are able to properly handle the case where
>> all data are invalid but we still are able to detect the case
>> where a next entry was referenced by some valid entry and
>> never found.
>>
>> Fixes: 57a9a117154c93 (KVM: arm64: vgic-its: Device table save/restore)
>> Fixes: eff484e0298da5 (KVM: arm64: vgic-its: ITT save and restore)
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
>>
>> ---
>>
>> need to CC stable
>>
>> v3 -> v4:
>> - set *valid at beginning of handle_l1_dte
>>
>> v2 -> v3:
>> - add comments
>> - added valid parameter
>> - vgic_its_restore_itt don't return +1 anymore
>> - reword the commit message
>>
>> v1 -> v2:
>> - if (ret > 0) ret = 0
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 95 ++++++++++++++++++++++++++++++++------------
>>  1 file changed, 70 insertions(+), 25 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index f51c1e1..eea14a1 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -1772,16 +1772,20 @@ static u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
>>  
>>  /**
>>   * entry_fn_t - Callback called on a table entry restore path
>> + *
>>   * @its: its handle
>>   * @id: id of the entry
>>   * @entry: pointer to the entry
>>   * @opaque: pointer to an opaque data
>> + * @valid: indicates whether valid data is associated to this entry
>> + * (the entry itself in case of linear table or entries in the next level,
>> + * in case of hierachical tables)
>>   *
>>   * Return: < 0 on error, 0 if last element was identified, id offset to next
>>   * element otherwise
>>   */
>>  typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
>> -			  void *opaque);
>> +			  void *opaque, bool *valid);
>>  
>>  /**
>>   * scan_its_table - Scan a contiguous table in guest RAM and applies a function
>> @@ -1794,29 +1798,34 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
>>   * @start_id: the ID of the first entry in the table
>>   * (non zero for 2d level tables)
>>   * @fn: function to apply on each entry
>> + * @opaque: opaque data passed to @fn
>> + * @valid: indicates whether the table contains any valid data
>> + * @last: returns whether the last valid entry was decoded
>>   *
>> - * Return: < 0 on error, 0 if last element was identified, 1 otherwise
>> - * (the last element may not be found on second level tables)
>> + * Return: < 0 on error, 0 on success
>>   */
>>  static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
>> -			  int start_id, entry_fn_t fn, void *opaque)
>> +			  int start_id, entry_fn_t fn, void *opaque,
>> +			  bool *valid, bool *last)
>>  {
>>  	void *entry = kzalloc(esz, GFP_KERNEL);
>>  	struct kvm *kvm = its->dev->kvm;
>>  	unsigned long len = size;
>>  	int id = start_id;
>>  	gpa_t gpa = base;
>> +	int next_offset = 0;
>>  	int ret;
>>  
>>  	while (len > 0) {
>> -		int next_offset;
>>  		size_t byte_offset;
>> +		bool entry_valid;
>>  
>>  		ret = kvm_read_guest(kvm, gpa, entry, esz);
>>  		if (ret)
>>  			goto out;
>>  
>> -		next_offset = fn(its, id, entry, opaque);
>> +		next_offset = fn(its, id, entry, opaque, &entry_valid);
>> +		*valid |= entry_valid;
>>  		if (next_offset <= 0) {
>>  			ret = next_offset;
>>  			goto out;
>> @@ -1827,9 +1836,15 @@ static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
>>  		gpa += byte_offset;
>>  		len -= byte_offset;
>>  	}
>> -	ret =  1;
>> -
>> +	/*
>> +	 * the table lookup was completed without identifying the
>> +	 * last valid entry (ie. next_offset > 0).
>> +	 */
> 
> but you never set last to false?  If you require the caller to set the
> variable to false, that should be documented, but it's weird.
> 
>> +	ret = 0;
>>  out:
>> +	if (!next_offset)
>> +		*last = true;
>> +
> 
> so if we scan the entire table to the end we won't set last?  Isn't that
> a bit strange?
> 
> Also, if we can get id of the valid out parameter and instead handle
> that within this function, I don't think you'll need the 'last' return
> value in vgic_its_restore_device_tables, and you could make this:
> 
> 	if (!next_offset && last)
> 		*last = true;
> 	else if (last)
> 		*last = false;
> 
>>  	kfree(entry);
>>  	return ret;
>>  }
>> @@ -1854,12 +1869,14 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
>>  
>>  /**
>>   * vgic_its_restore_ite - restore an interrupt translation entry
>> + *
>>   * @event_id: id used for indexing
>>   * @ptr: pointer to the ITE entry
>>   * @opaque: pointer to the its_device
>> + * @valid: indicates whether the ite is valid
>>   */
>>  static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>> -				void *ptr, void *opaque)
>> +				void *ptr, void *opaque, bool *valid)
>>  {
>>  	struct its_device *dev = (struct its_device *)opaque;
>>  	struct its_collection *collection;
>> @@ -1879,7 +1896,9 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>>  	coll_id = val & KVM_ITS_ITE_ICID_MASK;
>>  	lpi_id = (val & KVM_ITS_ITE_PINTID_MASK) >> KVM_ITS_ITE_PINTID_SHIFT;
>>  
>> -	if (!lpi_id)
>> +	*valid = !!lpi_id;
>> +
>> +	if (!*valid)
>>  		return 1; /* invalid entry, no choice but to scan next entry */
>>  
>>  	if (lpi_id < VGIC_MIN_LPI)
>> @@ -1940,6 +1959,14 @@ static int vgic_its_save_itt(struct vgic_its *its, struct its_device *device)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * vgic_its_restore_itt - restore the ITT of a device
>> + *
>> + * @its: its handle
>> + * @dev: device handle
>> + *
>> + * Return 0 on success, < 0 on error
>> + */
>>  static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
>>  {
>>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
>> @@ -1947,9 +1974,15 @@ static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
>>  	int ret;
>>  	int ite_esz = abi->ite_esz;
>>  	size_t max_size = BIT_ULL(dev->num_eventid_bits) * ite_esz;
>> +	bool valid = false, last = false;
>>  
>>  	ret = scan_its_table(its, base, max_size, ite_esz, 0,
>> -			     vgic_its_restore_ite, dev);
>> +			     vgic_its_restore_ite, dev, &valid, &last);
>> +
>> +	if (!ret && valid && !last) {
>> +		/* a next element was referenced but not found */
>> +		return -EINVAL;
> 
> So this is if we ever found a valid entry, but somehow it didn't lead us
> to the last entry, right?  Can't you handle that within the
> scan_its_table?
> 
> As I understand it, scan_its_table is in one of two modes, either it's
> linearly scanning, or it found a valid entry, and it's jumping from one
> entry to the next, given the offsets.  If it's in the second mode, and
> finds an invalid entry, it should return an error.
> 
> I think you can also get rid of the '*valid = false; return 1;' thing,
> which looks a bit strange.

Given the number of changes this rework will produce I guess this patch
wouldn't be candidate for cc'ed stable. Then shouldn't we consider to
first apply the fix proposed by Wanghaibin (cc'ed stable) and then apply
the rework in a second and subsequent patch?

Thanks

Eric
> 
>> +	}
>>  
>>  	return ret;
>>  }
>> @@ -1985,29 +2018,29 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev,
>>   * @id: device id the DTE corresponds to
>>   * @ptr: kernel VA where the 8 byte DTE is located
>>   * @opaque: unused
>> + * @valid: indicates whether the dte is valid
>>   *
>>   * Return: < 0 on error, 0 if the dte is the last one, id offset to the
>>   * next dte otherwise
>>   */
>>  static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
>> -				void *ptr, void *opaque)
>> +				void *ptr, void *opaque, bool *valid)
>>  {
>>  	struct its_device *dev;
>>  	gpa_t itt_addr;
>>  	u8 num_eventid_bits;
>>  	u64 entry = *(u64 *)ptr;
>> -	bool valid;
>>  	u32 offset;
>>  	int ret;
>>  
>>  	entry = le64_to_cpu(entry);
>>  
>> -	valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
>> +	*valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
>>  	num_eventid_bits = (entry & KVM_ITS_DTE_SIZE_MASK) + 1;
>>  	itt_addr = ((entry & KVM_ITS_DTE_ITTADDR_MASK)
>>  			>> KVM_ITS_DTE_ITTADDR_SHIFT) << 8;
>>  
>> -	if (!valid)
>> +	if (!*valid)
>>  		return 1;
>>  
>>  	/* dte entry is valid */
>> @@ -2082,13 +2115,14 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
>>   * @id: index of the entry in the L1 table
>>   * @addr: kernel VA
>>   * @opaque: unused
>> + * @valid: indicates whether any dte entry was found
>>   *
>>   * L1 table entries are scanned by steps of 1 entry
>>   * Return < 0 if error, 0 if last dte was found when scanning the L2
>>   * table, +1 otherwise (meaning next L1 entry must be scanned)
>>   */
>>  static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
>> -			 void *opaque)
>> +			 void *opaque, bool *valid)
>>  {
>>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
>>  	int l2_start_id = id * (SZ_64K / abi->dte_esz);
>> @@ -2096,21 +2130,29 @@ static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
>>  	int dte_esz = abi->dte_esz;
>>  	gpa_t gpa;
>>  	int ret;
>> +	bool last;
>>  
>>  	entry = le64_to_cpu(entry);
>>  
>> -	if (!(entry & KVM_ITS_L1E_VALID_MASK))
>> +	*valid = entry & KVM_ITS_L1E_VALID_MASK;
>> +
>> +	if (!*valid)
>>  		return 1;
>>  
>>  	gpa = entry & KVM_ITS_L1E_ADDR_MASK;
>>  
>>  	ret = scan_its_table(its, gpa, SZ_64K, dte_esz,
>> -			     l2_start_id, vgic_its_restore_dte, NULL);
>> +			     l2_start_id, vgic_its_restore_dte, NULL,
>> +			     valid, &last);
>>  
>> -	if (ret <= 0)
>> -		return ret;
>> +	/*
>> +	 * if the last dte has not been found in this L2 table, we
>> +	 * need to scan the next L1 entry
>> +	 */
>> +	if (!ret && !last)
>> +		return 1;
>>  
>> -	return 1;
>> +	return ret;
>>  }
>>  
>>  /**
>> @@ -2124,6 +2166,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>>  	int l1_esz, ret;
>>  	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>>  	gpa_t l1_gpa;
>> +	bool valid = false, last = false;
>>  
>>  	if (!(baser & GITS_BASER_VALID))
>>  		return 0;
>> @@ -2133,15 +2176,17 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>>  	if (baser & GITS_BASER_INDIRECT) {
>>  		l1_esz = GITS_LVL1_ENTRY_SIZE;
>>  		ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
>> -				     handle_l1_dte, NULL);
>> +				     handle_l1_dte, NULL, &valid, &last);
>>  	} else {
>>  		l1_esz = abi->dte_esz;
>>  		ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
>> -				     vgic_its_restore_dte, NULL);
>> +				     vgic_its_restore_dte, NULL, &valid, &last);
>>  	}
>>  
>> -	if (ret > 0)
>> -		ret = -EINVAL;
>> +	if (!ret && valid && !last) {
>> +		/* a next element was referenced but not found */
>> +		return -EINVAL;
>> +	}
> 
> Same comment as above.
> 
>>  
>>  	return ret;
>>  }
>> -- 
>> 2.5.5
>>
> Thanks,
> -Christoffer
>