Message-ID: <Yi+B5bZ1LpaNCUJT@google.com>
Date:   Mon, 14 Mar 2022 17:56:53 +0000
From:   Oliver Upton <oupton@...gle.com>
To:     Andrew Jones <drjones@...hat.com>
Cc:     Sean Christopherson <seanjc@...gle.com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, Thomas Huth <thuth@...hat.com>,
        Janosch Frank <frankja@...ux.ibm.com>,
        Claudio Imbrenda <imbrenda@...ux.ibm.com>,
        David Hildenbrand <david@...hat.com>,
        David Matlack <dmatlack@...gle.com>,
        Ben Gardon <bgardon@...gle.com>
Subject: Re: [RFC PATCH 000/105] KVM: selftests: Overhaul APIs, purge VCPU_ID

On Mon, Mar 14, 2022 at 12:06:53PM +0100, Andrew Jones wrote:
> On Fri, Mar 11, 2022 at 05:49:11AM +0000, Sean Christopherson wrote:
> > First off, hopefully I didn't just spam you with 106 emails.  In theory,
> > unless you're subscribed to LKML, you should see only the cover letter
> > and everything else should be on lore if you want to pull down the mbox
> > (instead of saying "LOL, 105 patches!?!?", or maybe after you say that).
> > 
> > This is a (very) early RFC for overhauling KVM's selftests APIs.  It's
> > compile tested only (maybe), there are no changelogs, etc...
> > 
> > My end goal with an overhaul is to get to a state where adding new
> > features and writing tests is less painful/disgusting (I feel dirty every
> > time I copy+paste VCPU_ID).  I opted to directly send only the cover
> > letter because most of the individual patches aren't all that interesting,
> > there's still 46 patches even if the per-test conversions are omitted, and
> > it's the final state that I really care about and want to discuss.
> > 
> > The overarching theme of my take on where to go with selftests is to stop
> > treating tests like second class citizens.  Stop hiding vcpu, kvm_vm, etc...
> > There's no sensitive data/constructs, and the encapsulation has led to
> > really, really bad and difficult to maintain code.  E.g. Want to call a
> > vCPU ioctl()?  Hope you have the VM...
> 
> Ack to dropping the privateness of structs.
> 
> > 
> > The other theme in the rework is to deduplicate code and try to set us
> > up for success in the future.  E.g. provide macros/helpers instead of
> > spamming CTRL-C => CTRL-V (see the -700 LoC).
> 
> Ack to more helper functions. I'm not sure what the best way to document
> or provide examples for the API is though. Currently we mostly rely on
> test writers to read other tests (I suppose the function headers help a
> bit, but, IMO, not much). Maybe we need a heavily commented example.c
> that can help test writers get started, along with better API function
> descriptions for anything exported from the lib.
> 

+1. Definitely guilty of copy/pasting a test then tweaking to fit the
problem I'm trying to solve. A barebones example would be helpful.

Haven't looked at the patches yet, but one of my whines about the
selftests is that every test winds up explicitly handling exit reasons
that percolate up from the libraries. Perhaps some helpers around ABORTs
and the like would be useful (and maybe Sean already did this!)

> > 
> > I was hoping to get this into a less shabby state before posting, but I'm
> > I'm going to be OOO for the next few weeks and want to get the ball rolling
> > instead of waiting another month or so.
> 
> Ideas look good to me, but I'll wait for the cleaned up series posted to
> the KVM ML to review it. Also, I see at least patch 1/105 is a fix. It'd
> be nice to post all fixes separately so they get in sooner than later.
> 
> Oh, some of the renaming doesn't look all that important to me, like
> prefixing with kvm_ or adding _arch_, but I don't have strong preferences
> on the names. Also, for the _arch_ functions it'd be nice to create
> common, weak functions which the arch must override. The common function
> would just assert. That should help people who want to port to other
> architectures determine what they need to implement first. And, for
> anything which an arch can optionally adopt a common implementation,
> *not* naming the common function with _arch_, but still defining it as
> weak, would make sense to me too.

I think it may make more sense to only define optional functions as
weak and let the compiler do the screaming for the required ones. Only
discovering that functions are missing at runtime could be annoying if
you're cross-compiling and running on a separate host with a different
architecture.

--
Oliver

Powered by blists - more mailing lists