by Oliver Upton

[permalink] [raw]

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

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

2024-02-14 17:00:44

by Sebastian Ene

[permalink] [raw]

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

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

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

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

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

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