[PATCH v3 00/10] KVM: arm64: vgic: Missing checks for REDIST/CPU and ITS regions above the VM IPA size

[PATCH v3 00/10] KVM: arm64: vgic: Missing checks for REDIST/CPU and ITS regions above the VM IPA size

[Ricardo Koller]

KVM doesn't check for redist, CPU interface, and ITS regions that extend
partially above the guest addressable IPA range (phys_size).  This can happen
when using the V[2|3]_ADDR_TYPE_CPU, ADDR_TYPE_REDIST[_REGION], or
ITS_ADDR_TYPE attributes to set a new region that extends partially above
phys_size (with the base below phys_size).  The issue is that vcpus can
potentially run into a situation where some redistributors are addressable and
others are not, or just the first half of the ITS is addressable.

Patches 1-4 fixes the issue for GICv2 and GICv3 (and the ITS). Patches 5-10 add
some selftests for all these fixes. While adding these tests, these add support
for some extra GICv2 and ITS device tests.

Changes:
v3: add missing checks for GICv2 and the ITS, plus tests for the fixes.
v2: adding a test for KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, and returning E2BIG
    instead of EINVAL (thanks Alexandru and Eric).

Ricardo Koller (10):
  kvm: arm64: vgic: Introduce vgic_check_iorange
  KVM: arm64: vgic-v3: Check redist region is not above the VM IPA size
  KVM: arm64: vgic-v2: Check cpu interface region is not above the VM
    IPA size
  KVM: arm64: vgic-v3: Check ITS region is not above the VM IPA size
  KVM: arm64: selftests: Make vgic_init gic version agnostic
  KVM: arm64: selftests: Make vgic_init/vm_gic_create version agnostic
  KVM: arm64: selftests: Add some tests for GICv2 in vgic_init
  KVM: arm64: selftests: Add tests for GIC redist/cpuif partially above
    IPA range
  KVM: arm64: selftests: Add test for legacy GICv3 REDIST base partially
    above IPA range
  KVM: arm64: selftests: Add basic ITS device tests

 arch/arm64/kvm/vgic/vgic-its.c                |   4 +-
 arch/arm64/kvm/vgic/vgic-kvm-device.c         |  29 +-
 arch/arm64/kvm/vgic/vgic-mmio-v3.c            |   6 +-
 arch/arm64/kvm/vgic/vgic-v3.c                 |   4 +
 arch/arm64/kvm/vgic/vgic.h                    |   4 +
 .../testing/selftests/kvm/aarch64/vgic_init.c | 372 +++++++++++++-----
 6 files changed, 317 insertions(+), 102 deletions(-)

-- 
2.33.0.685.g46640cef36-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 01/10] kvm: arm64: vgic: Introduce vgic_check_iorange

[Ricardo Koller]

Add the new vgic_check_iorange helper that checks that an iorange is
sane: the start address and size have valid alignments, the range is
within the addressable PA range, start+size doesn't overflow, and the
start wasn't already defined.

No functional change.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++
 arch/arm64/kvm/vgic/vgic.h            |  4 ++++
 2 files changed, 26 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
index 7740995de982..f714aded67b2 100644
--- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
+++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
@@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
 	return 0;
 }
 
+int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr,
+		       phys_addr_t addr, phys_addr_t alignment,
+		       phys_addr_t size)
+{
+	int ret;
+
+	ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment);
+	if (ret)
+		return ret;
+
+	if (!IS_ALIGNED(size, alignment))
+		return -EINVAL;
+
+	if (addr + size < addr)
+		return -EINVAL;
+
+	if (addr + size > kvm_phys_size(kvm))
+		return -E2BIG;
+
+	return 0;
+}
+
 static int vgic_check_type(struct kvm *kvm, int type_needed)
 {
 	if (kvm->arch.vgic.vgic_model != type_needed)
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 14a9218641f5..c4df4dcef31f 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -175,6 +175,10 @@ void vgic_irq_handle_resampling(struct vgic_irq *irq,
 int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
 		      phys_addr_t addr, phys_addr_t alignment);
 
+int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr,
+		       phys_addr_t addr, phys_addr_t alignment,
+		       phys_addr_t size);
+
 void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu);
 void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr);
 void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr);
-- 
2.33.0.685.g46640cef36-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 01/10] kvm: arm64: vgic: Introduce vgic_check_iorange

[Eric Auger]

Hi Ricardo,

On 9/28/21 8:47 PM, Ricardo Koller wrote:
> Add the new vgic_check_iorange helper that checks that an iorange is
> sane: the start address and size have valid alignments, the range is
> within the addressable PA range, start+size doesn't overflow, and the
> start wasn't already defined.
>
> No functional change.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++
>  arch/arm64/kvm/vgic/vgic.h            |  4 ++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> index 7740995de982..f714aded67b2 100644
> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
>  	return 0;
>  }
>  
> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr,
> +		       phys_addr_t addr, phys_addr_t alignment,
> +		       phys_addr_t size)
> +{
> +	int ret;
> +
> +	ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment);
nit: not related to this patch but I am just wondering why we are
passing phys_addr_t *ioaddr downto vgic_check_ioaddr and thus to

vgic_check_iorange()? This must be a leftover of some old code?

> +	if (ret)
> +		return ret;
> +
> +	if (!IS_ALIGNED(size, alignment))
> +		return -EINVAL;
> +
> +	if (addr + size < addr)
> +		return -EINVAL;
> +
> +	if (addr + size > kvm_phys_size(kvm))
> +		return -E2BIG;
> +
> +	return 0;
> +}
> +
>  static int vgic_check_type(struct kvm *kvm, int type_needed)
>  {
>  	if (kvm->arch.vgic.vgic_model != type_needed)
> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
> index 14a9218641f5..c4df4dcef31f 100644
> --- a/arch/arm64/kvm/vgic/vgic.h
> +++ b/arch/arm64/kvm/vgic/vgic.h
> @@ -175,6 +175,10 @@ void vgic_irq_handle_resampling(struct vgic_irq *irq,
>  int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
>  		      phys_addr_t addr, phys_addr_t alignment);
>  
> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr,
> +		       phys_addr_t addr, phys_addr_t alignment,
> +		       phys_addr_t size);
> +
>  void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu);
>  void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr);
>  void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr);
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 01/10] kvm: arm64: vgic: Introduce vgic_check_iorange

[Ricardo Koller]

On Wed, Sep 29, 2021 at 06:29:21PM +0200, Eric Auger wrote:
> Hi Ricardo,
> 
> On 9/28/21 8:47 PM, Ricardo Koller wrote:
> > Add the new vgic_check_iorange helper that checks that an iorange is
> > sane: the start address and size have valid alignments, the range is
> > within the addressable PA range, start+size doesn't overflow, and the
> > start wasn't already defined.
> >
> > No functional change.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++
> >  arch/arm64/kvm/vgic/vgic.h            |  4 ++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > index 7740995de982..f714aded67b2 100644
> > --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
> >  	return 0;
> >  }
> >  
> > +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr,
> > +		       phys_addr_t addr, phys_addr_t alignment,
> > +		       phys_addr_t size)
> > +{
> > +	int ret;
> > +
> > +	ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment);
> nit: not related to this patch but I am just wondering why we are
> passing phys_addr_t *ioaddr downto vgic_check_ioaddr and thus to
> 
> vgic_check_iorange()? This must be a leftover of some old code?
> 

It's used to check that the base of a region is not already set.
kvm_vgic_addr() uses it to make that check;
vgic_v3_alloc_redist_region() does not:

  rdreg->base = VGIC_ADDR_UNDEF; // so the "not already defined" check passes
  ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);

Thanks,
Ricardo

> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!IS_ALIGNED(size, alignment))
> > +		return -EINVAL;
> > +
> > +	if (addr + size < addr)
> > +		return -EINVAL;
> > +
> > +	if (addr + size > kvm_phys_size(kvm))
> > +		return -E2BIG;
> > +
> > +	return 0;
> > +}
> > +
> >  static int vgic_check_type(struct kvm *kvm, int type_needed)
> >  {
> >  	if (kvm->arch.vgic.vgic_model != type_needed)
> > diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
> > index 14a9218641f5..c4df4dcef31f 100644
> > --- a/arch/arm64/kvm/vgic/vgic.h
> > +++ b/arch/arm64/kvm/vgic/vgic.h
> > @@ -175,6 +175,10 @@ void vgic_irq_handle_resampling(struct vgic_irq *irq,
> >  int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
> >  		      phys_addr_t addr, phys_addr_t alignment);
> >  
> > +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr,
> > +		       phys_addr_t addr, phys_addr_t alignment,
> > +		       phys_addr_t size);
> > +
> >  void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu);
> >  void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr);
> >  void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr);
> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Eric
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 01/10] kvm: arm64: vgic: Introduce vgic_check_iorange

[Eric Auger]

Hi,

On 9/29/21 11:17 PM, Ricardo Koller wrote:
> On Wed, Sep 29, 2021 at 06:29:21PM +0200, Eric Auger wrote:
>> Hi Ricardo,
>>
>> On 9/28/21 8:47 PM, Ricardo Koller wrote:
>>> Add the new vgic_check_iorange helper that checks that an iorange is
>>> sane: the start address and size have valid alignments, the range is
>>> within the addressable PA range, start+size doesn't overflow, and the
>>> start wasn't already defined.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
>>> ---
>>>  arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++
>>>  arch/arm64/kvm/vgic/vgic.h            |  4 ++++
>>>  2 files changed, 26 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
>>> index 7740995de982..f714aded67b2 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
>>> @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
>>>  	return 0;
>>>  }
>>>  
>>> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr,
>>> +		       phys_addr_t addr, phys_addr_t alignment,
>>> +		       phys_addr_t size)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment);
>> nit: not related to this patch but I am just wondering why we are
>> passing phys_addr_t *ioaddr downto vgic_check_ioaddr and thus to
>>
>> vgic_check_iorange()? This must be a leftover of some old code?
>>
> It's used to check that the base of a region is not already set.
> kvm_vgic_addr() uses it to make that check;
> vgic_v3_alloc_redist_region() does not:
>
>   rdreg->base = VGIC_ADDR_UNDEF; // so the "not already defined" check passes
>   ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
Yes but I meant why a pointer?

Eric
>
> Thanks,
> Ricardo
>
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (!IS_ALIGNED(size, alignment))
>>> +		return -EINVAL;
>>> +
>>> +	if (addr + size < addr)
>>> +		return -EINVAL;
>>> +
>>> +	if (addr + size > kvm_phys_size(kvm))
>>> +		return -E2BIG;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int vgic_check_type(struct kvm *kvm, int type_needed)
>>>  {
>>>  	if (kvm->arch.vgic.vgic_model != type_needed)
>>> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
>>> index 14a9218641f5..c4df4dcef31f 100644
>>> --- a/arch/arm64/kvm/vgic/vgic.h
>>> +++ b/arch/arm64/kvm/vgic/vgic.h
>>> @@ -175,6 +175,10 @@ void vgic_irq_handle_resampling(struct vgic_irq *irq,
>>>  int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
>>>  		      phys_addr_t addr, phys_addr_t alignment);
>>>  
>>> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr,
>>> +		       phys_addr_t addr, phys_addr_t alignment,
>>> +		       phys_addr_t size);
>>> +
>>>  void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu);
>>>  void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr);
>>>  void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr);
>> Besides
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> Eric
>>

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 01/10] kvm: arm64: vgic: Introduce vgic_check_iorange

[Ricardo Koller]

On Thu, Sep 30, 2021 at 09:02:12AM +0200, Eric Auger wrote:
> Hi,
> 
> On 9/29/21 11:17 PM, Ricardo Koller wrote:
> > On Wed, Sep 29, 2021 at 06:29:21PM +0200, Eric Auger wrote:
> >> Hi Ricardo,
> >>
> >> On 9/28/21 8:47 PM, Ricardo Koller wrote:
> >>> Add the new vgic_check_iorange helper that checks that an iorange is
> >>> sane: the start address and size have valid alignments, the range is
> >>> within the addressable PA range, start+size doesn't overflow, and the
> >>> start wasn't already defined.
> >>>
> >>> No functional change.
> >>>
> >>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> >>> ---
> >>>  arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++
> >>>  arch/arm64/kvm/vgic/vgic.h            |  4 ++++
> >>>  2 files changed, 26 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> >>> index 7740995de982..f714aded67b2 100644
> >>> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> >>> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> >>> @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
> >>>  	return 0;
> >>>  }
> >>>  
> >>> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr,
> >>> +		       phys_addr_t addr, phys_addr_t alignment,
> >>> +		       phys_addr_t size)
> >>> +{
> >>> +	int ret;
> >>> +
> >>> +	ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment);
> >> nit: not related to this patch but I am just wondering why we are
> >> passing phys_addr_t *ioaddr downto vgic_check_ioaddr and thus to
> >>
> >> vgic_check_iorange()? This must be a leftover of some old code?
> >>
> > It's used to check that the base of a region is not already set.
> > kvm_vgic_addr() uses it to make that check;
> > vgic_v3_alloc_redist_region() does not:
> >
> >   rdreg->base = VGIC_ADDR_UNDEF; // so the "not already defined" check passes
> >   ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
> Yes but I meant why a pointer?

I can't think of any good reason. It must be some leftover as you said.

> 
> Eric
> >
> > Thanks,
> > Ricardo
> >
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	if (!IS_ALIGNED(size, alignment))
> >>> +		return -EINVAL;
> >>> +
> >>> +	if (addr + size < addr)
> >>> +		return -EINVAL;
> >>> +
> >>> +	if (addr + size > kvm_phys_size(kvm))
> >>> +		return -E2BIG;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>>  static int vgic_check_type(struct kvm *kvm, int type_needed)
> >>>  {
> >>>  	if (kvm->arch.vgic.vgic_model != type_needed)
> >>> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
> >>> index 14a9218641f5..c4df4dcef31f 100644
> >>> --- a/arch/arm64/kvm/vgic/vgic.h
> >>> +++ b/arch/arm64/kvm/vgic/vgic.h
> >>> @@ -175,6 +175,10 @@ void vgic_irq_handle_resampling(struct vgic_irq *irq,
> >>>  int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
> >>>  		      phys_addr_t addr, phys_addr_t alignment);
> >>>  
> >>> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr,
> >>> +		       phys_addr_t addr, phys_addr_t alignment,
> >>> +		       phys_addr_t size);
> >>> +
> >>>  void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu);
> >>>  void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr);
> >>>  void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr);
> >> Besides
> >> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> >> Eric
> >>
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 01/10] kvm: arm64: vgic: Introduce vgic_check_iorange

[Marc Zyngier]

On Thu, 30 Sep 2021 22:19:16 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Thu, Sep 30, 2021 at 09:02:12AM +0200, Eric Auger wrote:
> > Hi,
> > 
> > On 9/29/21 11:17 PM, Ricardo Koller wrote:
> > > On Wed, Sep 29, 2021 at 06:29:21PM +0200, Eric Auger wrote:
> > >> Hi Ricardo,
> > >>
> > >> On 9/28/21 8:47 PM, Ricardo Koller wrote:
> > >>> Add the new vgic_check_iorange helper that checks that an iorange is
> > >>> sane: the start address and size have valid alignments, the range is
> > >>> within the addressable PA range, start+size doesn't overflow, and the
> > >>> start wasn't already defined.
> > >>>
> > >>> No functional change.
> > >>>
> > >>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > >>> ---
> > >>>  arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++
> > >>>  arch/arm64/kvm/vgic/vgic.h            |  4 ++++
> > >>>  2 files changed, 26 insertions(+)
> > >>>
> > >>> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > >>> index 7740995de982..f714aded67b2 100644
> > >>> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > >>> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > >>> @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
> > >>>  	return 0;
> > >>>  }
> > >>>  
> > >>> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr,
> > >>> +		       phys_addr_t addr, phys_addr_t alignment,
> > >>> +		       phys_addr_t size)
> > >>> +{
> > >>> +	int ret;
> > >>> +
> > >>> +	ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment);
> > >> nit: not related to this patch but I am just wondering why we are
> > >> passing phys_addr_t *ioaddr downto vgic_check_ioaddr and thus to
> > >>
> > >> vgic_check_iorange()? This must be a leftover of some old code?
> > >>
> > > It's used to check that the base of a region is not already set.
> > > kvm_vgic_addr() uses it to make that check;
> > > vgic_v3_alloc_redist_region() does not:
> > >
> > >   rdreg->base = VGIC_ADDR_UNDEF; // so the "not already defined" check passes
> > >   ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
> > Yes but I meant why a pointer?
> 
> I can't think of any good reason. It must be some leftover as you said.

It definitely is. Please have a patch to fix that. Also, it doesn't
look like vgic_check_ioaddr() has any other user at the end of the
series. Worth getting rid of.

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 01/10] kvm: arm64: vgic: Introduce vgic_check_iorange

[Ricardo Koller]

On Fri, Oct 01, 2021 at 02:12:17PM +0100, Marc Zyngier wrote:
> On Thu, 30 Sep 2021 22:19:16 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Thu, Sep 30, 2021 at 09:02:12AM +0200, Eric Auger wrote:
> > > Hi,
> > > 
> > > On 9/29/21 11:17 PM, Ricardo Koller wrote:
> > > > On Wed, Sep 29, 2021 at 06:29:21PM +0200, Eric Auger wrote:
> > > >> Hi Ricardo,
> > > >>
> > > >> On 9/28/21 8:47 PM, Ricardo Koller wrote:
> > > >>> Add the new vgic_check_iorange helper that checks that an iorange is
> > > >>> sane: the start address and size have valid alignments, the range is
> > > >>> within the addressable PA range, start+size doesn't overflow, and the
> > > >>> start wasn't already defined.
> > > >>>
> > > >>> No functional change.
> > > >>>
> > > >>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > >>> ---
> > > >>>  arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++
> > > >>>  arch/arm64/kvm/vgic/vgic.h            |  4 ++++
> > > >>>  2 files changed, 26 insertions(+)
> > > >>>
> > > >>> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > > >>> index 7740995de982..f714aded67b2 100644
> > > >>> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > > >>> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > > >>> @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
> > > >>>  	return 0;
> > > >>>  }
> > > >>>  
> > > >>> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr,
> > > >>> +		       phys_addr_t addr, phys_addr_t alignment,
> > > >>> +		       phys_addr_t size)
> > > >>> +{
> > > >>> +	int ret;
> > > >>> +
> > > >>> +	ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment);
> > > >> nit: not related to this patch but I am just wondering why we are
> > > >> passing phys_addr_t *ioaddr downto vgic_check_ioaddr and thus to
> > > >>
> > > >> vgic_check_iorange()? This must be a leftover of some old code?
> > > >>
> > > > It's used to check that the base of a region is not already set.
> > > > kvm_vgic_addr() uses it to make that check;
> > > > vgic_v3_alloc_redist_region() does not:
> > > >
> > > >   rdreg->base = VGIC_ADDR_UNDEF; // so the "not already defined" check passes
> > > >   ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
> > > Yes but I meant why a pointer?
> > 
> > I can't think of any good reason. It must be some leftover as you said.
> 
> It definitely is. Please have a patch to fix that. Also, it doesn't
> look like vgic_check_ioaddr() has any other user at the end of the
> series. Worth getting rid of.

ACK fixing that and getting rid of vgic_check_ioaddr().

Thanks,
Ricardo

> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 02/10] KVM: arm64: vgic-v3: Check redist region is not above the VM IPA size

[Ricardo Koller]

Verify that the redistributor regions do not extend beyond the
VM-specified IPA range (phys_size). This can happen when using
KVM_VGIC_V3_ADDR_TYPE_REDIST or KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS
with:

  base + size > phys_size AND base < phys_size

Add the missing check into vgic_v3_alloc_redist_region() which is called
when setting the regions, and into vgic_v3_check_base() which is called
when attempting the first vcpu-run. The vcpu-run check does not apply to
KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS because the regions size is known
before the first vcpu-run. Note that using the REDIST_REGIONS API
results in a different check, which already exists, at first vcpu run:
that the number of redist regions is enough for all vcpus.

Finally, this patch also enables some extra tests in
vgic_v3_alloc_redist_region() by calculating "size" early for the legacy
redist api: like checking that the REDIST region can fit all the already
created vcpus.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++--
 arch/arm64/kvm/vgic/vgic-v3.c      | 4 ++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index a09cdc0b953c..9be02bf7865e 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -796,7 +796,9 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
 	struct vgic_dist *d = &kvm->arch.vgic;
 	struct vgic_redist_region *rdreg;
 	struct list_head *rd_regions = &d->rd_regions;
-	size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
+	int nr_vcpus = atomic_read(&kvm->online_vcpus);
+	size_t size = count ? count * KVM_VGIC_V3_REDIST_SIZE
+			    : nr_vcpus * KVM_VGIC_V3_REDIST_SIZE;
 	int ret;
 
 	/* cross the end of memory ? */
@@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
 
 	rdreg->base = VGIC_ADDR_UNDEF;
 
-	ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
+	ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size);
 	if (ret)
 		goto free;
 
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 21a6207fb2ee..27ee674631b3 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -486,6 +486,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
 		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
 			rdreg->base)
 			return false;
+
+		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
+			kvm_phys_size(kvm))
+			return false;
 	}
 
 	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
-- 
2.33.0.685.g46640cef36-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 02/10] KVM: arm64: vgic-v3: Check redist region is not above the VM IPA size

[Eric Auger]

Hi Ricardo,

On 9/28/21 8:47 PM, Ricardo Koller wrote:
> Verify that the redistributor regions do not extend beyond the
> VM-specified IPA range (phys_size). This can happen when using
> KVM_VGIC_V3_ADDR_TYPE_REDIST or KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS
> with:
>
>   base + size > phys_size AND base < phys_size
>
> Add the missing check into vgic_v3_alloc_redist_region() which is called
> when setting the regions, and into vgic_v3_check_base() which is called
> when attempting the first vcpu-run. The vcpu-run check does not apply to
> KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS because the regions size is known
> before the first vcpu-run. Note that using the REDIST_REGIONS API
> results in a different check, which already exists, at first vcpu run:
> that the number of redist regions is enough for all vcpus.
>
> Finally, this patch also enables some extra tests in
> vgic_v3_alloc_redist_region() by calculating "size" early for the legacy
> redist api: like checking that the REDIST region can fit all the already
> created vcpus.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++--
>  arch/arm64/kvm/vgic/vgic-v3.c      | 4 ++++
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index a09cdc0b953c..9be02bf7865e 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -796,7 +796,9 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
>  	struct vgic_dist *d = &kvm->arch.vgic;
>  	struct vgic_redist_region *rdreg;
>  	struct list_head *rd_regions = &d->rd_regions;
> -	size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
> +	int nr_vcpus = atomic_read(&kvm->online_vcpus);
> +	size_t size = count ? count * KVM_VGIC_V3_REDIST_SIZE
> +			    : nr_vcpus * KVM_VGIC_V3_REDIST_SIZE;

This actually fixes the  vgic_dist_overlap(kvm, base, size=0)

in case the number of online-vcpus at that time is less than the final
one (1st run), if count=0 (legacy API) do we ever check that the RDIST
(with accumulated vcpu rdists) does not overlap with dist.
in other words shouldn't we call vgic_dist_overlap(kvm, base, size)
again in vgic_v3_check_base().

Thanks

Eric

>  	int ret;
>  
>  	/* cross the end of memory ? */
> @@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
>  
>  	rdreg->base = VGIC_ADDR_UNDEF;
>  
> -	ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
> +	ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size);
>  	if (ret)
>  		goto free;
>  
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 21a6207fb2ee..27ee674631b3 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -486,6 +486,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
>  			rdreg->base)
>  			return false;
> +
> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
> +			kvm_phys_size(kvm))
> +			return false;
>  	}
>  
>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 02/10] KVM: arm64: vgic-v3: Check redist region is not above the VM IPA size

[Ricardo Koller]

Hi Eric,

On Wed, Sep 29, 2021 at 06:23:04PM +0200, Eric Auger wrote:
> Hi Ricardo,
> 
> On 9/28/21 8:47 PM, Ricardo Koller wrote:
> > Verify that the redistributor regions do not extend beyond the
> > VM-specified IPA range (phys_size). This can happen when using
> > KVM_VGIC_V3_ADDR_TYPE_REDIST or KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS
> > with:
> >
> >   base + size > phys_size AND base < phys_size
> >
> > Add the missing check into vgic_v3_alloc_redist_region() which is called
> > when setting the regions, and into vgic_v3_check_base() which is called
> > when attempting the first vcpu-run. The vcpu-run check does not apply to
> > KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS because the regions size is known
> > before the first vcpu-run. Note that using the REDIST_REGIONS API
> > results in a different check, which already exists, at first vcpu run:
> > that the number of redist regions is enough for all vcpus.
> >
> > Finally, this patch also enables some extra tests in
> > vgic_v3_alloc_redist_region() by calculating "size" early for the legacy
> > redist api: like checking that the REDIST region can fit all the already
> > created vcpus.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++--
> >  arch/arm64/kvm/vgic/vgic-v3.c      | 4 ++++
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > index a09cdc0b953c..9be02bf7865e 100644
> > --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > @@ -796,7 +796,9 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
> >  	struct vgic_dist *d = &kvm->arch.vgic;
> >  	struct vgic_redist_region *rdreg;
> >  	struct list_head *rd_regions = &d->rd_regions;
> > -	size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
> > +	int nr_vcpus = atomic_read(&kvm->online_vcpus);
> > +	size_t size = count ? count * KVM_VGIC_V3_REDIST_SIZE
> > +			    : nr_vcpus * KVM_VGIC_V3_REDIST_SIZE;
> 
> This actually fixes the  vgic_dist_overlap(kvm, base, size=0)
> 
> in case the number of online-vcpus at that time is less than the final
> one (1st run), if count=0 (legacy API) do we ever check that the RDIST
> (with accumulated vcpu rdists) does not overlap with dist.
> in other words shouldn't we call vgic_dist_overlap(kvm, base, size)
> again in vgic_v3_check_base().
> 

I think we're good; that's checked by vgic_v3_rdist_overlap(dist_base)
in vgic_v3_check_base(). This function uses the only region (legacy
case) using a size based on the online_vcpus (in
vgic_v3_rd_region_size()).  This exact situation is tested by
test_vgic_then_vcpus() in the vgic_init selftest.

Thanks,
Ricardo

> Thanks
> 
> Eric
> 
> >  	int ret;
> >  
> >  	/* cross the end of memory ? */
> > @@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
> >  
> >  	rdreg->base = VGIC_ADDR_UNDEF;
> >  
> > -	ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
> > +	ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size);
> >  	if (ret)
> >  		goto free;
> >  
> > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> > index 21a6207fb2ee..27ee674631b3 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v3.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> > @@ -486,6 +486,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
> >  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
> >  			rdreg->base)
> >  			return false;
> > +
> > +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
> > +			kvm_phys_size(kvm))
> > +			return false;
> >  	}
> >  
> >  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 02/10] KVM: arm64: vgic-v3: Check redist region is not above the VM IPA size

[Eric Auger]

Hi,

On 9/29/21 11:10 PM, Ricardo Koller wrote:
> Hi Eric,
>
> On Wed, Sep 29, 2021 at 06:23:04PM +0200, Eric Auger wrote:
>> Hi Ricardo,
>>
>> On 9/28/21 8:47 PM, Ricardo Koller wrote:
>>> Verify that the redistributor regions do not extend beyond the
>>> VM-specified IPA range (phys_size). This can happen when using
>>> KVM_VGIC_V3_ADDR_TYPE_REDIST or KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS
>>> with:
>>>
>>>   base + size > phys_size AND base < phys_size
>>>
>>> Add the missing check into vgic_v3_alloc_redist_region() which is called
>>> when setting the regions, and into vgic_v3_check_base() which is called
>>> when attempting the first vcpu-run. The vcpu-run check does not apply to
>>> KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS because the regions size is known
>>> before the first vcpu-run. Note that using the REDIST_REGIONS API
>>> results in a different check, which already exists, at first vcpu run:
>>> that the number of redist regions is enough for all vcpus.
>>>
>>> Finally, this patch also enables some extra tests in
>>> vgic_v3_alloc_redist_region() by calculating "size" early for the legacy
>>> redist api: like checking that the REDIST region can fit all the already
>>> created vcpus.
>>>
>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
>>> ---
>>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++--
>>>  arch/arm64/kvm/vgic/vgic-v3.c      | 4 ++++
>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>> index a09cdc0b953c..9be02bf7865e 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>>> @@ -796,7 +796,9 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
>>>  	struct vgic_dist *d = &kvm->arch.vgic;
>>>  	struct vgic_redist_region *rdreg;
>>>  	struct list_head *rd_regions = &d->rd_regions;
>>> -	size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
>>> +	int nr_vcpus = atomic_read(&kvm->online_vcpus);
>>> +	size_t size = count ? count * KVM_VGIC_V3_REDIST_SIZE
>>> +			    : nr_vcpus * KVM_VGIC_V3_REDIST_SIZE;
>> This actually fixes the  vgic_dist_overlap(kvm, base, size=0)
>>
>> in case the number of online-vcpus at that time is less than the final
>> one (1st run), if count=0 (legacy API) do we ever check that the RDIST
>> (with accumulated vcpu rdists) does not overlap with dist.
>> in other words shouldn't we call vgic_dist_overlap(kvm, base, size)
>> again in vgic_v3_check_base().
>>
> I think we're good; that's checked by vgic_v3_rdist_overlap(dist_base)
> in vgic_v3_check_base(). This function uses the only region (legacy
> case) using a size based on the online_vcpus (in
> vgic_v3_rd_region_size()).  This exact situation is tested by
> test_vgic_then_vcpus() in the vgic_init selftest.
Ah OK so that's fine then.

So looks good to me
Reviewed-by: Eric Auger <eric.auger@redhat.com>


Eric


>
> Thanks,
> Ricardo
>
>> Thanks
>>
>> Eric
>>
>>>  	int ret;
>>>  
>>>  	/* cross the end of memory ? */
>>> @@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
>>>  
>>>  	rdreg->base = VGIC_ADDR_UNDEF;
>>>  
>>> -	ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
>>> +	ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size);
>>>  	if (ret)
>>>  		goto free;
>>>  
>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>>> index 21a6207fb2ee..27ee674631b3 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>>> @@ -486,6 +486,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
>>>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
>>>  			rdreg->base)
>>>  			return false;
>>> +
>>> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
>>> +			kvm_phys_size(kvm))
>>> +			return false;
>>>  	}
>>>  
>>>  	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 02/10] KVM: arm64: vgic-v3: Check redist region is not above the VM IPA size

[Marc Zyngier]

On Tue, 28 Sep 2021 19:47:56 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> Verify that the redistributor regions do not extend beyond the
> VM-specified IPA range (phys_size). This can happen when using
> KVM_VGIC_V3_ADDR_TYPE_REDIST or KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS
> with:
> 
>   base + size > phys_size AND base < phys_size
> 
> Add the missing check into vgic_v3_alloc_redist_region() which is called
> when setting the regions, and into vgic_v3_check_base() which is called
> when attempting the first vcpu-run. The vcpu-run check does not apply to
> KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS because the regions size is known
> before the first vcpu-run. Note that using the REDIST_REGIONS API
> results in a different check, which already exists, at first vcpu run:
> that the number of redist regions is enough for all vcpus.
> 
> Finally, this patch also enables some extra tests in
> vgic_v3_alloc_redist_region() by calculating "size" early for the legacy
> redist api: like checking that the REDIST region can fit all the already
> created vcpus.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++--
>  arch/arm64/kvm/vgic/vgic-v3.c      | 4 ++++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index a09cdc0b953c..9be02bf7865e 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -796,7 +796,9 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
>  	struct vgic_dist *d = &kvm->arch.vgic;
>  	struct vgic_redist_region *rdreg;
>  	struct list_head *rd_regions = &d->rd_regions;
> -	size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
> +	int nr_vcpus = atomic_read(&kvm->online_vcpus);
> +	size_t size = count ? count * KVM_VGIC_V3_REDIST_SIZE
> +			    : nr_vcpus * KVM_VGIC_V3_REDIST_SIZE;
>  	int ret;
>  
>  	/* cross the end of memory ? */
> @@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
>  
>  	rdreg->base = VGIC_ADDR_UNDEF;
>  
> -	ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
> +	ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size);
>  	if (ret)
>  		goto free;
>  
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 21a6207fb2ee..27ee674631b3 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -486,6 +486,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
>  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
>  			rdreg->base)
>  			return false;
> +
> +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
> +			kvm_phys_size(kvm))
> +			return false;

Why can't we replace these two checks with a single call to your new
fancy helper?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 02/10] KVM: arm64: vgic-v3: Check redist region is not above the VM IPA size

[Ricardo Koller]

On Fri, Oct 01, 2021 at 02:14:29PM +0100, Marc Zyngier wrote:
> On Tue, 28 Sep 2021 19:47:56 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > Verify that the redistributor regions do not extend beyond the
> > VM-specified IPA range (phys_size). This can happen when using
> > KVM_VGIC_V3_ADDR_TYPE_REDIST or KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS
> > with:
> > 
> >   base + size > phys_size AND base < phys_size
> > 
> > Add the missing check into vgic_v3_alloc_redist_region() which is called
> > when setting the regions, and into vgic_v3_check_base() which is called
> > when attempting the first vcpu-run. The vcpu-run check does not apply to
> > KVM_VGIC_V3_ADDR_TYPE_REDIST_REGIONS because the regions size is known
> > before the first vcpu-run. Note that using the REDIST_REGIONS API
> > results in a different check, which already exists, at first vcpu run:
> > that the number of redist regions is enough for all vcpus.
> > 
> > Finally, this patch also enables some extra tests in
> > vgic_v3_alloc_redist_region() by calculating "size" early for the legacy
> > redist api: like checking that the REDIST region can fit all the already
> > created vcpus.
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++++--
> >  arch/arm64/kvm/vgic/vgic-v3.c      | 4 ++++
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > index a09cdc0b953c..9be02bf7865e 100644
> > --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > @@ -796,7 +796,9 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
> >  	struct vgic_dist *d = &kvm->arch.vgic;
> >  	struct vgic_redist_region *rdreg;
> >  	struct list_head *rd_regions = &d->rd_regions;
> > -	size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
> > +	int nr_vcpus = atomic_read(&kvm->online_vcpus);
> > +	size_t size = count ? count * KVM_VGIC_V3_REDIST_SIZE
> > +			    : nr_vcpus * KVM_VGIC_V3_REDIST_SIZE;
> >  	int ret;
> >  
> >  	/* cross the end of memory ? */
> > @@ -840,7 +842,7 @@ static int vgic_v3_alloc_redist_region(struct kvm *kvm, uint32_t index,
> >  
> >  	rdreg->base = VGIC_ADDR_UNDEF;
> >  
> > -	ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
> > +	ret = vgic_check_iorange(kvm, &rdreg->base, base, SZ_64K, size);
> >  	if (ret)
> >  		goto free;
> >  
> > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> > index 21a6207fb2ee..27ee674631b3 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v3.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> > @@ -486,6 +486,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
> >  		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
> >  			rdreg->base)
> >  			return false;
> > +
> > +		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
> > +			kvm_phys_size(kvm))
> > +			return false;
> 
> Why can't we replace these two checks with a single call to your new
> fancy helper?

ACK using the new helper (on rdreg base and size).

Thanks,
Ricardo

> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 03/10] KVM: arm64: vgic-v2: Check cpu interface region is not above the VM IPA size

[Ricardo Koller]

Verify that the GICv2 CPU interface does not extend beyond the
VM-specified IPA range (phys_size).

  base + size > phys_size AND base < phys_size

Add the missing check into kvm_vgic_addr() which is called when setting
the region. This patch also enables some superfluous checks for the
distributor (vgic_check_ioaddr was enough as alignment == size for the
distributors).

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-kvm-device.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
index f714aded67b2..b379eb81fddb 100644
--- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
+++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
@@ -79,7 +79,7 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
 {
 	int r = 0;
 	struct vgic_dist *vgic = &kvm->arch.vgic;
-	phys_addr_t *addr_ptr, alignment;
+	phys_addr_t *addr_ptr, alignment, size;
 	u64 undef_value = VGIC_ADDR_UNDEF;
 
 	mutex_lock(&kvm->lock);
@@ -88,16 +88,19 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
 		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
 		addr_ptr = &vgic->vgic_dist_base;
 		alignment = SZ_4K;
+		size = KVM_VGIC_V2_DIST_SIZE;
 		break;
 	case KVM_VGIC_V2_ADDR_TYPE_CPU:
 		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
 		addr_ptr = &vgic->vgic_cpu_base;
 		alignment = SZ_4K;
+		size = KVM_VGIC_V2_CPU_SIZE;
 		break;
 	case KVM_VGIC_V3_ADDR_TYPE_DIST:
 		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V3);
 		addr_ptr = &vgic->vgic_dist_base;
 		alignment = SZ_64K;
