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

[Ricardo Koller <ricarkol@google.com>]

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
> >>
>