| Message ID | 20231010170503.657189-4-apatel@ventanamicro.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/6] RISC-V: Add defines for SBI debug console extension | expand |
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
On Wed, Oct 11, 2023 at 12:56 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > 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. Okay, I will improve the commit description and cover-letter. > > > > > 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? If KVM user-space chooses not to handle SBI DBCN calls then it can disable SBI DBCN extension for Guest VCPUs using the ONE_REG ioctl() interface. > > > 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? The SBI DBCN (or debug console) is only an early console used for early prints and bootloaders. Once the proper console (like virtio console) is detected by the Guest kernel, it will switch the debug console to proper console. > > Where is the userspace side of this interface at? Where are the patches > to handle this new api you added? As mentioned in the cover letter, I have implemented it in KVMTOOL first. The patches can be found in riscv_sbi_dbcn_v1 branch at: https://github.com/avpatel/kvmtool.git More precisely, this commit: https://github.com/avpatel/kvmtool/commit/06a373ee8991f882ef79de3845a4c8d63cb189a6 > > > > > > > > > > > > > > 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 Regards, Anup
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, }; diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c index 9cd97091c723..b54fe52c915a 100644 --- a/arch/riscv/kvm/vcpu_sbi.c +++ b/arch/riscv/kvm/vcpu_sbi.c @@ -66,6 +66,10 @@ static const struct kvm_riscv_sbi_extension_entry sbi_ext[] = { .ext_idx = KVM_RISCV_SBI_EXT_PMU, .ext_ptr = &vcpu_sbi_ext_pmu, }, + { + .ext_idx = KVM_RISCV_SBI_EXT_DBCN, + .ext_ptr = &vcpu_sbi_ext_dbcn, + }, { .ext_idx = KVM_RISCV_SBI_EXT_EXPERIMENTAL, .ext_ptr = &vcpu_sbi_ext_experimental, diff --git a/arch/riscv/kvm/vcpu_sbi_replace.c b/arch/riscv/kvm/vcpu_sbi_replace.c index 7c4d5d38a339..347c5856347e 100644 --- a/arch/riscv/kvm/vcpu_sbi_replace.c +++ b/arch/riscv/kvm/vcpu_sbi_replace.c @@ -175,3 +175,34 @@ const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_srst = { .extid_end = SBI_EXT_SRST, .handler = kvm_sbi_ext_srst_handler, }; + +static int kvm_sbi_ext_dbcn_handler(struct kvm_vcpu *vcpu, + struct kvm_run *run, + struct kvm_vcpu_sbi_return *retdata) +{ + struct kvm_cpu_context *cp = &vcpu->arch.guest_context; + unsigned long funcid = cp->a6; + + switch (funcid) { + case SBI_EXT_DBCN_CONSOLE_WRITE: + case SBI_EXT_DBCN_CONSOLE_READ: + case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: + /* + * The SBI debug console functions are unconditionally + * forwarded to the userspace. + */ + kvm_riscv_vcpu_sbi_forward(vcpu, run); + retdata->uexit = true; + break; + default: + retdata->err_val = SBI_ERR_NOT_SUPPORTED; + } + + return 0; +} + +const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_dbcn = { + .extid_start = SBI_EXT_DBCN, + .extid_end = SBI_EXT_DBCN, + .handler = kvm_sbi_ext_dbcn_handler, +};
The SBI DBCN extension needs to be emulated in user-space so let us forward console_puts() call to user-space. 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(+)