+		size = KVM_VGIC_V3_DIST_SIZE;
 		break;
 	case KVM_VGIC_V3_ADDR_TYPE_REDIST: {
 		struct vgic_redist_region *rdreg;
@@ -162,7 +165,7 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
 		goto out;
 
 	if (write) {
-		r = vgic_check_ioaddr(kvm, addr_ptr, *addr, alignment);
+		r = vgic_check_iorange(kvm, addr_ptr, *addr, alignment, size);
 		if (!r)
 			*addr_ptr = *addr;
 	} else {
-- 
2.33.0.685.g46640cef36-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 03/10] KVM: arm64: vgic-v2: Check cpu interface region is not above the VM IPA size

[Eric Auger]

Hi Ricardo,

On 9/28/21 8:47 PM, Ricardo Koller wrote:
> Verify that the GICv2 CPU interface does not extend beyond the
> VM-specified IPA range (phys_size).
>
>   base + size > phys_size AND base < phys_size
>
> Add the missing check into kvm_vgic_addr() which is called when setting
> the region. This patch also enables some superfluous checks for the
> distributor (vgic_check_ioaddr was enough as alignment == size for the
> distributors).
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic-kvm-device.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> index f714aded67b2..b379eb81fddb 100644
> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> @@ -79,7 +79,7 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
>  {
>  	int r = 0;
>  	struct vgic_dist *vgic = &kvm->arch.vgic;
> -	phys_addr_t *addr_ptr, alignment;
> +	phys_addr_t *addr_ptr, alignment, size;
>  	u64 undef_value = VGIC_ADDR_UNDEF;
>  
>  	mutex_lock(&kvm->lock);
> @@ -88,16 +88,19 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
>  		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
>  		addr_ptr = &vgic->vgic_dist_base;
>  		alignment = SZ_4K;
> +		size = KVM_VGIC_V2_DIST_SIZE;
>  		break;
>  	case KVM_VGIC_V2_ADDR_TYPE_CPU:
>  		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
>  		addr_ptr = &vgic->vgic_cpu_base;
>  		alignment = SZ_4K;
> +		size = KVM_VGIC_V2_CPU_SIZE;
>  		break;
>  	case KVM_VGIC_V3_ADDR_TYPE_DIST:
>  		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V3);
>  		addr_ptr = &vgic->vgic_dist_base;
>  		alignment = SZ_64K;
> +		size = KVM_VGIC_V3_DIST_SIZE;
>  		break;
>  	case KVM_VGIC_V3_ADDR_TYPE_REDIST: {
>  		struct vgic_redist_region *rdreg;
> @@ -162,7 +165,7 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
>  		goto out;
>  
>  	if (write) {
> -		r = vgic_check_ioaddr(kvm, addr_ptr, *addr, alignment);
> +		r = vgic_check_iorange(kvm, addr_ptr, *addr, alignment, size);
>  		if (!r)
>  			*addr_ptr = *addr;
>  	} else {
Looks god to me

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


Eric

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 04/10] KVM: arm64: vgic-v3: Check ITS region is not above the VM IPA size

[Ricardo Koller]

Verify that the ITS region does not extend beyond the VM-specified IPA
range (phys_size).

  base + size > phys_size AND base < phys_size

Add the missing check into vgic_its_set_attr() which is called when
setting the region.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 61728c543eb9..321743b87002 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2710,8 +2710,8 @@ static int vgic_its_set_attr(struct kvm_device *dev,
 		if (copy_from_user(&addr, uaddr, sizeof(addr)))
 			return -EFAULT;
 
-		ret = vgic_check_ioaddr(dev->kvm, &its->vgic_its_base,
-					addr, SZ_64K);
+		ret = vgic_check_iorange(dev->kvm, &its->vgic_its_base,
+					 addr, SZ_64K, KVM_VGIC_V3_ITS_SIZE);
 		if (ret)
 			return ret;
 
-- 
2.33.0.685.g46640cef36-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 04/10] KVM: arm64: vgic-v3: Check ITS region is not above the VM IPA size

[Eric Auger]

On 9/28/21 8:47 PM, Ricardo Koller wrote:
> Verify that the ITS region does not extend beyond the VM-specified IPA
> range (phys_size).
>
>   base + size > phys_size AND base < phys_size
>
> Add the missing check into vgic_its_set_attr() which is called when
> setting the region.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 61728c543eb9..321743b87002 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -2710,8 +2710,8 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>  		if (copy_from_user(&addr, uaddr, sizeof(addr)))
>  			return -EFAULT;
>  
> -		ret = vgic_check_ioaddr(dev->kvm, &its->vgic_its_base,
> -					addr, SZ_64K);
> +		ret = vgic_check_iorange(dev->kvm, &its->vgic_its_base,
> +					 addr, SZ_64K, KVM_VGIC_V3_ITS_SIZE);
>  		if (ret)
>  			return ret;
>  
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 05/10] KVM: arm64: selftests: Make vgic_init gic version agnostic

[Ricardo Koller]

As a preparation for the next commits which will add some tests for
GICv2, make aarch64/vgic_init GIC version agnostic. Add a new generic
run_tests function(gic_dev_type) that starts all applicable tests using
GICv3 or GICv2. GICv2 tests are attempted if GICv3 is not available in
the system. There are currently no GICv2 tests, but the test passes now
in GICv2 systems.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../testing/selftests/kvm/aarch64/vgic_init.c | 156 +++++++++++-------
 1 file changed, 95 insertions(+), 61 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
index 623f31a14326..896a29f2503d 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -22,6 +22,9 @@
 
 #define GICR_TYPER 0x8
 
+#define VGIC_DEV_IS_V2(_d) ((_d) == KVM_DEV_TYPE_ARM_VGIC_V2)
+#define VGIC_DEV_IS_V3(_d) ((_d) == KVM_DEV_TYPE_ARM_VGIC_V3)
+
 struct vm_gic {
 	struct kvm_vm *vm;
 	int gic_fd;
@@ -30,8 +33,8 @@ struct vm_gic {
 static int max_ipa_bits;
 
 /* helper to access a redistributor register */
-static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
-			     uint32_t *val, bool write)
+static int access_v3_redist_reg(int gicv3_fd, int vcpu, int offset,
+				uint32_t *val, bool write)
 {
 	uint64_t attr = REG_OFFSET(vcpu, offset);
 
@@ -58,7 +61,7 @@ static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
 	return 0;
 }
 
-static struct vm_gic vm_gic_create(void)
+static struct vm_gic vm_gic_v3_create(void)
 {
 	struct vm_gic v;
 
@@ -80,7 +83,7 @@ static void vm_gic_destroy(struct vm_gic *v)
  * device gets created, a legacy RDIST region is set at @0x0
  * and a DIST region is set @0x60000
  */
-static void subtest_dist_rdist(struct vm_gic *v)
+static void subtest_v3_dist_rdist(struct vm_gic *v)
 {
 	int ret;
 	uint64_t addr;
@@ -145,7 +148,7 @@ static void subtest_dist_rdist(struct vm_gic *v)
 }
 
 /* Test the new REDIST region API */
-static void subtest_redist_regions(struct vm_gic *v)
+static void subtest_v3_redist_regions(struct vm_gic *v)
 {
 	uint64_t addr, expected_addr;
 	int ret;
@@ -249,7 +252,7 @@ static void subtest_redist_regions(struct vm_gic *v)
  * VGIC KVM device is created and initialized before the secondary CPUs
  * get created
  */
-static void test_vgic_then_vcpus(void)
+static void test_v3_vgic_then_vcpus(uint32_t gic_dev_type)
 {
 	struct vm_gic v;
 	int ret, i;
@@ -257,7 +260,7 @@ static void test_vgic_then_vcpus(void)
 	v.vm = vm_create_default(0, 0, guest_code);
 	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
 
-	subtest_dist_rdist(&v);
+	subtest_v3_dist_rdist(&v);
 
 	/* Add the rest of the VCPUs */
 	for (i = 1; i < NR_VCPUS; ++i)
@@ -270,14 +273,14 @@ static void test_vgic_then_vcpus(void)
 }
 
 /* All the VCPUs are created before the VGIC KVM device gets initialized */
-static void test_vcpus_then_vgic(void)
+static void test_v3_vcpus_then_vgic(uint32_t gic_dev_type)
 {
 	struct vm_gic v;
 	int ret;
 
-	v = vm_gic_create();
+	v = vm_gic_v3_create();
 
-	subtest_dist_rdist(&v);
+	subtest_v3_dist_rdist(&v);
 
 	ret = run_vcpu(v.vm, 3);
 	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
@@ -285,15 +288,15 @@ static void test_vcpus_then_vgic(void)
 	vm_gic_destroy(&v);
 }
 
-static void test_new_redist_regions(void)
+static void test_v3_new_redist_regions(void)
 {
 	void *dummy = NULL;
 	struct vm_gic v;
 	uint64_t addr;
 	int ret;
 
-	v = vm_gic_create();
-	subtest_redist_regions(&v);
+	v = vm_gic_v3_create();
+	subtest_v3_redist_regions(&v);
 	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
 			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
 
@@ -303,8 +306,8 @@ static void test_new_redist_regions(void)
 
 	/* step2 */
 
-	v = vm_gic_create();
-	subtest_redist_regions(&v);
+	v = vm_gic_v3_create();
+	subtest_v3_redist_regions(&v);
 
 	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 0, 2);
 	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
@@ -317,8 +320,8 @@ static void test_new_redist_regions(void)
 
 	/* step 3 */
 
-	v = vm_gic_create();
-	subtest_redist_regions(&v);
+	v = vm_gic_v3_create();
+	subtest_v3_redist_regions(&v);
 
 	_kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
 			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, dummy, true);
@@ -338,7 +341,7 @@ static void test_new_redist_regions(void)
 	vm_gic_destroy(&v);
 }
 
-static void test_typer_accesses(void)
+static void test_v3_typer_accesses(void)
 {
 	struct vm_gic v;
 	uint64_t addr;
@@ -351,12 +354,12 @@ static void test_typer_accesses(void)
 
 	vm_vcpu_add_default(v.vm, 3, guest_code);
 
-	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
 	TEST_ASSERT(ret && errno == EINVAL, "attempting to read GICR_TYPER of non created vcpu");
 
 	vm_vcpu_add_default(v.vm, 1, guest_code);
 
-	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
 	TEST_ASSERT(ret && errno == EBUSY, "read GICR_TYPER before GIC initialized");
 
 	vm_vcpu_add_default(v.vm, 2, guest_code);
@@ -365,7 +368,7 @@ static void test_typer_accesses(void)
 			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
 
 	for (i = 0; i < NR_VCPUS ; i++) {
-		ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
+		ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
 		TEST_ASSERT(!ret && !val, "read GICR_TYPER before rdist region setting");
 	}
 
@@ -374,10 +377,10 @@ static void test_typer_accesses(void)
 			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
 
 	/* The 2 first rdists should be put there (vcpu 0 and 3) */
-	ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && !val, "read typer of rdist #0");
 
-	ret = access_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #1");
 
 	addr = REDIST_REGION_ATTR_ADDR(10, 0x100000, 0, 1);
@@ -385,11 +388,11 @@ static void test_typer_accesses(void)
 				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
 	TEST_ASSERT(ret && errno == EINVAL, "collision with previous rdist region");
 
-	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x100,
 		    "no redist region attached to vcpu #1 yet, last cannot be returned");
 
-	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x200,
 		    "no redist region attached to vcpu #2, last cannot be returned");
 
@@ -397,10 +400,10 @@ static void test_typer_accesses(void)
 	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
 			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
 
-	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
 
-	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x210,
 		    "read typer of rdist #1, last properly returned");
 
@@ -417,7 +420,7 @@ static void test_typer_accesses(void)
  * rdist region #2 @0x200000 2 rdist capacity
  *     rdists: 1, 2
  */
-static void test_last_bit_redist_regions(void)
+static void test_v3_last_bit_redist_regions(void)
 {
 	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
 	struct vm_gic v;
@@ -444,29 +447,29 @@ static void test_last_bit_redist_regions(void)
 	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
 			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
 
-	ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
 
-	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
 
-	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x200, "read typer of rdist #2");
 
-	ret = access_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #3");
 
-	ret = access_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #5");
 
-	ret = access_redist_reg(v.gic_fd, 4, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 4, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x410, "read typer of rdist #4");
 
 	vm_gic_destroy(&v);
 }
 
 /* Test last bit with legacy region */
-static void test_last_bit_single_rdist(void)
+static void test_v3_last_bit_single_rdist(void)
 {
 	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
 	struct vm_gic v;
@@ -485,28 +488,32 @@ static void test_last_bit_single_rdist(void)
 	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
 			  KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
 
-	ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
 
-	ret = access_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x300, "read typer of rdist #1");
 
-	ret = access_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #2");
 
-	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #3");
 
-	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
+	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
 	TEST_ASSERT(!ret && val == 0x210, "read typer of rdist #3");
 
 	vm_gic_destroy(&v);
 }
 
-void test_kvm_device(void)
+/*
+ * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
+ */
+int test_kvm_device(uint32_t gic_dev_type)
 {
 	struct vm_gic v;
 	int ret, fd;
+	uint32_t other;
 
 	v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
 
@@ -514,38 +521,65 @@ void test_kvm_device(void)
 	ret = _kvm_create_device(v.vm, 0, true, &fd);
 	TEST_ASSERT(ret && errno == ENODEV, "unsupported device");
 
-	/* trial mode with VGIC_V3 device */
-	ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true, &fd);
-	if (ret) {
-		print_skip("GICv3 not supported");
-		exit(KSFT_SKIP);
-	}
-	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+	/* trial mode */
+	ret = _kvm_create_device(v.vm, gic_dev_type, true, &fd);
+	if (ret)
+		return ret;
+	v.gic_fd = kvm_create_device(v.vm, gic_dev_type, false);
+
+	ret = _kvm_create_device(v.vm, gic_dev_type, false, &fd);
+	TEST_ASSERT(ret && errno == EEXIST, "create GIC device twice");
 
-	ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false, &fd);
-	TEST_ASSERT(ret && errno == EEXIST, "create GICv3 device twice");
+	kvm_create_device(v.vm, gic_dev_type, true);
 
-	kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
+	/* try to create the other gic_dev_type */
+	other = VGIC_DEV_IS_V2(gic_dev_type) ? KVM_DEV_TYPE_ARM_VGIC_V3
+					     : KVM_DEV_TYPE_ARM_VGIC_V2;
 
-	if (!_kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, true, &fd)) {
-		ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, false, &fd);
-		TEST_ASSERT(ret && errno == EINVAL, "create GICv2 while v3 exists");
+	if (!_kvm_create_device(v.vm, other, true, &fd)) {
+		ret = _kvm_create_device(v.vm, other, false, &fd);
+		TEST_ASSERT(ret && errno == EINVAL,
+				"create GIC device while other version exists");
 	}
 
 	vm_gic_destroy(&v);
+
+	return 0;
+}
+
+void run_tests(uint32_t gic_dev_type)
+{
+	if (VGIC_DEV_IS_V3(gic_dev_type)) {
+		test_v3_vcpus_then_vgic(gic_dev_type);
+		test_v3_vgic_then_vcpus(gic_dev_type);
+		test_v3_new_redist_regions();
+		test_v3_typer_accesses();
+		test_v3_last_bit_redist_regions();
+		test_v3_last_bit_single_rdist();
+	}
 }
 
 int main(int ac, char **av)
 {
+	int ret;
+
 	max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
 
-	test_kvm_device();
-	test_vcpus_then_vgic();
-	test_vgic_then_vcpus();
-	test_new_redist_regions();
-	test_typer_accesses();
-	test_last_bit_redist_regions();
-	test_last_bit_single_rdist();
+	ret = test_kvm_device(KVM_DEV_TYPE_ARM_VGIC_V3);
+	if (!ret) {
+		pr_info("Running GIC_v3 tests.\n");
+		run_tests(KVM_DEV_TYPE_ARM_VGIC_V3);
+		return 0;
+	}
+
+	ret = test_kvm_device(KVM_DEV_TYPE_ARM_VGIC_V2);
+	if (!ret) {
+		pr_info("Running GIC_v2 tests.\n");
+		run_tests(KVM_DEV_TYPE_ARM_VGIC_V2);
+		return 0;
+	}
 
+	print_skip("No GICv2 nor GICv3 support");
+	exit(KSFT_SKIP);
 	return 0;
 }
-- 
2.33.0.685.g46640cef36-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 05/10] KVM: arm64: selftests: Make vgic_init gic version agnostic

[Eric Auger]

