[PATCH 3/3] RISC-V: KVM: Introduce extension_enabled

[Anup Patel]

On Mon, May 22, 2023 at 2:59 PM Andrew Jones <ajones at ventanamicro.com> wrote:
>
> On Fri, May 19, 2023 at 08:35:31PM +0530, Anup Patel wrote:
> > On Wed, Apr 26, 2023 at 10:43 PM Andrew Jones <ajones at ventanamicro.com> wrote:
> > >
> > > We need three SBI extension status values (uninitialized, enabled,
> > > and disabled). Pairing another boolean array, extension_enabled[],
> > > with the boolean array extension_disabled[] provides four states.
> > > Using a pair of boolean arrays, which may eventually be changed to
> > > a pair of bitmaps, is more space efficient than using one enum
> > > status field. Apply the new (enabled=1,disabled=0) state, which
> > > means either the extension doesn't have a probe function or that
> > > its probe was successful, to avoid more than one probe of the
> > > extension.
> > >
> > > Signed-off-by: Andrew Jones <ajones at ventanamicro.com>
> > > ---
> > >  arch/riscv/include/asm/kvm_vcpu_sbi.h |  9 +++++++++
> > >  arch/riscv/kvm/vcpu_sbi.c             | 11 ++++++++++-
> > >  2 files changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > index 4278125a38a5..e3c5e1d15e93 100644
> > > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > @@ -16,6 +16,15 @@
> > >
> > >  struct kvm_vcpu_sbi_context {
> > >         int return_handled;
> > > +       /*
> > > +        * extension_enabled[] and extension_disabled[] provide SBI
> > > +        * extensions four status values, of which we need three:
> > > +        * (0,0) uninitialized, (1,0) enabled, (0,1) disabled. Using
> > > +        * a pair of boolean arrays, which may eventually be changed
> > > +        * to a pair of bitmaps, is more space efficient than using
> > > +        * one enum status field.
> > > +        */
> > > +       bool extension_enabled[KVM_RISCV_SBI_EXT_MAX];
> >
> > The extension_enabled[] is not an appropriate name.
> >
> > How about extension_probe_allowed[] ?
>
> "probe allowed" sounds to me like "we may call probe", but the meaning we
> want is "we've already called probe and it succeeded, so the extension
> is available" (or that we don't have probe, meaning the extension is
> assumed available). I prefer 'available' for extensions over 'enabled',
> but we already have 'disabled', so 'enabled' appeared to fit better.
> Actually, it's probably best to improve the readability and simplicity,
> at the expense of size, by dropping both the extension_enabled and
> extension_disabled boolean arrays and switching to an 'extension_status'
> enum, where we have SBI_EXT_UNINITIALIZED, SBI_EXT_AVAILABLE, and
> SBI_EXT_UNAVAILABLE.

I agree. Let's switch to enum.

Regards,
Anup

>
> Thanks,
> drew
>
>
> >
> > Any better name ?
> >
> > Regards,
> > Anup
> >
> > >         bool extension_disabled[KVM_RISCV_SBI_EXT_MAX];
> > >  };
> > >
> > > diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> > > index a1a82f0fbad2..344d38bbe06a 100644
> > > --- a/arch/riscv/kvm/vcpu_sbi.c
> > > +++ b/arch/riscv/kvm/vcpu_sbi.c
> > > @@ -155,6 +155,12 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
> > >         if (!sext)
> > >                 return -ENOENT;
> > >
> > > +       /*
> > > +        * We can't set scontext->extension_enabled[] to reg_val since the
> > > +        * extension may have a probe() function which needs to confirm
> > > +        * enablement first. Only set extension_disabled[] here and leave
> > > +        * the extension_enabled[] setting to kvm_vcpu_sbi_find_ext().
> > > +        */
> > >         scontext->extension_disabled[sext->ext_idx] = !reg_val;
> > >
> > >         return 0;
> > > @@ -317,7 +323,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> > >                 ext = entry->ext_ptr;
> > >
> > >                 if (ext->extid_start <= extid && ext->extid_end >= extid) {
> > > -                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
> > > +                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX ||
> > > +                           scontext->extension_enabled[entry->ext_idx])
> > >                                 return ext;
> > >                         if (scontext->extension_disabled[entry->ext_idx])
> > >                                 return NULL;
> > > @@ -325,6 +332,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> > >                                 scontext->extension_disabled[entry->ext_idx] = true;
> > >                                 return NULL;
> > >                         }
> > > +
> > > +                       scontext->extension_enabled[entry->ext_idx] = true;
> > >                         return ext;
> > >                 }
> > >         }
> > > --
> > > 2.39.2
> > >
>
> --
> kvm-riscv mailing list
> kvm-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv