On Wed, Jun 7, 2023 at 5:37 AM Zhi Wang <zhi.wang.linux@xxxxxxxxx> wrote:
>
> On Tue, 6 Jun 2023 10:30:42 -0700
> Vipin Sharma <vipinsh@xxxxxxxxxx> wrote:
>
> > On Mon, Jun 05, 2023 at 10:35:20PM +0800, Zhi Wang wrote:
> > > On Fri, 2 Jun 2023 09:09:07 -0700
> > > Vipin Sharma <vipinsh@xxxxxxxxxx> wrote:
> > >
> > > > Document what the page table walker do when walker callback function returns
> > > > a value.
> > > >
> > > > Current documentation is not correct as negative error of -EAGAIN on a
> > > > non-shared page table walker doesn't terminate the walker and continues
> > > > to the next step.
> > > >
> > > > There might be a better place to keep this information, for now this
> > > > documentation will work as a reference guide until a better way is
> > > > found.
> > > >
> > >
> > > After reading the whole patch series, I was thinking it might be a good
> > > time to improve the way how the visitor function and page table walker
> > > talk to each other. The error code is good enough before, but its meaning
> > > seems limited and vague when the visitor function wants to express more about
> > > what exactly happens inside. I am not sure if it is a good idea to continue
> > > that way: 1. found a new situation. 2. choosing a error code for visitor
> > > function. 3. walker translates the error code into the situation to
> > > handle. 4. document the error code and its actual meaning.
> > >
> > > Eventually I am afraid that we are going to abuse the error code.
> >
> > I agree that error numbers are not sufficient and this will become more
> > difficult and cumbersome for more cases in future if we need different
> > behavior based on different error codes for different visitor functions.
> >
> > >
> > > What about introducing a set of flags for the visitor function to express
> > > what happened and simplify the existing error code?
> > >
> >
> > If I understood correctly what you meant, I think this will also end up
> > having the same issue down the line, we are just switching errors with
> > flags as they might not be able to express everything.
> >
> > "Flags for visitor function to express what happened" - This is what
> > ret val and errors do.
> >
>
> Thanks so much for the efforts of the sample code.
>
> But when the "ret val" is an error code for pgtable matters, It turns vague.
> We have -EAGAIN to represent "retry" and "-ENONET" to represent PTE not there,
> and they seems end up with different behaviors in different types of pgtable
> walk. That is what I feels off.
>
> visitor_cb has two different requirements of returning the status: 1)
> something wrong happens *not* related to the pgtable, e.g. failing to
> allocate memory. 2) something happens related to the pgtable. e.g PTE doesn't
> exists.
>
> For 1) It is natural to return an error code and the caller might just bail out
> via its error handling path.
>
> I think the core problem is: the two different usages are mixed and they don't
> actually fit with each other. 2) is requiring more details in the future other
> than a simple error code.
>
>
> For 2) I think it is better have a set of flags. the name of the flags can
> carry more explicit meanings than error code. E.g.:
>
> ------------------
>
> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> index 4cd6762bda80..b3f24b321cd7 100644
> --- a/arch/arm64/include/asm/kvm_pgtable.h
> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> @@ -204,6 +204,15 @@ enum kvm_pgtable_walk_flags {
> KVM_PGTABLE_WALK_HANDLE_FAULT = BIT(4),
> };
>
> +struct kvm_pgtable_walk_status {
> + union {
> + u8 raw;
> + struct {
> + unsigned retry:1;
> + unsigned stop:1;
> + unsigned ignore:1;
> + /* more to come */
> + };
> + };
> +};
> +
> struct kvm_pgtable_visit_ctx {
> kvm_pte_t *ptep;
> kvm_pte_t old;
> @@ -213,8 +222,10 @@ struct kvm_pgtable_visit_ctx {
> u64 end;
> u32 level;
> enum kvm_pgtable_walk_flags flags;
> + struct kvm_pgtable_walk_status *status;
> };
>
> typedef int (*kvm_pgtable_visitor_fn_t)(const struct kvm_pgtable_visit_ctx *ctx,
> enum kvm_pgtable_walk_flags visit);
>
> ----------------
>
> Visitor functions sets the flags via ctx->status and kvm_pgtable_walk_xxx can
> check the bits in the ctx to decide what to do for the next.
>
> I can cook a patch for re-factoring this part if we think it is a good idea.
>
This can also be one option. I will wait till others also weigh in on
how to make walkers and callbacks work together for more use cases.