On 9/28/21 8:47 PM, Ricardo Koller wrote:
> As a preparation for the next commits which will add some tests for
> GICv2, make aarch64/vgic_init GIC version agnostic. Add a new generic
> run_tests function(gic_dev_type) that starts all applicable tests using
> GICv3 or GICv2. GICv2 tests are attempted if GICv3 is not available in
> the system. There are currently no GICv2 tests, but the test passes now
> in GICv2 systems.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  .../testing/selftests/kvm/aarch64/vgic_init.c | 156 +++++++++++-------
>  1 file changed, 95 insertions(+), 61 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> index 623f31a14326..896a29f2503d 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> @@ -22,6 +22,9 @@
>  
>  #define GICR_TYPER 0x8
>  
> +#define VGIC_DEV_IS_V2(_d) ((_d) == KVM_DEV_TYPE_ARM_VGIC_V2)
> +#define VGIC_DEV_IS_V3(_d) ((_d) == KVM_DEV_TYPE_ARM_VGIC_V3)
> +
>  struct vm_gic {
>  	struct kvm_vm *vm;
>  	int gic_fd;
> @@ -30,8 +33,8 @@ struct vm_gic {
>  static int max_ipa_bits;
>  
>  /* helper to access a redistributor register */
> -static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
> -			     uint32_t *val, bool write)
> +static int access_v3_redist_reg(int gicv3_fd, int vcpu, int offset,
> +				uint32_t *val, bool write)
>  {
>  	uint64_t attr = REG_OFFSET(vcpu, offset);
>  
> @@ -58,7 +61,7 @@ static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
>  	return 0;
>  }
>  
> -static struct vm_gic vm_gic_create(void)
> +static struct vm_gic vm_gic_v3_create(void)
>  {
>  	struct vm_gic v;
>  
> @@ -80,7 +83,7 @@ static void vm_gic_destroy(struct vm_gic *v)
>   * device gets created, a legacy RDIST region is set at @0x0
>   * and a DIST region is set @0x60000
>   */
> -static void subtest_dist_rdist(struct vm_gic *v)
> +static void subtest_v3_dist_rdist(struct vm_gic *v)
>  {
>  	int ret;
>  	uint64_t addr;
> @@ -145,7 +148,7 @@ static void subtest_dist_rdist(struct vm_gic *v)
>  }
>  
>  /* Test the new REDIST region API */
> -static void subtest_redist_regions(struct vm_gic *v)
> +static void subtest_v3_redist_regions(struct vm_gic *v)
>  {
>  	uint64_t addr, expected_addr;
>  	int ret;
> @@ -249,7 +252,7 @@ static void subtest_redist_regions(struct vm_gic *v)
>   * VGIC KVM device is created and initialized before the secondary CPUs
>   * get created
>   */
> -static void test_vgic_then_vcpus(void)
> +static void test_v3_vgic_then_vcpus(uint32_t gic_dev_type)
>  {
>  	struct vm_gic v;
>  	int ret, i;
> @@ -257,7 +260,7 @@ static void test_vgic_then_vcpus(void)
>  	v.vm = vm_create_default(0, 0, guest_code);
>  	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>  
> -	subtest_dist_rdist(&v);
> +	subtest_v3_dist_rdist(&v);
>  
>  	/* Add the rest of the VCPUs */
>  	for (i = 1; i < NR_VCPUS; ++i)
> @@ -270,14 +273,14 @@ static void test_vgic_then_vcpus(void)
>  }
>  
>  /* All the VCPUs are created before the VGIC KVM device gets initialized */
> -static void test_vcpus_then_vgic(void)
> +static void test_v3_vcpus_then_vgic(uint32_t gic_dev_type)
>  {
>  	struct vm_gic v;
>  	int ret;
>  
> -	v = vm_gic_create();
> +	v = vm_gic_v3_create();
>  
> -	subtest_dist_rdist(&v);
> +	subtest_v3_dist_rdist(&v);
>  
>  	ret = run_vcpu(v.vm, 3);
>  	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
> @@ -285,15 +288,15 @@ static void test_vcpus_then_vgic(void)
>  	vm_gic_destroy(&v);
>  }
>  
> -static void test_new_redist_regions(void)
> +static void test_v3_new_redist_regions(void)
>  {
>  	void *dummy = NULL;
>  	struct vm_gic v;
>  	uint64_t addr;
>  	int ret;
>  
> -	v = vm_gic_create();
> -	subtest_redist_regions(&v);
> +	v = vm_gic_v3_create();
> +	subtest_v3_redist_regions(&v);
>  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>  			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
>  
> @@ -303,8 +306,8 @@ static void test_new_redist_regions(void)
>  
>  	/* step2 */
>  
> -	v = vm_gic_create();
> -	subtest_redist_regions(&v);
> +	v = vm_gic_v3_create();
> +	subtest_v3_redist_regions(&v);
>  
>  	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 0, 2);
>  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> @@ -317,8 +320,8 @@ static void test_new_redist_regions(void)
>  
>  	/* step 3 */
>  
> -	v = vm_gic_create();
> -	subtest_redist_regions(&v);
> +	v = vm_gic_v3_create();
> +	subtest_v3_redist_regions(&v);
>  
>  	_kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, dummy, true);
> @@ -338,7 +341,7 @@ static void test_new_redist_regions(void)
>  	vm_gic_destroy(&v);
>  }
>  
> -static void test_typer_accesses(void)
> +static void test_v3_typer_accesses(void)
>  {
>  	struct vm_gic v;
>  	uint64_t addr;
> @@ -351,12 +354,12 @@ static void test_typer_accesses(void)
>  
>  	vm_vcpu_add_default(v.vm, 3, guest_code);
>  
> -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
>  	TEST_ASSERT(ret && errno == EINVAL, "attempting to read GICR_TYPER of non created vcpu");
>  
>  	vm_vcpu_add_default(v.vm, 1, guest_code);
>  
> -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
>  	TEST_ASSERT(ret && errno == EBUSY, "read GICR_TYPER before GIC initialized");
>  
>  	vm_vcpu_add_default(v.vm, 2, guest_code);
> @@ -365,7 +368,7 @@ static void test_typer_accesses(void)
>  			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
>  
>  	for (i = 0; i < NR_VCPUS ; i++) {
> -		ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> +		ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
>  		TEST_ASSERT(!ret && !val, "read GICR_TYPER before rdist region setting");
>  	}
>  
> @@ -374,10 +377,10 @@ static void test_typer_accesses(void)
>  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>  
>  	/* The 2 first rdists should be put there (vcpu 0 and 3) */
> -	ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && !val, "read typer of rdist #0");
>  
> -	ret = access_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #1");
>  
>  	addr = REDIST_REGION_ATTR_ADDR(10, 0x100000, 0, 1);
> @@ -385,11 +388,11 @@ static void test_typer_accesses(void)
>  				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>  	TEST_ASSERT(ret && errno == EINVAL, "collision with previous rdist region");
>  
> -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x100,
>  		    "no redist region attached to vcpu #1 yet, last cannot be returned");
>  
> -	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x200,
>  		    "no redist region attached to vcpu #2, last cannot be returned");
>  
> @@ -397,10 +400,10 @@ static void test_typer_accesses(void)
>  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>  
> -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
>  
> -	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x210,
>  		    "read typer of rdist #1, last properly returned");
>  
> @@ -417,7 +420,7 @@ static void test_typer_accesses(void)
>   * rdist region #2 @0x200000 2 rdist capacity
>   *     rdists: 1, 2
>   */
> -static void test_last_bit_redist_regions(void)
> +static void test_v3_last_bit_redist_regions(void)
>  {
>  	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
>  	struct vm_gic v;
> @@ -444,29 +447,29 @@ static void test_last_bit_redist_regions(void)
>  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>  
> -	ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
>  
> -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
>  
> -	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x200, "read typer of rdist #2");
>  
> -	ret = access_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #3");
>  
> -	ret = access_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #5");
>  
> -	ret = access_redist_reg(v.gic_fd, 4, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 4, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x410, "read typer of rdist #4");
>  
>  	vm_gic_destroy(&v);
>  }
>  
>  /* Test last bit with legacy region */
> -static void test_last_bit_single_rdist(void)
> +static void test_v3_last_bit_single_rdist(void)
>  {
>  	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
>  	struct vm_gic v;
> @@ -485,28 +488,32 @@ static void test_last_bit_single_rdist(void)
>  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>  			  KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
>  
> -	ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
>  
> -	ret = access_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x300, "read typer of rdist #1");
>  
> -	ret = access_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #2");
>  
> -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #3");
>  
> -	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> +	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
>  	TEST_ASSERT(!ret && val == 0x210, "read typer of rdist #3");
>  
>  	vm_gic_destroy(&v);
>  }
>  
> -void test_kvm_device(void)
> +/*
> + * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
> + */
> +int test_kvm_device(uint32_t gic_dev_type)
>  {
>  	struct vm_gic v;
>  	int ret, fd;
> +	uint32_t other;
>  
>  	v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
>  
> @@ -514,38 +521,65 @@ void test_kvm_device(void)
>  	ret = _kvm_create_device(v.vm, 0, true, &fd);
>  	TEST_ASSERT(ret && errno == ENODEV, "unsupported device");
>  
> -	/* trial mode with VGIC_V3 device */
> -	ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true, &fd);
> -	if (ret) {
> -		print_skip("GICv3 not supported");
> -		exit(KSFT_SKIP);
> -	}
> -	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +	/* trial mode */
> +	ret = _kvm_create_device(v.vm, gic_dev_type, true, &fd);
> +	if (ret)
> +		return ret;
> +	v.gic_fd = kvm_create_device(v.vm, gic_dev_type, false);
> +
> +	ret = _kvm_create_device(v.vm, gic_dev_type, false, &fd);
> +	TEST_ASSERT(ret && errno == EEXIST, "create GIC device twice");
>  
> -	ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false, &fd);
> -	TEST_ASSERT(ret && errno == EEXIST, "create GICv3 device twice");
> +	kvm_create_device(v.vm, gic_dev_type, true);
>  
> -	kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
> +	/* try to create the other gic_dev_type */
> +	other = VGIC_DEV_IS_V2(gic_dev_type) ? KVM_DEV_TYPE_ARM_VGIC_V3
> +					     : KVM_DEV_TYPE_ARM_VGIC_V2;
>  
> -	if (!_kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, true, &fd)) {
> -		ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, false, &fd);
> -		TEST_ASSERT(ret && errno == EINVAL, "create GICv2 while v3 exists");
> +	if (!_kvm_create_device(v.vm, other, true, &fd)) {
> +		ret = _kvm_create_device(v.vm, other, false, &fd);
> +		TEST_ASSERT(ret && errno == EINVAL,
> +				"create GIC device while other version exists");
>  	}
>  
>  	vm_gic_destroy(&v);
> +
> +	return 0;
> +}
> +
> +void run_tests(uint32_t gic_dev_type)
> +{
> +	if (VGIC_DEV_IS_V3(gic_dev_type)) {
> +		test_v3_vcpus_then_vgic(gic_dev_type);
> +		test_v3_vgic_then_vcpus(gic_dev_type);
> +		test_v3_new_redist_regions();
> +		test_v3_typer_accesses();
> +		test_v3_last_bit_redist_regions();
> +		test_v3_last_bit_single_rdist();
> +	}
>  }
>  
>  int main(int ac, char **av)
>  {
> +	int ret;
> +
>  	max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
>  
> -	test_kvm_device();
> -	test_vcpus_then_vgic();
> -	test_vgic_then_vcpus();
> -	test_new_redist_regions();
> -	test_typer_accesses();
> -	test_last_bit_redist_regions();
> -	test_last_bit_single_rdist();
> +	ret = test_kvm_device(KVM_DEV_TYPE_ARM_VGIC_V3);
> +	if (!ret) {
> +		pr_info("Running GIC_v3 tests.\n");
> +		run_tests(KVM_DEV_TYPE_ARM_VGIC_V3);
> +		return 0;
If the GICv3 supports compat with GICv2, I think you could be able to
run both tests consecutively. So maybe don't return?

Besides looks good
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

> +	}
> +
> +	ret = test_kvm_device(KVM_DEV_TYPE_ARM_VGIC_V2);
> +	if (!ret) {
> +		pr_info("Running GIC_v2 tests.\n");
> +		run_tests(KVM_DEV_TYPE_ARM_VGIC_V2);
> +		return 0;
> +	}

>  
> +	print_skip("No GICv2 nor GICv3 support");
> +	exit(KSFT_SKIP);
>  	return 0;
>  }

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 05/10] KVM: arm64: selftests: Make vgic_init gic version agnostic

[Ricardo Koller]

On Wed, Sep 29, 2021 at 07:12:59PM +0200, Eric Auger wrote:
> 
> 
> On 9/28/21 8:47 PM, Ricardo Koller wrote:
> > As a preparation for the next commits which will add some tests for
> > GICv2, make aarch64/vgic_init GIC version agnostic. Add a new generic
> > run_tests function(gic_dev_type) that starts all applicable tests using
> > GICv3 or GICv2. GICv2 tests are attempted if GICv3 is not available in
> > the system. There are currently no GICv2 tests, but the test passes now
> > in GICv2 systems.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  .../testing/selftests/kvm/aarch64/vgic_init.c | 156 +++++++++++-------
> >  1 file changed, 95 insertions(+), 61 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > index 623f31a14326..896a29f2503d 100644
> > --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > @@ -22,6 +22,9 @@
> >  
> >  #define GICR_TYPER 0x8
> >  
> > +#define VGIC_DEV_IS_V2(_d) ((_d) == KVM_DEV_TYPE_ARM_VGIC_V2)
> > +#define VGIC_DEV_IS_V3(_d) ((_d) == KVM_DEV_TYPE_ARM_VGIC_V3)
> > +
> >  struct vm_gic {
> >  	struct kvm_vm *vm;
> >  	int gic_fd;
> > @@ -30,8 +33,8 @@ struct vm_gic {
> >  static int max_ipa_bits;
> >  
> >  /* helper to access a redistributor register */
> > -static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
> > -			     uint32_t *val, bool write)
> > +static int access_v3_redist_reg(int gicv3_fd, int vcpu, int offset,
> > +				uint32_t *val, bool write)
> >  {
> >  	uint64_t attr = REG_OFFSET(vcpu, offset);
> >  
> > @@ -58,7 +61,7 @@ static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
> >  	return 0;
> >  }
> >  
> > -static struct vm_gic vm_gic_create(void)
> > +static struct vm_gic vm_gic_v3_create(void)
> >  {
> >  	struct vm_gic v;
> >  
> > @@ -80,7 +83,7 @@ static void vm_gic_destroy(struct vm_gic *v)
> >   * device gets created, a legacy RDIST region is set at @0x0
> >   * and a DIST region is set @0x60000
> >   */
> > -static void subtest_dist_rdist(struct vm_gic *v)
> > +static void subtest_v3_dist_rdist(struct vm_gic *v)
> >  {
> >  	int ret;
> >  	uint64_t addr;
> > @@ -145,7 +148,7 @@ static void subtest_dist_rdist(struct vm_gic *v)
> >  }
> >  
> >  /* Test the new REDIST region API */
> > -static void subtest_redist_regions(struct vm_gic *v)
> > +static void subtest_v3_redist_regions(struct vm_gic *v)
> >  {
> >  	uint64_t addr, expected_addr;
> >  	int ret;
> > @@ -249,7 +252,7 @@ static void subtest_redist_regions(struct vm_gic *v)
> >   * VGIC KVM device is created and initialized before the secondary CPUs
> >   * get created
> >   */
> > -static void test_vgic_then_vcpus(void)
> > +static void test_v3_vgic_then_vcpus(uint32_t gic_dev_type)
> >  {
> >  	struct vm_gic v;
> >  	int ret, i;
> > @@ -257,7 +260,7 @@ static void test_vgic_then_vcpus(void)
> >  	v.vm = vm_create_default(0, 0, guest_code);
> >  	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> >  
> > -	subtest_dist_rdist(&v);
> > +	subtest_v3_dist_rdist(&v);
> >  
> >  	/* Add the rest of the VCPUs */
> >  	for (i = 1; i < NR_VCPUS; ++i)
> > @@ -270,14 +273,14 @@ static void test_vgic_then_vcpus(void)
> >  }
> >  
> >  /* All the VCPUs are created before the VGIC KVM device gets initialized */
> > -static void test_vcpus_then_vgic(void)
> > +static void test_v3_vcpus_then_vgic(uint32_t gic_dev_type)
> >  {
> >  	struct vm_gic v;
> >  	int ret;
> >  
> > -	v = vm_gic_create();
> > +	v = vm_gic_v3_create();
> >  
> > -	subtest_dist_rdist(&v);
> > +	subtest_v3_dist_rdist(&v);
> >  
> >  	ret = run_vcpu(v.vm, 3);
> >  	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
> > @@ -285,15 +288,15 @@ static void test_vcpus_then_vgic(void)
> >  	vm_gic_destroy(&v);
> >  }
> >  
> > -static void test_new_redist_regions(void)
> > +static void test_v3_new_redist_regions(void)
> >  {
> >  	void *dummy = NULL;
> >  	struct vm_gic v;
> >  	uint64_t addr;
> >  	int ret;
> >  
> > -	v = vm_gic_create();
> > -	subtest_redist_regions(&v);
> > +	v = vm_gic_v3_create();
> > +	subtest_v3_redist_regions(&v);
> >  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> >  			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> >  
> > @@ -303,8 +306,8 @@ static void test_new_redist_regions(void)
> >  
> >  	/* step2 */
> >  
> > -	v = vm_gic_create();
> > -	subtest_redist_regions(&v);
> > +	v = vm_gic_v3_create();
> > +	subtest_v3_redist_regions(&v);
> >  
> >  	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 0, 2);
> >  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > @@ -317,8 +320,8 @@ static void test_new_redist_regions(void)
> >  
> >  	/* step 3 */
> >  
> > -	v = vm_gic_create();
> > -	subtest_redist_regions(&v);
> > +	v = vm_gic_v3_create();
> > +	subtest_v3_redist_regions(&v);
> >  
> >  	_kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> >  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, dummy, true);
> > @@ -338,7 +341,7 @@ static void test_new_redist_regions(void)
> >  	vm_gic_destroy(&v);
> >  }
> >  
> > -static void test_typer_accesses(void)
> > +static void test_v3_typer_accesses(void)
> >  {
> >  	struct vm_gic v;
> >  	uint64_t addr;
> > @@ -351,12 +354,12 @@ static void test_typer_accesses(void)
> >  
> >  	vm_vcpu_add_default(v.vm, 3, guest_code);
> >  
> > -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(ret && errno == EINVAL, "attempting to read GICR_TYPER of non created vcpu");
> >  
> >  	vm_vcpu_add_default(v.vm, 1, guest_code);
> >  
> > -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(ret && errno == EBUSY, "read GICR_TYPER before GIC initialized");
> >  
> >  	vm_vcpu_add_default(v.vm, 2, guest_code);
> > @@ -365,7 +368,7 @@ static void test_typer_accesses(void)
> >  			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> >  
> >  	for (i = 0; i < NR_VCPUS ; i++) {
> > -		ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> > +		ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> >  		TEST_ASSERT(!ret && !val, "read GICR_TYPER before rdist region setting");
> >  	}
> >  
> > @@ -374,10 +377,10 @@ static void test_typer_accesses(void)
> >  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> >  
> >  	/* The 2 first rdists should be put there (vcpu 0 and 3) */
> > -	ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && !val, "read typer of rdist #0");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #1");
> >  
> >  	addr = REDIST_REGION_ATTR_ADDR(10, 0x100000, 0, 1);
> > @@ -385,11 +388,11 @@ static void test_typer_accesses(void)
> >  				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> >  	TEST_ASSERT(ret && errno == EINVAL, "collision with previous rdist region");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x100,
> >  		    "no redist region attached to vcpu #1 yet, last cannot be returned");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x200,
> >  		    "no redist region attached to vcpu #2, last cannot be returned");
> >  
> > @@ -397,10 +400,10 @@ static void test_typer_accesses(void)
> >  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> >  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> >  
> > -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x210,
> >  		    "read typer of rdist #1, last properly returned");
> >  
> > @@ -417,7 +420,7 @@ static void test_typer_accesses(void)
> >   * rdist region #2 @0x200000 2 rdist capacity
> >   *     rdists: 1, 2
> >   */
> > -static void test_last_bit_redist_regions(void)
> > +static void test_v3_last_bit_redist_regions(void)
> >  {
> >  	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
> >  	struct vm_gic v;
> > @@ -444,29 +447,29 @@ static void test_last_bit_redist_regions(void)
> >  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> >  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> >  
> > -	ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x200, "read typer of rdist #2");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #3");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #5");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 4, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 4, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x410, "read typer of rdist #4");
> >  
> >  	vm_gic_destroy(&v);
> >  }
> >  
> >  /* Test last bit with legacy region */
> > -static void test_last_bit_single_rdist(void)
> > +static void test_v3_last_bit_single_rdist(void)
> >  {
> >  	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
> >  	struct vm_gic v;
> > @@ -485,28 +488,32 @@ static void test_last_bit_single_rdist(void)
> >  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> >  			  KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> >  
> > -	ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x300, "read typer of rdist #1");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #2");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #3");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x210, "read typer of rdist #3");
> >  
> >  	vm_gic_destroy(&v);
> >  }
> >  
> > -void test_kvm_device(void)
> > +/*
> > + * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
> > + */
> > +int test_kvm_device(uint32_t gic_dev_type)
> >  {
> >  	struct vm_gic v;
> >  	int ret, fd;
> > +	uint32_t other;
> >  
> >  	v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
> >  
> > @@ -514,38 +521,65 @@ void test_kvm_device(void)
> >  	ret = _kvm_create_device(v.vm, 0, true, &fd);
> >  	TEST_ASSERT(ret && errno == ENODEV, "unsupported device");
> >  
> > -	/* trial mode with VGIC_V3 device */
> > -	ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true, &fd);
> > -	if (ret) {
> > -		print_skip("GICv3 not supported");
> > -		exit(KSFT_SKIP);
> > -	}
> > -	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> > +	/* trial mode */
> > +	ret = _kvm_create_device(v.vm, gic_dev_type, true, &fd);
> > +	if (ret)
> > +		return ret;
> > +	v.gic_fd = kvm_create_device(v.vm, gic_dev_type, false);
> > +
> > +	ret = _kvm_create_device(v.vm, gic_dev_type, false, &fd);
> > +	TEST_ASSERT(ret && errno == EEXIST, "create GIC device twice");
> >  
> > -	ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false, &fd);
> > -	TEST_ASSERT(ret && errno == EEXIST, "create GICv3 device twice");
> > +	kvm_create_device(v.vm, gic_dev_type, true);
> >  
> > -	kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
> > +	/* try to create the other gic_dev_type */
> > +	other = VGIC_DEV_IS_V2(gic_dev_type) ? KVM_DEV_TYPE_ARM_VGIC_V3
> > +					     : KVM_DEV_TYPE_ARM_VGIC_V2;
> >  
> > -	if (!_kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, true, &fd)) {
> > -		ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, false, &fd);
> > -		TEST_ASSERT(ret && errno == EINVAL, "create GICv2 while v3 exists");
> > +	if (!_kvm_create_device(v.vm, other, true, &fd)) {
> > +		ret = _kvm_create_device(v.vm, other, false, &fd);
> > +		TEST_ASSERT(ret && errno == EINVAL,
> > +				"create GIC device while other version exists");
> >  	}
> >  
> >  	vm_gic_destroy(&v);
> > +
> > +	return 0;
> > +}
> > +
> > +void run_tests(uint32_t gic_dev_type)
> > +{
> > +	if (VGIC_DEV_IS_V3(gic_dev_type)) {
> > +		test_v3_vcpus_then_vgic(gic_dev_type);
> > +		test_v3_vgic_then_vcpus(gic_dev_type);
> > +		test_v3_new_redist_regions();
> > +		test_v3_typer_accesses();
> > +		test_v3_last_bit_redist_regions();
> > +		test_v3_last_bit_single_rdist();
> > +	}
> >  }
> >  
> >  int main(int ac, char **av)
> >  {
> > +	int ret;
> > +
> >  	max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
> >  
> > -	test_kvm_device();
> > -	test_vcpus_then_vgic();
> > -	test_vgic_then_vcpus();
> > -	test_new_redist_regions();
> > -	test_typer_accesses();
> > -	test_last_bit_redist_regions();
> > -	test_last_bit_single_rdist();
> > +	ret = test_kvm_device(KVM_DEV_TYPE_ARM_VGIC_V3);
> > +	if (!ret) {
> > +		pr_info("Running GIC_v3 tests.\n");
> > +		run_tests(KVM_DEV_TYPE_ARM_VGIC_V3);
> > +		return 0;
> If the GICv3 supports compat with GICv2, I think you could be able to
> run both tests consecutively. So maybe don't return?
> 

Good point, will try that (not returning).

Thanks,
Ricardo

> Besides looks good
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> Eric
> 
> > +	}
> > +
> > +	ret = test_kvm_device(KVM_DEV_TYPE_ARM_VGIC_V2);
> > +	if (!ret) {
> > +		pr_info("Running GIC_v2 tests.\n");
> > +		run_tests(KVM_DEV_TYPE_ARM_VGIC_V2);
> > +		return 0;
> > +	}
> 
> >  
> > +	print_skip("No GICv2 nor GICv3 support");
> > +	exit(KSFT_SKIP);
> >  	return 0;
> >  }
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 05/10] KVM: arm64: selftests: Make vgic_init gic version agnostic

[Marc Zyngier]

On Wed, 29 Sep 2021 18:12:59 +0100,
Eric Auger <eric.auger@redhat.com> wrote:
> 
> If the GICv3 supports compat with GICv2, I think you could be able to
> run both tests consecutively. So maybe don't return?

You'll need to recreate a full VM though. Even if the HW supports the
compat mode, our GICv3 emulation doesn't.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 05/10] KVM: arm64: selftests: Make vgic_init gic version agnostic

[Ricardo Koller]

On Thu, Sep 30, 2021 at 09:05:18AM +0100, Marc Zyngier wrote:
> On Wed, 29 Sep 2021 18:12:59 +0100,
> Eric Auger <eric.auger@redhat.com> wrote:
> > 
> > If the GICv3 supports compat with GICv2, I think you could be able to
> > run both tests consecutively. So maybe don't return?
> 
> You'll need to recreate a full VM though. Even if the HW supports the
> compat mode, our GICv3 emulation doesn't.

I'm not sure how much work would that entail, but sounds like it might
be too much for a selftest.

Thanks,
Ricardo

> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 06/10] KVM: arm64: selftests: Make vgic_init/vm_gic_create version agnostic

[Ricardo Koller]

Make vm_gic_create GIC version agnostic in the vgic_init test. Also
add a nr_vcpus arg into it instead of defaulting to NR_VCPUS.

No functional change.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../testing/selftests/kvm/aarch64/vgic_init.c | 21 +++++++++++--------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
index 896a29f2503d..b24067dbdac0 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -28,6 +28,7 @@
 struct vm_gic {
 	struct kvm_vm *vm;
 	int gic_fd;
+	uint32_t gic_dev_type;
 };
 
 static int max_ipa_bits;
@@ -61,12 +62,13 @@ static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
 	return 0;
 }
 
-static struct vm_gic vm_gic_v3_create(void)
+static struct vm_gic vm_gic_create_with_vcpus(uint32_t gic_dev_type, uint32_t nr_vcpus)
 {
 	struct vm_gic v;
 
-	v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
-	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+	v.gic_dev_type = gic_dev_type;
+	v.vm = vm_create_default_with_vcpus(nr_vcpus, 0, 0, guest_code, NULL);
+	v.gic_fd = kvm_create_device(v.vm, gic_dev_type, false);
 
 	return v;
 }
@@ -153,6 +155,8 @@ static void subtest_v3_redist_regions(struct vm_gic *v)
 	uint64_t addr, expected_addr;
 	int ret;
 
+	TEST_ASSERT(VGIC_DEV_IS_V3(v->gic_dev_type), "Only applies to v3");
+
 	ret = kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
 				     KVM_VGIC_V3_ADDR_TYPE_REDIST);
 	TEST_ASSERT(!ret, "Multiple redist regions advertised");
@@ -257,8 +261,7 @@ static void test_v3_vgic_then_vcpus(uint32_t gic_dev_type)
 	struct vm_gic v;
 	int ret, i;
 
-	v.vm = vm_create_default(0, 0, guest_code);
-	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+	v = vm_gic_create_with_vcpus(gic_dev_type, 1);
 
 	subtest_v3_dist_rdist(&v);
 
@@ -278,7 +281,7 @@ static void test_v3_vcpus_then_vgic(uint32_t gic_dev_type)
 	struct vm_gic v;
 	int ret;
 
-	v = vm_gic_v3_create();
+	v = vm_gic_create_with_vcpus(gic_dev_type, NR_VCPUS);
 
 	subtest_v3_dist_rdist(&v);
 
@@ -295,7 +298,7 @@ static void test_v3_new_redist_regions(void)
 	uint64_t addr;
 	int ret;
 
-	v = vm_gic_v3_create();
+	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
 	subtest_v3_redist_regions(&v);
 	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
 			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
@@ -306,7 +309,7 @@ static void test_v3_new_redist_regions(void)
 
 	/* step2 */
 
-	v = vm_gic_v3_create();
+	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
 	subtest_v3_redist_regions(&v);
 
 	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 0, 2);
@@ -320,7 +323,7 @@ static void test_v3_new_redist_regions(void)
 
 	/* step 3 */
 
-	v = vm_gic_v3_create();
+	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
 	subtest_v3_redist_regions(&v);
 
 	_kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-- 
2.33.0.685.g46640cef36-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 06/10] KVM: arm64: selftests: Make vgic_init/vm_gic_create version agnostic

[Eric Auger]

On 9/28/21 8:48 PM, Ricardo Koller wrote:
> Make vm_gic_create GIC version agnostic in the vgic_init test. Also
> add a nr_vcpus arg into it instead of defaulting to NR_VCPUS.
>
> No functional change.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  .../testing/selftests/kvm/aarch64/vgic_init.c | 21 +++++++++++--------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> index 896a29f2503d..b24067dbdac0 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> @@ -28,6 +28,7 @@
>  struct vm_gic {
>  	struct kvm_vm *vm;
>  	int gic_fd;
> +	uint32_t gic_dev_type;
>  };
>  
>  static int max_ipa_bits;
> @@ -61,12 +62,13 @@ static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
>  	return 0;
>  }
>  
> -static struct vm_gic vm_gic_v3_create(void)
> +static struct vm_gic vm_gic_create_with_vcpus(uint32_t gic_dev_type, uint32_t nr_vcpus)
>  {
>  	struct vm_gic v;
>  
> -	v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
> -	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +	v.gic_dev_type = gic_dev_type;
> +	v.vm = vm_create_default_with_vcpus(nr_vcpus, 0, 0, guest_code, NULL);
> +	v.gic_fd = kvm_create_device(v.vm, gic_dev_type, false);
>  
>  	return v;
>  }
> @@ -153,6 +155,8 @@ static void subtest_v3_redist_regions(struct vm_gic *v)
>  	uint64_t addr, expected_addr;
>  	int ret;
>  
> +	TEST_ASSERT(VGIC_DEV_IS_V3(v->gic_dev_type), "Only applies to v3");
Is that really needed? why here and not in other locations. I would remove.
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
> +
>  	ret = kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>  				     KVM_VGIC_V3_ADDR_TYPE_REDIST);
>  	TEST_ASSERT(!ret, "Multiple redist regions advertised");
> @@ -257,8 +261,7 @@ static void test_v3_vgic_then_vcpus(uint32_t gic_dev_type)
>  	struct vm_gic v;
>  	int ret, i;
>  
> -	v.vm = vm_create_default(0, 0, guest_code);
> -	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +	v = vm_gic_create_with_vcpus(gic_dev_type, 1);
>  
>  	subtest_v3_dist_rdist(&v);
>  
> @@ -278,7 +281,7 @@ static void test_v3_vcpus_then_vgic(uint32_t gic_dev_type)
>  	struct vm_gic v;
>  	int ret;
>  
> -	v = vm_gic_v3_create();
> +	v = vm_gic_create_with_vcpus(gic_dev_type, NR_VCPUS);
>  
>  	subtest_v3_dist_rdist(&v);
>  
> @@ -295,7 +298,7 @@ static void test_v3_new_redist_regions(void)
>  	uint64_t addr;
>  	int ret;
>  
> -	v = vm_gic_v3_create();
> +	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
>  	subtest_v3_redist_regions(&v);
>  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>  			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> @@ -306,7 +309,7 @@ static void test_v3_new_redist_regions(void)
>  
>  	/* step2 */
>  
> -	v = vm_gic_v3_create();
> +	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
>  	subtest_v3_redist_regions(&v);
>  
>  	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 0, 2);
> @@ -320,7 +323,7 @@ static void test_v3_new_redist_regions(void)
>  
>  	/* step 3 */
>  
> -	v = vm_gic_v3_create();
> +	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
>  	subtest_v3_redist_regions(&v);
>  
>  	_kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 07/10] KVM: arm64: selftests: Add some tests for GICv2 in vgic_init

[Ricardo Koller]

Add some GICv2 tests: general KVM device tests and DIST/REDIST overlap
tests.  Do this by making test_vcpus_then_vgic and test_vgic_then_vcpus
in vgic_init GIC version agnostic.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../testing/selftests/kvm/aarch64/vgic_init.c | 107 ++++++++++++------
 1 file changed, 75 insertions(+), 32 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
index b24067dbdac0..92f5c6ca6b8b 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -79,74 +79,116 @@ static void vm_gic_destroy(struct vm_gic *v)
 	kvm_vm_free(v->vm);
 }
 
+struct vgic_region_attr {
+	uint64_t attr;
+	uint64_t size;
+	uint64_t alignment;
+};
+
+struct vgic_region_attr gic_v3_dist_region = {
+	.attr = KVM_VGIC_V3_ADDR_TYPE_DIST,
+	.size = 0x10000,
+	.alignment = 0x10000,
+};
+
+struct vgic_region_attr gic_v3_redist_region = {
+	.attr = KVM_VGIC_V3_ADDR_TYPE_REDIST,
+	.size = NR_VCPUS * 0x20000,
+	.alignment = 0x10000,
+};
+
+struct vgic_region_attr gic_v2_dist_region = {
+	.attr = KVM_VGIC_V2_ADDR_TYPE_DIST,
+	.size = 0x1000,
+	.alignment = 0x1000,
+};
+
+struct vgic_region_attr gic_v2_cpu_region = {
+	.attr = KVM_VGIC_V2_ADDR_TYPE_CPU,
+	.size = 0x2000,
+	.alignment = 0x1000,
+};
+
 /**
- * Helper routine that performs KVM device tests in general and
- * especially ARM_VGIC_V3 ones. Eventually the ARM_VGIC_V3
- * device gets created, a legacy RDIST region is set at @0x0
- * and a DIST region is set @0x60000
+ * Helper routine that performs KVM device tests in general. Eventually the
+ * ARM_VGIC (GICv2 or GICv3) device gets created with an overlapping
+ * DIST/REDIST. A RDIST region (legacy in the case of GICv3) is set at @0x0 and
+ * a DIST region is set @0x70000 for GICv3 and @0x1000 for GICv2.
  */
-static void subtest_v3_dist_rdist(struct vm_gic *v)
+static void subtest_dist_rdist(struct vm_gic *v)
 {
 	int ret;
 	uint64_t addr;
+	struct vgic_region_attr rdist; /* CPU interface in GICv2*/
+	struct vgic_region_attr dist;
+
+	rdist = VGIC_DEV_IS_V3(v->gic_dev_type) ? gic_v3_redist_region
+						: gic_v2_cpu_region;
+	dist = VGIC_DEV_IS_V3(v->gic_dev_type) ? gic_v3_dist_region
+						: gic_v2_dist_region;
 
 	/* Check existing group/attributes */
 	kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-			      KVM_VGIC_V3_ADDR_TYPE_DIST);
+			      dist.attr);
 
 	kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-			      KVM_VGIC_V3_ADDR_TYPE_REDIST);
+			      rdist.attr);
 
 	/* check non existing attribute */
-	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, 0);
+	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, -1);
 	TEST_ASSERT(ret && errno == ENXIO, "attribute not supported");
 
 	/* misaligned DIST and REDIST address settings */
-	addr = 0x1000;
+	addr = dist.alignment / 0x10;
 	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-				 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
-	TEST_ASSERT(ret && errno == EINVAL, "GICv3 dist base not 64kB aligned");
+				 dist.attr, &addr, true);
+	TEST_ASSERT(ret && errno == EINVAL, "GIC dist base not aligned");
 
+	addr = rdist.alignment / 0x10;
 	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
-	TEST_ASSERT(ret && errno == EINVAL, "GICv3 redist base not 64kB aligned");
+				 rdist.attr, &addr, true);
+	TEST_ASSERT(ret && errno == EINVAL, "GIC redist/cpu base not aligned");
 
 	/* out of range address */
 	if (max_ipa_bits) {
 		addr = 1ULL << max_ipa_bits;
 		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-					 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
+					 dist.attr, &addr, true);
 		TEST_ASSERT(ret && errno == E2BIG, "dist address beyond IPA limit");
 
 		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-					 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+					 rdist.attr, &addr, true);
 		TEST_ASSERT(ret && errno == E2BIG, "redist address beyond IPA limit");
 	}
 
 	/* set REDIST base address @0x0*/
 	addr = 0x00000;
 	kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-			  KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+			  rdist.attr, &addr, true);
 
 	/* Attempt to create a second legacy redistributor region */
 	addr = 0xE0000;
 	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
-	TEST_ASSERT(ret && errno == EEXIST, "GICv3 redist base set again");
+				 rdist.attr, &addr, true);
+	TEST_ASSERT(ret && errno == EEXIST, "GIC redist base set again");
 
-	/* Attempt to mix legacy and new redistributor regions */
-	addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 0x100000, 0, 0);
-	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
-	TEST_ASSERT(ret && errno == EINVAL, "attempt to mix GICv3 REDIST and REDIST_REGION");
+	if (VGIC_DEV_IS_V3(v->gic_dev_type)) {
+		/* Attempt to mix legacy and new redistributor regions */
+		addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 0x100000, 0, 0);
+		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+					 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION,
+					 &addr, true);
+		TEST_ASSERT(ret && errno == EINVAL,
+			    "attempt to mix GICv3 REDIST and REDIST_REGION");
+	}
 
 	/*
 	 * Set overlapping DIST / REDIST, cannot be detected here. Will be detected
 	 * on first vcpu run instead.
 	 */
-	addr = 3 * 2 * 0x10000;
-	kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, KVM_VGIC_V3_ADDR_TYPE_DIST,
-			  &addr, true);
+	addr = rdist.size - rdist.alignment;
+	kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			  dist.attr, &addr, true);
 }
 
 /* Test the new REDIST region API */
