On Tue, Feb 13, 2024 at 05:10:30PM +0000, Oliver Upton wrote:

On Tue, Feb 13, 2024 at 04:59:01PM +0000, Sebastian Ene wrote:

quoted

On Tue, Feb 13, 2024 at 12:42:42AM +0000, Oliver Upton wrote:

quoted

On Wed, Feb 07, 2024 at 02:48:33PM +0000, Sebastian Ene wrote:

[...]

quoted

quoted

quoted

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

Allright, I will create separate entries for getting the ipa_space and the start
level which will be exposed in the vm debugfs dir.

quoted

quoted

quoted

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

Ack, will stick to this approach in this case.

quoted

quoted

quoted

 	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.

Right, I see what you mean, thanks for the clarification.

quoted

quoted

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

Thanks for the feedback,
Seb

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