Re: [PATCH 3/6] RISC-V: KVM: Forward SBI DBCN extension to user-space

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Oct 11, 2023 at 12:02:30PM +0530, Anup Patel wrote:
> On Tue, Oct 10, 2023 at 10:45 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Oct 10, 2023 at 10:35:00PM +0530, Anup Patel wrote:
> > > The SBI DBCN extension needs to be emulated in user-space
> >
> > Why?
> 
> The SBI debug console is similar to a console port available to
> KVM Guest so the KVM user space tool (i.e. QEMU-KVM or
> KVMTOOL) can redirect the input/output of SBI debug console
> wherever it wants (e.g.  telnet, file, stdio, etc).
> 
> We forward SBI DBCN calls to KVM user space so that the
> in-kernel KVM does not need to be aware of the guest
> console devices.

Hint, my "Why" was attempting to get you to write a better changelog
description, which would include the above information.  Please read the
kernel documentation for hints on how to do this so that we know what
why changes are being made.

> > > so let
> > > us forward console_puts() call to user-space.
> >
> > What could go wrong!
> >
> > Why does userspace have to get involved in a console message?  Why is
> > this needed at all?  The kernel can not handle userspace consoles as
> > obviously they have to be re-entrant and irq safe.
> 
> As mentioned above, these are KVM guest console messages which
> the VMM (i.e. KVM user-space) can choose to manage on its own.

If it chooses not to, what happens?

> This is more about providing flexibility to KVM user-space which
> allows it to manage guest console devices.

Why not use the normal virtio console device interface instead of making
a riscv-custom one?

Where is the userspace side of this interface at?  Where are the patches
to handle this new api you added?

> 
> >
> > >
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > ---
> > >  arch/riscv/include/asm/kvm_vcpu_sbi.h |  1 +
> > >  arch/riscv/include/uapi/asm/kvm.h     |  1 +
> > >  arch/riscv/kvm/vcpu_sbi.c             |  4 ++++
> > >  arch/riscv/kvm/vcpu_sbi_replace.c     | 31 +++++++++++++++++++++++++++
> > >  4 files changed, 37 insertions(+)
> > >
> > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > index 8d6d4dce8a5e..a85f95eb6e85 100644
> > > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > @@ -69,6 +69,7 @@ extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_ipi;
> > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_rfence;
> > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_srst;
> > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_hsm;
> > > +extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_dbcn;
> > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_experimental;
> > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_vendor;
> > >
> > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > index 917d8cc2489e..60d3b21dead7 100644
> > > --- a/arch/riscv/include/uapi/asm/kvm.h
> > > +++ b/arch/riscv/include/uapi/asm/kvm.h
> > > @@ -156,6 +156,7 @@ enum KVM_RISCV_SBI_EXT_ID {
> > >       KVM_RISCV_SBI_EXT_PMU,
> > >       KVM_RISCV_SBI_EXT_EXPERIMENTAL,
> > >       KVM_RISCV_SBI_EXT_VENDOR,
> > > +     KVM_RISCV_SBI_EXT_DBCN,
> > >       KVM_RISCV_SBI_EXT_MAX,
> >
> > You just broke a user/kernel ABI here, why?
> 
> The KVM_RISCV_SBI_EXT_MAX only represents the number
> of entries in "enum KVM_RISCV_SBI_EXT_ID" so we are not
> breaking "enum KVM_RISCV_SBI_EXT_ID" rather appending
> new ID to existing enum.

So you are sure that userspace never actually tests or sends that _MAX
value anywhere?  If not, why is it even needed?

thanks,

greg k-h