@@ -256,14 +298,14 @@ static void subtest_v3_redist_regions(struct vm_gic *v)
  * VGIC KVM device is created and initialized before the secondary CPUs
  * get created
  */
-static void test_v3_vgic_then_vcpus(uint32_t gic_dev_type)
+static void test_vgic_then_vcpus(uint32_t gic_dev_type)
 {
 	struct vm_gic v;
 	int ret, i;
 
 	v = vm_gic_create_with_vcpus(gic_dev_type, 1);
 
-	subtest_v3_dist_rdist(&v);
+	subtest_dist_rdist(&v);
 
 	/* Add the rest of the VCPUs */
 	for (i = 1; i < NR_VCPUS; ++i)
@@ -276,14 +318,14 @@ static void test_v3_vgic_then_vcpus(uint32_t gic_dev_type)
 }
 
 /* All the VCPUs are created before the VGIC KVM device gets initialized */
-static void test_v3_vcpus_then_vgic(uint32_t gic_dev_type)
+static void test_vcpus_then_vgic(uint32_t gic_dev_type)
 {
 	struct vm_gic v;
 	int ret;
 
 	v = vm_gic_create_with_vcpus(gic_dev_type, NR_VCPUS);
 
-	subtest_v3_dist_rdist(&v);
+	subtest_dist_rdist(&v);
 
 	ret = run_vcpu(v.vm, 3);
 	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
@@ -552,9 +594,10 @@ int test_kvm_device(uint32_t gic_dev_type)
 
 void run_tests(uint32_t gic_dev_type)
 {
+	test_vcpus_then_vgic(gic_dev_type);
+	test_vgic_then_vcpus(gic_dev_type);
+
 	if (VGIC_DEV_IS_V3(gic_dev_type)) {
-		test_v3_vcpus_then_vgic(gic_dev_type);
-		test_v3_vgic_then_vcpus(gic_dev_type);
 		test_v3_new_redist_regions();
 		test_v3_typer_accesses();
 		test_v3_last_bit_redist_regions();
-- 
2.33.0.685.g46640cef36-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 07/10] KVM: arm64: selftests: Add some tests for GICv2 in vgic_init

[Eric Auger]

Hi Ricardo,

On 9/28/21 8:48 PM, Ricardo Koller wrote:
> Add some GICv2 tests: general KVM device tests and DIST/REDIST overlap
> tests.  Do this by making test_vcpus_then_vgic and test_vgic_then_vcpus
> in vgic_init GIC version agnostic.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  .../testing/selftests/kvm/aarch64/vgic_init.c | 107 ++++++++++++------
>  1 file changed, 75 insertions(+), 32 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> index b24067dbdac0..92f5c6ca6b8b 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> @@ -79,74 +79,116 @@ static void vm_gic_destroy(struct vm_gic *v)
>  	kvm_vm_free(v->vm);
>  }
>  
> +struct vgic_region_attr {
> +	uint64_t attr;
> +	uint64_t size;
> +	uint64_t alignment;
> +};
> +
> +struct vgic_region_attr gic_v3_dist_region = {
> +	.attr = KVM_VGIC_V3_ADDR_TYPE_DIST,
> +	.size = 0x10000,
> +	.alignment = 0x10000,
> +};
> +
> +struct vgic_region_attr gic_v3_redist_region = {
> +	.attr = KVM_VGIC_V3_ADDR_TYPE_REDIST,
> +	.size = NR_VCPUS * 0x20000,
> +	.alignment = 0x10000,
> +};
> +
> +struct vgic_region_attr gic_v2_dist_region = {
> +	.attr = KVM_VGIC_V2_ADDR_TYPE_DIST,
> +	.size = 0x1000,
> +	.alignment = 0x1000,
> +};
> +
> +struct vgic_region_attr gic_v2_cpu_region = {
> +	.attr = KVM_VGIC_V2_ADDR_TYPE_CPU,
> +	.size = 0x2000,
> +	.alignment = 0x1000,
> +};
> +
>  /**
> - * Helper routine that performs KVM device tests in general and
> - * especially ARM_VGIC_V3 ones. Eventually the ARM_VGIC_V3
> - * device gets created, a legacy RDIST region is set at @0x0
> - * and a DIST region is set @0x60000
> + * Helper routine that performs KVM device tests in general. Eventually the
> + * ARM_VGIC (GICv2 or GICv3) device gets created with an overlapping
> + * DIST/REDIST. A RDIST region (legacy in the case of GICv3) is set at @0x0 and
> + * a DIST region is set @0x70000 for GICv3 and @0x1000 for GICv2.
I would add "Assumption is 4 vcpus are going to be used hence the overlap".
Also the RDIST is GICv3 only. In the above comment also mention the CPU I/F.
>   */
> -static void subtest_v3_dist_rdist(struct vm_gic *v)
> +static void subtest_dist_rdist(struct vm_gic *v)
>  {
>  	int ret;
>  	uint64_t addr;
> +	struct vgic_region_attr rdist; /* CPU interface in GICv2*/
> +	struct vgic_region_attr dist;
> +
> +	rdist = VGIC_DEV_IS_V3(v->gic_dev_type) ? gic_v3_redist_region
> +						: gic_v2_cpu_region;
> +	dist = VGIC_DEV_IS_V3(v->gic_dev_type) ? gic_v3_dist_region
> +						: gic_v2_dist_region;
>  
>  	/* Check existing group/attributes */
>  	kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> -			      KVM_VGIC_V3_ADDR_TYPE_DIST);
> +			      dist.attr);
>  
>  	kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> -			      KVM_VGIC_V3_ADDR_TYPE_REDIST);
> +			      rdist.attr);
>  
>  	/* check non existing attribute */
> -	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, 0);
> +	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, -1);
was that necessary?
>  	TEST_ASSERT(ret && errno == ENXIO, "attribute not supported");
>  
>  	/* misaligned DIST and REDIST address settings */
> -	addr = 0x1000;
> +	addr = dist.alignment / 0x10;
>  	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> -				 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
> -	TEST_ASSERT(ret && errno == EINVAL, "GICv3 dist base not 64kB aligned");
> +				 dist.attr, &addr, true);
> +	TEST_ASSERT(ret && errno == EINVAL, "GIC dist base not aligned");
>  
> +	addr = rdist.alignment / 0x10;
>  	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> -				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> -	TEST_ASSERT(ret && errno == EINVAL, "GICv3 redist base not 64kB aligned");
> +				 rdist.attr, &addr, true);
> +	TEST_ASSERT(ret && errno == EINVAL, "GIC redist/cpu base not aligned");
>  
>  	/* out of range address */
>  	if (max_ipa_bits) {
>  		addr = 1ULL << max_ipa_bits;
>  		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> -					 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
> +					 dist.attr, &addr, true);
>  		TEST_ASSERT(ret && errno == E2BIG, "dist address beyond IPA limit");
>  
>  		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> -					 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> +					 rdist.attr, &addr, true);
>  		TEST_ASSERT(ret && errno == E2BIG, "redist address beyond IPA limit");
>  	}
>  
>  	/* set REDIST base address @0x0*/
>  	addr = 0x00000;
>  	kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> -			  KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> +			  rdist.attr, &addr, true);
>  
>  	/* Attempt to create a second legacy redistributor region */
>  	addr = 0xE0000;
>  	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> -				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> -	TEST_ASSERT(ret && errno == EEXIST, "GICv3 redist base set again");
> +				 rdist.attr, &addr, true);
> +	TEST_ASSERT(ret && errno == EEXIST, "GIC redist base set again");
>  
> -	/* Attempt to mix legacy and new redistributor regions */
> -	addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 0x100000, 0, 0);
> -	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> -				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> -	TEST_ASSERT(ret && errno == EINVAL, "attempt to mix GICv3 REDIST and REDIST_REGION");
> +	if (VGIC_DEV_IS_V3(v->gic_dev_type)) {
Instead you could check
    ret = kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
                     KVM_VGIC_V3_ADDR_TYPE_REDIST);
> +		/* Attempt to mix legacy and new redistributor regions */
> +		addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 0x100000, 0, 0);
> +		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +					 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION,
> +					 &addr, true);
> +		TEST_ASSERT(ret && errno == EINVAL,
> +			    "attempt to mix GICv3 REDIST and REDIST_REGION");
> +	}
>  
>  	/*
>  	 * Set overlapping DIST / REDIST, cannot be detected here. Will be detected
>  	 * on first vcpu run instead.
>  	 */
> -	addr = 3 * 2 * 0x10000;
> -	kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, KVM_VGIC_V3_ADDR_TYPE_DIST,
> -			  &addr, true);
> +	addr = rdist.size - rdist.alignment;
> +	kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			  dist.attr, &addr, true);
>  }
>  
>  /* Test the new REDIST region API */
> @@ -256,14 +298,14 @@ static void subtest_v3_redist_regions(struct vm_gic *v)
>   * VGIC KVM device is created and initialized before the secondary CPUs
>   * get created
>   */
> -static void test_v3_vgic_then_vcpus(uint32_t gic_dev_type)
> +static void test_vgic_then_vcpus(uint32_t gic_dev_type)
>  {
>  	struct vm_gic v;
>  	int ret, i;
>  
>  	v = vm_gic_create_with_vcpus(gic_dev_type, 1);
>  
> -	subtest_v3_dist_rdist(&v);
> +	subtest_dist_rdist(&v);
>  
>  	/* Add the rest of the VCPUs */
>  	for (i = 1; i < NR_VCPUS; ++i)
> @@ -276,14 +318,14 @@ static void test_v3_vgic_then_vcpus(uint32_t gic_dev_type)
>  }
>  
>  /* All the VCPUs are created before the VGIC KVM device gets initialized */
> -static void test_v3_vcpus_then_vgic(uint32_t gic_dev_type)
> +static void test_vcpus_then_vgic(uint32_t gic_dev_type)
>  {
>  	struct vm_gic v;
>  	int ret;
>  
>  	v = vm_gic_create_with_vcpus(gic_dev_type, NR_VCPUS);
>  
> -	subtest_v3_dist_rdist(&v);
> +	subtest_dist_rdist(&v);
>  
>  	ret = run_vcpu(v.vm, 3);
>  	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
> @@ -552,9 +594,10 @@ int test_kvm_device(uint32_t gic_dev_type)
>  
>  void run_tests(uint32_t gic_dev_type)
>  {
> +	test_vcpus_then_vgic(gic_dev_type);
> +	test_vgic_then_vcpus(gic_dev_type);
> +
>  	if (VGIC_DEV_IS_V3(gic_dev_type)) {
> -		test_v3_vcpus_then_vgic(gic_dev_type);
> -		test_v3_vgic_then_vcpus(gic_dev_type);
>  		test_v3_new_redist_regions();
>  		test_v3_typer_accesses();
>  		test_v3_last_bit_redist_regions();
Thanks

Eric

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 07/10] KVM: arm64: selftests: Add some tests for GICv2 in vgic_init

[Ricardo Koller]

On Thu, Sep 30, 2021 at 09:42:50AM +0200, Eric Auger wrote:
> Hi Ricardo,
> 
> On 9/28/21 8:48 PM, Ricardo Koller wrote:
> > Add some GICv2 tests: general KVM device tests and DIST/REDIST overlap
> > tests.  Do this by making test_vcpus_then_vgic and test_vgic_then_vcpus
> > in vgic_init GIC version agnostic.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  .../testing/selftests/kvm/aarch64/vgic_init.c | 107 ++++++++++++------
> >  1 file changed, 75 insertions(+), 32 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > index b24067dbdac0..92f5c6ca6b8b 100644
> > --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > @@ -79,74 +79,116 @@ static void vm_gic_destroy(struct vm_gic *v)
> >  	kvm_vm_free(v->vm);
> >  }
> >  
> > +struct vgic_region_attr {
> > +	uint64_t attr;
> > +	uint64_t size;
> > +	uint64_t alignment;
> > +};
> > +
> > +struct vgic_region_attr gic_v3_dist_region = {
> > +	.attr = KVM_VGIC_V3_ADDR_TYPE_DIST,
> > +	.size = 0x10000,
> > +	.alignment = 0x10000,
> > +};
> > +
> > +struct vgic_region_attr gic_v3_redist_region = {
> > +	.attr = KVM_VGIC_V3_ADDR_TYPE_REDIST,
> > +	.size = NR_VCPUS * 0x20000,
> > +	.alignment = 0x10000,
> > +};
> > +
> > +struct vgic_region_attr gic_v2_dist_region = {
> > +	.attr = KVM_VGIC_V2_ADDR_TYPE_DIST,
> > +	.size = 0x1000,
> > +	.alignment = 0x1000,
> > +};
> > +
> > +struct vgic_region_attr gic_v2_cpu_region = {
> > +	.attr = KVM_VGIC_V2_ADDR_TYPE_CPU,
> > +	.size = 0x2000,
> > +	.alignment = 0x1000,
> > +};
> > +
> >  /**
> > - * Helper routine that performs KVM device tests in general and
> > - * especially ARM_VGIC_V3 ones. Eventually the ARM_VGIC_V3
> > - * device gets created, a legacy RDIST region is set at @0x0
> > - * and a DIST region is set @0x60000
> > + * Helper routine that performs KVM device tests in general. Eventually the
> > + * ARM_VGIC (GICv2 or GICv3) device gets created with an overlapping
> > + * DIST/REDIST. A RDIST region (legacy in the case of GICv3) is set at @0x0 and
> > + * a DIST region is set @0x70000 for GICv3 and @0x1000 for GICv2.
> I would add "Assumption is 4 vcpus are going to be used hence the overlap".

ACK

> Also the RDIST is GICv3 only. In the above comment also mention the CPU I/F.

Will do. I wasn't sure if it was better to refer to both of them as CPU
interfaces, or redistributors (half correct in both cases).

> >   */
> > -static void subtest_v3_dist_rdist(struct vm_gic *v)
> > +static void subtest_dist_rdist(struct vm_gic *v)
> >  {
> >  	int ret;
> >  	uint64_t addr;
> > +	struct vgic_region_attr rdist; /* CPU interface in GICv2*/
> > +	struct vgic_region_attr dist;
> > +
> > +	rdist = VGIC_DEV_IS_V3(v->gic_dev_type) ? gic_v3_redist_region
> > +						: gic_v2_cpu_region;
> > +	dist = VGIC_DEV_IS_V3(v->gic_dev_type) ? gic_v3_dist_region
> > +						: gic_v2_dist_region;
> >  
> >  	/* Check existing group/attributes */
> >  	kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > -			      KVM_VGIC_V3_ADDR_TYPE_DIST);
> > +			      dist.attr);
> >  
> >  	kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > -			      KVM_VGIC_V3_ADDR_TYPE_REDIST);
> > +			      rdist.attr);
> >  
> >  	/* check non existing attribute */
> > -	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, 0);
> > +	ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, -1);
> was that necessary?

Yeah, turns out that 0 is used in v2:

#define KVM_VGIC_V2_ADDR_TYPE_DIST	0

> >  	TEST_ASSERT(ret && errno == ENXIO, "attribute not supported");
> >  
> >  	/* misaligned DIST and REDIST address settings */
> > -	addr = 0x1000;
> > +	addr = dist.alignment / 0x10;
> >  	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > -				 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
> > -	TEST_ASSERT(ret && errno == EINVAL, "GICv3 dist base not 64kB aligned");
> > +				 dist.attr, &addr, true);
> > +	TEST_ASSERT(ret && errno == EINVAL, "GIC dist base not aligned");
> >  
> > +	addr = rdist.alignment / 0x10;
> >  	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > -				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> > -	TEST_ASSERT(ret && errno == EINVAL, "GICv3 redist base not 64kB aligned");
> > +				 rdist.attr, &addr, true);
> > +	TEST_ASSERT(ret && errno == EINVAL, "GIC redist/cpu base not aligned");
> >  
> >  	/* out of range address */
> >  	if (max_ipa_bits) {
> >  		addr = 1ULL << max_ipa_bits;
> >  		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > -					 KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
> > +					 dist.attr, &addr, true);
> >  		TEST_ASSERT(ret && errno == E2BIG, "dist address beyond IPA limit");
> >  
> >  		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > -					 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> > +					 rdist.attr, &addr, true);
> >  		TEST_ASSERT(ret && errno == E2BIG, "redist address beyond IPA limit");
> >  	}
> >  
> >  	/* set REDIST base address @0x0*/
> >  	addr = 0x00000;
> >  	kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > -			  KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> > +			  rdist.attr, &addr, true);
> >  
> >  	/* Attempt to create a second legacy redistributor region */
> >  	addr = 0xE0000;
> >  	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > -				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> > -	TEST_ASSERT(ret && errno == EEXIST, "GICv3 redist base set again");
> > +				 rdist.attr, &addr, true);
> > +	TEST_ASSERT(ret && errno == EEXIST, "GIC redist base set again");
> >  
> > -	/* Attempt to mix legacy and new redistributor regions */
> > -	addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 0x100000, 0, 0);
> > -	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > -				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> > -	TEST_ASSERT(ret && errno == EINVAL, "attempt to mix GICv3 REDIST and REDIST_REGION");
> > +	if (VGIC_DEV_IS_V3(v->gic_dev_type)) {
> Instead you could check
>     ret = kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>                      KVM_VGIC_V3_ADDR_TYPE_REDIST);

ACK, will do that instead.

