Re: [PATCH v5 4/4] KVM: arm64: Initialize the ptdump parser with stage-2 attributes

From: Oliver Upton <oliver.upton@linux.dev>
To: Sebastian Ene <sebastianene@google.com>
Cc: catalin.marinas@arm.com, gshan@redhat.com, james.morse@arm.com,
	mark.rutland@arm.com, maz@kernel.org, rananta@google.com,
	ricarkol@google.com, ryan.roberts@arm.com, shahuang@redhat.com,
	suzuki.poulose@arm.com, will@kernel.org, yuzenghui@huawei.com,
	kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, kernel-team@android.com,
	vdonnefort@google.com
Subject: Re: [PATCH v5 4/4] KVM: arm64: Initialize the ptdump parser with stage-2 attributes
Date: Tue, 13 Feb 2024 17:10:30 +0000	[thread overview]
Message-ID: <ZcuihokcWA3q-pZS@linux.dev> (raw)
In-Reply-To: <Zcuf1ZUvwhxBobuG@google.com>

On Tue, Feb 13, 2024 at 04:59:01PM +0000, Sebastian Ene wrote:
> On Tue, Feb 13, 2024 at 12:42:42AM +0000, Oliver Upton wrote:
> > On Wed, Feb 07, 2024 at 02:48:33PM +0000, Sebastian Ene wrote:

[...]

> > > +
> > > +	snprintf(marker_msg, MARKER_MSG_LEN, "IPA bits %2u start lvl %1d",
> > > +		 pgtable->ia_bits, pgtable->start_level);
> > > +
> > > +	ipa_addr_marker[0].name = marker_msg;
> > 
> > Is the dynamic name worth the added complexity? I see nothing wrong with
> > exposing additional debugfs files for simple attributes like the IPA
> > range and page table levels.
> > 
> > I know it isn't *that* much, just looking for every opportunity to
> > simplify further.
> > 
> 
> We can keep them separate, I have no strong opinion about this. I think
> this was Vincent's, original suggestion to have them so I will check with
> him as well.

Well, if we get to the place where there's a single struct containing
all of the required data upfront then this becomes less of an issue.
This is useful information still, so let's see if we can go about it the
other way.

> > > +	ret = kvm_ptdump_parser_init(&parser_state, mmu->pgt, m);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > 
> > Can this be done at open(), or am I missing something?
> > 
> 
> I guess we can do this in open() but then we will have to add again that
> struct that wraps some ptdump specific state tracking. It seemed a bit cleaner in
> this way. What do you think ?

Allocating something that looks like an iterator end embedding it in
->private isn't too uncommon.

> > >  	write_lock(&guest_kvm->mmu_lock);
> > >  	ret = kvm_ptdump_show_common(m, mmu->pgt, &parser_state);
> > >  	write_unlock(&guest_kvm->mmu_lock);
> > >  
> > > +	kvm_ptdump_parser_teardown(&parser_state);
> > 
> > Same question here, can this happen at close()? I guess you'll need a
> > struct to encapsulate pg_state and a pointer to the VM at least.
> >
> 
> Right, I tried to avoid using a separate struct as we discussed in v4.

Sorry, I hope I didn't confuse you in my prior feedback.

What I had issue with was the multiple layers of function ptr / ops
structs for managing the file interface. I have zero concerns with
organizing the _data_ for the walk this way.

> > Actually, come to think of it, if you embed all of the data you need for
> > the walker into a structure you can just do a single allocation for it
> > upfront.

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel