Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 24 Jan 2020 11:45:38 +0100
From:   Andrew Jones <drjones@redhat.com>
To:     Paolo Bonzini <pbonzini@redhat.com>
Cc:     Ben Gardon <bgardon@google.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, linux-kselftest@vger.kernel.org,
        Cannon Matthews <cannonmatthews@google.com>,
        Peter Xu <peterx@redhat.com>, Peter Shier <pshier@google.com>,
        Oliver Upton <oupton@google.com>,
        Marc Zyngier <Marc.Zyngier@arm.com>,
        Cornelia Huck <cohuck@redhat.com>,
        Christian Borntraeger <borntraeger@de.ibm.com>
Subject: Re: [PATCH v4 06/10] KVM: selftests: Add support for vcpu_args_set
 to aarch64 and s390x
Message-ID: <20200124104538.44i5ut5uzclz443i@kamzik.brq.redhat.com>
References: <20200123180436.99487-1-bgardon@google.com>
 <20200123180436.99487-7-bgardon@google.com>
 <4dbb6d1b-3162-d9b3-4ebb-5e4061776bb6@redhat.com>
 <20200124093543.m5oqo7fnjnc2scko@kamzik.brq.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200124093543.m5oqo7fnjnc2scko@kamzik.brq.redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jan 24, 2020 at 10:35:43AM +0100, Andrew Jones wrote:
> > > +void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
> > > +{
> > > +	va_list ap;
> > > +
> > > +	TEST_ASSERT(num >= 1 && num <= 8, "Unsupported number of args,\n"
> > > +		    "  num: %u\n",
> > > +		    num);
> 
> Weird line breaking. I see it came from the x86 implementation, but it's
> weird there too... Personally I'd just put it all on one line, because
> my vt100 died two decades ago.
> 
> > > +
> > > +	va_start(ap, num);
> > > +
> > > +	for (i = 0; i < num; i++)
> > > +		set_reg(vm, vcpuid, ARM64_CORE_REG(regs.regs[num]),
>                                                              ^^ should be 'i'

The declaration of 'i' is also missing.

> 
> > > +			va_arg(ap, uint64_t));
> 
> nit: I'd use {} because of the line break. Or just not break the line and
> bust the 80 char "limit" (RIP vt100).
>

Thanks,
drew