> > +		/* Attempt to mix legacy and new redistributor regions */
> > +		addr = REDIST_REGION_ATTR_ADDR(NR_VCPUS, 0x100000, 0, 0);
> > +		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +					 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION,
> > +					 &addr, true);
> > +		TEST_ASSERT(ret && errno == EINVAL,
> > +			    "attempt to mix GICv3 REDIST and REDIST_REGION");
> > +	}
> >  
> >  	/*
> >  	 * Set overlapping DIST / REDIST, cannot be detected here. Will be detected
> >  	 * on first vcpu run instead.
> >  	 */
> > -	addr = 3 * 2 * 0x10000;
> > -	kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, KVM_VGIC_V3_ADDR_TYPE_DIST,
> > -			  &addr, true);
> > +	addr = rdist.size - rdist.alignment;
> > +	kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			  dist.attr, &addr, true);
> >  }
> >  
> >  /* Test the new REDIST region API */
> > @@ -256,14 +298,14 @@ static void subtest_v3_redist_regions(struct vm_gic *v)
> >   * VGIC KVM device is created and initialized before the secondary CPUs
> >   * get created
> >   */
> > -static void test_v3_vgic_then_vcpus(uint32_t gic_dev_type)
> > +static void test_vgic_then_vcpus(uint32_t gic_dev_type)
> >  {
> >  	struct vm_gic v;
> >  	int ret, i;
> >  
> >  	v = vm_gic_create_with_vcpus(gic_dev_type, 1);
> >  
> > -	subtest_v3_dist_rdist(&v);
> > +	subtest_dist_rdist(&v);
> >  
> >  	/* Add the rest of the VCPUs */
> >  	for (i = 1; i < NR_VCPUS; ++i)
> > @@ -276,14 +318,14 @@ static void test_v3_vgic_then_vcpus(uint32_t gic_dev_type)
> >  }
> >  
> >  /* All the VCPUs are created before the VGIC KVM device gets initialized */
> > -static void test_v3_vcpus_then_vgic(uint32_t gic_dev_type)
> > +static void test_vcpus_then_vgic(uint32_t gic_dev_type)
> >  {
> >  	struct vm_gic v;
> >  	int ret;
> >  
> >  	v = vm_gic_create_with_vcpus(gic_dev_type, NR_VCPUS);
> >  
> > -	subtest_v3_dist_rdist(&v);
> > +	subtest_dist_rdist(&v);
> >  
> >  	ret = run_vcpu(v.vm, 3);
> >  	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
> > @@ -552,9 +594,10 @@ int test_kvm_device(uint32_t gic_dev_type)
> >  
> >  void run_tests(uint32_t gic_dev_type)
> >  {
> > +	test_vcpus_then_vgic(gic_dev_type);
> > +	test_vgic_then_vcpus(gic_dev_type);
> > +
> >  	if (VGIC_DEV_IS_V3(gic_dev_type)) {
> > -		test_v3_vcpus_then_vgic(gic_dev_type);
> > -		test_v3_vgic_then_vcpus(gic_dev_type);
> >  		test_v3_new_redist_regions();
> >  		test_v3_typer_accesses();
> >  		test_v3_last_bit_redist_regions();
> Thanks
> 
> Eric
> 

Thanks,
Ricardo
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 08/10] KVM: arm64: selftests: Add tests for GIC redist/cpuif partially above IPA range

[Ricardo Koller]

Add tests for checking that KVM returns the right error when trying to
set GICv2 CPU interfaces or GICv3 Redistributors partially above the
addressable IPA range. Also tighten the IPA range by replacing
KVM_CAP_ARM_VM_IPA_SIZE with the IPA range currently configured for the
guest (i.e., the default).

The check for the GICv3 redistributor created using the REDIST legacy
API is not sufficient as this new test only checks the check done using
vcpus already created when setting the base. The next commit will add
the missing test which verifies that the KVM check is done at first vcpu
run.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../testing/selftests/kvm/aarch64/vgic_init.c | 46 ++++++++++++++-----
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
index 92f5c6ca6b8b..77a1941e61fa 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -31,7 +31,7 @@ struct vm_gic {
 	uint32_t gic_dev_type;
 };
 
-static int max_ipa_bits;
+static uint64_t max_phys_size;
 
 /* helper to access a redistributor register */
 static int access_v3_redist_reg(int gicv3_fd, int vcpu, int offset,
@@ -150,16 +150,21 @@ static void subtest_dist_rdist(struct vm_gic *v)
 	TEST_ASSERT(ret && errno == EINVAL, "GIC redist/cpu base not aligned");
 
 	/* out of range address */
-	if (max_ipa_bits) {
-		addr = 1ULL << max_ipa_bits;
-		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-					 dist.attr, &addr, true);
-		TEST_ASSERT(ret && errno == E2BIG, "dist address beyond IPA limit");
+	addr = max_phys_size;
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 dist.attr, &addr, true);
+	TEST_ASSERT(ret && errno == E2BIG, "dist address beyond IPA limit");
 
-		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-					 rdist.attr, &addr, true);
-		TEST_ASSERT(ret && errno == E2BIG, "redist address beyond IPA limit");
-	}
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 rdist.attr, &addr, true);
+	TEST_ASSERT(ret && errno == E2BIG, "redist address beyond IPA limit");
+
+	/* Space for half a rdist (a rdist is: 2 * rdist.alignment). */
+	addr = max_phys_size - dist.alignment;
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 rdist.attr, &addr, true);
+	TEST_ASSERT(ret && errno == E2BIG,
+			"half of the redist is beyond IPA limit");
 
 	/* set REDIST base address @0x0*/
 	addr = 0x00000;
@@ -248,7 +253,21 @@ static void subtest_v3_redist_regions(struct vm_gic *v)
 	kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
 			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
 
-	addr = REDIST_REGION_ATTR_ADDR(1, 1ULL << max_ipa_bits, 0, 2);
+	addr = REDIST_REGION_ATTR_ADDR(1, max_phys_size, 0, 2);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret && errno == E2BIG,
+		    "register redist region with base address beyond IPA range");
+
+	/* The last redist is above the pa range. */
+	addr = REDIST_REGION_ATTR_ADDR(1, max_phys_size - 0x10000, 0, 2);
+	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
+	TEST_ASSERT(ret && errno == E2BIG,
+		    "register redist region with base address beyond IPA range");
+
+	/* The last redist is above the pa range. */
+	addr = REDIST_REGION_ATTR_ADDR(2, max_phys_size - 0x30000, 0, 2);
 	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
 				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
 	TEST_ASSERT(ret && errno == E2BIG,
@@ -608,8 +627,13 @@ void run_tests(uint32_t gic_dev_type)
 int main(int ac, char **av)
 {
 	int ret;
+	int max_ipa_bits, pa_bits;
 
 	max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
+	pa_bits = vm_guest_mode_params[VM_MODE_DEFAULT].pa_bits;
+	TEST_ASSERT(max_ipa_bits && pa_bits <= max_ipa_bits,
+		"The default PA range should not be larger than the max.");
+	max_phys_size = 1ULL << pa_bits;
 
 	ret = test_kvm_device(KVM_DEV_TYPE_ARM_VGIC_V3);
 	if (!ret) {
-- 
2.33.0.685.g46640cef36-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 08/10] KVM: arm64: selftests: Add tests for GIC redist/cpuif partially above IPA range

[Eric Auger]

Hi Ricardo,

On 9/28/21 8:48 PM, Ricardo Koller wrote:
> Add tests for checking that KVM returns the right error when trying to
> set GICv2 CPU interfaces or GICv3 Redistributors partially above the
> addressable IPA range. Also tighten the IPA range by replacing
> KVM_CAP_ARM_VM_IPA_SIZE with the IPA range currently configured for the
> guest (i.e., the default).
>
> The check for the GICv3 redistributor created using the REDIST legacy
> API is not sufficient as this new test only checks the check done using
> vcpus already created when setting the base. The next commit will add
> the missing test which verifies that the KVM check is done at first vcpu
> run.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  .../testing/selftests/kvm/aarch64/vgic_init.c | 46 ++++++++++++++-----
>  1 file changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> index 92f5c6ca6b8b..77a1941e61fa 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> @@ -31,7 +31,7 @@ struct vm_gic {
>  	uint32_t gic_dev_type;
>  };
>  
> -static int max_ipa_bits;
> +static uint64_t max_phys_size;
>  
>  /* helper to access a redistributor register */
>  static int access_v3_redist_reg(int gicv3_fd, int vcpu, int offset,
> @@ -150,16 +150,21 @@ static void subtest_dist_rdist(struct vm_gic *v)
>  	TEST_ASSERT(ret && errno == EINVAL, "GIC redist/cpu base not aligned");
>  
>  	/* out of range address */
> -	if (max_ipa_bits) {
> -		addr = 1ULL << max_ipa_bits;
> -		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> -					 dist.attr, &addr, true);
> -		TEST_ASSERT(ret && errno == E2BIG, "dist address beyond IPA limit");
> +	addr = max_phys_size;
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 dist.attr, &addr, true);
> +	TEST_ASSERT(ret && errno == E2BIG, "dist address beyond IPA limit");
>  
> -		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> -					 rdist.attr, &addr, true);
> -		TEST_ASSERT(ret && errno == E2BIG, "redist address beyond IPA limit");
> -	}
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 rdist.attr, &addr, true);
> +	TEST_ASSERT(ret && errno == E2BIG, "redist address beyond IPA limit");
> +
> +	/* Space for half a rdist (a rdist is: 2 * rdist.alignment). */
> +	addr = max_phys_size - dist.alignment;
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 rdist.attr, &addr, true);
> +	TEST_ASSERT(ret && errno == E2BIG,
> +			"half of the redist is beyond IPA limit");
>  
>  	/* set REDIST base address @0x0*/
>  	addr = 0x00000;
> @@ -248,7 +253,21 @@ static void subtest_v3_redist_regions(struct vm_gic *v)
>  	kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
>  
> -	addr = REDIST_REGION_ATTR_ADDR(1, 1ULL << max_ipa_bits, 0, 2);
> +	addr = REDIST_REGION_ATTR_ADDR(1, max_phys_size, 0, 2);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret && errno == E2BIG,
> +		    "register redist region with base address beyond IPA range");
> +
> +	/* The last redist is above the pa range. */
> +	addr = REDIST_REGION_ATTR_ADDR(1, max_phys_size - 0x10000, 0, 2);
> +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> +	TEST_ASSERT(ret && errno == E2BIG,
> +		    "register redist region with base address beyond IPA range");
s/base address/top address
> +
> +	/* The last redist is above the pa range. */
> +	addr = REDIST_REGION_ATTR_ADDR(2, max_phys_size - 0x30000, 0, 2);
>  	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>  				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
Why this second check?
>  	TEST_ASSERT(ret && errno == E2BIG,
> @@ -608,8 +627,13 @@ void run_tests(uint32_t gic_dev_type)
>  int main(int ac, char **av)
>  {
>  	int ret;
> +	int max_ipa_bits, pa_bits;
>  
>  	max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
> +	pa_bits = vm_guest_mode_params[VM_MODE_DEFAULT].pa_bits;
> +	TEST_ASSERT(max_ipa_bits && pa_bits <= max_ipa_bits,
> +		"The default PA range should not be larger than the max.");
Isn't it already enforced in the test infra instead?
I see in lib/kvm_util.c

#ifdef __aarch64__
        if (vm->pa_bits != 40)
                vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits);
#endif

vm_open()
> +	max_phys_size = 1ULL << pa_bits;
>  
>  	ret = test_kvm_device(KVM_DEV_TYPE_ARM_VGIC_V3);
>  	if (!ret) {
Eric

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 08/10] KVM: arm64: selftests: Add tests for GIC redist/cpuif partially above IPA range

[Ricardo Koller]

On Thu, Sep 30, 2021 at 10:51:05AM +0200, Eric Auger wrote:
> Hi Ricardo,
> 
> On 9/28/21 8:48 PM, Ricardo Koller wrote:
> > Add tests for checking that KVM returns the right error when trying to
> > set GICv2 CPU interfaces or GICv3 Redistributors partially above the
> > addressable IPA range. Also tighten the IPA range by replacing
> > KVM_CAP_ARM_VM_IPA_SIZE with the IPA range currently configured for the
> > guest (i.e., the default).
> >
> > The check for the GICv3 redistributor created using the REDIST legacy
> > API is not sufficient as this new test only checks the check done using
> > vcpus already created when setting the base. The next commit will add
> > the missing test which verifies that the KVM check is done at first vcpu
> > run.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  .../testing/selftests/kvm/aarch64/vgic_init.c | 46 ++++++++++++++-----
> >  1 file changed, 35 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > index 92f5c6ca6b8b..77a1941e61fa 100644
> > --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > @@ -31,7 +31,7 @@ struct vm_gic {
> >  	uint32_t gic_dev_type;
> >  };
> >  
> > -static int max_ipa_bits;
> > +static uint64_t max_phys_size;
> >  
> >  /* helper to access a redistributor register */
> >  static int access_v3_redist_reg(int gicv3_fd, int vcpu, int offset,
> > @@ -150,16 +150,21 @@ static void subtest_dist_rdist(struct vm_gic *v)
> >  	TEST_ASSERT(ret && errno == EINVAL, "GIC redist/cpu base not aligned");
> >  
> >  	/* out of range address */
> > -	if (max_ipa_bits) {
> > -		addr = 1ULL << max_ipa_bits;
> > -		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > -					 dist.attr, &addr, true);
> > -		TEST_ASSERT(ret && errno == E2BIG, "dist address beyond IPA limit");
> > +	addr = max_phys_size;
> > +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +				 dist.attr, &addr, true);
> > +	TEST_ASSERT(ret && errno == E2BIG, "dist address beyond IPA limit");
> >  
> > -		ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > -					 rdist.attr, &addr, true);
> > -		TEST_ASSERT(ret && errno == E2BIG, "redist address beyond IPA limit");
> > -	}
> > +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +				 rdist.attr, &addr, true);
> > +	TEST_ASSERT(ret && errno == E2BIG, "redist address beyond IPA limit");
> > +
> > +	/* Space for half a rdist (a rdist is: 2 * rdist.alignment). */
> > +	addr = max_phys_size - dist.alignment;
> > +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +				 rdist.attr, &addr, true);
> > +	TEST_ASSERT(ret && errno == E2BIG,
> > +			"half of the redist is beyond IPA limit");
> >  
> >  	/* set REDIST base address @0x0*/
> >  	addr = 0x00000;
> > @@ -248,7 +253,21 @@ static void subtest_v3_redist_regions(struct vm_gic *v)
> >  	kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> >  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> >  
> > -	addr = REDIST_REGION_ATTR_ADDR(1, 1ULL << max_ipa_bits, 0, 2);
> > +	addr = REDIST_REGION_ATTR_ADDR(1, max_phys_size, 0, 2);
> > +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> > +	TEST_ASSERT(ret && errno == E2BIG,
> > +		    "register redist region with base address beyond IPA range");
> > +
> > +	/* The last redist is above the pa range. */
> > +	addr = REDIST_REGION_ATTR_ADDR(1, max_phys_size - 0x10000, 0, 2);
> > +	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> > +	TEST_ASSERT(ret && errno == E2BIG,
> > +		    "register redist region with base address beyond IPA range");
> s/base address/top address

ACK

> > +
> > +	/* The last redist is above the pa range. */
> > +	addr = REDIST_REGION_ATTR_ADDR(2, max_phys_size - 0x30000, 0, 2);
> >  	ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> >  				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> Why this second check?

Wanted to test having two redists (just a bonus test). This one is
sligthtly more interesting than the previous one, so will just remove
the previous one.

> >  	TEST_ASSERT(ret && errno == E2BIG,
> > @@ -608,8 +627,13 @@ void run_tests(uint32_t gic_dev_type)
> >  int main(int ac, char **av)
> >  {
> >  	int ret;
> > +	int max_ipa_bits, pa_bits;
> >  
> >  	max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
> > +	pa_bits = vm_guest_mode_params[VM_MODE_DEFAULT].pa_bits;
> > +	TEST_ASSERT(max_ipa_bits && pa_bits <= max_ipa_bits,
> > +		"The default PA range should not be larger than the max.");
> Isn't it already enforced in the test infra instead?
> I see in lib/kvm_util.c
> 
> #ifdef __aarch64__
>         if (vm->pa_bits != 40)
>                 vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits);
> #endif

You are right, and this is a weird place to make that check.

> 
> vm_open()
> > +	max_phys_size = 1ULL << pa_bits;
> >  
> >  	ret = test_kvm_device(KVM_DEV_TYPE_ARM_VGIC_V3);
> >  	if (!ret) {
> Eric
>

Thanks,
Ricardo
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 09/10] KVM: arm64: selftests: Add test for legacy GICv3 REDIST base partially above IPA range

[Ricardo Koller]

Add a new test into vgic_init which checks that the first vcpu fails to
run if there is not sufficient REDIST space below the addressable IPA
range.  This only applies to the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API
as the required REDIST space is not know when setting the DIST region.

Note that using the REDIST_REGION API results in a different check at
first vcpu run: that the number of redist regions is enough for all
vcpus. And there is already a test for that case in, the first step of
test_v3_new_redist_regions.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../testing/selftests/kvm/aarch64/vgic_init.c | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
index 77a1941e61fa..417a9a515cad 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -570,6 +570,39 @@ static void test_v3_last_bit_single_rdist(void)
 	vm_gic_destroy(&v);
 }
 
+/* Uses the legacy REDIST region API. */
+static void test_v3_redist_ipa_range_check_at_vcpu_run(void)
+{
+	struct vm_gic v;
+	int ret, i;
+	uint64_t addr;
+
+	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, 1);
+
+	/* Set space for 3 redists, we have 1 vcpu, so this succeeds. */
+	addr = max_phys_size - (3 * 2 * 0x10000);
+	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
+
+	addr = 0x00000;
+	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
+
+	/* Add the rest of the VCPUs */
+	for (i = 1; i < NR_VCPUS; ++i)
+		vm_vcpu_add_default(v.vm, i, guest_code);
+
+	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
+
+	/* Attempt to run a vcpu without enough redist space. */
+	ret = run_vcpu(v.vm, 2);
+	TEST_ASSERT(ret && errno == EINVAL,
+		"redist base+size above PA range detected on 1st vcpu run");
+
+	vm_gic_destroy(&v);
+}
+
 /*
  * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
  */
@@ -621,6 +654,7 @@ void run_tests(uint32_t gic_dev_type)
 		test_v3_typer_accesses();
 		test_v3_last_bit_redist_regions();
 		test_v3_last_bit_single_rdist();
+		test_v3_redist_ipa_range_check_at_vcpu_run();
 	}
 }
 
-- 
2.33.0.685.g46640cef36-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 09/10] KVM: arm64: selftests: Add test for legacy GICv3 REDIST base partially above IPA range

[Eric Auger]

On 9/28/21 8:48 PM, Ricardo Koller wrote:
> Add a new test into vgic_init which checks that the first vcpu fails to
> run if there is not sufficient REDIST space below the addressable IPA
> range.  This only applies to the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API
> as the required REDIST space is not know when setting the DIST region.
>
> Note that using the REDIST_REGION API results in a different check at
> first vcpu run: that the number of redist regions is enough for all
> vcpus. And there is already a test for that case in, the first step of
> test_v3_new_redist_regions.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  .../testing/selftests/kvm/aarch64/vgic_init.c | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> index 77a1941e61fa..417a9a515cad 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> @@ -570,6 +570,39 @@ static void test_v3_last_bit_single_rdist(void)
>  	vm_gic_destroy(&v);
>  }
>  
> +/* Uses the legacy REDIST region API. */
> +static void test_v3_redist_ipa_range_check_at_vcpu_run(void)
> +{
> +	struct vm_gic v;
> +	int ret, i;
> +	uint64_t addr;
> +
> +	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, 1);
> +
> +	/* Set space for 3 redists, we have 1 vcpu, so this succeeds. */
> +	addr = max_phys_size - (3 * 2 * 0x10000);
> +	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +				 KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> +
> +	addr = 0x00000;
> +	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
> +
> +	/* Add the rest of the VCPUs */
> +	for (i = 1; i < NR_VCPUS; ++i)
> +		vm_vcpu_add_default(v.vm, i, guest_code);
> +
> +	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> +
> +	/* Attempt to run a vcpu without enough redist space. */
> +	ret = run_vcpu(v.vm, 2);
> +	TEST_ASSERT(ret && errno == EINVAL,
> +		"redist base+size above PA range detected on 1st vcpu run");
> +
> +	vm_gic_destroy(&v);
> +}
> +
>  /*
>   * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
>   */
> @@ -621,6 +654,7 @@ void run_tests(uint32_t gic_dev_type)
>  		test_v3_typer_accesses();
>  		test_v3_last_bit_redist_regions();
>  		test_v3_last_bit_single_rdist();
> +		test_v3_redist_ipa_range_check_at_vcpu_run();
>  	}
>  }
>  
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 10/10] KVM: arm64: selftests: Add basic ITS device tests

[Ricardo Koller]

Add some ITS device tests: general KVM device tests (address not defined
already, address aligned) and tests for the ITS region being within the
addressable IPA range.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../testing/selftests/kvm/aarch64/vgic_init.c | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
index 417a9a515cad..180221ec325d 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -603,6 +603,47 @@ static void test_v3_redist_ipa_range_check_at_vcpu_run(void)
 	vm_gic_destroy(&v);
 }
 
+static void test_v3_its_region(void)
+{
+	struct vm_gic v;
+	uint64_t addr;
+	int its_fd, ret;
+
+	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
+	its_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_ITS, false);
+
+	addr = 0x401000;
+	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
+	TEST_ASSERT(ret && errno == EINVAL,
+		"ITS region with misaligned address");
+
+	addr = max_phys_size;
+	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
+	TEST_ASSERT(ret && errno == E2BIG,
+		"register ITS region with base address beyond IPA range");
+
+	addr = max_phys_size - 0x10000;
+	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
+	TEST_ASSERT(ret && errno == E2BIG,
+		"Half of ITS region is beyond IPA range");
+
+	/* This one succeeds setting the ITS base */
+	addr = 0x400000;
+	kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
+
+	addr = 0x300000;
+	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
+	TEST_ASSERT(ret && errno == EEXIST, "ITS base set again");
+
+	close(its_fd);
+	vm_gic_destroy(&v);
+}
+
 /*
  * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
  */
@@ -655,6 +696,7 @@ void run_tests(uint32_t gic_dev_type)
 		test_v3_last_bit_redist_regions();
 		test_v3_last_bit_single_rdist();
 		test_v3_redist_ipa_range_check_at_vcpu_run();
+		test_v3_its_region();
 	}
 }
 
-- 
2.33.0.685.g46640cef36-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 10/10] KVM: arm64: selftests: Add basic ITS device tests

[Eric Auger]

Hi Ricardo,

On 9/29/21 2:10 AM, Ricardo Koller wrote:
> Add some ITS device tests: general KVM device tests (address not defined
> already, address aligned) and tests for the ITS region being within the
> addressable IPA range.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  .../testing/selftests/kvm/aarch64/vgic_init.c | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> index 417a9a515cad..180221ec325d 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> @@ -603,6 +603,47 @@ static void test_v3_redist_ipa_range_check_at_vcpu_run(void)
>  	vm_gic_destroy(&v);
>  }
>  
> +static void test_v3_its_region(void)
> +{
> +	struct vm_gic v;
> +	uint64_t addr;
> +	int its_fd, ret;
> +
> +	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
> +	its_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_ITS, false);
this may fail if the ITS device has not been registered by KVM (host GICv2)

Maybe refine the patch title mentionning this is an ITS device "init" test.
as per Documentation/virt/kvm/devices/arm-vgic-its.rst we could also try
instantiating the ITS before the GIC and try instantiating several ITSs
with overlapping addresses.
But I would totally understand if you consider this out of the scope of
your  fixes + tests.

Thanks!

Eric
> +
> +	addr = 0x401000;
> +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> +	TEST_ASSERT(ret && errno == EINVAL,
> +		"ITS region with misaligned address");
> +
> +	addr = max_phys_size;
> +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> +	TEST_ASSERT(ret && errno == E2BIG,
> +		"register ITS region with base address beyond IPA range");
> +
> +	addr = max_phys_size - 0x10000;
> +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> +	TEST_ASSERT(ret && errno == E2BIG,
> +		"Half of ITS region is beyond IPA range");
> +
> +	/* This one succeeds setting the ITS base */
> +	addr = 0x400000;
> +	kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> +
> +	addr = 0x300000;
> +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> +	TEST_ASSERT(ret && errno == EEXIST, "ITS base set again");
> +
> +	close(its_fd);
> +	vm_gic_destroy(&v);
> +}
> +
>  /*
>   * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
>   */
> @@ -655,6 +696,7 @@ void run_tests(uint32_t gic_dev_type)
>  		test_v3_last_bit_redist_regions();
>  		test_v3_last_bit_single_rdist();
>  		test_v3_redist_ipa_range_check_at_vcpu_run();
> +		test_v3_its_region();
>  	}
>  }
>  

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 10/10] KVM: arm64: selftests: Add basic ITS device tests

[Ricardo Koller]

Hi Eric,

On Thu, Sep 30, 2021 at 11:14:02AM +0200, Eric Auger wrote:
> Hi Ricardo,
> 
> On 9/29/21 2:10 AM, Ricardo Koller wrote:
> > Add some ITS device tests: general KVM device tests (address not defined
> > already, address aligned) and tests for the ITS region being within the
> > addressable IPA range.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  .../testing/selftests/kvm/aarch64/vgic_init.c | 42 +++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > index 417a9a515cad..180221ec325d 100644
> > --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > @@ -603,6 +603,47 @@ static void test_v3_redist_ipa_range_check_at_vcpu_run(void)
> >  	vm_gic_destroy(&v);
> >  }
> >  
> > +static void test_v3_its_region(void)
> > +{
> > +	struct vm_gic v;
> > +	uint64_t addr;
> > +	int its_fd, ret;
> > +
> > +	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
> > +	its_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_ITS, false);
> this may fail if the ITS device has not been registered by KVM (host GICv2)

At the moment it's just called in the GICv3 case. It seems that
registering a GICv3 device results in having an ITS registered as well
(from kvm_register_vgic_device()). I'm assuming this won't change;
we might as well check that assumption. What do you think?

Thanks,
Ricardo

> 
> Maybe refine the patch title mentionning this is an ITS device "init" test.
> as per Documentation/virt/kvm/devices/arm-vgic-its.rst we could also try
> instantiating the ITS before the GIC and try instantiating several ITSs
> with overlapping addresses.
> But I would totally understand if you consider this out of the scope of
> your  fixes + tests.
> 
> Thanks!
> 
> Eric
> > +
> > +	addr = 0x401000;
> > +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> > +	TEST_ASSERT(ret && errno == EINVAL,
> > +		"ITS region with misaligned address");
> > +
> > +	addr = max_phys_size;
> > +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> > +	TEST_ASSERT(ret && errno == E2BIG,
> > +		"register ITS region with base address beyond IPA range");
> > +
> > +	addr = max_phys_size - 0x10000;
> > +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> > +	TEST_ASSERT(ret && errno == E2BIG,
> > +		"Half of ITS region is beyond IPA range");
> > +
> > +	/* This one succeeds setting the ITS base */
> > +	addr = 0x400000;
> > +	kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> > +
> > +	addr = 0x300000;
> > +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> > +	TEST_ASSERT(ret && errno == EEXIST, "ITS base set again");
> > +
> > +	close(its_fd);
> > +	vm_gic_destroy(&v);
> > +}
> > +
> >  /*
> >   * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
> >   */
> > @@ -655,6 +696,7 @@ void run_tests(uint32_t gic_dev_type)
> >  		test_v3_last_bit_redist_regions();
> >  		test_v3_last_bit_single_rdist();
> >  		test_v3_redist_ipa_range_check_at_vcpu_run();
> > +		test_v3_its_region();
> >  	}
> >  }
> >  
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 10/10] KVM: arm64: selftests: Add basic ITS device tests

[Eric Auger]

Hi Ricardo,

On 9/30/21 10:10 PM, Ricardo Koller wrote:
> Hi Eric,
>
> On Thu, Sep 30, 2021 at 11:14:02AM +0200, Eric Auger wrote:
>> Hi Ricardo,
>>
>> On 9/29/21 2:10 AM, Ricardo Koller wrote:
>>> Add some ITS device tests: general KVM device tests (address not defined
>>> already, address aligned) and tests for the ITS region being within the
>>> addressable IPA range.
>>>
>>> Signed-off-by: Ricardo Koller <ricarkol@google.com>
>>> ---
>>>  .../testing/selftests/kvm/aarch64/vgic_init.c | 42 +++++++++++++++++++
>>>  1 file changed, 42 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> index 417a9a515cad..180221ec325d 100644
>>> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> @@ -603,6 +603,47 @@ static void test_v3_redist_ipa_range_check_at_vcpu_run(void)
>>>  	vm_gic_destroy(&v);
>>>  }
>>>  
>>> +static void test_v3_its_region(void)
>>> +{
>>> +	struct vm_gic v;
>>> +	uint64_t addr;
>>> +	int its_fd, ret;
>>> +
>>> +	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
>>> +	its_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_ITS, false);
>> this may fail if the ITS device has not been registered by KVM (host GICv2)
> At the moment it's just called in the GICv3 case. It seems that
OK I missed that. in that case that's fine.

Thanks

Eric
> registering a GICv3 device results in having an ITS registered as well
> (from kvm_register_vgic_device()). I'm assuming this won't change;
> we might as well check that assumption. What do you think?
>
> Thanks,
> Ricardo
>
>> Maybe refine the patch title mentionning this is an ITS device "init" test.
>> as per Documentation/virt/kvm/devices/arm-vgic-its.rst we could also try
>> instantiating the ITS before the GIC and try instantiating several ITSs
>> with overlapping addresses.
>> But I would totally understand if you consider this out of the scope of
>> your  fixes + tests.
>>
>> Thanks!
>>
>> Eric
>>> +
>>> +	addr = 0x401000;
>>> +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
>>> +	TEST_ASSERT(ret && errno == EINVAL,
>>> +		"ITS region with misaligned address");
>>> +
>>> +	addr = max_phys_size;
>>> +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
>>> +	TEST_ASSERT(ret && errno == E2BIG,
>>> +		"register ITS region with base address beyond IPA range");
>>> +
>>> +	addr = max_phys_size - 0x10000;
>>> +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
>>> +	TEST_ASSERT(ret && errno == E2BIG,
>>> +		"Half of ITS region is beyond IPA range");
>>> +
>>> +	/* This one succeeds setting the ITS base */
>>> +	addr = 0x400000;
>>> +	kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
>>> +
>>> +	addr = 0x300000;
>>> +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
>>> +	TEST_ASSERT(ret && errno == EEXIST, "ITS base set again");
>>> +
>>> +	close(its_fd);
>>> +	vm_gic_destroy(&v);
>>> +}
>>> +
>>>  /*
>>>   * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
>>>   */
>>> @@ -655,6 +696,7 @@ void run_tests(uint32_t gic_dev_type)
>>>  		test_v3_last_bit_redist_regions();
>>>  		test_v3_last_bit_single_rdist();
>>>  		test_v3_redist_ipa_range_check_at_vcpu_run();
>>> +		test_v3_its_region();
>>>  	}
>>>  }
>>>  

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 10/10] KVM: arm64: selftests: Add basic ITS device tests

[Ricardo Koller]

On Thu, Sep 30, 2021 at 11:14:02AM +0200, Eric Auger wrote:
> Hi Ricardo,
> 
> On 9/29/21 2:10 AM, Ricardo Koller wrote:
> > Add some ITS device tests: general KVM device tests (address not defined
> > already, address aligned) and tests for the ITS region being within the
> > addressable IPA range.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  .../testing/selftests/kvm/aarch64/vgic_init.c | 42 +++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > index 417a9a515cad..180221ec325d 100644
> > --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > @@ -603,6 +603,47 @@ static void test_v3_redist_ipa_range_check_at_vcpu_run(void)
> >  	vm_gic_destroy(&v);
> >  }
> >  
> > +static void test_v3_its_region(void)
> > +{
> > +	struct vm_gic v;
> > +	uint64_t addr;
> > +	int its_fd, ret;
> > +
> > +	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
> > +	its_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_ITS, false);
> this may fail if the ITS device has not been registered by KVM (host GICv2)
> 
> Maybe refine the patch title mentionning this is an ITS device "init" test.
> as per Documentation/virt/kvm/devices/arm-vgic-its.rst we could also try
> instantiating the ITS before the GIC and try instantiating several ITSs
> with overlapping addresses.
> But I would totally understand if you consider this out of the scope of
> your  fixes + tests.

Will just send a v4 with init tests for now. ACK on changing the patch
title.

Thanks,
Ricardo

> 
> Thanks!
> 
> Eric
> > +
> > +	addr = 0x401000;
> > +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> > +	TEST_ASSERT(ret && errno == EINVAL,
> > +		"ITS region with misaligned address");
> > +
> > +	addr = max_phys_size;
> > +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> > +	TEST_ASSERT(ret && errno == E2BIG,
> > +		"register ITS region with base address beyond IPA range");
> > +
> > +	addr = max_phys_size - 0x10000;
> > +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> > +	TEST_ASSERT(ret && errno == E2BIG,
> > +		"Half of ITS region is beyond IPA range");
> > +
> > +	/* This one succeeds setting the ITS base */
> > +	addr = 0x400000;
> > +	kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> > +
> > +	addr = 0x300000;
> > +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> > +	TEST_ASSERT(ret && errno == EEXIST, "ITS base set again");
> > +
> > +	close(its_fd);
> > +	vm_gic_destroy(&v);
> > +}
> > +
> >  /*
> >   * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
> >   */
> > @@ -655,6 +696,7 @@ void run_tests(uint32_t gic_dev_type)
> >  		test_v3_last_bit_redist_regions();
> >  		test_v3_last_bit_single_rdist();
> >  		test_v3_redist_ipa_range_check_at_vcpu_run();
> > +		test_v3_its_region();
> >  	}
> >  }
> >  
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 10/10] KVM: arm64: selftests: Add basic ITS device tests

[Ricardo Koller]

Add some ITS device tests: general KVM device tests (address not defined
already, address aligned) and tests for the ITS region being within the
addressable IPA range.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../testing/selftests/kvm/aarch64/vgic_init.c | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
index 417a9a515cad..180221ec325d 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -603,6 +603,47 @@ static void test_v3_redist_ipa_range_check_at_vcpu_run(void)
 	vm_gic_destroy(&v);
 }
 
+static void test_v3_its_region(void)
+{
+	struct vm_gic v;
+	uint64_t addr;
+	int its_fd, ret;
+
+	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
+	its_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_ITS, false);
+
+	addr = 0x401000;
+	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
+	TEST_ASSERT(ret && errno == EINVAL,
+		"ITS region with misaligned address");
+
+	addr = max_phys_size;
+	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
+	TEST_ASSERT(ret && errno == E2BIG,
+		"register ITS region with base address beyond IPA range");
+
+	addr = max_phys_size - 0x10000;
+	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
+	TEST_ASSERT(ret && errno == E2BIG,
+		"Half of ITS region is beyond IPA range");
+
+	/* This one succeeds setting the ITS base */
+	addr = 0x400000;
+	kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
+
+	addr = 0x300000;
+	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
+	TEST_ASSERT(ret && errno == EEXIST, "ITS base set again");
+
+	close(its_fd);
+	vm_gic_destroy(&v);
+}
+
 /*
  * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
  */
@@ -655,6 +696,7 @@ void run_tests(uint32_t gic_dev_type)
 		test_v3_last_bit_redist_regions();
 		test_v3_last_bit_single_rdist();
 		test_v3_redist_ipa_range_check_at_vcpu_run();
+		test_v3_its_region();
 	}
 }
 
-- 
2.33.0.685.g46640cef36-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 10/10] KVM: arm64: selftests: Add basic ITS device tests

[Ricardo Koller]

On Tue, Sep 28, 2021 at 01:11:57PM -0700, Ricardo Koller wrote:
> Add some ITS device tests: general KVM device tests (address not defined
> already, address aligned) and tests for the ITS region being within the
> addressable IPA range.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  .../testing/selftests/kvm/aarch64/vgic_init.c | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> index 417a9a515cad..180221ec325d 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> @@ -603,6 +603,47 @@ static void test_v3_redist_ipa_range_check_at_vcpu_run(void)
>  	vm_gic_destroy(&v);
>  }
>  
> +static void test_v3_its_region(void)
> +{
> +	struct vm_gic v;
> +	uint64_t addr;
> +	int its_fd, ret;
> +
> +	v = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS);
> +	its_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_ITS, false);
> +
> +	addr = 0x401000;
> +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> +	TEST_ASSERT(ret && errno == EINVAL,
> +		"ITS region with misaligned address");
> +
> +	addr = max_phys_size;
> +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> +	TEST_ASSERT(ret && errno == E2BIG,
> +		"register ITS region with base address beyond IPA range");
> +
> +	addr = max_phys_size - 0x10000;
> +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> +	TEST_ASSERT(ret && errno == E2BIG,
> +		"Half of ITS region is beyond IPA range");
> +
> +	/* This one succeeds setting the ITS base */
> +	addr = 0x400000;
> +	kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> +
> +	addr = 0x300000;
> +	ret = _kvm_device_access(its_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +			  KVM_VGIC_ITS_ADDR_TYPE, &addr, true);
> +	TEST_ASSERT(ret && errno == EEXIST, "ITS base set again");
> +
> +	close(its_fd);
> +	vm_gic_destroy(&v);
> +}
> +
>  /*
>   * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
>   */
> @@ -655,6 +696,7 @@ void run_tests(uint32_t gic_dev_type)
>  		test_v3_last_bit_redist_regions();
>  		test_v3_last_bit_single_rdist();
>  		test_v3_redist_ipa_range_check_at_vcpu_run();
> +		test_v3_its_region();
>  	}
>  }
>  
> -- 
> 2.33.0.685.g46640cef36-goog
> 

Please ignore this email. This patch is already included in the right trhread.

Sorry and thanks,
Ricardo
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm