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

From: Marc Zyngier <maz@kernel.org>
To: Ricardo Koller <ricarkol@google.com>
Cc: Eric Auger <eric.auger@redhat.com>,
	kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
	drjones@redhat.com, alexandru.elisei@arm.com,
	Paolo Bonzini <pbonzini@redhat.com>,
	oupton@google.com, james.morse@arm.com, suzuki.poulose@arm.com,
	shuah@kernel.org, jingzhangos@google.com, pshier@google.com,
	rananta@google.com, reijiw@google.com
Subject: Re: [PATCH v3 01/10] kvm: arm64: vgic: Introduce vgic_check_iorange
Date: Fri, 01 Oct 2021 14:12:17 +0100	[thread overview]
Message-ID: <87k0iwsxce.wl-maz@kernel.org> (raw)
In-Reply-To: <YVYp1E7bqIFttXF+@google.com>

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.