[PATCH v2 00/10] vITS Migration fixes and reset

[PATCH v2 00/10] vITS Migration fixes and reset

[Eric Auger]

This series fixes various bugs observed when saving/restoring the
ITS state before the guest writes the ITS registers (on first boot or
after reset/reboot).

This is a follow up of Wanghaibin's series [1] plus additional
patches following additional code review. It also proposes one
ITS reset implementation. I dared to include 2 Wanghaibin patches,
taking into account the discussions held on the ML but obviously
they can live separately.

Currently, the in-kernel emulated ITS is not reset. After a
reset/reboot, the ITS register values and caches are left
unchanged. Registers may point to some tables in guest memory
which do not exist anymore. If an ITS state backup is initiated
before the guest re-writes the registers, the save fails
because inconsistencies are detected. Also restore of data saved
as such moment is failing.

Patches [1-6] attempt to fix the migration issues without
implementing the reset.
As such they may be candidate for stable:
- do not fail restore if device or ITT tables only contain invalid
  entries
- allow clearing GITS_CREADR/CWRITER whatever CBASER queue size
- limit the cases where we return -EINVAL in table save()
- never attempt to use GITS_BASER<n> and GITS_CBASER if they are
  not valid.
- systematically do both device and collection save/restore even if
  one fails.

Patches [7-10] implement a new ITS reset IOCTL
  - maybe patch "free caches when GITS_BASER Valid bit is cleared" can
    be omitted if we consider the caches should not be cleared when
    GITS_BASER<n>.Valid is cleared.

Best Regards

Eric

Git: complete series available at
https://github.com/eauger/linux/tree/v4.14-rc2-its-reset-v2

* Testing:
- on Cavium using a virtio-net-pci guest and various sequences of
  guest shutdown -r now, virsh reset, virsh suspend/resume,
  virsh reboot, virsh save.restore, virsh shutdown

References:
[1] [RFC PATCH 0/3] fix migrate failed when vm is in booting
https://www.spinics.net/lists/kvm-arm/msg27121.html

History:
v1 -> v2:
- added KVM: arm/arm64: vgic-its: Always attempt to save/restore
  device and collection tables

PATCH v1
- series including 2 modified patches of Wanghaibin


Eric Auger (8):
  KVM: arm/arm64: vgic-its: Always allow clearing GITS_CREADR/CWRITER
  KVM: arm/arm64: vgic-its: Improve error reporting on device table save
  KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving
    tables
  KVM: arm/arm64: vgic-its: Check GITS_CBASER validity before processing
    commands
  KVM: arm/arm64: vgic-its: Always attempt to save/restore device and
    collection tables
  KVM: arm/arm64: vgic-its: free caches when GITS_BASER Valid bit is
    cleared
  KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET
  KVM: arm/arm64: vgic-its: Implement KVM_DEV_ARM_ITS_CTRL_RESET

wanghaibin (2):
  KVM: arm/arm64: vgic-its: fix return value for restore
  KVM: arm/arm64: vgic-its: new helper functions to free the caches

 Documentation/virtual/kvm/devices/arm-vgic-its.txt |  16 ++
 arch/arm/include/uapi/asm/kvm.h                    |   1 +
 arch/arm64/include/uapi/asm/kvm.h                  |   1 +
 virt/kvm/arm/vgic/vgic-its.c                       | 222 +++++++++++++--------
 4 files changed, 158 insertions(+), 82 deletions(-)

-- 
2.5.5

[PATCH v2 01/10] KVM: arm/arm64: vgic-its: fix return value for restore

[Eric Auger]

From: wanghaibin <wanghaibin.wang@huawei.com>

This patch fix the migrate restore tables failure.

The same scene, at the destination, the restore tables
interface traversal guest memory, and check the dte/ite
is valid or not.  If all dtes/ites are invalid, we will do
try next one, and the last it will take the 1 return value,
but currently, it be treated as error. That's not correct.

This patch try to fix this problem.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>

---

need to CC stable

v1 -> v2:
- if (ret > 0) ret = 0
---
 virt/kvm/arm/vgic/vgic-its.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index f51c1e1..fbbc97b 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2018,7 +2018,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
 		return PTR_ERR(dev);
 
 	ret = vgic_its_restore_itt(its, dev);
-	if (ret) {
+	if (ret < 0) {
 		vgic_its_free_device(its->dev->kvm, dev);
 		return ret;
 	}
@@ -2141,7 +2141,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 	}
 
 	if (ret > 0)
-		ret = -EINVAL;
+		ret = 0;
 
 	return ret;
 }
-- 
2.5.5

Re: [PATCH v2 01/10] KVM: arm/arm64: vgic-its: fix return value for restore

[Andre Przywara]

Hi,

On 27/09/17 14:28, Eric Auger wrote:
> From: wanghaibin <wanghaibin.wang@huawei.com>
> 
> This patch fix the migrate restore tables failure.
> 
> The same scene, at the destination, the restore tables
> interface traversal guest memory, and check the dte/ite
> is valid or not.  If all dtes/ites are invalid, we will do
> try next one, and the last it will take the 1 return value,
> but currently, it be treated as error. That's not correct.
> 
> This patch try to fix this problem.
> 
> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>

Looks right to me. But I wonder if we actually should go over the file
and unify the return value semantics or at least document them.
It's a bit puzzling to have functions which return negative errors and 0
*or 1* on success, and then functions which go with the traditional C
convention. That would help explaining the second hunk.

Also this return value handling is a bit weird in cases, like in
handle_l1_dte():

	if (ret <= 0)
		return ret;
	return 1;

which looks like a glorified "return ret;" in that case to me.

But actually this is just nitpicking and the actual patch seems correct.

Cheers,
Andre.

> ---
> 
> need to CC stable
> 
> v1 -> v2:
> - if (ret > 0) ret = 0
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index f51c1e1..fbbc97b 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2018,7 +2018,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
>  		return PTR_ERR(dev);
>  
>  	ret = vgic_its_restore_itt(its, dev);
> -	if (ret) {
> +	if (ret < 0) {
>  		vgic_its_free_device(its->dev->kvm, dev);
>  		return ret;
>  	}
> @@ -2141,7 +2141,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>  	}
>  
>  	if (ret > 0)
> -		ret = -EINVAL;
> +		ret = 0;
>  
>  	return ret;
>  }
> 

Re: [PATCH v2 01/10] KVM: arm/arm64: vgic-its: fix return value for restore

[Auger Eric]

Hi Andre,

On 06/10/2017 16:37, Andre Przywara wrote:
> Hi,
> 
> On 27/09/17 14:28, Eric Auger wrote:
>> From: wanghaibin <wanghaibin.wang@huawei.com>
>>
>> This patch fix the migrate restore tables failure.
>>
>> The same scene, at the destination, the restore tables
>> interface traversal guest memory, and check the dte/ite
>> is valid or not.  If all dtes/ites are invalid, we will do
>> try next one, and the last it will take the 1 return value,
>> but currently, it be treated as error. That's not correct.
>>
>> This patch try to fix this problem.
>>
>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
> 
> Looks right to me. But I wonder if we actually should go over the file
> and unify the return value semantics or at least document them.
> It's a bit puzzling to have functions which return negative errors and 0
> *or 1* on success, and then functions which go with the traditional C
> convention. That would help explaining the second hunk.
> 
> Also this return value handling is a bit weird in cases, like in
> handle_l1_dte():
> 
> 	if (ret <= 0)
> 		return ret;
> 	return 1;
> 
> which looks like a glorified "return ret;" in that case to me.
Yes that's sadly true.

Yep this error handling is a mess I must confess. I will try to better
document and unify as much as possible.

Thanks a lot for your review.

Eric
> 
> But actually this is just nitpicking and the actual patch seems correct.
> 
> Cheers,
> Andre.
> 
>> ---
>>
>> need to CC stable
>>
>> v1 -> v2:
>> - if (ret > 0) ret = 0
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index f51c1e1..fbbc97b 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -2018,7 +2018,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
>>  		return PTR_ERR(dev);
>>  
>>  	ret = vgic_its_restore_itt(its, dev);
>> -	if (ret) {
>> +	if (ret < 0) {
>>  		vgic_its_free_device(its->dev->kvm, dev);
>>  		return ret;
>>  	}
>> @@ -2141,7 +2141,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>>  	}
>>  
>>  	if (ret > 0)
>> -		ret = -EINVAL;
>> +		ret = 0;
>>  
>>  	return ret;
>>  }
>>

Re: [PATCH v2 01/10] KVM: arm/arm64: vgic-its: fix return value for restore

[Christoffer Dall]

On Wed, Sep 27, 2017 at 03:28:31PM +0200, Eric Auger wrote:
> From: wanghaibin <wanghaibin.wang@huawei.com>
> 
> This patch fix the migrate restore tables failure.
> 
> The same scene, at the destination, the restore tables
> interface traversal guest memory, and check the dte/ite
> is valid or not.  If all dtes/ites are invalid, we will do
> try next one, and the last it will take the 1 return value,
> but currently, it be treated as error. That's not correct.
> 
> This patch try to fix this problem.

This commit message really needs work.

> 
> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
> 
> ---
> 
> need to CC stable
> 
> v1 -> v2:
> - if (ret > 0) ret = 0
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index f51c1e1..fbbc97b 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2018,7 +2018,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
>  		return PTR_ERR(dev);
>  
>  	ret = vgic_its_restore_itt(its, dev);
> -	if (ret) {
> +	if (ret < 0) {
>  		vgic_its_free_device(its->dev->kvm, dev);
>  		return ret;
>  	}
> @@ -2141,7 +2141,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>  	}
>  
>  	if (ret > 0)
> -		ret = -EINVAL;
> +		ret = 0;
>  
>  	return ret;
>  }
> -- 
> 2.5.5
> 

And I agree with the idea of cleaning up the return values.  I think it
could be improved by always returning 0 on some success and -ERR on some
error, and carry an additional return value is a parameter, indicating
if the last entry was found, which can be omitted (be NULL) by callers
who do not care.

Otherwise:
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

[PATCH v2 02/10] KVM: arm/arm64: vgic-its: Always allow clearing GITS_CREADR/CWRITER

[Eric Auger]

If the GITS_CBASER Size field is 0, which can correspond to a
reset value, the userspace fails to set the GITS_CREADR/CWRITER
offsets to 0. This failure is not justified.

Let's allow this setting which can also correspond to a reset value.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

need to CC stable
---
 virt/kvm/arm/vgic/vgic-its.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index fbbc97b..76bed2d 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1329,7 +1329,7 @@ static void vgic_mmio_write_its_cwriter(struct kvm *kvm, struct vgic_its *its,
 
 	reg = update_64bit_reg(its->cwriter, addr & 7, len, val);
 	reg = ITS_CMD_OFFSET(reg);
-	if (reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
+	if (reg && reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
 		mutex_unlock(&its->cmd_lock);
 		return;
 	}
@@ -1370,7 +1370,7 @@ static int vgic_mmio_uaccess_write_its_creadr(struct kvm *kvm,
 	}
 
 	cmd_offset = ITS_CMD_OFFSET(val);
-	if (cmd_offset >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
+	if (cmd_offset && cmd_offset >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
 		ret = -EINVAL;
 		goto out;
 	}
-- 
2.5.5

Re: [PATCH v2 02/10] KVM: arm/arm64: vgic-its: Always allow clearing GITS_CREADR/CWRITER

[Andre Przywara]

Hi,

On 27/09/17 14:28, Eric Auger wrote:
> If the GITS_CBASER Size field is 0, which can correspond to a
> reset value, the userspace fails to set the GITS_CREADR/CWRITER
> offsets to 0. This failure is not justified.
> 
> Let's allow this setting which can also correspond to a reset value.

But the GITS_CBASER size field has a +1 encoding, so can never be 0, if
I understand the manual (and ITS_CMD_BUFFER_SIZE) correctly.

What am I missing here?

Cheers,
Andre.

> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> need to CC stable
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index fbbc97b..76bed2d 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1329,7 +1329,7 @@ static void vgic_mmio_write_its_cwriter(struct kvm *kvm, struct vgic_its *its,
>  
>  	reg = update_64bit_reg(its->cwriter, addr & 7, len, val);
>  	reg = ITS_CMD_OFFSET(reg);
> -	if (reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
> +	if (reg && reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
>  		mutex_unlock(&its->cmd_lock);
>  		return;
>  	}
> @@ -1370,7 +1370,7 @@ static int vgic_mmio_uaccess_write_its_creadr(struct kvm *kvm,
>  	}
>  
>  	cmd_offset = ITS_CMD_OFFSET(val);
> -	if (cmd_offset >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
> +	if (cmd_offset && cmd_offset >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
>  		ret = -EINVAL;
>  		goto out;
>  	}
> 

Re: [PATCH v2 02/10] KVM: arm/arm64: vgic-its: Always allow clearing GITS_CREADR/CWRITER

[Auger Eric]

Hi,

On 06/10/2017 16:37, Andre Przywara wrote:
> Hi,
> 
> On 27/09/17 14:28, Eric Auger wrote:
>> If the GITS_CBASER Size field is 0, which can correspond to a
>> reset value, the userspace fails to set the GITS_CREADR/CWRITER
>> offsets to 0. This failure is not justified.
>>
>> Let's allow this setting which can also correspond to a reset value.
> 
> But the GITS_CBASER size field has a +1 encoding, so can never be 0, if
> I understand the manual (and ITS_CMD_BUFFER_SIZE) correctly.
Oh OK, you're. I missed that. I would have sworn I've seen that failure
but I must have misinterpreted it.

Thanks!

Eric
> 
> What am I missing here?
> 
> Cheers,
> Andre.
> 
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> need to CC stable
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index fbbc97b..76bed2d 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -1329,7 +1329,7 @@ static void vgic_mmio_write_its_cwriter(struct kvm *kvm, struct vgic_its *its,
>>  
>>  	reg = update_64bit_reg(its->cwriter, addr & 7, len, val);
>>  	reg = ITS_CMD_OFFSET(reg);
>> -	if (reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
>> +	if (reg && reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
>>  		mutex_unlock(&its->cmd_lock);
>>  		return;
>>  	}
>> @@ -1370,7 +1370,7 @@ static int vgic_mmio_uaccess_write_its_creadr(struct kvm *kvm,
>>  	}
>>  
>>  	cmd_offset = ITS_CMD_OFFSET(val);
>> -	if (cmd_offset >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
>> +	if (cmd_offset && cmd_offset >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
>>  		ret = -EINVAL;
>>  		goto out;
>>  	}
>>

Re: [PATCH v2 02/10] KVM: arm/arm64: vgic-its: Always allow clearing GITS_CREADR/CWRITER

[Christoffer Dall]

On Fri, Oct 06, 2017 at 05:29:02PM +0200, Auger Eric wrote:
> Hi,
> 
> On 06/10/2017 16:37, Andre Przywara wrote:
> > Hi,
> > 
> > On 27/09/17 14:28, Eric Auger wrote:
> >> If the GITS_CBASER Size field is 0, which can correspond to a
> >> reset value, the userspace fails to set the GITS_CREADR/CWRITER
> >> offsets to 0. This failure is not justified.
> >>
> >> Let's allow this setting which can also correspond to a reset value.
> > 
> > But the GITS_CBASER size field has a +1 encoding, so can never be 0, if
> > I understand the manual (and ITS_CMD_BUFFER_SIZE) correctly.
> Oh OK, you're. I missed that. I would have sworn I've seen that failure
> but I must have misinterpreted it.
> 

What is CWRITER is written to a brand new ITS before the CBASER is
written?

I don't see us initializing the cbaser field anywhere?

Thanks,
-Christoffer

> > 
> > What am I missing here?
> > 
> > Cheers,
> > Andre.
> > 
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>
> >> ---
> >>
> >> need to CC stable
> >> ---
> >>  virt/kvm/arm/vgic/vgic-its.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >> index fbbc97b..76bed2d 100644
> >> --- a/virt/kvm/arm/vgic/vgic-its.c
> >> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >> @@ -1329,7 +1329,7 @@ static void vgic_mmio_write_its_cwriter(struct kvm *kvm, struct vgic_its *its,
> >>  
> >>  	reg = update_64bit_reg(its->cwriter, addr & 7, len, val);
> >>  	reg = ITS_CMD_OFFSET(reg);
> >> -	if (reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
> >> +	if (reg && reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
> >>  		mutex_unlock(&its->cmd_lock);
> >>  		return;
> >>  	}
> >> @@ -1370,7 +1370,7 @@ static int vgic_mmio_uaccess_write_its_creadr(struct kvm *kvm,
> >>  	}
> >>  
> >>  	cmd_offset = ITS_CMD_OFFSET(val);
> >> -	if (cmd_offset >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
> >> +	if (cmd_offset && cmd_offset >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
> >>  		ret = -EINVAL;
> >>  		goto out;
> >>  	}
> >>

Re: [PATCH v2 02/10] KVM: arm/arm64: vgic-its: Always allow clearing GITS_CREADR/CWRITER

[Auger Eric]

Hi Christoffer,
On 13/10/2017 13:44, Christoffer Dall wrote:
> On Fri, Oct 06, 2017 at 05:29:02PM +0200, Auger Eric wrote:
>> Hi,
>>
>> On 06/10/2017 16:37, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 27/09/17 14:28, Eric Auger wrote:
>>>> If the GITS_CBASER Size field is 0, which can correspond to a
>>>> reset value, the userspace fails to set the GITS_CREADR/CWRITER
>>>> offsets to 0. This failure is not justified.
>>>>
>>>> Let's allow this setting which can also correspond to a reset value.
>>>
>>> But the GITS_CBASER size field has a +1 encoding, so can never be 0, if
>>> I understand the manual (and ITS_CMD_BUFFER_SIZE) correctly.
>> Oh OK, you're. I missed that. I would have sworn I've seen that failure
>> but I must have misinterpreted it.
>>
> 
> What is CWRITER is written to a brand new ITS before the CBASER is
> written?
> 
> I don't see us initializing the cbaser field anywhere?
in vgic_its_create() its is allocated with kzalloc so its->cbaser is
initialized at this moment.

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 
>>>
>>> What am I missing here?
>>>
>>> Cheers,
>>> Andre.
>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>
>>>> ---
>>>>
>>>> need to CC stable
>>>> ---
>>>>  virt/kvm/arm/vgic/vgic-its.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>>> index fbbc97b..76bed2d 100644
>>>> --- a/virt/kvm/arm/vgic/vgic-its.c
>>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>>> @@ -1329,7 +1329,7 @@ static void vgic_mmio_write_its_cwriter(struct kvm *kvm, struct vgic_its *its,
>>>>  
>>>>  	reg = update_64bit_reg(its->cwriter, addr & 7, len, val);
>>>>  	reg = ITS_CMD_OFFSET(reg);
>>>> -	if (reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
>>>> +	if (reg && reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
>>>>  		mutex_unlock(&its->cmd_lock);
>>>>  		return;
>>>>  	}
>>>> @@ -1370,7 +1370,7 @@ static int vgic_mmio_uaccess_write_its_creadr(struct kvm *kvm,
>>>>  	}
>>>>  
>>>>  	cmd_offset = ITS_CMD_OFFSET(val);
>>>> -	if (cmd_offset >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
>>>> +	if (cmd_offset && cmd_offset >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
>>>>  		ret = -EINVAL;
>>>>  		goto out;
>>>>  	}
>>>>

Re: [PATCH v2 02/10] KVM: arm/arm64: vgic-its: Always allow clearing GITS_CREADR/CWRITER

[Christoffer Dall]

On Fri, Oct 13, 2017 at 01:54:34PM +0200, Auger Eric wrote:
> Hi Christoffer,
> On 13/10/2017 13:44, Christoffer Dall wrote:
> > On Fri, Oct 06, 2017 at 05:29:02PM +0200, Auger Eric wrote:
> >> Hi,
> >>
> >> On 06/10/2017 16:37, Andre Przywara wrote:
> >>> Hi,
> >>>
> >>> On 27/09/17 14:28, Eric Auger wrote:
> >>>> If the GITS_CBASER Size field is 0, which can correspond to a
> >>>> reset value, the userspace fails to set the GITS_CREADR/CWRITER
> >>>> offsets to 0. This failure is not justified.
> >>>>
> >>>> Let's allow this setting which can also correspond to a reset value.
> >>>
> >>> But the GITS_CBASER size field has a +1 encoding, so can never be 0, if
> >>> I understand the manual (and ITS_CMD_BUFFER_SIZE) correctly.
> >> Oh OK, you're. I missed that. I would have sworn I've seen that failure
> >> but I must have misinterpreted it.
> >>
> > 
> > What is CWRITER is written to a brand new ITS before the CBASER is
> > written?
> > 
> > I don't see us initializing the cbaser field anywhere?
> in vgic_its_create() its is allocated with kzalloc so its->cbaser is
> initialized at this moment.
> 

Right, so it can be 0, and we still need your patch, contrary to Andre's
comment.  Am I missing something?

Thanks,
-Christoffer

Re: [PATCH v2 02/10] KVM: arm/arm64: vgic-its: Always allow clearing GITS_CREADR/CWRITER

[Auger Eric]

Hi Christoffer,

On 13/10/2017 19:54, Christoffer Dall wrote:
> On Fri, Oct 13, 2017 at 01:54:34PM +0200, Auger Eric wrote:
>> Hi Christoffer,
>> On 13/10/2017 13:44, Christoffer Dall wrote:
>>> On Fri, Oct 06, 2017 at 05:29:02PM +0200, Auger Eric wrote:
>>>> Hi,
>>>>
>>>> On 06/10/2017 16:37, Andre Przywara wrote:
>>>>> Hi,
>>>>>
>>>>> On 27/09/17 14:28, Eric Auger wrote:
>>>>>> If the GITS_CBASER Size field is 0, which can correspond to a
>>>>>> reset value, the userspace fails to set the GITS_CREADR/CWRITER
>>>>>> offsets to 0. This failure is not justified.
>>>>>>
>>>>>> Let's allow this setting which can also correspond to a reset value.
>>>>>
>>>>> But the GITS_CBASER size field has a +1 encoding, so can never be 0, if
>>>>> I understand the manual (and ITS_CMD_BUFFER_SIZE) correctly.
>>>> Oh OK, you're. I missed that. I would have sworn I've seen that failure
>>>> but I must have misinterpreted it.
>>>>
>>>
>>> What is CWRITER is written to a brand new ITS before the CBASER is
>>> written?
>>>
>>> I don't see us initializing the cbaser field anywhere?
>> in vgic_its_create() its is allocated with kzalloc so its->cbaser is
>> initialized at this moment.
>>
> 
> Right, so it can be 0, and we still need your patch, contrary to Andre's
> comment.  Am I missing something?

No Andre is right, cbaser = 0 but ITS_CMD_BUFFER_SIZE(its->cbaser)
returns 4kB.

#define ITS_CMD_BUFFER_SIZE(baser)      ((((baser) & 0xff) + 1) << 12)

So the check doesn't fail.

Thanks

Eric


> 
> Thanks,
> -Christoffer
> 

Re: [PATCH v2 02/10] KVM: arm/arm64: vgic-its: Always allow clearing GITS_CREADR/CWRITER

[Christoffer Dall]

On Sat, Oct 14, 2017 at 10:53:01AM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 13/10/2017 19:54, Christoffer Dall wrote:
> > On Fri, Oct 13, 2017 at 01:54:34PM +0200, Auger Eric wrote:
> >> Hi Christoffer,
> >> On 13/10/2017 13:44, Christoffer Dall wrote:
> >>> On Fri, Oct 06, 2017 at 05:29:02PM +0200, Auger Eric wrote:
> >>>> Hi,
> >>>>
> >>>> On 06/10/2017 16:37, Andre Przywara wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On 27/09/17 14:28, Eric Auger wrote:
> >>>>>> If the GITS_CBASER Size field is 0, which can correspond to a
> >>>>>> reset value, the userspace fails to set the GITS_CREADR/CWRITER
> >>>>>> offsets to 0. This failure is not justified.
> >>>>>>
> >>>>>> Let's allow this setting which can also correspond to a reset value.
> >>>>>
> >>>>> But the GITS_CBASER size field has a +1 encoding, so can never be 0, if
> >>>>> I understand the manual (and ITS_CMD_BUFFER_SIZE) correctly.
> >>>> Oh OK, you're. I missed that. I would have sworn I've seen that failure
> >>>> but I must have misinterpreted it.
> >>>>
> >>>
> >>> What is CWRITER is written to a brand new ITS before the CBASER is
> >>> written?
> >>>
> >>> I don't see us initializing the cbaser field anywhere?
> >> in vgic_its_create() its is allocated with kzalloc so its->cbaser is
> >> initialized at this moment.
> >>
> > 
> > Right, so it can be 0, and we still need your patch, contrary to Andre's
> > comment.  Am I missing something?
> 
> No Andre is right, cbaser = 0 but ITS_CMD_BUFFER_SIZE(its->cbaser)
> returns 4kB.
> 
> #define ITS_CMD_BUFFER_SIZE(baser)      ((((baser) & 0xff) + 1) << 12)
> 
> So the check doesn't fail.
> 
Duh, I didn't check the macro and thought it just applied a mask and
shift.

Thanks,
-Christoffer

[PATCH v2 03/10] KVM: arm/arm64: vgic-its: Improve error reporting on device table save

[Eric Auger]

At the moment the device table save() returns -EINVAL if
vgic_its_check_id() fails to return the gpa of the entry
associated to the device/collection id. Let vgic_its_check_id()
return an int instead of a bool and return a more precised
error value:
- EINVAL in case the id is out of range
- EFAULT if the gpa is not provisionned or is not valid

We also check first the GITS_BASER<n> Valid bit is set.

This allows the userspace to discriminate failure reasons.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

need to CC stable
---
 virt/kvm/arm/vgic/vgic-its.c | 53 ++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 76bed2d..c1f7972 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -688,14 +688,24 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
 }
 
 /*
- * Check whether an ID can be stored into the corresponding guest table.
+ * vgic_its_check_id - Check whether an ID can be stored into
+ * the corresponding guest table.
+ *
  * 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 accessible).
  * For this we have to read the respective first level entry.
+ *
+ * @its: its handle
+ * @baser: GITS_BASER<n> register
+ * @id: id of the device/collection
+ * @eaddr: output gpa of the corresponding table entry
+ *
+ * Return: 0 on success, -EINVAL if @id is out of range, -EFAULT if
+ * the address cannot be computed or is not valid
  */
-static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
-			      gpa_t *eaddr)
+static int vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
+			     gpa_t *eaddr)
 {
 	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
 	u64 indirect_ptr, type = GITS_BASER_TYPE(baser);
@@ -703,50 +713,56 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
 	int index;
 	gfn_t gfn;
 
+	if (!(baser & GITS_BASER_VALID))
+		return -EFAULT;
+
 	switch (type) {
 	case GITS_BASER_TYPE_DEVICE:
 		if (id >= BIT_ULL(VITS_TYPER_DEVBITS))
-			return false;
+			return -EINVAL;
 		break;
 	case GITS_BASER_TYPE_COLLECTION:
 		/* as GITS_TYPER.CIL == 0, ITS supports 16-bit collection ID */
 		if (id >= BIT_ULL(16))
-			return false;
+			return -EINVAL;
 		break;
 	default:
-		return false;
+		return -EINVAL;
 	}
 
 	if (!(baser & GITS_BASER_INDIRECT)) {
 		phys_addr_t addr;
 
 		if (id >= (l1_tbl_size / esz))
-			return false;
+			return -EINVAL;
 
 		addr = BASER_ADDRESS(baser) + id * esz;
 		gfn = addr >> PAGE_SHIFT;
 
 		if (eaddr)
 			*eaddr = addr;
-		return kvm_is_visible_gfn(its->dev->kvm, gfn);
+		if (kvm_is_visible_gfn(its->dev->kvm, gfn))
+			return 0;
+		else
+			return -EFAULT;
 	}
 
 	/* calculate and check the index into the 1st level */
 	index = id / (SZ_64K / esz);
 	if (index >= (l1_tbl_size / sizeof(u64)))
-		return false;
+		return -EINVAL;
 
 	/* Each 1st level entry is represented by a 64-bit value. */
 	if (kvm_read_guest(its->dev->kvm,
 			   BASER_ADDRESS(baser) + index * sizeof(indirect_ptr),
 			   &indirect_ptr, sizeof(indirect_ptr)))
-		return false;
+		return -EFAULT;
 
 	indirect_ptr = le64_to_cpu(indirect_ptr);
 
 	/* check the valid bit of the first level entry */
 	if (!(indirect_ptr & BIT_ULL(63)))
-		return false;
+		return -EFAULT;
 
 	/*
 	 * Mask the guest physical address and calculate the frame number.
@@ -762,7 +778,10 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
 
 	if (eaddr)
 		*eaddr = indirect_ptr;
-	return kvm_is_visible_gfn(its->dev->kvm, gfn);
+	if (kvm_is_visible_gfn(its->dev->kvm, gfn))
+		return 0;
+	else
+		return -EFAULT;
 }
 
 static int vgic_its_alloc_collection(struct vgic_its *its,
@@ -771,7 +790,7 @@ static int vgic_its_alloc_collection(struct vgic_its *its,
 {
 	struct its_collection *collection;
 
-	if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
+	if (vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
 		return E_ITS_MAPC_COLLECTION_OOR;
 
 	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
@@ -943,7 +962,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
 	struct its_device *device;
 
-	if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
+	if (vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
 		return E_ITS_MAPD_DEVICE_OOR;
 
 	if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
@@ -2060,9 +2079,9 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
 		int ret;
 		gpa_t eaddr;
 
-		if (!vgic_its_check_id(its, baser,
-				       dev->device_id, &eaddr))
-			return -EINVAL;
+		ret = vgic_its_check_id(its, baser, dev->device_id, &eaddr);
+		if (ret)
+			return ret;
 
 		ret = vgic_its_save_itt(its, dev);
 		if (ret)
-- 
2.5.5

Re: [PATCH v2 03/10] KVM: arm/arm64: vgic-its: Improve error reporting on device table save

[Andre Przywara]

Hi,

On 27/09/17 14:28, Eric Auger wrote:
> At the moment the device table save() returns -EINVAL if
> vgic_its_check_id() fails to return the gpa of the entry
> associated to the device/collection id. Let vgic_its_check_id()
> return an int instead of a bool and return a more precised
> error value:
> - EINVAL in case the id is out of range
> - EFAULT if the gpa is not provisionned or is not valid
> 
> We also check first the GITS_BASER<n> Valid bit is set.
> 
> This allows the userspace to discriminate failure reasons.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> need to CC stable
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 53 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 76bed2d..c1f7972 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -688,14 +688,24 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
>  }
>  
>  /*

kernel-doc requires two asterisks to recognise conforming comments [1].

> - * Check whether an ID can be stored into the corresponding guest table.
> + * vgic_its_check_id - Check whether an ID can be stored into
> + * the corresponding guest table.
> + *
>   * 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 accessible).
>   * For this we have to read the respective first level entry.

And the more elaborate explanations should come after the arguments section.

The rest looks OK.

Cheers,
Andre.

[1] Documentation/kernel-doc-nano-HOWTO.txt
> + *
> + * @its: its handle
> + * @baser: GITS_BASER<n> register
> + * @id: id of the device/collection
> + * @eaddr: output gpa of the corresponding table entry
> + *
> + * Return: 0 on success, -EINVAL if @id is out of range, -EFAULT if
> + * the address cannot be computed or is not valid
>   */
> -static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> -			      gpa_t *eaddr)
> +static int vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> +			     gpa_t *eaddr)
>  {
>  	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>  	u64 indirect_ptr, type = GITS_BASER_TYPE(baser);
> @@ -703,50 +713,56 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>  	int index;
>  	gfn_t gfn;
>  
> +	if (!(baser & GITS_BASER_VALID))
> +		return -EFAULT;
> +
>  	switch (type) {
>  	case GITS_BASER_TYPE_DEVICE:
>  		if (id >= BIT_ULL(VITS_TYPER_DEVBITS))
> -			return false;
> +			return -EINVAL;
>  		break;
>  	case GITS_BASER_TYPE_COLLECTION:
>  		/* as GITS_TYPER.CIL == 0, ITS supports 16-bit collection ID */
>  		if (id >= BIT_ULL(16))
> -			return false;
> +			return -EINVAL;
>  		break;
>  	default:
> -		return false;
> +		return -EINVAL;
>  	}
>  
>  	if (!(baser & GITS_BASER_INDIRECT)) {
>  		phys_addr_t addr;
>  
>  		if (id >= (l1_tbl_size / esz))
> -			return false;
> +			return -EINVAL;
>  
>  		addr = BASER_ADDRESS(baser) + id * esz;
>  		gfn = addr >> PAGE_SHIFT;
>  
>  		if (eaddr)
>  			*eaddr = addr;
> -		return kvm_is_visible_gfn(its->dev->kvm, gfn);
> +		if (kvm_is_visible_gfn(its->dev->kvm, gfn))
> +			return 0;
> +		else
> +			return -EFAULT;
>  	}
>  
>  	/* calculate and check the index into the 1st level */
>  	index = id / (SZ_64K / esz);
>  	if (index >= (l1_tbl_size / sizeof(u64)))
> -		return false;
> +		return -EINVAL;
>  
>  	/* Each 1st level entry is represented by a 64-bit value. */
>  	if (kvm_read_guest(its->dev->kvm,
>  			   BASER_ADDRESS(baser) + index * sizeof(indirect_ptr),
>  			   &indirect_ptr, sizeof(indirect_ptr)))
> -		return false;
> +		return -EFAULT;
>  
>  	indirect_ptr = le64_to_cpu(indirect_ptr);
>  
>  	/* check the valid bit of the first level entry */
>  	if (!(indirect_ptr & BIT_ULL(63)))
> -		return false;
> +		return -EFAULT;
>  
>  	/*
>  	 * Mask the guest physical address and calculate the frame number.
> @@ -762,7 +778,10 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>  
>  	if (eaddr)
>  		*eaddr = indirect_ptr;
> -	return kvm_is_visible_gfn(its->dev->kvm, gfn);
> +	if (kvm_is_visible_gfn(its->dev->kvm, gfn))
> +		return 0;
> +	else
> +		return -EFAULT;
>  }
>  
>  static int vgic_its_alloc_collection(struct vgic_its *its,
> @@ -771,7 +790,7 @@ static int vgic_its_alloc_collection(struct vgic_its *its,
>  {
>  	struct its_collection *collection;
>  
> -	if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
> +	if (vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
>  		return E_ITS_MAPC_COLLECTION_OOR;
>  
>  	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
> @@ -943,7 +962,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>  	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
>  	struct its_device *device;
>  
> -	if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
> +	if (vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
>  		return E_ITS_MAPD_DEVICE_OOR;
>  
>  	if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
> @@ -2060,9 +2079,9 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
>  		int ret;
>  		gpa_t eaddr;
>  
> -		if (!vgic_its_check_id(its, baser,
> -				       dev->device_id, &eaddr))
> -			return -EINVAL;
> +		ret = vgic_its_check_id(its, baser, dev->device_id, &eaddr);
> +		if (ret)
> +			return ret;
>  
>  		ret = vgic_its_save_itt(its, dev);
>  		if (ret)
> 

Re: [PATCH v2 03/10] KVM: arm/arm64: vgic-its: Improve error reporting on device table save

[Christoffer Dall]

On Wed, Sep 27, 2017 at 03:28:33PM +0200, Eric Auger wrote:
> At the moment the device table save() returns -EINVAL if
> vgic_its_check_id() fails to return the gpa of the entry
> associated to the device/collection id. Let vgic_its_check_id()
> return an int instead of a bool and return a more precised
> error value:
> - EINVAL in case the id is out of range
> - EFAULT if the gpa is not provisionned or is not valid
> 

This is just to ease debugging, yes?

Seems a bit invasive to me for that purpose.

> We also check first the GITS_BASER<n> Valid bit is set.
> 
> This allows the userspace to discriminate failure reasons.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> need to CC stable

Is this solving a real issue that someone has?  Otherwise I don't think
it should be cc'ed to stable, nor am I sure it warrants merging as a
fix.

Thanks,
-Christoffer

> ---
>  virt/kvm/arm/vgic/vgic-its.c | 53 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 76bed2d..c1f7972 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -688,14 +688,24 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
>  }
>  
>  /*
> - * Check whether an ID can be stored into the corresponding guest table.
> + * vgic_its_check_id - Check whether an ID can be stored into
> + * the corresponding guest table.
> + *
>   * 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 accessible).
>   * For this we have to read the respective first level entry.
> + *
> + * @its: its handle
> + * @baser: GITS_BASER<n> register
> + * @id: id of the device/collection
> + * @eaddr: output gpa of the corresponding table entry
> + *
> + * Return: 0 on success, -EINVAL if @id is out of range, -EFAULT if
> + * the address cannot be computed or is not valid
>   */
> -static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> -			      gpa_t *eaddr)
> +static int vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> +			     gpa_t *eaddr)
>  {
>  	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>  	u64 indirect_ptr, type = GITS_BASER_TYPE(baser);
> @@ -703,50 +713,56 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>  	int index;
>  	gfn_t gfn;
>  
> +	if (!(baser & GITS_BASER_VALID))
> +		return -EFAULT;
> +
>  	switch (type) {
>  	case GITS_BASER_TYPE_DEVICE:
>  		if (id >= BIT_ULL(VITS_TYPER_DEVBITS))
> -			return false;
> +			return -EINVAL;
>  		break;
>  	case GITS_BASER_TYPE_COLLECTION:
>  		/* as GITS_TYPER.CIL == 0, ITS supports 16-bit collection ID */
>  		if (id >= BIT_ULL(16))
> -			return false;
> +			return -EINVAL;
>  		break;
>  	default:
> -		return false;
> +		return -EINVAL;
>  	}
>  
>  	if (!(baser & GITS_BASER_INDIRECT)) {
>  		phys_addr_t addr;
>  
>  		if (id >= (l1_tbl_size / esz))
> -			return false;
> +			return -EINVAL;
>  
>  		addr = BASER_ADDRESS(baser) + id * esz;
>  		gfn = addr >> PAGE_SHIFT;
>  
>  		if (eaddr)
>  			*eaddr = addr;
> -		return kvm_is_visible_gfn(its->dev->kvm, gfn);
> +		if (kvm_is_visible_gfn(its->dev->kvm, gfn))
> +			return 0;
> +		else
> +			return -EFAULT;
>  	}
>  
>  	/* calculate and check the index into the 1st level */
>  	index = id / (SZ_64K / esz);
>  	if (index >= (l1_tbl_size / sizeof(u64)))
> -		return false;
> +		return -EINVAL;
>  
>  	/* Each 1st level entry is represented by a 64-bit value. */
>  	if (kvm_read_guest(its->dev->kvm,
>  			   BASER_ADDRESS(baser) + index * sizeof(indirect_ptr),
>  			   &indirect_ptr, sizeof(indirect_ptr)))
> -		return false;
> +		return -EFAULT;
>  
>  	indirect_ptr = le64_to_cpu(indirect_ptr);
>  
>  	/* check the valid bit of the first level entry */
>  	if (!(indirect_ptr & BIT_ULL(63)))
> -		return false;
> +		return -EFAULT;
>  
>  	/*
>  	 * Mask the guest physical address and calculate the frame number.
> @@ -762,7 +778,10 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>  
>  	if (eaddr)
>  		*eaddr = indirect_ptr;
> -	return kvm_is_visible_gfn(its->dev->kvm, gfn);
> +	if (kvm_is_visible_gfn(its->dev->kvm, gfn))
> +		return 0;
> +	else
> +		return -EFAULT;
>  }
>  
>  static int vgic_its_alloc_collection(struct vgic_its *its,
> @@ -771,7 +790,7 @@ static int vgic_its_alloc_collection(struct vgic_its *its,
>  {
>  	struct its_collection *collection;
>  
> -	if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
> +	if (vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
>  		return E_ITS_MAPC_COLLECTION_OOR;
>  
>  	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
> @@ -943,7 +962,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>  	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
>  	struct its_device *device;
>  
> -	if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
> +	if (vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
>  		return E_ITS_MAPD_DEVICE_OOR;
>  
>  	if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
> @@ -2060,9 +2079,9 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
>  		int ret;
>  		gpa_t eaddr;
>  
> -		if (!vgic_its_check_id(its, baser,
> -				       dev->device_id, &eaddr))
> -			return -EINVAL;
> +		ret = vgic_its_check_id(its, baser, dev->device_id, &eaddr);
> +		if (ret)
> +			return ret;
>  
>  		ret = vgic_its_save_itt(its, dev);
>  		if (ret)
> -- 
> 2.5.5
> 

Re: [PATCH v2 03/10] KVM: arm/arm64: vgic-its: Improve error reporting on device table save

[Auger Eric]

Hi,

On 13/10/2017 15:16, Christoffer Dall wrote:
> On Wed, Sep 27, 2017 at 03:28:33PM +0200, Eric Auger wrote:
>> At the moment the device table save() returns -EINVAL if
>> vgic_its_check_id() fails to return the gpa of the entry
>> associated to the device/collection id. Let vgic_its_check_id()
>> return an int instead of a bool and return a more precised
>> error value:
>> - EINVAL in case the id is out of range
>> - EFAULT if the gpa is not provisionned or is not valid
>>
> 
> This is just to ease debugging, yes?

I understood user-space should be able to discriminate between bad guest
programming and values corrupted by the userspace (regs for instance).
In first case QEMU should not abort. In latter case it should abort.

In vgic_its_check_id we are checking the L1 entry validity bit and in
case it is invalid we can't compute the GPA of the entry. I was thinking
we should return -EFAULT in that case. But maybe returning EFAULT in
case the BASER<n> address is not reachable also is wrong because that
may be caused by the userspace writing a wrong value. Sigh ...

Thanks

Eric
> 
> Seems a bit invasive to me for that purpose.
> 
>> We also check first the GITS_BASER<n> Valid bit is set.
>>
>> This allows the userspace to discriminate failure reasons.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> need to CC stable
> 
> Is this solving a real issue that someone has?  Otherwise I don't think
> it should be cc'ed to stable, nor am I sure it warrants merging as a
> fix.
> 
> Thanks,
> -Christoffer
> 
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 53 ++++++++++++++++++++++++++++++--------------
>>  1 file changed, 36 insertions(+), 17 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 76bed2d..c1f7972 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -688,14 +688,24 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
>>  }
>>  
>>  /*
>> - * Check whether an ID can be stored into the corresponding guest table.
>> + * vgic_its_check_id - Check whether an ID can be stored into
>> + * the corresponding guest table.
>> + *
>>   * 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 accessible).
>>   * For this we have to read the respective first level entry.
>> + *
>> + * @its: its handle
>> + * @baser: GITS_BASER<n> register
>> + * @id: id of the device/collection
>> + * @eaddr: output gpa of the corresponding table entry
>> + *
>> + * Return: 0 on success, -EINVAL if @id is out of range, -EFAULT if
>> + * the address cannot be computed or is not valid
>>   */
>> -static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>> -			      gpa_t *eaddr)
>> +static int vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>> +			     gpa_t *eaddr)
>>  {
>>  	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>>  	u64 indirect_ptr, type = GITS_BASER_TYPE(baser);
>> @@ -703,50 +713,56 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>>  	int index;
>>  	gfn_t gfn;
>>  
>> +	if (!(baser & GITS_BASER_VALID))
>> +		return -EFAULT;
>> +
>>  	switch (type) {
>>  	case GITS_BASER_TYPE_DEVICE:
>>  		if (id >= BIT_ULL(VITS_TYPER_DEVBITS))
>> -			return false;
>> +			return -EINVAL;
>>  		break;
>>  	case GITS_BASER_TYPE_COLLECTION:
>>  		/* as GITS_TYPER.CIL == 0, ITS supports 16-bit collection ID */
>>  		if (id >= BIT_ULL(16))
>> -			return false;
>> +			return -EINVAL;
>>  		break;
>>  	default:
>> -		return false;
>> +		return -EINVAL;
>>  	}
>>  
>>  	if (!(baser & GITS_BASER_INDIRECT)) {
>>  		phys_addr_t addr;
>>  
>>  		if (id >= (l1_tbl_size / esz))
>> -			return false;
>> +			return -EINVAL;
>>  
>>  		addr = BASER_ADDRESS(baser) + id * esz;
>>  		gfn = addr >> PAGE_SHIFT;
>>  
>>  		if (eaddr)
>>  			*eaddr = addr;
>> -		return kvm_is_visible_gfn(its->dev->kvm, gfn);
>> +		if (kvm_is_visible_gfn(its->dev->kvm, gfn))
>> +			return 0;
>> +		else
>> +			return -EFAULT;
>>  	}
>>  
>>  	/* calculate and check the index into the 1st level */
>>  	index = id / (SZ_64K / esz);
>>  	if (index >= (l1_tbl_size / sizeof(u64)))
>> -		return false;
>> +		return -EINVAL;
>>  
>>  	/* Each 1st level entry is represented by a 64-bit value. */
>>  	if (kvm_read_guest(its->dev->kvm,
>>  			   BASER_ADDRESS(baser) + index * sizeof(indirect_ptr),
>>  			   &indirect_ptr, sizeof(indirect_ptr)))
>> -		return false;
>> +		return -EFAULT;
>>  
>>  	indirect_ptr = le64_to_cpu(indirect_ptr);
>>  
>>  	/* check the valid bit of the first level entry */
>>  	if (!(indirect_ptr & BIT_ULL(63)))
>> -		return false;
>> +		return -EFAULT;
>>  
>>  	/*
>>  	 * Mask the guest physical address and calculate the frame number.
>> @@ -762,7 +778,10 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>>  
>>  	if (eaddr)
>>  		*eaddr = indirect_ptr;
>> -	return kvm_is_visible_gfn(its->dev->kvm, gfn);
>> +	if (kvm_is_visible_gfn(its->dev->kvm, gfn))
>> +		return 0;
>> +	else
>> +		return -EFAULT;
>>  }
>>  
>>  static int vgic_its_alloc_collection(struct vgic_its *its,
>> @@ -771,7 +790,7 @@ static int vgic_its_alloc_collection(struct vgic_its *its,
>>  {
>>  	struct its_collection *collection;
>>  
>> -	if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
>> +	if (vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
>>  		return E_ITS_MAPC_COLLECTION_OOR;
>>  
>>  	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
>> @@ -943,7 +962,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>>  	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
>>  	struct its_device *device;
>>  
>> -	if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
>> +	if (vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
>>  		return E_ITS_MAPD_DEVICE_OOR;
>>  
>>  	if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
>> @@ -2060,9 +2079,9 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
>>  		int ret;
>>  		gpa_t eaddr;
>>  
>> -		if (!vgic_its_check_id(its, baser,
>> -				       dev->device_id, &eaddr))
>> -			return -EINVAL;
>> +		ret = vgic_its_check_id(its, baser, dev->device_id, &eaddr);
>> +		if (ret)
>> +			return ret;
>>  
>>  		ret = vgic_its_save_itt(its, dev);
>>  		if (ret)
>> -- 
>> 2.5.5
>>

Re: [PATCH v2 03/10] KVM: arm/arm64: vgic-its: Improve error reporting on device table save

[Christoffer Dall]

On Fri, Oct 13, 2017 at 04:22:25PM +0200, Auger Eric wrote:
> Hi,
> 
> On 13/10/2017 15:16, Christoffer Dall wrote:
> > On Wed, Sep 27, 2017 at 03:28:33PM +0200, Eric Auger wrote:
> >> At the moment the device table save() returns -EINVAL if
> >> vgic_its_check_id() fails to return the gpa of the entry
> >> associated to the device/collection id. Let vgic_its_check_id()
> >> return an int instead of a bool and return a more precised
> >> error value:
> >> - EINVAL in case the id is out of range
> >> - EFAULT if the gpa is not provisionned or is not valid
> >>
> > 
> > This is just to ease debugging, yes?
> 
> I understood user-space should be able to discriminate between bad guest
> programming and values corrupted by the userspace (regs for instance).
> In first case QEMU should not abort. In latter case it should abort.

So what is userspace supposed to do in the first case?

> 
> In vgic_its_check_id we are checking the L1 entry validity bit and in
> case it is invalid we can't compute the GPA of the entry. I was thinking
> we should return -EFAULT in that case. But maybe returning EFAULT in
> case the BASER<n> address is not reachable also is wrong because that
> may be caused by the userspace writing a wrong value. Sigh ...
> 

I think if either userspace or the guest programmed something that
cannot be traversed, then you just don't save/restore the ITS properly,
because it's broken anyway, so I don't think we need to replicate the
*same broken state* at the destination.

Maybe I'm missing part of the picture here.

Thanks,
-Christoffer

Re: [PATCH v2 03/10] KVM: arm/arm64: vgic-its: Improve error reporting on device table save

[Auger Eric]

Hi Christoffer,

On 13/10/2017 19:56, Christoffer Dall wrote:
> On Fri, Oct 13, 2017 at 04:22:25PM +0200, Auger Eric wrote:
>> Hi,
>>
>> On 13/10/2017 15:16, Christoffer Dall wrote:
>>> On Wed, Sep 27, 2017 at 03:28:33PM +0200, Eric Auger wrote:
>>>> At the moment the device table save() returns -EINVAL if
>>>> vgic_its_check_id() fails to return the gpa of the entry
>>>> associated to the device/collection id. Let vgic_its_check_id()
>>>> return an int instead of a bool and return a more precised
>>>> error value:
>>>> - EINVAL in case the id is out of range
>>>> - EFAULT if the gpa is not provisionned or is not valid
>>>>
>>>
>>> This is just to ease debugging, yes?
>>
>> I understood user-space should be able to discriminate between bad guest
>> programming and values corrupted by the userspace (regs for instance).
>> In first case QEMU should not abort. In latter case it should abort.
> 
> So what is userspace supposed to do in the first case?

I was referring to https://www.spinics.net/lists/kvm/msg148791.html.
QEMU is supposed to write a message in that case but not cause an abort().

This is what is actually implemented on QEMU side. In case the ioctl
returns -EFAULT, we don't abort but simply warn. However at the moment
we return -EINVAL in some circumstances where - I think - we should
return -EFAULT. Hence this patch attempting to be more precise on the
cause of the failure instead of abruptly returning -EINVAL here.

Thanks

Eric
> 
>>
>> In vgic_its_check_id we are checking the L1 entry validity bit and in
>> case it is invalid we can't compute the GPA of the entry. I was thinking
>> we should return -EFAULT in that case. But maybe returning EFAULT in
>> case the BASER<n> address is not reachable also is wrong because that
>> may be caused by the userspace writing a wrong value. Sigh ...
>>
> 
> I think if either userspace or the guest programmed something that
> cannot be traversed, then you just don't save/restore the ITS properly,
> because it's broken anyway, so I don't think we need to replicate the
> *same broken state* at the destination.
> 
> Maybe I'm missing part of the picture here.

> 
> Thanks,
> -Christoffer
> 

Re: [PATCH v2 03/10] KVM: arm/arm64: vgic-its: Improve error reporting on device table save

[Christoffer Dall]

On Sat, Oct 14, 2017 at 10:52:45AM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 13/10/2017 19:56, Christoffer Dall wrote:
> > On Fri, Oct 13, 2017 at 04:22:25PM +0200, Auger Eric wrote:
> >> Hi,
> >>
> >> On 13/10/2017 15:16, Christoffer Dall wrote:
> >>> On Wed, Sep 27, 2017 at 03:28:33PM +0200, Eric Auger wrote:
> >>>> At the moment the device table save() returns -EINVAL if
> >>>> vgic_its_check_id() fails to return the gpa of the entry
> >>>> associated to the device/collection id. Let vgic_its_check_id()
> >>>> return an int instead of a bool and return a more precised
> >>>> error value:
> >>>> - EINVAL in case the id is out of range
> >>>> - EFAULT if the gpa is not provisionned or is not valid
> >>>>
> >>>
> >>> This is just to ease debugging, yes?
> >>
> >> I understood user-space should be able to discriminate between bad guest
> >> programming and values corrupted by the userspace (regs for instance).
> >> In first case QEMU should not abort. In latter case it should abort.
> > 
> > So what is userspace supposed to do in the first case?
> 
> I was referring to https://www.spinics.net/lists/kvm/msg148791.html.
> QEMU is supposed to write a message in that case but not cause an abort().
> 
> This is what is actually implemented on QEMU side. In case the ioctl
> returns -EFAULT, we don't abort but simply warn. However at the moment
> we return -EINVAL in some circumstances where - I think - we should
> return -EFAULT. Hence this patch attempting to be more precise on the
> cause of the failure instead of abruptly returning -EINVAL here.
> 

ok, thanks makes sense.  Thanks for sharing the background.

-Christoffer

[PATCH v2 04/10] KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables

[Eric Auger]

At the moment we don't properly check the GITS_BASER<n>.Valid
bit before saving the collection and device tables.

On Collection table save() we use the gpa field whereas the Valid bit
should be used. On device table save() there is no check. This can
cause various bugs, among which a subsequent fault when accessing
the table in guest memory.

Let's systematically check the Valid bit before doing anything.

We also unifomize the code between save and restore.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index c1f7972..60ecf91 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2067,11 +2067,12 @@ static int vgic_its_device_cmp(void *priv, struct list_head *a,
 static int vgic_its_save_device_tables(struct vgic_its *its)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
+	u64 baser = its->baser_device_table;
 	struct its_device *dev;
 	int dte_esz = abi->dte_esz;
-	u64 baser;
 
-	baser = its->baser_device_table;
+	if (!(baser & GITS_BASER_VALID))
+		return 0;
 
 	list_sort(NULL, &its->device_list, vgic_its_device_cmp);
 
@@ -2217,17 +2218,17 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
 static int vgic_its_save_collection_table(struct vgic_its *its)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
+	u64 baser = its->baser_coll_table;
+	gpa_t gpa = BASER_ADDRESS(baser);
 	struct its_collection *collection;
 	u64 val;
-	gpa_t gpa;
 	size_t max_size, filled = 0;
 	int ret, cte_esz = abi->cte_esz;
 
-	gpa = BASER_ADDRESS(its->baser_coll_table);
-	if (!gpa)
+	if (!(baser & GITS_BASER_VALID))
 		return 0;
 
-	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
+	max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
 
 	list_for_each_entry(collection, &its->collection_list, coll_list) {
 		ret = vgic_its_save_cte(its, collection, gpa, cte_esz);
@@ -2258,17 +2259,18 @@ static int vgic_its_save_collection_table(struct vgic_its *its)
 static int vgic_its_restore_collection_table(struct vgic_its *its)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
+	u64 baser = its->baser_coll_table;
 	int cte_esz = abi->cte_esz;
 	size_t max_size, read = 0;
 	gpa_t gpa;
 	int ret;
 
-	if (!(its->baser_coll_table & GITS_BASER_VALID))
+	if (!(baser & GITS_BASER_VALID))
 		return 0;
 
-	gpa = BASER_ADDRESS(its->baser_coll_table);
+	gpa = BASER_ADDRESS(baser);
 
-	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
+	max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
 
 	while (read < max_size) {
 		ret = vgic_its_restore_cte(its, gpa, cte_esz);
-- 
2.5.5

Re: [PATCH v2 04/10] KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables

[Andre Przywara]

Hi,

On 27/09/17 14:28, Eric Auger wrote:
> At the moment we don't properly check the GITS_BASER<n>.Valid
> bit before saving the collection and device tables.
> 
> On Collection table save() we use the gpa field whereas the Valid bit
> should be used. On device table save() there is no check. This can
> cause various bugs, among which a subsequent fault when accessing
> the table in guest memory.
> 
> Let's systematically check the Valid bit before doing anything.
> 
> We also unifomize the code between save and restore.

          uniformize

Apart from that:

> Signed-off-by: Eric Auger <eric.auger@redhat.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  virt/kvm/arm/vgic/vgic-its.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index c1f7972..60ecf91 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2067,11 +2067,12 @@ static int vgic_its_device_cmp(void *priv, struct list_head *a,
>  static int vgic_its_save_device_tables(struct vgic_its *its)
>  {
>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> +	u64 baser = its->baser_device_table;
>  	struct its_device *dev;
>  	int dte_esz = abi->dte_esz;
> -	u64 baser;
>  
> -	baser = its->baser_device_table;
> +	if (!(baser & GITS_BASER_VALID))
> +		return 0;
>  
>  	list_sort(NULL, &its->device_list, vgic_its_device_cmp);
>  
> @@ -2217,17 +2218,17 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
>  static int vgic_its_save_collection_table(struct vgic_its *its)
>  {
>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> +	u64 baser = its->baser_coll_table;
> +	gpa_t gpa = BASER_ADDRESS(baser);
>  	struct its_collection *collection;
>  	u64 val;
> -	gpa_t gpa;
>  	size_t max_size, filled = 0;
>  	int ret, cte_esz = abi->cte_esz;
>  
> -	gpa = BASER_ADDRESS(its->baser_coll_table);
> -	if (!gpa)
> +	if (!(baser & GITS_BASER_VALID))
>  		return 0;
>  
> -	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
> +	max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>  
>  	list_for_each_entry(collection, &its->collection_list, coll_list) {
>  		ret = vgic_its_save_cte(its, collection, gpa, cte_esz);
> @@ -2258,17 +2259,18 @@ static int vgic_its_save_collection_table(struct vgic_its *its)
>  static int vgic_its_restore_collection_table(struct vgic_its *its)
>  {
>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> +	u64 baser = its->baser_coll_table;
>  	int cte_esz = abi->cte_esz;
>  	size_t max_size, read = 0;
>  	gpa_t gpa;
>  	int ret;
>  
> -	if (!(its->baser_coll_table & GITS_BASER_VALID))
> +	if (!(baser & GITS_BASER_VALID))
>  		return 0;
>  
> -	gpa = BASER_ADDRESS(its->baser_coll_table);
> +	gpa = BASER_ADDRESS(baser);
>  
> -	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
> +	max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>  
>  	while (read < max_size) {
>  		ret = vgic_its_restore_cte(its, gpa, cte_esz);
> 

Re: [PATCH v2 04/10] KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables

[Christoffer Dall]

On Wed, Sep 27, 2017 at 03:28:34PM +0200, Eric Auger wrote:
> At the moment we don't properly check the GITS_BASER<n>.Valid
> bit before saving the collection and device tables.
> 
> On Collection table save() we use the gpa field whereas the Valid bit

'Collection table save()' looks weird. Just use the actual function name
when referring to logic in the code: vgic_its_save_collection_table().

> should be used. On device table save() there is no check. This can
> cause various bugs, among which a subsequent fault when accessing
> the table in guest memory.
> 
> Let's systematically check the Valid bit before doing anything.
> 
> We also unifomize the code between save and restore.

Otherwise:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index c1f7972..60ecf91 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2067,11 +2067,12 @@ static int vgic_its_device_cmp(void *priv, struct list_head *a,
>  static int vgic_its_save_device_tables(struct vgic_its *its)
>  {
>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> +	u64 baser = its->baser_device_table;
>  	struct its_device *dev;
>  	int dte_esz = abi->dte_esz;
> -	u64 baser;
>  
> -	baser = its->baser_device_table;
> +	if (!(baser & GITS_BASER_VALID))
> +		return 0;
>  
>  	list_sort(NULL, &its->device_list, vgic_its_device_cmp);
>  
> @@ -2217,17 +2218,17 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
>  static int vgic_its_save_collection_table(struct vgic_its *its)
>  {
>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> +	u64 baser = its->baser_coll_table;
> +	gpa_t gpa = BASER_ADDRESS(baser);
>  	struct its_collection *collection;
>  	u64 val;
> -	gpa_t gpa;
>  	size_t max_size, filled = 0;
>  	int ret, cte_esz = abi->cte_esz;
>  
> -	gpa = BASER_ADDRESS(its->baser_coll_table);
> -	if (!gpa)
> +	if (!(baser & GITS_BASER_VALID))
>  		return 0;
>  
> -	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
> +	max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>  
>  	list_for_each_entry(collection, &its->collection_list, coll_list) {
>  		ret = vgic_its_save_cte(its, collection, gpa, cte_esz);
> @@ -2258,17 +2259,18 @@ static int vgic_its_save_collection_table(struct vgic_its *its)
>  static int vgic_its_restore_collection_table(struct vgic_its *its)
>  {
>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> +	u64 baser = its->baser_coll_table;
>  	int cte_esz = abi->cte_esz;
>  	size_t max_size, read = 0;
>  	gpa_t gpa;
>  	int ret;
>  
> -	if (!(its->baser_coll_table & GITS_BASER_VALID))
> +	if (!(baser & GITS_BASER_VALID))
>  		return 0;
>  
> -	gpa = BASER_ADDRESS(its->baser_coll_table);
> +	gpa = BASER_ADDRESS(baser);
>  
> -	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
> +	max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>  
>  	while (read < max_size) {
>  		ret = vgic_its_restore_cte(its, gpa, cte_esz);
> -- 
> 2.5.5
> 

[PATCH v2 05/10] KVM: arm/arm64: vgic-its: Check GITS_CBASER validity before processing commands

[Eric Auger]

At the moment vgic_its_process_commands() does not
check the CBASER is valid before processing any command.
Let's fix that.

Also rename cbaser local variable into cbaser_pa to avoid
any confusion with the full register.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 60ecf91..720552c 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1301,17 +1301,20 @@ static void vgic_mmio_write_its_cbaser(struct kvm *kvm, struct vgic_its *its,
 /* Must be called with the cmd_lock held. */
 static void vgic_its_process_commands(struct kvm *kvm, struct vgic_its *its)
 {
-	gpa_t cbaser;
+	gpa_t cbaser_pa;
 	u64 cmd_buf[4];
 
-	/* Commands are only processed when the ITS is enabled. */
-	if (!its->enabled)
+	/*
+	 * Commands are only processed when the ITS is enabled and
+	 * CBASER is valid
+	 */
+	if (!its->enabled || (!(its->cbaser & GITS_CBASER_VALID)))
 		return;
 
-	cbaser = CBASER_ADDRESS(its->cbaser);
+	cbaser_pa = CBASER_ADDRESS(its->cbaser);
 
 	while (its->cwriter != its->creadr) {
-		int ret = kvm_read_guest(kvm, cbaser + its->creadr,
+		int ret = kvm_read_guest(kvm, cbaser_pa + its->creadr,
 					 cmd_buf, ITS_CMD_SIZE);
 		/*
 		 * If kvm_read_guest() fails, this could be due to the guest
-- 
2.5.5

Re: [PATCH v2 05/10] KVM: arm/arm64: vgic-its: Check GITS_CBASER validity before processing commands

[Andre Przywara]

Hi,

On 27/09/17 14:28, Eric Auger wrote:
> At the moment vgic_its_process_commands() does not
> check the CBASER is valid before processing any command.
> Let's fix that.

Good catch, but actually it goes a bit further:

"When GITS_CTLR.Enabled is written from 0 to 1 behavior is UNPREDICTABLE
if any of the following conditions are true:
· GITS_CBASER.Valid == 0.
...
"

So I think we need to check it upon setting CTLR.Enabled.
I did this in Xen, but forgot to fix this in KVM as well:
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/vgic-v3-its.c;hb=HEAD#l1164

Cheers,
Andre.

> 
> Also rename cbaser local variable into cbaser_pa to avoid
> any confusion with the full register.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 60ecf91..720552c 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1301,17 +1301,20 @@ static void vgic_mmio_write_its_cbaser(struct kvm *kvm, struct vgic_its *its,
>  /* Must be called with the cmd_lock held. */
>  static void vgic_its_process_commands(struct kvm *kvm, struct vgic_its *its)
>  {
> -	gpa_t cbaser;
> +	gpa_t cbaser_pa;
>  	u64 cmd_buf[4];
>  
> -	/* Commands are only processed when the ITS is enabled. */
> -	if (!its->enabled)
> +	/*
> +	 * Commands are only processed when the ITS is enabled and
> +	 * CBASER is valid
> +	 */
> +	if (!its->enabled || (!(its->cbaser & GITS_CBASER_VALID)))
>  		return;
>  
> -	cbaser = CBASER_ADDRESS(its->cbaser);
> +	cbaser_pa = CBASER_ADDRESS(its->cbaser);
>  
>  	while (its->cwriter != its->creadr) {
> -		int ret = kvm_read_guest(kvm, cbaser + its->creadr,
> +		int ret = kvm_read_guest(kvm, cbaser_pa + its->creadr,
>  					 cmd_buf, ITS_CMD_SIZE);
>  		/*
>  		 * If kvm_read_guest() fails, this could be due to the guest
> 

Re: [PATCH v2 05/10] KVM: arm/arm64: vgic-its: Check GITS_CBASER validity before processing commands

[Auger Eric]

Hi Andre,

On 06/10/2017 16:38, Andre Przywara wrote:
> Hi,
> 
> On 27/09/17 14:28, Eric Auger wrote:
>> At the moment vgic_its_process_commands() does not
>> check the CBASER is valid before processing any command.
>> Let's fix that.
> 
> Good catch, but actually it goes a bit further:
> 
> "When GITS_CTLR.Enabled is written from 0 to 1 behavior is UNPREDICTABLE
> if any of the following conditions are true:
> · GITS_CBASER.Valid == 0.
> ...
> "
> 
> So I think we need to check it upon setting CTLR.Enabled.
> I did this in Xen, but forgot to fix this in KVM as well:
> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/vgic-v3-its.c;hb=HEAD#l1164

OK. I will add this check.

Thanks

Eric
> 
> Cheers,
> Andre.
> 
>>
>> Also rename cbaser local variable into cbaser_pa to avoid
>> any confusion with the full register.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 60ecf91..720552c 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -1301,17 +1301,20 @@ static void vgic_mmio_write_its_cbaser(struct kvm *kvm, struct vgic_its *its,
>>  /* Must be called with the cmd_lock held. */
>>  static void vgic_its_process_commands(struct kvm *kvm, struct vgic_its *its)
>>  {
>> -	gpa_t cbaser;
>> +	gpa_t cbaser_pa;
>>  	u64 cmd_buf[4];
>>  
>> -	/* Commands are only processed when the ITS is enabled. */
>> -	if (!its->enabled)
>> +	/*
>> +	 * Commands are only processed when the ITS is enabled and
>> +	 * CBASER is valid
>> +	 */
>> +	if (!its->enabled || (!(its->cbaser & GITS_CBASER_VALID)))
>>  		return;
>>  
>> -	cbaser = CBASER_ADDRESS(its->cbaser);
>> +	cbaser_pa = CBASER_ADDRESS(its->cbaser);
>>  
>>  	while (its->cwriter != its->creadr) {
>> -		int ret = kvm_read_guest(kvm, cbaser + its->creadr,
>> +		int ret = kvm_read_guest(kvm, cbaser_pa + its->creadr,
>>  					 cmd_buf, ITS_CMD_SIZE);
>>  		/*
>>  		 * If kvm_read_guest() fails, this could be due to the guest
>>

[PATCH v2 06/10] KVM: arm/arm64: vgic-its: Always attempt to save/restore device and collection tables

[Eric Auger]

In case the device table save fails, we currently do not
attempt to save the collection table. However it may
happen that the device table fails because the structures
in memory are inconsistent with device GITS_BASER however
this does not mean collection backup can't be performed and
wouldn't succeed. Same on restore path. Without this patch,
after a reset and in case the device table fails in case of
L1 entry not valid, the guest gets stuck on restore.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

candidate to be CC'ed stable
---
 virt/kvm/arm/vgic/vgic-its.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 720552c..9e6b556 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2304,12 +2304,9 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
 	}
 
 	ret = vgic_its_save_device_tables(its);
-	if (ret)
-		goto out;
 
-	ret = vgic_its_save_collection_table(its);
+	ret |= vgic_its_save_collection_table(its);
 
-out:
 	unlock_all_vcpus(kvm);
 	mutex_unlock(&its->its_lock);
 	mutex_unlock(&kvm->lock);
@@ -2336,11 +2333,9 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
 	}
 
 	ret = vgic_its_restore_collection_table(its);
-	if (ret)
-		goto out;
 
-	ret = vgic_its_restore_device_tables(its);
-out:
+	ret |= vgic_its_restore_device_tables(its);
+
 	unlock_all_vcpus(kvm);
 	mutex_unlock(&its->its_lock);
 	mutex_unlock(&kvm->lock);
-- 
2.5.5

Re: [PATCH v2 06/10] KVM: arm/arm64: vgic-its: Always attempt to save/restore device and collection tables

[Andre Przywara]

Hi,

On 27/09/17 14:28, Eric Auger wrote:
> In case the device table save fails, we currently do not
> attempt to save the collection table. However it may
> happen that the device table fails because the structures
> in memory are inconsistent with device GITS_BASER however
> this does not mean collection backup can't be performed and
> wouldn't succeed. Same on restore path. Without this patch,
> after a reset and in case the device table fails in case of
> L1 entry not valid, the guest gets stuck on restore.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> candidate to be CC'ed stable
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 720552c..9e6b556 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2304,12 +2304,9 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
>  	}
>  
>  	ret = vgic_its_save_device_tables(its);
> -	if (ret)
> -		goto out;
>  
> -	ret = vgic_its_save_collection_table(its);
> +	ret |= vgic_its_save_collection_table(its);
>  
> -out:
>  	unlock_all_vcpus(kvm);
>  	mutex_unlock(&its->its_lock);
>  	mutex_unlock(&kvm->lock);
> @@ -2336,11 +2333,9 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
>  	}
>  
>  	ret = vgic_its_restore_collection_table(its);

While the save functions above and this _v0 function here all use the
standard C return semantics (==0 on success, failure otherwise),
vgic_its_restore_collection_table() and the function call below can
return 1 if successful, AFAICS. I don't think this handled correctly here?

Cheers,
Andre.

> -	if (ret)
> -		goto out;
>  
> -	ret = vgic_its_restore_device_tables(its);
> -out:
> +	ret |= vgic_its_restore_device_tables(its);
> +
>  	unlock_all_vcpus(kvm);
>  	mutex_unlock(&its->its_lock);
>  	mutex_unlock(&kvm->lock);
> 

Re: [PATCH v2 06/10] KVM: arm/arm64: vgic-its: Always attempt to save/restore device and collection tables

[Auger Eric]

Hi Andre,

On 06/10/2017 16:38, Andre Przywara wrote:
> Hi,
> 
> On 27/09/17 14:28, Eric Auger wrote:
>> In case the device table save fails, we currently do not
>> attempt to save the collection table. However it may
>> happen that the device table fails because the structures
>> in memory are inconsistent with device GITS_BASER however
>> this does not mean collection backup can't be performed and
>> wouldn't succeed. Same on restore path. Without this patch,
>> after a reset and in case the device table fails in case of
>> L1 entry not valid, the guest gets stuck on restore.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> candidate to be CC'ed stable
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 720552c..9e6b556 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -2304,12 +2304,9 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
>>  	}
>>  
>>  	ret = vgic_its_save_device_tables(its);
>> -	if (ret)
>> -		goto out;
>>  
>> -	ret = vgic_its_save_collection_table(its);
>> +	ret |= vgic_its_save_collection_table(its);
>>  
>> -out:
>>  	unlock_all_vcpus(kvm);
>>  	mutex_unlock(&its->its_lock);
>>  	mutex_unlock(&kvm->lock);
>> @@ -2336,11 +2333,9 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
>>  	}
>>  
>>  	ret = vgic_its_restore_collection_table(its);
> 
> While the save functions above and this _v0 function here all use the
> standard C return semantics (==0 on success, failure otherwise),
> vgic_its_restore_collection_table() and the function call below can
> return 1 if successful, AFAICS. I don't think this handled correctly here?
After 01/10, vgic_its_restore_device_tables() can't return +1 anymore.
However you're right vgic_its_restore_collection_table can restore + 1
if the collection table is completely filled and this is wrong. I will
fix that.

Thanks

Eric
> 
> Cheers,
> Andre.
> 
>> -	if (ret)
>> -		goto out;
>>  
>> -	ret = vgic_its_restore_device_tables(its);
>> -out:
>> +	ret |= vgic_its_restore_device_tables(its);
>> +
>>  	unlock_all_vcpus(kvm);
>>  	mutex_unlock(&its->its_lock);
>>  	mutex_unlock(&kvm->lock);
>>

[PATCH v2 07/10] KVM: arm/arm64: vgic-its: new helper functions to free the caches

[Eric Auger]

From: wanghaibin <wanghaibin.wang@huawei.com>

We create 2 new functions that frees the device and
collection lists. this is currently called by vgic_its_destroy()
and we will add other callers in subsequent patches.

We also remove the check on its->device_list.next as it looks
unnecessary:

The kvm device is removed by kvm_destroy_devices which loops on
all the devices added to kvm->devices. kvm_ioctl_create_device
only adds the device to kvm_devices once the lists have been
initialized (in vgic_create_its).

We also move vgic_its_free_device to prepare for new callers.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>

---
[Eric] removed its->device_list.next which is not needed as
pointed out by Wanghaibin. Reword the commit message
---
 virt/kvm/arm/vgic/vgic-its.c | 76 ++++++++++++++++++++++++--------------------
 1 file changed, 41 insertions(+), 35 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 9e6b556..0df6d5f 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -611,6 +611,45 @@ static void its_free_ite(struct kvm *kvm, struct its_ite *ite)
 	kfree(ite);
 }
 
+static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
+{
+	struct its_ite *ite, *tmp;
+
+	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
+		its_free_ite(kvm, ite);
+	list_del(&dev->dev_list);
+	kfree(dev);
+}
+
+static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
+{
+	struct list_head *cur, *temp;
+
+	mutex_lock(&its->its_lock);
+	list_for_each_safe(cur, temp, &its->device_list) {
+		struct its_device *dev;
+
+		dev = list_entry(cur, struct its_device, dev_list);
+		vgic_its_free_device(kvm, dev);
+	}
+	mutex_unlock(&its->its_lock);
+}
+
+static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its *its)
+{
+	struct list_head *cur, *temp;
+
+	list_for_each_safe(cur, temp, &its->collection_list) {
+		struct its_collection *coll;
+
+		coll = list_entry(cur, struct its_collection, coll_list);
+		list_del(cur);
+		kfree(coll);
+	}
+	mutex_unlock(&its->its_lock);
+}
+
+
 static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
 {
 	return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT_ULL(size) - 1);
@@ -1634,46 +1673,13 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
 	return vgic_its_set_abi(its, NR_ITS_ABIS - 1);
 }
 
-static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
-{
-	struct its_ite *ite, *tmp;
-
-	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
-		its_free_ite(kvm, ite);
-	list_del(&dev->dev_list);
-	kfree(dev);
-}
-
 static void vgic_its_destroy(struct kvm_device *kvm_dev)
 {
 	struct kvm *kvm = kvm_dev->kvm;
 	struct vgic_its *its = kvm_dev->private;
-	struct list_head *cur, *temp;
-
-	/*
-	 * We may end up here without the lists ever having been initialized.
-	 * Check this and bail out early to avoid dereferencing a NULL pointer.
-	 */
-	if (!its->device_list.next)
-		return;
-
-	mutex_lock(&its->its_lock);
-	list_for_each_safe(cur, temp, &its->device_list) {
-		struct its_device *dev;
-
-		dev = list_entry(cur, struct its_device, dev_list);
-		vgic_its_free_device(kvm, dev);
-	}
-
-	list_for_each_safe(cur, temp, &its->collection_list) {
-		struct its_collection *coll;
-
-		coll = list_entry(cur, struct its_collection, coll_list);
-		list_del(cur);
-		kfree(coll);
-	}
-	mutex_unlock(&its->its_lock);
 
+	vgic_its_free_device_list(kvm, its);
+	vgic_its_free_collection_list(kvm, its);
 	kfree(its);
 }
 
-- 
2.5.5

Re: [PATCH v2 07/10] KVM: arm/arm64: vgic-its: new helper functions to free the caches

[Christoffer Dall]

On Wed, Sep 27, 2017 at 03:28:37PM +0200, Eric Auger wrote:
> From: wanghaibin <wanghaibin.wang@huawei.com>
> 
> We create 2 new functions that frees the device and

           two                   free

> collection lists. this is currently called by vgic_its_destroy()

                    These are

> and we will add other callers in subsequent patches.
> 
> We also remove the check on its->device_list.next as it looks
> unnecessary:

Could you elude to why you're doing this in the first place in the next
version of the commit message?  Thanks.

> 
> The kvm device is removed by kvm_destroy_devices which loops on
> all the devices added to kvm->devices. kvm_ioctl_create_device
> only adds the device to kvm_devices once the lists have been
> initialized (in vgic_create_its).

I don't understand what this paragraph is trying to tell me beyond what
some code already does irrelevant to this patch?

> 
> We also move vgic_its_free_device to prepare for new callers.
> 
> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> [Eric] removed its->device_list.next which is not needed as
> pointed out by Wanghaibin. Reword the commit message
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 76 ++++++++++++++++++++++++--------------------
>  1 file changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 9e6b556..0df6d5f 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -611,6 +611,45 @@ static void its_free_ite(struct kvm *kvm, struct its_ite *ite)
>  	kfree(ite);
>  }
>  
> +static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
> +{
> +	struct its_ite *ite, *tmp;
> +
> +	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
> +		its_free_ite(kvm, ite);
> +	list_del(&dev->dev_list);
> +	kfree(dev);
> +}
> +
> +static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
> +{
> +	struct list_head *cur, *temp;
> +
> +	mutex_lock(&its->its_lock);
> +	list_for_each_safe(cur, temp, &its->device_list) {
> +		struct its_device *dev;
> +
> +		dev = list_entry(cur, struct its_device, dev_list);
> +		vgic_its_free_device(kvm, dev);
> +	}
> +	mutex_unlock(&its->its_lock);

this changes semantics from locking across freeing both devices and
collections to taking the locks separately.  Is that valid?

> +}
> +
> +static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its *its)
> +{
> +	struct list_head *cur, *temp;
> +
> +	list_for_each_safe(cur, temp, &its->collection_list) {
> +		struct its_collection *coll;
> +
> +		coll = list_entry(cur, struct its_collection, coll_list);
> +		list_del(cur);
> +		kfree(coll);
> +	}
> +	mutex_unlock(&its->its_lock);

no mutex_lock ?

> +}
> +
> +
>  static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
>  {
>  	return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT_ULL(size) - 1);
> @@ -1634,46 +1673,13 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
>  	return vgic_its_set_abi(its, NR_ITS_ABIS - 1);
>  }
>  
> -static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
> -{
> -	struct its_ite *ite, *tmp;
> -
> -	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
> -		its_free_ite(kvm, ite);
> -	list_del(&dev->dev_list);
> -	kfree(dev);
> -}
> -
>  static void vgic_its_destroy(struct kvm_device *kvm_dev)
>  {
>  	struct kvm *kvm = kvm_dev->kvm;
>  	struct vgic_its *its = kvm_dev->private;
> -	struct list_head *cur, *temp;
> -
> -	/*
> -	 * We may end up here without the lists ever having been initialized.
> -	 * Check this and bail out early to avoid dereferencing a NULL pointer.
> -	 */
> -	if (!its->device_list.next)
> -		return;

I don't think this is valid.  We can actually have a non-initialized
list and without this check, list_for_each_entry_safe in
vgic_its_free_device_list will crash the kernel.

Note that an initialized empty list_head doesn't have head and tail
pointing to NULL, but pointing to the list_head itself.

> -
> -	mutex_lock(&its->its_lock);
> -	list_for_each_safe(cur, temp, &its->device_list) {
> -		struct its_device *dev;
> -
> -		dev = list_entry(cur, struct its_device, dev_list);
> -		vgic_its_free_device(kvm, dev);
> -	}
> -
> -	list_for_each_safe(cur, temp, &its->collection_list) {
> -		struct its_collection *coll;
> -
> -		coll = list_entry(cur, struct its_collection, coll_list);
> -		list_del(cur);
> -		kfree(coll);
> -	}
> -	mutex_unlock(&its->its_lock);
>  
> +	vgic_its_free_device_list(kvm, its);
> +	vgic_its_free_collection_list(kvm, its);
>  	kfree(its);
>  }
>  
> -- 
> 2.5.5
> 

Thanks,
-Christoffer

Re: [PATCH v2 07/10] KVM: arm/arm64: vgic-its: new helper functions to free the caches

[Auger Eric]

Hi Christoffer,

On 13/10/2017 15:35, Christoffer Dall wrote:
> On Wed, Sep 27, 2017 at 03:28:37PM +0200, Eric Auger wrote:
>> From: wanghaibin <wanghaibin.wang@huawei.com>
>>
>> We create 2 new functions that frees the device and
> 
>            two                   free
> 
>> collection lists. this is currently called by vgic_its_destroy()
> 
>                     These are
> 
>> and we will add other callers in subsequent patches.
>>
>> We also remove the check on its->device_list.next as it looks
>> unnecessary:
> 
> Could you elude to why you're doing this in the first place in the next
> version of the commit message?  Thanks.
> 
>>
>> The kvm device is removed by kvm_destroy_devices which loops on
>> all the devices added to kvm->devices. kvm_ioctl_create_device
>> only adds the device to kvm_devices once the lists have been
>> initialized (in vgic_create_its).
> 
> I don't understand what this paragraph is trying to tell me beyond what
> some code already does irrelevant to this patch?

This paragraph was an attempt to explain why we could remove the above
check but it looks I need to rephrase ;-)

Thanks

Eric
> 
>>
>> We also move vgic_its_free_device to prepare for new callers.
>>
>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>> [Eric] removed its->device_list.next which is not needed as
>> pointed out by Wanghaibin. Reword the commit message
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 76 ++++++++++++++++++++++++--------------------
>>  1 file changed, 41 insertions(+), 35 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 9e6b556..0df6d5f 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -611,6 +611,45 @@ static void its_free_ite(struct kvm *kvm, struct its_ite *ite)
>>  	kfree(ite);
>>  }
>>  
>> +static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
>> +{
>> +	struct its_ite *ite, *tmp;
>> +
>> +	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
>> +		its_free_ite(kvm, ite);
>> +	list_del(&dev->dev_list);
>> +	kfree(dev);
>> +}
>> +
>> +static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
>> +{
>> +	struct list_head *cur, *temp;
>> +
>> +	mutex_lock(&its->its_lock);
>> +	list_for_each_safe(cur, temp, &its->device_list) {
>> +		struct its_device *dev;
>> +
>> +		dev = list_entry(cur, struct its_device, dev_list);
>> +		vgic_its_free_device(kvm, dev);
>> +	}
>> +	mutex_unlock(&its->its_lock);
> 
> this changes semantics from locking across freeing both devices and
> collections to taking the locks separately.  Is that valid?
> 
>> +}
>> +
>> +static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its *its)
>> +{
>> +	struct list_head *cur, *temp;
>> +
>> +	list_for_each_safe(cur, temp, &its->collection_list) {
>> +		struct its_collection *coll;
>> +
>> +		coll = list_entry(cur, struct its_collection, coll_list);
>> +		list_del(cur);
>> +		kfree(coll);
>> +	}
>> +	mutex_unlock(&its->its_lock);
> 
> no mutex_lock ?
> 
>> +}
>> +
>> +
>>  static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
>>  {
>>  	return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT_ULL(size) - 1);
>> @@ -1634,46 +1673,13 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
>>  	return vgic_its_set_abi(its, NR_ITS_ABIS - 1);
>>  }
>>  
>> -static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
>> -{
>> -	struct its_ite *ite, *tmp;
>> -
>> -	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
>> -		its_free_ite(kvm, ite);
>> -	list_del(&dev->dev_list);
>> -	kfree(dev);
>> -}
>> -
>>  static void vgic_its_destroy(struct kvm_device *kvm_dev)
>>  {
>>  	struct kvm *kvm = kvm_dev->kvm;
>>  	struct vgic_its *its = kvm_dev->private;
>> -	struct list_head *cur, *temp;
>> -
>> -	/*
>> -	 * We may end up here without the lists ever having been initialized.
>> -	 * Check this and bail out early to avoid dereferencing a NULL pointer.
>> -	 */
>> -	if (!its->device_list.next)
>> -		return;
> 
> I don't think this is valid.  We can actually have a non-initialized
> list and without this check, list_for_each_entry_safe in
> vgic_its_free_device_list will crash the kernel.
> 
> Note that an initialized empty list_head doesn't have head and tail
> pointing to NULL, but pointing to the list_head itself.
> 
>> -
>> -	mutex_lock(&its->its_lock);
>> -	list_for_each_safe(cur, temp, &its->device_list) {
>> -		struct its_device *dev;
>> -
>> -		dev = list_entry(cur, struct its_device, dev_list);
>> -		vgic_its_free_device(kvm, dev);
>> -	}
>> -
>> -	list_for_each_safe(cur, temp, &its->collection_list) {
>> -		struct its_collection *coll;
>> -
>> -		coll = list_entry(cur, struct its_collection, coll_list);
>> -		list_del(cur);
>> -		kfree(coll);
>> -	}
>> -	mutex_unlock(&its->its_lock);
>>  
>> +	vgic_its_free_device_list(kvm, its);
>> +	vgic_its_free_collection_list(kvm, its);
>>  	kfree(its);
>>  }
>>  
>> -- 
>> 2.5.5
>>
> 
> Thanks,
> -Christoffer
> 

Re: [PATCH v2 07/10] KVM: arm/arm64: vgic-its: new helper functions to free the caches

[Auger Eric]

Hi Christoffer,

On 13/10/2017 15:35, Christoffer Dall wrote:
> On Wed, Sep 27, 2017 at 03:28:37PM +0200, Eric Auger wrote:
>> From: wanghaibin <wanghaibin.wang@huawei.com>
>>
>> We create 2 new functions that frees the device and
> 
>            two                   free
> 
>> collection lists. this is currently called by vgic_its_destroy()


First my apologies as most of your comments have been left out of the
v3-v4 respin by oversight. Some comments below.
> 
>                     These are
> 
>> and we will add other callers in subsequent patches.
>>
>> We also remove the check on its->device_list.next as it looks
>> unnecessary:
> 
> Could you elude to why you're doing this in the first place in the next
> version of the commit message?  Thanks.
> 
>>
>> The kvm device is removed by kvm_destroy_devices which loops on
>> all the devices added to kvm->devices. kvm_ioctl_create_device
>> only adds the device to kvm_devices once the lists have been
>> initialized (in vgic_create_its).
> 
> I don't understand what this paragraph is trying to tell me beyond what
> some code already does irrelevant to this patch?
> 
>>
>> We also move vgic_its_free_device to prepare for new callers.
>>
>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>> [Eric] removed its->device_list.next which is not needed as
>> pointed out by Wanghaibin. Reword the commit message
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 76 ++++++++++++++++++++++++--------------------
>>  1 file changed, 41 insertions(+), 35 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 9e6b556..0df6d5f 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -611,6 +611,45 @@ static void its_free_ite(struct kvm *kvm, struct its_ite *ite)
>>  	kfree(ite);
>>  }
>>  
>> +static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
>> +{
>> +	struct its_ite *ite, *tmp;
>> +
>> +	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
>> +		its_free_ite(kvm, ite);
>> +	list_del(&dev->dev_list);
>> +	kfree(dev);
>> +}
>> +
>> +static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
>> +{
>> +	struct list_head *cur, *temp;
>> +
>> +	mutex_lock(&its->its_lock);
>> +	list_for_each_safe(cur, temp, &its->device_list) {
>> +		struct its_device *dev;
>> +
>> +		dev = list_entry(cur, struct its_device, dev_list);
>> +		vgic_its_free_device(kvm, dev);
>> +	}
>> +	mutex_unlock(&its->its_lock);
> 


> this changes semantics from locking across freeing both devices and
> collections to taking the locks separately.  Is that valid?

Handling deletion of device and collection separately is valid I think
as MAPC (vgic_its_cmd_handle_mapc) and MAPD(vgic_its_cmd_handle_mapd)
commands do that separately.

However, ..., a collection can be referred by an ITE and I should reset
the ite->collection = NULL for all ITEs referencing a deleted ITE.
vgic_its_free_collection do that.

By the way, vgic_its_unmap_device() is same as vgic_its_free_device() so
I can remove vgic_its_free_device.


> 
>> +}
>> +
>> +static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its *its)
>> +{
>> +	struct list_head *cur, *temp;
>> +
>> +	list_for_each_safe(cur, temp, &its->collection_list) {
>> +		struct its_collection *coll;
>> +
>> +		coll = list_entry(cur, struct its_collection, coll_list);
>> +		list_del(cur);
>> +		kfree(coll);
>> +	}
>> +	mutex_unlock(&its->its_lock);
> 
> no mutex_lock ?
damned.
> 
>> +}
>> +
>> +
>>  static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
>>  {
>>  	return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT_ULL(size) - 1);
>> @@ -1634,46 +1673,13 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
>>  	return vgic_its_set_abi(its, NR_ITS_ABIS - 1);
>>  }
>>  
>> -static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
>> -{
>> -	struct its_ite *ite, *tmp;
>> -
>> -	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
>> -		its_free_ite(kvm, ite);
>> -	list_del(&dev->dev_list);
>> -	kfree(dev);
>> -}
>> -
>>  static void vgic_its_destroy(struct kvm_device *kvm_dev)
>>  {
>>  	struct kvm *kvm = kvm_dev->kvm;
>>  	struct vgic_its *its = kvm_dev->private;
>> -	struct list_head *cur, *temp;
>> -
>> -	/*
>> -	 * We may end up here without the lists ever having been initialized.
>> -	 * Check this and bail out early to avoid dereferencing a NULL pointer.
>> -	 */
>> -	if (!its->device_list.next)
>> -		return;
> 
> I don't think this is valid.  We can actually have a non-initialized
> list and without this check, list_for_each_entry_safe in
> vgic_its_free_device_list will crash the kernel.

I think you agreed on my previous statement:
https://www.spinics.net/lists/kvm-arm/msg27198.html


I understand the sequence is:
1) vm_ioctl_create_device
   |_ ops->create
      |_ vgic_create_its
         INIT_LIST_HEAD(&its->device_list);
	 INIT_LIST_HEAD(&its->collection_list);
      list_add(&dev->vm_node, &kvm->devices);

kvm_destroy_devices
    list_for_each_entry_safe(dev, tmp, &kvm->devices, vm_node) {
       ops->destroy
           |_ vgic_its_destroy

so vgic_its_destroy is called on an its device that was added to the
kvm->devices list. If so the list was created.

Then we have vgic_mmio_write_its_baser() which is new caller introduced
in subsequent patch.

for vgic_mmio_write_its_baser() to be called,  vgic_register_its_iodev
must have been called. This latter is called on set_attr=vgic_its_set_attr
set_attr can be called only if the fd is created. This happens in
kvm_ioctl_create_device after ops->create() has been successful, ie
meaning the lists are created.

What do I miss? What is the case you identified where the device_list is
not initialized?

Thanks

Eric
> 
> Note that an initialized empty list_head doesn't have head and tail
> pointing to NULL, but pointing to the list_head itself.





> 
>> -
>> -	mutex_lock(&its->its_lock);
>> -	list_for_each_safe(cur, temp, &its->device_list) {
>> -		struct its_device *dev;
>> -
>> -		dev = list_entry(cur, struct its_device, dev_list);
>> -		vgic_its_free_device(kvm, dev);
>> -	}
>> -
>> -	list_for_each_safe(cur, temp, &its->collection_list) {
>> -		struct its_collection *coll;
>> -
>> -		coll = list_entry(cur, struct its_collection, coll_list);
>> -		list_del(cur);
>> -		kfree(coll);
>> -	}
>> -	mutex_unlock(&its->its_lock);
>>  
>> +	vgic_its_free_device_list(kvm, its);
>> +	vgic_its_free_collection_list(kvm, its);
>>  	kfree(its);
>>  }
>>  
>> -- 
>> 2.5.5
>>
> 
> Thanks,
> -Christoffer
> 

Re: [PATCH v2 07/10] KVM: arm/arm64: vgic-its: new helper functions to free the caches

[Christoffer Dall]

On Sat, Oct 21, 2017 at 11:02:27AM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 13/10/2017 15:35, Christoffer Dall wrote:
> > On Wed, Sep 27, 2017 at 03:28:37PM +0200, Eric Auger wrote:
> >> From: wanghaibin <wanghaibin.wang@huawei.com>
> >>
> >> We create 2 new functions that frees the device and
> > 
> >            two                   free
> > 
> >> collection lists. this is currently called by vgic_its_destroy()
> 
> 
> First my apologies as most of your comments have been left out of the
> v3-v4 respin by oversight. Some comments below.
> > 
> >                     These are
> > 
> >> and we will add other callers in subsequent patches.
> >>
> >> We also remove the check on its->device_list.next as it looks
> >> unnecessary:
> > 
> > Could you elude to why you're doing this in the first place in the next
> > version of the commit message?  Thanks.
> > 
> >>
> >> The kvm device is removed by kvm_destroy_devices which loops on
> >> all the devices added to kvm->devices. kvm_ioctl_create_device
> >> only adds the device to kvm_devices once the lists have been
> >> initialized (in vgic_create_its).
> > 
> > I don't understand what this paragraph is trying to tell me beyond what
> > some code already does irrelevant to this patch?
> > 
> >>
> >> We also move vgic_its_free_device to prepare for new callers.
> >>
> >> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>
> >> ---
> >> [Eric] removed its->device_list.next which is not needed as
> >> pointed out by Wanghaibin. Reword the commit message
> >> ---
> >>  virt/kvm/arm/vgic/vgic-its.c | 76 ++++++++++++++++++++++++--------------------
> >>  1 file changed, 41 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >> index 9e6b556..0df6d5f 100644
> >> --- a/virt/kvm/arm/vgic/vgic-its.c
> >> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >> @@ -611,6 +611,45 @@ static void its_free_ite(struct kvm *kvm, struct its_ite *ite)
> >>  	kfree(ite);
> >>  }
> >>  
> >> +static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
> >> +{
> >> +	struct its_ite *ite, *tmp;
> >> +
> >> +	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
> >> +		its_free_ite(kvm, ite);
> >> +	list_del(&dev->dev_list);
> >> +	kfree(dev);
> >> +}
> >> +
> >> +static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
> >> +{
> >> +	struct list_head *cur, *temp;
> >> +
> >> +	mutex_lock(&its->its_lock);
> >> +	list_for_each_safe(cur, temp, &its->device_list) {
> >> +		struct its_device *dev;
> >> +
> >> +		dev = list_entry(cur, struct its_device, dev_list);
> >> +		vgic_its_free_device(kvm, dev);
> >> +	}
> >> +	mutex_unlock(&its->its_lock);
> > 
> 
> 
> > this changes semantics from locking across freeing both devices and
> > collections to taking the locks separately.  Is that valid?
> 
> Handling deletion of device and collection separately is valid I think
> as MAPC (vgic_its_cmd_handle_mapc) and MAPD(vgic_its_cmd_handle_mapd)
> commands do that separately.
> 
> However, ..., a collection can be referred by an ITE and I should reset
> the ite->collection = NULL for all ITEs referencing a deleted ITE.
> vgic_its_free_collection do that.
> 
> By the way, vgic_its_unmap_device() is same as vgic_its_free_device() so
> I can remove vgic_its_free_device.
> 
> 
> > 
> >> +}
> >> +
> >> +static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its *its)
> >> +{
> >> +	struct list_head *cur, *temp;
> >> +
> >> +	list_for_each_safe(cur, temp, &its->collection_list) {
> >> +		struct its_collection *coll;
> >> +
> >> +		coll = list_entry(cur, struct its_collection, coll_list);
> >> +		list_del(cur);
> >> +		kfree(coll);
> >> +	}
> >> +	mutex_unlock(&its->its_lock);
> > 
> > no mutex_lock ?
> damned.
> > 
> >> +}
> >> +
> >> +
> >>  static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
> >>  {
> >>  	return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT_ULL(size) - 1);
> >> @@ -1634,46 +1673,13 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
> >>  	return vgic_its_set_abi(its, NR_ITS_ABIS - 1);
> >>  }
> >>  
> >> -static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
> >> -{
> >> -	struct its_ite *ite, *tmp;
> >> -
> >> -	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
> >> -		its_free_ite(kvm, ite);
> >> -	list_del(&dev->dev_list);
> >> -	kfree(dev);
> >> -}
> >> -
> >>  static void vgic_its_destroy(struct kvm_device *kvm_dev)
> >>  {
> >>  	struct kvm *kvm = kvm_dev->kvm;
> >>  	struct vgic_its *its = kvm_dev->private;
> >> -	struct list_head *cur, *temp;
> >> -
> >> -	/*
> >> -	 * We may end up here without the lists ever having been initialized.
> >> -	 * Check this and bail out early to avoid dereferencing a NULL pointer.
> >> -	 */
> >> -	if (!its->device_list.next)
> >> -		return;
> > 
> > I don't think this is valid.  We can actually have a non-initialized
> > list and without this check, list_for_each_entry_safe in
> > vgic_its_free_device_list will crash the kernel.
> 
> I think you agreed on my previous statement:
> https://www.spinics.net/lists/kvm-arm/msg27198.html
> 
> 
> I understand the sequence is:
> 1) vm_ioctl_create_device
>    |_ ops->create
>       |_ vgic_create_its
>          INIT_LIST_HEAD(&its->device_list);
> 	 INIT_LIST_HEAD(&its->collection_list);
>       list_add(&dev->vm_node, &kvm->devices);
> 
> kvm_destroy_devices
>     list_for_each_entry_safe(dev, tmp, &kvm->devices, vm_node) {
>        ops->destroy
>            |_ vgic_its_destroy
> 
> so vgic_its_destroy is called on an its device that was added to the
> kvm->devices list. If so the list was created.
> 
> Then we have vgic_mmio_write_its_baser() which is new caller introduced
> in subsequent patch.
> 
> for vgic_mmio_write_its_baser() to be called,  vgic_register_its_iodev
> must have been called. This latter is called on set_attr=vgic_its_set_attr
> set_attr can be called only if the fd is created. This happens in
> kvm_ioctl_create_device after ops->create() has been successful, ie
> meaning the lists are created.
> 
> What do I miss? What is the case you identified where the device_list is
> not initialized?
> 

I am probably just remembering incorrect, I just thought we identified
some strange flow where this could happen, but I can't do that anymore,
so I'll stop asking this question.  Sorry about that.

-Christoffer

[PATCH v2 08/10] KVM: arm/arm64: vgic-its: free caches when GITS_BASER Valid bit is cleared

[Eric Auger]

When the GITS_BASER<n>.Valid gets cleared, the data structures in
guest RAM are not provisionned anymore. The device, collection
and LPI lists stored in the in-kernel ITS represent the same
information in some form of cache. So let's void the cache.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 0df6d5f..eaefba2 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1471,8 +1471,9 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
 				      unsigned long val)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
-	u64 entry_size, device_type;
+	u64 entry_size;
 	u64 reg, *regptr, clearbits = 0;
+	int type;
 
 	/* When GITS_CTLR.Enable is 1, we ignore write accesses. */
 	if (its->enabled)
@@ -1482,12 +1483,12 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
 	case 0:
 		regptr = &its->baser_device_table;
 		entry_size = abi->dte_esz;
-		device_type = GITS_BASER_TYPE_DEVICE;
+		type = GITS_BASER_TYPE_DEVICE;
 		break;
 	case 1:
 		regptr = &its->baser_coll_table;
 		entry_size = abi->cte_esz;
-		device_type = GITS_BASER_TYPE_COLLECTION;
+		type = GITS_BASER_TYPE_COLLECTION;
 		clearbits = GITS_BASER_INDIRECT;
 		break;
 	default:
@@ -1499,10 +1500,24 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
 	reg &= ~clearbits;
 
 	reg |= (entry_size - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
-	reg |= device_type << GITS_BASER_TYPE_SHIFT;
+	reg |= (u64)type << GITS_BASER_TYPE_SHIFT;
 	reg = vgic_sanitise_its_baser(reg);
 
 	*regptr = reg;
+
+	if (reg & GITS_BASER_VALID)
+		return;
+
+	switch (type) {
+	case GITS_BASER_TYPE_DEVICE:
+		vgic_its_free_device_list(kvm, its);
+		break;
+	case GITS_BASER_TYPE_COLLECTION:
+		vgic_its_free_collection_list(kvm, its);
+		break;
+	default:
+		break;
+	}
 }
 
 static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
-- 
2.5.5

Re: [PATCH v2 08/10] KVM: arm/arm64: vgic-its: free caches when GITS_BASER Valid bit is cleared

[Christoffer Dall]

On Wed, Sep 27, 2017 at 03:28:38PM +0200, Eric Auger wrote:
> When the GITS_BASER<n>.Valid gets cleared, the data structures in
> guest RAM are not provisionned anymore. The device, collection

                    provisioned

		    (but did you really mean valid?)

> and LPI lists stored in the in-kernel ITS represent the same
> information in some form of cache. So let's void the cache.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 0df6d5f..eaefba2 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1471,8 +1471,9 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>  				      unsigned long val)
>  {
>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> -	u64 entry_size, device_type;
> +	u64 entry_size;
>  	u64 reg, *regptr, clearbits = 0;
> +	int type;
>  
>  	/* When GITS_CTLR.Enable is 1, we ignore write accesses. */
>  	if (its->enabled)
> @@ -1482,12 +1483,12 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>  	case 0:
>  		regptr = &its->baser_device_table;
>  		entry_size = abi->dte_esz;
> -		device_type = GITS_BASER_TYPE_DEVICE;
> +		type = GITS_BASER_TYPE_DEVICE;
>  		break;
>  	case 1:
>  		regptr = &its->baser_coll_table;
>  		entry_size = abi->cte_esz;
> -		device_type = GITS_BASER_TYPE_COLLECTION;
> +		type = GITS_BASER_TYPE_COLLECTION;
>  		clearbits = GITS_BASER_INDIRECT;
>  		break;
>  	default:
> @@ -1499,10 +1500,24 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>  	reg &= ~clearbits;
>  
>  	reg |= (entry_size - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
> -	reg |= device_type << GITS_BASER_TYPE_SHIFT;
> +	reg |= (u64)type << GITS_BASER_TYPE_SHIFT;
>  	reg = vgic_sanitise_its_baser(reg);
>  
>  	*regptr = reg;
> +
> +	if (reg & GITS_BASER_VALID)
> +		return;
> +
> +	switch (type) {
> +	case GITS_BASER_TYPE_DEVICE:
> +		vgic_its_free_device_list(kvm, its);
> +		break;
> +	case GITS_BASER_TYPE_COLLECTION:
> +		vgic_its_free_collection_list(kvm, its);
> +		break;
> +	default:
> +		break;
> +	}

This block deserves a comment:
	/* Table no longer valid: clear cached data */

I would also inverse the logic and do it within an if-statement if
(!(reg & GITS_BASER_VALID)), but it's up to you.


>  }
>  
>  static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
> -- 
> 2.5.5
> 

Thanks,
-Christoffer

Re: [PATCH v2 08/10] KVM: arm/arm64: vgic-its: free caches when GITS_BASER Valid bit is cleared

[Auger Eric]

Hi Christoffer,

On 13/10/2017 17:19, Christoffer Dall wrote:
> On Wed, Sep 27, 2017 at 03:28:38PM +0200, Eric Auger wrote:
>> When the GITS_BASER<n>.Valid gets cleared, the data structures in
>> guest RAM are not provisionned anymore. The device, collection
> 
>                     provisioned
> 
> 		    (but did you really mean valid?)
well the GITS_BASER<n>.Valid bit is reset, meaning the tables may be
freed / un-provisioned by the driver. I can use valid though.

Thanks

Eric
> 
>> and LPI lists stored in the in-kernel ITS represent the same
>> information in some form of cache. So let's void the cache.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++++----
>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 0df6d5f..eaefba2 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -1471,8 +1471,9 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>>  				      unsigned long val)
>>  {
>>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
>> -	u64 entry_size, device_type;
>> +	u64 entry_size;
>>  	u64 reg, *regptr, clearbits = 0;
>> +	int type;
>>  
>>  	/* When GITS_CTLR.Enable is 1, we ignore write accesses. */
>>  	if (its->enabled)
>> @@ -1482,12 +1483,12 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>>  	case 0:
>>  		regptr = &its->baser_device_table;
>>  		entry_size = abi->dte_esz;
>> -		device_type = GITS_BASER_TYPE_DEVICE;
>> +		type = GITS_BASER_TYPE_DEVICE;
>>  		break;
>>  	case 1:
>>  		regptr = &its->baser_coll_table;
>>  		entry_size = abi->cte_esz;
>> -		device_type = GITS_BASER_TYPE_COLLECTION;
>> +		type = GITS_BASER_TYPE_COLLECTION;
>>  		clearbits = GITS_BASER_INDIRECT;
>>  		break;
>>  	default:
>> @@ -1499,10 +1500,24 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>>  	reg &= ~clearbits;
>>  
>>  	reg |= (entry_size - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
>> -	reg |= device_type << GITS_BASER_TYPE_SHIFT;
>> +	reg |= (u64)type << GITS_BASER_TYPE_SHIFT;
>>  	reg = vgic_sanitise_its_baser(reg);
>>  
>>  	*regptr = reg;
>> +
>> +	if (reg & GITS_BASER_VALID)
>> +		return;
>> +
>> +	switch (type) {
>> +	case GITS_BASER_TYPE_DEVICE:
>> +		vgic_its_free_device_list(kvm, its);
>> +		break;
>> +	case GITS_BASER_TYPE_COLLECTION:
>> +		vgic_its_free_collection_list(kvm, its);
>> +		break;
>> +	default:
>> +		break;
>> +	}
> 
> This block deserves a comment:
> 	/* Table no longer valid: clear cached data */
> 
> I would also inverse the logic and do it within an if-statement if
> (!(reg & GITS_BASER_VALID)), but it's up to you.
OK
> 
> 
>>  }
>>  
>>  static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
>> -- 
>> 2.5.5
>>
> 
> Thanks,
> -Christoffer
> 

Re: [PATCH v2 08/10] KVM: arm/arm64: vgic-its: free caches when GITS_BASER Valid bit is cleared

[Christoffer Dall]

On Fri, Oct 13, 2017 at 05:34:44PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 13/10/2017 17:19, Christoffer Dall wrote:
> > On Wed, Sep 27, 2017 at 03:28:38PM +0200, Eric Auger wrote:
> >> When the GITS_BASER<n>.Valid gets cleared, the data structures in
> >> guest RAM are not provisionned anymore. The device, collection
> > 
> >                     provisioned
> > 
> > 		    (but did you really mean valid?)
> well the GITS_BASER<n>.Valid bit is reset, meaning the tables may be
> freed / un-provisioned by the driver. I can use valid though.

To me, the provision is the act of establishing something, but once it's
provisioned it just is, and then it isn't anymore.  So your sentence
"...not provisioned anymore" tells me that there was a process where
someone was constantly establishing new data sructures in guest ram, and
the process stops.  But what I think you want to say is that there were
data structures in place, but when the bit gets cleared, those data
structures are no longer used, i.e. they are no longer valid.

This is really just a wording thing, and I'm not a native English
speaker, but looking up provisioning in the dictionary seems to support
my claim here.

Thanks,
-Christoffer

[PATCH v2 09/10] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

[Eric Auger]

At the moment, the in-kernel emulated ITS is not properly reset.
On guest restart/reset some registers keep their old values and
internal structures like device, ITE, collection lists are not freed.

This may lead to various bugs. Among them, we can have incorrect state
backup or failure when saving the ITS state at early guest boot stage.

This patch documents a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
the KVM_DEV_ARM_VGIC_GRP_CTRL group.

Upon this action, we can reset registers and especially those
pointing to tables previously allocated by the guest and free
the internal data structures storing the list of devices, collections
and lpis.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reported-by: wanghaibin <wanghaibin.wang@huawei.com>

---

v1 -> v2:
- Describe architecturally-defined reset values
---
 Documentation/virtual/kvm/devices/arm-vgic-its.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
index eb06beb..047358c 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
@@ -33,6 +33,10 @@ Groups:
       request the initialization of the ITS, no additional parameter in
       kvm_device_attr.addr.
 
+    KVM_DEV_ARM_ITS_CTRL_RESET
+      reset the ITS, no additional parameter in kvm_device_attr.addr.
+      See "ITS Reset State" section.
+
     KVM_DEV_ARM_ITS_SAVE_TABLES
       save the ITS table data into guest RAM, at the location provisioned
       by the guest in corresponding registers/table entries.
@@ -157,3 +161,15 @@ Then vcpus can be started.
  - pINTID is the physical LPI ID; if zero, it means the entry is not valid
    and other fields are not meaningful.
  - ICID is the collection ID
+
+ ITS Reset State:
+ ----------------
+
+- the ITS is not enabled and quiescent:
+  GITS_CTLR.Enabled = 0 .Quiescent=1
+- caches are empty
+- No collection or device table is provisionned
+  GITS_BASER<n>.Valid = 0
+- the command queue is not allocated:
+  GITS_CBASER = 0, GITS_CREADR = 0, GITS_CWRITER = 0
+- The ABI version corresponds to the one set before reset
-- 
2.5.5

Re: [PATCH v2 09/10] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

[Peter Maydell]

On 27 September 2017 at 14:28, Eric Auger <eric.auger@redhat.com> wrote:
> At the moment, the in-kernel emulated ITS is not properly reset.
> On guest restart/reset some registers keep their old values and
> internal structures like device, ITE, collection lists are not freed.
>
> This may lead to various bugs. Among them, we can have incorrect state
> backup or failure when saving the ITS state at early guest boot stage.
>
> This patch documents a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
> the KVM_DEV_ARM_VGIC_GRP_CTRL group.
>
> Upon this action, we can reset registers and especially those
> pointing to tables previously allocated by the guest and free
> the internal data structures storing the list of devices, collections
> and lpis.

I think we could probably expand this to explain why our usual
approach to reset doesn't work, something like:

"The usual approach for device reset of having userspace write
the reset values of the registers to the kernel via the register
read/write APIs doesn't work for the ITS because it has some
internal state (caches) which is not exposed as registers,
and there is no register interface for "drop cached data without
writing it back to RAM". So we need a KVM API which mimics the
hardware's reset line, to provide the equivalent behaviour to
a "pull the power cord out of the back of the machine" reset."


> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
>
> ---
>
> v1 -> v2:
> - Describe architecturally-defined reset values
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index eb06beb..047358c 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -33,6 +33,10 @@ Groups:
>        request the initialization of the ITS, no additional parameter in
>        kvm_device_attr.addr.
>
> +    KVM_DEV_ARM_ITS_CTRL_RESET
> +      reset the ITS, no additional parameter in kvm_device_attr.addr.
> +      See "ITS Reset State" section.

Why is the init attribute for the ITS named
KVM_DEV_ARM_VGIC_CTRL_INIT, but the reset attribute is
KVM_DEV_ARM_ITS_CTRL_RESET ? Seems a bit inconsistent.

> +
>      KVM_DEV_ARM_ITS_SAVE_TABLES
>        save the ITS table data into guest RAM, at the location provisioned
>        by the guest in corresponding registers/table entries.
> @@ -157,3 +161,15 @@ Then vcpus can be started.
>   - pINTID is the physical LPI ID; if zero, it means the entry is not valid
>     and other fields are not meaningful.
>   - ICID is the collection ID
> +
> + ITS Reset State:
> + ----------------
> +
> +- the ITS is not enabled and quiescent:
> +  GITS_CTLR.Enabled = 0 .Quiescent=1
> +- caches are empty
> +- No collection or device table is provisionned

"provisioned"

> +  GITS_BASER<n>.Valid = 0
> +- the command queue is not allocated:
> +  GITS_CBASER = 0, GITS_CREADR = 0, GITS_CWRITER = 0
> +- The ABI version corresponds to the one set before reset

Rather than trying to spell out all the reset values here,
it might be better to just say that RESET returns the ITS to
the same state that it is in after it is first created and
inited. (That is the behaviour that userspace wants.)

Happy with the general principle of the new userspace ABI.

thanks
-- PMM

Re: [PATCH v2 09/10] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

[Auger Eric]

Hi Peter,

On 12/10/2017 12:57, Peter Maydell wrote:
> On 27 September 2017 at 14:28, Eric Auger <eric.auger@redhat.com> wrote:
>> At the moment, the in-kernel emulated ITS is not properly reset.
>> On guest restart/reset some registers keep their old values and
>> internal structures like device, ITE, collection lists are not freed.
>>
>> This may lead to various bugs. Among them, we can have incorrect state
>> backup or failure when saving the ITS state at early guest boot stage.
>>
>> This patch documents a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
>> the KVM_DEV_ARM_VGIC_GRP_CTRL group.
>>
>> Upon this action, we can reset registers and especially those
>> pointing to tables previously allocated by the guest and free
>> the internal data structures storing the list of devices, collections
>> and lpis.
> 
> I think we could probably expand this to explain why our usual
> approach to reset doesn't work, something like:
> 
> "The usual approach for device reset of having userspace write
> the reset values of the registers to the kernel via the register
> read/write APIs doesn't work for the ITS because it has some
> internal state (caches) which is not exposed as registers,
> and there is no register interface for "drop cached data without
> writing it back to RAM". So we need a KVM API which mimics the
> hardware's reset line, to provide the equivalent behaviour to
> a "pull the power cord out of the back of the machine" reset."

Sure I will add this explanation.

Thanks

Eric
> 
> 
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
>>
>> ---
>>
>> v1 -> v2:
>> - Describe architecturally-defined reset values
>> ---
>>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> index eb06beb..047358c 100644
>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> @@ -33,6 +33,10 @@ Groups:
>>        request the initialization of the ITS, no additional parameter in
>>        kvm_device_attr.addr.
>>
>> +    KVM_DEV_ARM_ITS_CTRL_RESET
>> +      reset the ITS, no additional parameter in kvm_device_attr.addr.
>> +      See "ITS Reset State" section.
> 
> Why is the init attribute for the ITS named
> KVM_DEV_ARM_VGIC_CTRL_INIT, but the reset attribute is
> KVM_DEV_ARM_ITS_CTRL_RESET ? Seems a bit inconsistent.
> 
>> +
>>      KVM_DEV_ARM_ITS_SAVE_TABLES
>>        save the ITS table data into guest RAM, at the location provisioned
>>        by the guest in corresponding registers/table entries.
>> @@ -157,3 +161,15 @@ Then vcpus can be started.
>>   - pINTID is the physical LPI ID; if zero, it means the entry is not valid
>>     and other fields are not meaningful.
>>   - ICID is the collection ID
>> +
>> + ITS Reset State:
>> + ----------------
>> +
>> +- the ITS is not enabled and quiescent:
>> +  GITS_CTLR.Enabled = 0 .Quiescent=1
>> +- caches are empty
>> +- No collection or device table is provisionned
> 
> "provisioned"
> 
>> +  GITS_BASER<n>.Valid = 0
>> +- the command queue is not allocated:
>> +  GITS_CBASER = 0, GITS_CREADR = 0, GITS_CWRITER = 0
>> +- The ABI version corresponds to the one set before reset
> 
> Rather than trying to spell out all the reset values here,
> it might be better to just say that RESET returns the ITS to
> the same state that it is in after it is first created and
> inited. (That is the behaviour that userspace wants.)
> 
> Happy with the general principle of the new userspace ABI.
> 
> thanks
> -- PMM
> 

Re: [PATCH v2 09/10] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

[Christoffer Dall]

Hi Eric,

On Wed, Sep 27, 2017 at 03:28:39PM +0200, Eric Auger wrote:
> At the moment, the in-kernel emulated ITS is not properly reset.
> On guest restart/reset some registers keep their old values and
> internal structures like device, ITE, collection lists are not freed.

                                      , and collection ...
> 
> This may lead to various bugs. Among them, we can have incorrect state
> backup or failure when saving the ITS state at early guest boot stage.
> 
> This patch documents a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
> the KVM_DEV_ARM_VGIC_GRP_CTRL group.
> 
> Upon this action, we can reset registers and especially those
> pointing to tables previously allocated by the guest and free
> the internal data structures storing the list of devices, collections
> and lpis.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
> 
> ---
> 
> v1 -> v2:
> - Describe architecturally-defined reset values
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index eb06beb..047358c 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -33,6 +33,10 @@ Groups:
>        request the initialization of the ITS, no additional parameter in
>        kvm_device_attr.addr.
>  
> +    KVM_DEV_ARM_ITS_CTRL_RESET
> +      reset the ITS, no additional parameter in kvm_device_attr.addr.
> +      See "ITS Reset State" section.
> +
>      KVM_DEV_ARM_ITS_SAVE_TABLES
>        save the ITS table data into guest RAM, at the location provisioned
>        by the guest in corresponding registers/table entries.
> @@ -157,3 +161,15 @@ Then vcpus can be started.
>   - pINTID is the physical LPI ID; if zero, it means the entry is not valid
>     and other fields are not meaningful.
>   - ICID is the collection ID
> +
> + ITS Reset State:
> + ----------------

Are the bullet points below the state after the device has been reset
(after KVM_DEV_ARM_ITS_CTRL_RESET returns) or ?  I think this should be
clarified.

> +
> +- the ITS is not enabled and quiescent:

     The

> +  GITS_CTLR.Enabled = 0 .Quiescent=1
> +- caches are empty
     Caches  (would it make more sense to say that there is no
     internally cached state?)
> +- No collection or device table is provisionned

                                   are used:

> +  GITS_BASER<n>.Valid = 0
> +- the command queue is not allocated:

     The

> +  GITS_CBASER = 0, GITS_CREADR = 0, GITS_CWRITER = 0
> +- The ABI version corresponds to the one set before reset

Do you mean that resetting the ITS cannot change the ABI version used
for save/restore, and therefore remains the same as it the version
configured when the device was first created ?

> -- 
> 2.5.5
> 

Thanks, (and sorry for being pedantic about ABI wordings)
-Christoffer

Re: [PATCH v2 09/10] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

[Auger Eric]

Hi Christoffer,

On 13/10/2017 17:26, Christoffer Dall wrote:
> Hi Eric,
> 
> On Wed, Sep 27, 2017 at 03:28:39PM +0200, Eric Auger wrote:
>> At the moment, the in-kernel emulated ITS is not properly reset.
>> On guest restart/reset some registers keep their old values and
>> internal structures like device, ITE, collection lists are not freed.
> 
>                                       , and collection ...
>>
>> This may lead to various bugs. Among them, we can have incorrect state
>> backup or failure when saving the ITS state at early guest boot stage.
>>
>> This patch documents a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
>> the KVM_DEV_ARM_VGIC_GRP_CTRL group.
>>
>> Upon this action, we can reset registers and especially those
>> pointing to tables previously allocated by the guest and free
>> the internal data structures storing the list of devices, collections
>> and lpis.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
>>
>> ---
>>
>> v1 -> v2:
>> - Describe architecturally-defined reset values
>> ---
>>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> index eb06beb..047358c 100644
>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> @@ -33,6 +33,10 @@ Groups:
>>        request the initialization of the ITS, no additional parameter in
>>        kvm_device_attr.addr.
>>  
>> +    KVM_DEV_ARM_ITS_CTRL_RESET
>> +      reset the ITS, no additional parameter in kvm_device_attr.addr.
>> +      See "ITS Reset State" section.
>> +
>>      KVM_DEV_ARM_ITS_SAVE_TABLES
>>        save the ITS table data into guest RAM, at the location provisioned
>>        by the guest in corresponding registers/table entries.
>> @@ -157,3 +161,15 @@ Then vcpus can be started.
>>   - pINTID is the physical LPI ID; if zero, it means the entry is not valid
>>     and other fields are not meaningful.
>>   - ICID is the collection ID
>> +
>> + ITS Reset State:
>> + ----------------
> 
> Are the bullet points below the state after the device has been reset
> (after KVM_DEV_ARM_ITS_CTRL_RESET returns) or ?  I think this should be
> clarified.

yes this is after reset.
> 
>> +
>> +- the ITS is not enabled and quiescent:
> 
>      The
> 
>> +  GITS_CTLR.Enabled = 0 .Quiescent=1
>> +- caches are empty
>      Caches  (would it make more sense to say that there is no
>      internally cached state?)
>> +- No collection or device table is provisionned
> 
>                                    are used:
referenced?
> 
>> +  GITS_BASER<n>.Valid = 0
>> +- the command queue is not allocated:
> 
>      The
> 
>> +  GITS_CBASER = 0, GITS_CREADR = 0, GITS_CWRITER = 0
>> +- The ABI version corresponds to the one set before reset
> 
> Do you mean that resetting the ITS cannot change the ABI version used
> for save/restore, and therefore remains the same as it the version
> configured when the device was first created ?
Yes the ABI version stays the same.
> 
>> -- 
>> 2.5.5
>>
> 
> Thanks, (and sorry for being pedantic about ABI wordings)
no worries, I know it is important ;-)

Thanks

Eric
> -Christoffer
> 

Re: [PATCH v2 09/10] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

[Christoffer Dall]

On Fri, Oct 13, 2017 at 05:41:10PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 13/10/2017 17:26, Christoffer Dall wrote:
> > Hi Eric,
> > 
> > On Wed, Sep 27, 2017 at 03:28:39PM +0200, Eric Auger wrote:
> >> At the moment, the in-kernel emulated ITS is not properly reset.
> >> On guest restart/reset some registers keep their old values and
> >> internal structures like device, ITE, collection lists are not freed.
> > 
> >                                       , and collection ...
> >>
> >> This may lead to various bugs. Among them, we can have incorrect state
> >> backup or failure when saving the ITS state at early guest boot stage.
> >>
> >> This patch documents a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
> >> the KVM_DEV_ARM_VGIC_GRP_CTRL group.
> >>
> >> Upon this action, we can reset registers and especially those
> >> pointing to tables previously allocated by the guest and free
> >> the internal data structures storing the list of devices, collections
> >> and lpis.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
> >>
> >> ---
> >>
> >> v1 -> v2:
> >> - Describe architecturally-defined reset values
> >> ---
> >>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 16 ++++++++++++++++
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> >> index eb06beb..047358c 100644
> >> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> >> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> >> @@ -33,6 +33,10 @@ Groups:
> >>        request the initialization of the ITS, no additional parameter in
> >>        kvm_device_attr.addr.
> >>  
> >> +    KVM_DEV_ARM_ITS_CTRL_RESET
> >> +      reset the ITS, no additional parameter in kvm_device_attr.addr.
> >> +      See "ITS Reset State" section.
> >> +
> >>      KVM_DEV_ARM_ITS_SAVE_TABLES
> >>        save the ITS table data into guest RAM, at the location provisioned
> >>        by the guest in corresponding registers/table entries.
> >> @@ -157,3 +161,15 @@ Then vcpus can be started.
> >>   - pINTID is the physical LPI ID; if zero, it means the entry is not valid
> >>     and other fields are not meaningful.
> >>   - ICID is the collection ID
> >> +
> >> + ITS Reset State:
> >> + ----------------
> > 
> > Are the bullet points below the state after the device has been reset
> > (after KVM_DEV_ARM_ITS_CTRL_RESET returns) or ?  I think this should be
> > clarified.
> 
> yes this is after reset.
> > 
> >> +
> >> +- the ITS is not enabled and quiescent:
> > 
> >      The
> > 
> >> +  GITS_CTLR.Enabled = 0 .Quiescent=1
> >> +- caches are empty
> >      Caches  (would it make more sense to say that there is no
> >      internally cached state?)
> >> +- No collection or device table is provisionned
> > 
> >                                    are used:
> referenced?

I think used is more plainspoken and intuitively understood.

> > 
> >> +  GITS_BASER<n>.Valid = 0
> >> +- the command queue is not allocated:
> > 
> >      The
> > 
> >> +  GITS_CBASER = 0, GITS_CREADR = 0, GITS_CWRITER = 0
> >> +- The ABI version corresponds to the one set before reset
> > 
> > Do you mean that resetting the ITS cannot change the ABI version used
> > for save/restore, and therefore remains the same as it the version
> > configured when the device was first created ?
> Yes the ABI version stays the same.
> > 
> >> -- 
> >> 2.5.5
> >>
> > 
> > Thanks, (and sorry for being pedantic about ABI wordings)
> no worries, I know it is important ;-)
> 
Thanks :)
-Christoffer

[PATCH v2 10/10] KVM: arm/arm64: vgic-its: Implement KVM_DEV_ARM_ITS_CTRL_RESET

[Eric Auger]

On reset we clear the valid bits of GITS_CBASER and GITS_BASER<n>.
We also clear command queue registers and free the device,
collection and lpi lists.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arch/arm/include/uapi/asm/kvm.h   |  1 +
 arch/arm64/include/uapi/asm/kvm.h |  1 +
 virt/kvm/arm/vgic/vgic-its.c      | 18 ++++++++++++++++++
 3 files changed, 20 insertions(+)

diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index 5db2d4c..7ef0c06 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -215,6 +215,7 @@ struct kvm_arch_memory_slot {
 #define   KVM_DEV_ARM_ITS_SAVE_TABLES		1
 #define   KVM_DEV_ARM_ITS_RESTORE_TABLES	2
 #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
+#define   KVM_DEV_ARM_ITS_CTRL_RESET		4
 
 /* KVM_IRQ_LINE irq field index values */
 #define KVM_ARM_IRQ_TYPE_SHIFT		24
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 9f3ca24..b5306ce 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -227,6 +227,7 @@ struct kvm_arch_memory_slot {
 #define   KVM_DEV_ARM_ITS_SAVE_TABLES           1
 #define   KVM_DEV_ARM_ITS_RESTORE_TABLES        2
 #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
+#define   KVM_DEV_ARM_ITS_CTRL_RESET		4
 
 /* Device Control API on vcpu fd */
 #define KVM_ARM_VCPU_PMU_V3_CTRL	0
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index eaefba2..fa18946 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2380,6 +2380,19 @@ static int vgic_its_commit_v0(struct vgic_its *its)
 	return 0;
 }
 
+static void vgic_its_reset(struct kvm *kvm, struct vgic_its *its)
+{
+	/* We need to keep the ABI specific field values */
+	its->baser_coll_table &= ~GITS_BASER_VALID;
+	its->baser_device_table &= ~GITS_BASER_VALID;
+	its->cbaser = 0;
+	its->creadr = 0;
+	its->cwriter = 0;
+	its->enabled = 0;
+	vgic_its_free_device_list(kvm, its);
+	vgic_its_free_collection_list(kvm, its);
+}
+
 static int vgic_its_has_attr(struct kvm_device *dev,
 			     struct kvm_device_attr *attr)
 {
@@ -2394,6 +2407,8 @@ static int vgic_its_has_attr(struct kvm_device *dev,
 		switch (attr->attr) {
 		case KVM_DEV_ARM_VGIC_CTRL_INIT:
 			return 0;
+		case KVM_DEV_ARM_ITS_CTRL_RESET:
+			return 0;
 		case KVM_DEV_ARM_ITS_SAVE_TABLES:
 			return 0;
 		case KVM_DEV_ARM_ITS_RESTORE_TABLES:
@@ -2438,6 +2453,9 @@ static int vgic_its_set_attr(struct kvm_device *dev,
 		case KVM_DEV_ARM_VGIC_CTRL_INIT:
 			/* Nothing to do */
 			return 0;
+		case KVM_DEV_ARM_ITS_CTRL_RESET:
+			vgic_its_reset(dev->kvm, its);
+			return 0;
 		case KVM_DEV_ARM_ITS_SAVE_TABLES:
 			return abi->save_tables(its);
 		case KVM_DEV_ARM_ITS_RESTORE_TABLES:
-- 
2.5.5

Re: [PATCH v2 10/10] KVM: arm/arm64: vgic-its: Implement KVM_DEV_ARM_ITS_CTRL_RESET

[Christoffer Dall]

On Wed, Sep 27, 2017 at 03:28:40PM +0200, Eric Auger wrote:
> On reset we clear the valid bits of GITS_CBASER and GITS_BASER<n>.
> We also clear command queue registers and free the device,
> collection and lpi lists.

            , and lpi lists.

(https://imgur.com/gallery/fycHx for your amusemnt)


Otherwise:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arch/arm/include/uapi/asm/kvm.h   |  1 +
>  arch/arm64/include/uapi/asm/kvm.h |  1 +
>  virt/kvm/arm/vgic/vgic-its.c      | 18 ++++++++++++++++++
>  3 files changed, 20 insertions(+)
> 
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index 5db2d4c..7ef0c06 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -215,6 +215,7 @@ struct kvm_arch_memory_slot {
>  #define   KVM_DEV_ARM_ITS_SAVE_TABLES		1
>  #define   KVM_DEV_ARM_ITS_RESTORE_TABLES	2
>  #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
> +#define   KVM_DEV_ARM_ITS_CTRL_RESET		4
>  
>  /* KVM_IRQ_LINE irq field index values */
>  #define KVM_ARM_IRQ_TYPE_SHIFT		24
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 9f3ca24..b5306ce 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -227,6 +227,7 @@ struct kvm_arch_memory_slot {
>  #define   KVM_DEV_ARM_ITS_SAVE_TABLES           1
>  #define   KVM_DEV_ARM_ITS_RESTORE_TABLES        2
>  #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
> +#define   KVM_DEV_ARM_ITS_CTRL_RESET		4
>  
>  /* Device Control API on vcpu fd */
>  #define KVM_ARM_VCPU_PMU_V3_CTRL	0
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index eaefba2..fa18946 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2380,6 +2380,19 @@ static int vgic_its_commit_v0(struct vgic_its *its)
>  	return 0;
>  }
>  
> +static void vgic_its_reset(struct kvm *kvm, struct vgic_its *its)
> +{
> +	/* We need to keep the ABI specific field values */
> +	its->baser_coll_table &= ~GITS_BASER_VALID;
> +	its->baser_device_table &= ~GITS_BASER_VALID;
> +	its->cbaser = 0;
> +	its->creadr = 0;
> +	its->cwriter = 0;
> +	its->enabled = 0;
> +	vgic_its_free_device_list(kvm, its);
> +	vgic_its_free_collection_list(kvm, its);
> +}
> +
>  static int vgic_its_has_attr(struct kvm_device *dev,
>  			     struct kvm_device_attr *attr)
>  {
> @@ -2394,6 +2407,8 @@ static int vgic_its_has_attr(struct kvm_device *dev,
>  		switch (attr->attr) {
>  		case KVM_DEV_ARM_VGIC_CTRL_INIT:
>  			return 0;
> +		case KVM_DEV_ARM_ITS_CTRL_RESET:
> +			return 0;
>  		case KVM_DEV_ARM_ITS_SAVE_TABLES:
>  			return 0;
>  		case KVM_DEV_ARM_ITS_RESTORE_TABLES:
> @@ -2438,6 +2453,9 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>  		case KVM_DEV_ARM_VGIC_CTRL_INIT:
>  			/* Nothing to do */
>  			return 0;
> +		case KVM_DEV_ARM_ITS_CTRL_RESET:
> +			vgic_its_reset(dev->kvm, its);
> +			return 0;
>  		case KVM_DEV_ARM_ITS_SAVE_TABLES:
>  			return abi->save_tables(its);
>  		case KVM_DEV_ARM_ITS_RESTORE_TABLES:
> -- 
> 2.5.5
>