tech-unixplatformspec@lists.riscv.org | SBI Debug Console Extension Proposal (Draft v1)

Home Messages Hashtags Subgroups

Date Date 1 - 20 of 21

SBI Debug Console Extension Proposal (Draft v1)

Darius Rad #1750 On Wed, Jun 01, 2022 at 09:47:32PM +0530, Anup Patel wrote: Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. It seems to me that the rationale and summary of the extension is insufficient, and non-normative commentary is lacking. The ensuing mailing list discussion suggests that there are additional requirements and assumptions that influenced the design of the interface, but are not mentioned here. It is important to document such information, in order to have a constructive discussion now, and also to preserve such information for readers in the future who may not have the benefit of reading the mailing list discussions. // darius
More All Messages By This Member
atishp@... #1745 On Fri, Jun 3, 2022 at 12:23 AM Heinrich Schuchardt <heinrich.schuchardt@...> wrote: On 6/3/22 08:56, Atish Kumar Patra wrote: On Thu, Jun 2, 2022 at 2:13 AM Heinrich Schuchardt <heinrich.schuchardt@...> wrote: On 6/2/22 10:44, Anup Patel wrote: On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt <heinrich.schuchardt@...> wrote: On 6/1/22 18:17, Anup Patel wrote: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: Thanks for starting to close this gap. 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. I miss a discussion of the conflicts that can arise if the configuration of the serial console is changed by the caller. Do we need an ecall that closes the SBI console to further access? Usually, the serial port related code in M-mode firmware only uses status and data registers so for most serial ports support the M-mode firmware will adapt to serial port configuration changes. In fact, this is why we never had a special SBI call for serial port reconfiguration in legacy SBI v0.1 as well. In case of virtualization, the serial port (or console) is emulated so the special SBI call is not useful for virtualization. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Isn't it M-mode software that has to program the MMU to allow all harts in M-mode and S-mode access to the memory area? What is the S-mode software to do about the memory area prior to calling sbi_debug_console_set_area()? Actually, it's the S-mode software which is voluntarily giving a portion of its memory to be used as shared memory. The proposal only mandates that whatever memory is selected by S-mode software should be a regular cacheable memory (not IO memory). Also, if Svpbmt is available then S-mode software should only use memory type 0 in the PTEs. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) The console output is needed on the very start of the S-mode software, before setting up anything. Can we avoid this extra function? Can we simply assume that the caller of sbi_debug_console_puts() provides a physical address pointer to a memory area that is suitable? Theoretically, we can avoid the extra function to set shared memory area but it will complicate things in future when we have supervisor software encrypting it's own memory (using special ISA support) because in this case supervisor software will have to unprotect memory every time the sbi_debug_console_puts() is called and protect it again after the call. Currently this function is just a nop(). It is not needed in this revision of the extension. The function might be called repeatedly by different threads with different values. How do you want to keep track of all of these different areas? Memory shared between different security realms will arise in many different scenarios. As this is not console specific it should be in a separate extension. That extension should be defined once we have clarity about how security realms are managed. I would like to understand more about the security concerns you raised. Can you please elaborate on this ? I have no security concerns. My thought was that it might be better to manage shared memory in a separate extension for all use cases. Ahh I see. Sorry I misunderstood your comment. The designated shared memory for debug console is shared across the platforms while STA or PMU use case will have per cpu shared memory. I agree that all the concerns related to shared memory can be discussed together and all the common constraints can be described at one place. But defining a separate extension for the shared memory may be problematic as we need to manage the extension dependencies. Best regards Heinrich As you pointed out, there are other possible use cases(e.g STA, PMU) for shared memory between S & M mode or VS & HS mode. It would be good to address these concerns right now instead of discussing those in each individual extension context. Set the shared memory area specified by `addr_div_by_2` and `size` %s/addr_div_by_2/addr_div_by_4/ Okay, I will update. parameters. The `addr_div_by_4` parameter is base address of the %s/is base/is the base/ Okay, I will update. shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. Why shifting the address? I would prefer to keep it simple for the caller. If the alignment is not suitable, return an error. But why is an alignment needed here at all? And why 4 aligned? For RV32 S-mode, the physical address space is 34bits wide but "unsigned long" is 32bits wide. This is because Sv32 PTEs allow 34bits of PPN. In fact, even instructions such as HFENCE.GVMA have this "address right shift by 2" requirement based on this rationale. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) I would prefer to simply pass a physical address pointer here with no requirements on alignment. And no prior SBI call. sbi_debug_console_set_area() might be called with different values by different threads. An offset is ambiguous as it does not define to which of the different shared areas it relates. Please, use a pointer. Do we need num_chars? Are we expecting to provide binary output? Using 0x00 as terminator would be adequate in most cases. Bare-metal tests (or assembly sources) can print sub-strings from a large per-populated string in shared memory. Assuming that string is always terminated by 0x00 in sbi_debug_console_puts() will break this flexibility. OK What is the requirement on the console? Does it have to support 8bit output to allow for UTF-8? We need to clarify this. Suggestions ? The platform specification would be the right place to require 8-bit support for the console. Do we make any assumptions about encoding? Same as above, this needs more clarification. Suggestions ? We should add to the platform specification that UTF-8 output is assumed on the serial console. I am of the opinion to keep such encoding related assumptions to be minimal. How would we handle a console set up to 7bit + parity if a character > 0x7f is sent? I would consider this to be part of the clarification we add for encoding. We should state if extra bits are ignored or the bytes are not send. The easiest thing is to just ignore the extra bits. So let't state this here. Best regards Heinrich Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter %s/shard/shared/ Okay, I will update. is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. There could be other reasons of failures: * set up of the UART failed in OpenSBI * no UART defined in the device-tree * ... So let us add SBI_ERR_FAILED to the list. Okay, I will add. Best regards Heinrich Regards, Anup
More All Messages By This Member
Heinrich Schuchardt #1744 On 6/3/22 08:56, Atish Kumar Patra wrote: On Thu, Jun 2, 2022 at 2:13 AM Heinrich Schuchardt <heinrich.schuchardt@...> wrote: On 6/2/22 10:44, Anup Patel wrote: On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt <heinrich.schuchardt@...> wrote: On 6/1/22 18:17, Anup Patel wrote: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: Thanks for starting to close this gap. 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. I miss a discussion of the conflicts that can arise if the configuration of the serial console is changed by the caller. Do we need an ecall that closes the SBI console to further access? Usually, the serial port related code in M-mode firmware only uses status and data registers so for most serial ports support the M-mode firmware will adapt to serial port configuration changes. In fact, this is why we never had a special SBI call for serial port reconfiguration in legacy SBI v0.1 as well. In case of virtualization, the serial port (or console) is emulated so the special SBI call is not useful for virtualization. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Isn't it M-mode software that has to program the MMU to allow all harts in M-mode and S-mode access to the memory area? What is the S-mode software to do about the memory area prior to calling sbi_debug_console_set_area()? Actually, it's the S-mode software which is voluntarily giving a portion of its memory to be used as shared memory. The proposal only mandates that whatever memory is selected by S-mode software should be a regular cacheable memory (not IO memory). Also, if Svpbmt is available then S-mode software should only use memory type 0 in the PTEs. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) The console output is needed on the very start of the S-mode software, before setting up anything. Can we avoid this extra function? Can we simply assume that the caller of sbi_debug_console_puts() provides a physical address pointer to a memory area that is suitable? Theoretically, we can avoid the extra function to set shared memory area but it will complicate things in future when we have supervisor software encrypting it's own memory (using special ISA support) because in this case supervisor software will have to unprotect memory every time the sbi_debug_console_puts() is called and protect it again after the call. Currently this function is just a nop(). It is not needed in this revision of the extension. The function might be called repeatedly by different threads with different values. How do you want to keep track of all of these different areas? Memory shared between different security realms will arise in many different scenarios. As this is not console specific it should be in a separate extension. That extension should be defined once we have clarity about how security realms are managed. I would like to understand more about the security concerns you raised. Can you please elaborate on this ? I have no security concerns. My thought was that it might be better to manage shared memory in a separate extension for all use cases. Best regards Heinrich As you pointed out, there are other possible use cases(e.g STA, PMU) for shared memory between S & M mode or VS & HS mode. It would be good to address these concerns right now instead of discussing those in each individual extension context. Set the shared memory area specified by `addr_div_by_2` and `size` %s/addr_div_by_2/addr_div_by_4/ Okay, I will update. parameters. The `addr_div_by_4` parameter is base address of the %s/is base/is the base/ Okay, I will update. shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. Why shifting the address? I would prefer to keep it simple for the caller. If the alignment is not suitable, return an error. But why is an alignment needed here at all? And why 4 aligned? For RV32 S-mode, the physical address space is 34bits wide but "unsigned long" is 32bits wide. This is because Sv32 PTEs allow 34bits of PPN. In fact, even instructions such as HFENCE.GVMA have this "address right shift by 2" requirement based on this rationale. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) I would prefer to simply pass a physical address pointer here with no requirements on alignment. And no prior SBI call. sbi_debug_console_set_area() might be called with different values by different threads. An offset is ambiguous as it does not define to which of the different shared areas it relates. Please, use a pointer. Do we need num_chars? Are we expecting to provide binary output? Using 0x00 as terminator would be adequate in most cases. Bare-metal tests (or assembly sources) can print sub-strings from a large per-populated string in shared memory. Assuming that string is always terminated by 0x00 in sbi_debug_console_puts() will break this flexibility. OK What is the requirement on the console? Does it have to support 8bit output to allow for UTF-8? We need to clarify this. Suggestions ? The platform specification would be the right place to require 8-bit support for the console. Do we make any assumptions about encoding? Same as above, this needs more clarification. Suggestions ? We should add to the platform specification that UTF-8 output is assumed on the serial console. I am of the opinion to keep such encoding related assumptions to be minimal. How would we handle a console set up to 7bit + parity if a character > 0x7f is sent? I would consider this to be part of the clarification we add for encoding. We should state if extra bits are ignored or the bytes are not send. The easiest thing is to just ignore the extra bits. So let't state this here. Best regards Heinrich Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter %s/shard/shared/ Okay, I will update. is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. There could be other reasons of failures: * set up of the UART failed in OpenSBI * no UART defined in the device-tree * ... So let us add SBI_ERR_FAILED to the list. Okay, I will add. Best regards Heinrich Regards, Anup
More All Messages By This Member
atishp@... #1743 On Thu, Jun 2, 2022 at 2:13 AM Heinrich Schuchardt <heinrich.schuchardt@...> wrote: On 6/2/22 10:44, Anup Patel wrote: On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt <heinrich.schuchardt@...> wrote: On 6/1/22 18:17, Anup Patel wrote: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: Thanks for starting to close this gap. 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. I miss a discussion of the conflicts that can arise if the configuration of the serial console is changed by the caller. Do we need an ecall that closes the SBI console to further access? Usually, the serial port related code in M-mode firmware only uses status and data registers so for most serial ports support the M-mode firmware will adapt to serial port configuration changes. In fact, this is why we never had a special SBI call for serial port reconfiguration in legacy SBI v0.1 as well. In case of virtualization, the serial port (or console) is emulated so the special SBI call is not useful for virtualization. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Isn't it M-mode software that has to program the MMU to allow all harts in M-mode and S-mode access to the memory area? What is the S-mode software to do about the memory area prior to calling sbi_debug_console_set_area()? Actually, it's the S-mode software which is voluntarily giving a portion of its memory to be used as shared memory. The proposal only mandates that whatever memory is selected by S-mode software should be a regular cacheable memory (not IO memory). Also, if Svpbmt is available then S-mode software should only use memory type 0 in the PTEs. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) The console output is needed on the very start of the S-mode software, before setting up anything. Can we avoid this extra function? Can we simply assume that the caller of sbi_debug_console_puts() provides a physical address pointer to a memory area that is suitable? Theoretically, we can avoid the extra function to set shared memory area but it will complicate things in future when we have supervisor software encrypting it's own memory (using special ISA support) because in this case supervisor software will have to unprotect memory every time the sbi_debug_console_puts() is called and protect it again after the call. Currently this function is just a nop(). It is not needed in this revision of the extension. The function might be called repeatedly by different threads with different values. How do you want to keep track of all of these different areas? Memory shared between different security realms will arise in many different scenarios. As this is not console specific it should be in a separate extension. That extension should be defined once we have clarity about how security realms are managed. I would like to understand more about the security concerns you raised. Can you please elaborate on this ? As you pointed out, there are other possible use cases(e.g STA, PMU) for shared memory between S & M mode or VS & HS mode. It would be good to address these concerns right now instead of discussing those in each individual extension context. Set the shared memory area specified by `addr_div_by_2` and `size` %s/addr_div_by_2/addr_div_by_4/ Okay, I will update. parameters. The `addr_div_by_4` parameter is base address of the %s/is base/is the base/ Okay, I will update. shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. Why shifting the address? I would prefer to keep it simple for the caller. If the alignment is not suitable, return an error. But why is an alignment needed here at all? And why 4 aligned? For RV32 S-mode, the physical address space is 34bits wide but "unsigned long" is 32bits wide. This is because Sv32 PTEs allow 34bits of PPN. In fact, even instructions such as HFENCE.GVMA have this "address right shift by 2" requirement based on this rationale. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) I would prefer to simply pass a physical address pointer here with no requirements on alignment. And no prior SBI call. sbi_debug_console_set_area() might be called with different values by different threads. An offset is ambiguous as it does not define to which of the different shared areas it relates. Please, use a pointer. Do we need num_chars? Are we expecting to provide binary output? Using 0x00 as terminator would be adequate in most cases. Bare-metal tests (or assembly sources) can print sub-strings from a large per-populated string in shared memory. Assuming that string is always terminated by 0x00 in sbi_debug_console_puts() will break this flexibility. OK What is the requirement on the console? Does it have to support 8bit output to allow for UTF-8? We need to clarify this. Suggestions ? The platform specification would be the right place to require 8-bit support for the console. Do we make any assumptions about encoding? Same as above, this needs more clarification. Suggestions ? We should add to the platform specification that UTF-8 output is assumed on the serial console. I am of the opinion to keep such encoding related assumptions to be minimal. How would we handle a console set up to 7bit + parity if a character > 0x7f is sent? I would consider this to be part of the clarification we add for encoding. We should state if extra bits are ignored or the bytes are not send. The easiest thing is to just ignore the extra bits. So let't state this here. Best regards Heinrich Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter %s/shard/shared/ Okay, I will update. is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. There could be other reasons of failures: * set up of the UART failed in OpenSBI * no UART defined in the device-tree * ... So let us add SBI_ERR_FAILED to the list. Okay, I will add. Best regards Heinrich Regards, Anup
More All Messages By This Member
Anup Patel #1733 On Thu, Jun 2, 2022 at 8:30 PM Ved Shanbhogue <ved@...> wrote: On Thu, Jun 02, 2022 at 07:52:55PM +0530, Anup Patel wrote: On Thu, Jun 2, 2022 at 6:29 PM Ved Shanbhogue <ved@...> wrote: This is very slow for virtualized world particularly KVM RISC-V because each SBI v0.1 putchar() or getchar() will trap to KVM RISC-V and KVM RISC-V will forward it to user-space QEMU or KVMTOOL. This means each early print character using SBI v0.1 putchar() will go all the way to host user-space and come back. This is horribly slow for KVM Guest. This becomes further slower for nested virtualization. Since this is for debug and really early phase debug till enough of the guest boots up to use a VFIO based char driver provided by the VMM, I am not sure that the slowness matters. Even this SBI call I expected the VMM to intercept and if the VMM has all emulation in the user space VMM - e.g. the console tty is open by the user space VMM, I am not sure this would avoid that trip to user space. If the motivation is primarily VM debug then perhaps a standardized set of hypercalls implemented by KVM makes more sense than SBI calls that would need to be built into the M-mode firmware? We have a requirement in the OS-A platform spec to mandate a particular type of UART for early prints but there were objections on selecting one type of UART over another. This SBI debug console extension is also a way to avoid standardizing a particular type of UART in OS-A platform spec. I worry about bugs/security issues that can be caused by M-mode firmware accessing strings in untrusted memory. The VirtIO based para-virt devices rely heavily on shared memory so I think it is possible to address security concerns related to shared memory. Yes, and now that I understand the motivation better why dont we define this as a hypercall/pv-ops interface to a VMM than a SBI call to the M-mode firmware and needing to build a virt-io like framework in firmware. The VirtIO framework has rings in shared memory but over here it is just shared memory without any formatted data structure. The API as defined does not say whether the address is a virtual address or a physical address. It is a physical address. I will clarify this in Draft v2. Thanks. I was not sure since we had all the discussion about Svpbmt. Based on discussion it did not seem like it needs to be much fancier than this as this is for early OS/VMM code till it has enough functionality to directly interact with a uart. The goal of the shared memory based SBI call for early prints is to minimize the number of traps which in-turn helps virtualization to drastically reduce boot-time. Understand that better now. But if that is the main motivation then I am not understanding why we would want to push all of this into M-mode firmware vs. defining a set of standardized pv-ops to be used by guest OSes. The SBI debug console also provides a standard way of early prints for supervisor software running natively (directly under M-mode) irrespective of the type of UART/Console available on the console so it's not just for a virtualized world. Although, the virtualized world has an added performance advantage due to reduced traps. Regards, Anup
More All Messages By This Member
Ved Shanbhogue #1729 On Thu, Jun 02, 2022 at 07:52:55PM +0530, Anup Patel wrote: On Thu, Jun 2, 2022 at 6:29 PM Ved Shanbhogue <ved@...> wrote: This is very slow for virtualized world particularly KVM RISC-V because each SBI v0.1 putchar() or getchar() will trap to KVM RISC-V and KVM RISC-V will forward it to user-space QEMU or KVMTOOL. This means each early print character using SBI v0.1 putchar() will go all the way to host user-space and come back. This is horribly slow for KVM Guest. This becomes further slower for nested virtualization. Since this is for debug and really early phase debug till enough of the guest boots up to use a VFIO based char driver provided by the VMM, I am not sure that the slowness matters. Even this SBI call I expected the VMM to intercept and if the VMM has all emulation in the user space VMM - e.g. the console tty is open by the user space VMM, I am not sure this would avoid that trip to user space. If the motivation is primarily VM debug then perhaps a standardized set of hypercalls implemented by KVM makes more sense than SBI calls that would need to be built into the M-mode firmware? I worry about bugs/security issues that can be caused by M-mode firmware accessing strings in untrusted memory. The VirtIO based para-virt devices rely heavily on shared memory so I think it is possible to address security concerns related to shared memory. Yes, and now that I understand the motivation better why dont we define this as a hypercall/pv-ops interface to a VMM than a SBI call to the M-mode firmware and needing to build a virt-io like framework in firmware. The API as defined does not say whether the address is a virtual address or a physical address. It is a physical address. I will clarify this in Draft v2. Thanks. I was not sure since we had all the discussion about Svpbmt. Based on discussion it did not seem like it needs to be much fancier than this as this is for early OS/VMM code till it has enough functionality to directly interact with a uart. The goal of the shared memory based SBI call for early prints is to minimize the number of traps which in-turn helps virtualization to drastically reduce boot-time. Understand that better now. But if that is the main motivation then I am not understanding why we would want to push all of this into M-mode firmware vs. defining a set of standardized pv-ops to be used by guest OSes. regards ved
More All Messages By This Member
Anup Patel #1726 On Thu, Jun 2, 2022 at 6:29 PM Ved Shanbhogue <ved@...> wrote: Should we keep this simple in the SBI - only have register based inputs - to send and receive 1 byte in each call? Keeping it a simple out_byte or in_byte - a serial port like interface seems the simplest and most secure. The legacy SBI v0.1 putchar() and getchar() are byte-level send/receive calls. This is very slow for virtualized world particularly KVM RISC-V because each SBI v0.1 putchar() or getchar() will trap to KVM RISC-V and KVM RISC-V will forward it to user-space QEMU or KVMTOOL. This means each early print character using SBI v0.1 putchar() will go all the way to host user-space and come back. This is horribly slow for KVM Guest. This becomes further slower for nested virtualization. The MMIO based early prints are further worse because we have at least two MMIO traps for every character where each MMIO exits to host user-space. The shared memory based SBI puts() drastically reduces the number of SBI traps hence reducing boot time with early prints enabled. I worry about bugs/security issues that can be caused by M-mode firmware accessing strings in untrusted memory. The VirtIO based para-virt devices rely heavily on shared memory so I think it is possible to address security concerns related to shared memory. The API as defined does not say whether the address is a virtual address or a physical address. It is a physical address. I will clarify this in Draft v2. If it is a virtual address then the SBI call will need to use a MPRV load/store to gather this data and will also need to deal with page fault, access faults, etc. that may occur on such accesses. Yes, MPRV (or HLV/HSV) based load/store have performance issues particularly due to page faults. These are prevalent in some of the legacy SBI v0.1 calls. With SBI v0.2 (or higher), we have tried to ensure that we don't use virtual addresses as parameter in newer SBI calls. Based on discussion it did not seem like it needs to be much fancier than this as this is for early OS/VMM code till it has enough functionality to directly interact with a uart. The goal of the shared memory based SBI call for early prints is to minimize the number of traps which in-turn helps virtualization to drastically reduce boot-time. Regards, Anup regards ved On Thu, Jun 2, 2022 at 7:43 AM Anup Patel <apatel@...> wrote: On Thu, Jun 2, 2022 at 2:58 PM Heiko Stübner <heiko@...> wrote: Am Donnerstag, 2. Juni 2022, 10:50:56 CEST schrieb Heiko Stübner: Am Donnerstag, 2. Juni 2022, 10:47:58 CEST schrieb Anup Patel: On Thu, Jun 2, 2022 at 12:02 AM Heiko Stübner <heiko@...> wrote: Hi Anup, Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) Set the shared memory area specified by `addr_div_by_2` and `size` typo in the "div_by_2" (not 4 like below and in the function itself) ? parameters. The `addr_div_by_4` parameter is base address of the shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. This will vastly reduce the number of needed ecalls when outputting characters, so this will probably improve performance quite a bit :-) I guess I still would like to have an _additional_ single-character putc call. As mentioned in the other thread [0], especially on consumer hardware [where there is no elaborate debug infrastructure] this can be a very handy debugging tool even in the earliest stages of a booting kernel (both before relocation and even inside the startup assembly). I.e. just doing a li a7, 1 li a6, 0 li a0, 36 ecall in any kernel assembly will just output a "$" character right now, without needing any preparation at all - same with using the current sbi_console_putchar() directly in c-code. This _can_ be very helpful in some cases, so I guess it would be nice to keep such a functionality around also in the new spec. You can easily create multiple pre-populated strings using ".asciiz" in assembly sources. Just set the base address of pre-populated strings once on boot hart and print from anywhere using usual 4-5 instruction (similar to what posted above). ok, sounds like a plan as well :-) though, how does that relate to the time before MMU setup? I.e. in response to Heinrich's mail you talk about svpbmt, so I guess you expect virtual memory there, so what is the expected value-type before the mmu is setup in S-mode ? The memory type should be 0 (i.e. PMA) for the shared memory between SBI implementation and supervisor software. Before MMU setup, the memory type is by default 0 (i.e. PMA) so we don't have to mandate any memory type for MMU disabled case. We only have issue on systems with Svpmbt where supervisor software can potentially map the shared memory as non-cacheable or IO (memory type != 0) using PTE memory type bits. In addition to above, a SBI implementation must ensure that the shared memory address provided by supervisor software is a regular memory (not MMIO device). This can be easily achieved in OpenSBI, KVM RISC-V, and various hypervisors. Regards, Anup [0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html Regards, Anup
More All Messages By This Member
洛佳 Luo Jia #1724 Edited Hello! Glad to see a new proposal to the SBI standard. Besides to discussion before, I may (if possible or proper) suggest an idea of not implementing this extension, for SBI should provide transparency of peripherals to kernel other than wrap every possible drivers in SBI itself. Am I totally wrong, or are there any further ideas to this topic? Thanks :) Best Regards, Luo Jia
More All Messages By This Member
Anup Patel #1723 On Thu, Jun 2, 2022 at 2:43 PM Heinrich Schuchardt <heinrich.schuchardt@...> wrote: On 6/2/22 10:44, Anup Patel wrote: On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt <heinrich.schuchardt@...> wrote: On 6/1/22 18:17, Anup Patel wrote: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: Thanks for starting to close this gap. 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. I miss a discussion of the conflicts that can arise if the configuration of the serial console is changed by the caller. Do we need an ecall that closes the SBI console to further access? Usually, the serial port related code in M-mode firmware only uses status and data registers so for most serial ports support the M-mode firmware will adapt to serial port configuration changes. In fact, this is why we never had a special SBI call for serial port reconfiguration in legacy SBI v0.1 as well. In case of virtualization, the serial port (or console) is emulated so the special SBI call is not useful for virtualization. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Isn't it M-mode software that has to program the MMU to allow all harts in M-mode and S-mode access to the memory area? What is the S-mode software to do about the memory area prior to calling sbi_debug_console_set_area()? Actually, it's the S-mode software which is voluntarily giving a portion of its memory to be used as shared memory. The proposal only mandates that whatever memory is selected by S-mode software should be a regular cacheable memory (not IO memory). Also, if Svpbmt is available then S-mode software should only use memory type 0 in the PTEs. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) The console output is needed on the very start of the S-mode software, before setting up anything. Can we avoid this extra function? Can we simply assume that the caller of sbi_debug_console_puts() provides a physical address pointer to a memory area that is suitable? Theoretically, we can avoid the extra function to set shared memory area but it will complicate things in future when we have supervisor software encrypting it's own memory (using special ISA support) because in this case supervisor software will have to unprotect memory every time the sbi_debug_console_puts() is called and protect it again after the call. Currently this function is just a nop(). It is not needed in this revision of the extension. The function might be called repeatedly by different threads with different values. How do you want to keep track of all of these different areas? The shared memory area in case of this SBI extension will be shared across all HARTs so the SBI implementation will ensure atomicity in setting/reading shared memory coordinates. This way multiple HARTs can call the sbi_debug_console_set_area() but only the last call will be in effect. Following text in the proposal explains above: "The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time." Maybe this text is not giving a clear picture ? Memory shared between different security realms will arise in many different scenarios. As this is not console specific it should be in a separate extension. That extension should be defined once we have clarity about how security realms are managed. In the case of this extension, the shared memory is global across all HARTs and it mostly contains bytes to be printed. In case of steal time accounting, the shared memory is separate for each HART and it is a well-defined data structure. Clearly, the usage of shared memory is extension specific. The VirtIO devices are a good (and time tested) example of shared memory based approaches. Over there as well, the shared memory (i.e. various VirtIO rings) are setup by Guest and there is no central pool of shared memory (i.e no dedicated VirtIO device managing shared memory). Similar to VirtIO world, we should let SBI extension define its own shared memory usage and API. Regards, Anup Set the shared memory area specified by `addr_div_by_2` and `size` %s/addr_div_by_2/addr_div_by_4/ Okay, I will update. parameters. The `addr_div_by_4` parameter is base address of the %s/is base/is the base/ Okay, I will update. shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. Why shifting the address? I would prefer to keep it simple for the caller. If the alignment is not suitable, return an error. But why is an alignment needed here at all? And why 4 aligned? For RV32 S-mode, the physical address space is 34bits wide but "unsigned long" is 32bits wide. This is because Sv32 PTEs allow 34bits of PPN. In fact, even instructions such as HFENCE.GVMA have this "address right shift by 2" requirement based on this rationale. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) I would prefer to simply pass a physical address pointer here with no requirements on alignment. And no prior SBI call. sbi_debug_console_set_area() might be called with different values by different threads. An offset is ambiguous as it does not define to which of the different shared areas it relates. Please, use a pointer. Do we need num_chars? Are we expecting to provide binary output? Using 0x00 as terminator would be adequate in most cases. Bare-metal tests (or assembly sources) can print sub-strings from a large per-populated string in shared memory. Assuming that string is always terminated by 0x00 in sbi_debug_console_puts() will break this flexibility. OK What is the requirement on the console? Does it have to support 8bit output to allow for UTF-8? We need to clarify this. Suggestions ? The platform specification would be the right place to require 8-bit support for the console. Do we make any assumptions about encoding? Same as above, this needs more clarification. Suggestions ? We should add to the platform specification that UTF-8 output is assumed on the serial console. I am of the opinion to keep such encoding related assumptions to be minimal. How would we handle a console set up to 7bit + parity if a character > 0x7f is sent? I would consider this to be part of the clarification we add for encoding. We should state if extra bits are ignored or the bytes are not send. The easiest thing is to just ignore the extra bits. So let't state this here. Best regards Heinrich Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter %s/shard/shared/ Okay, I will update. is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. There could be other reasons of failures: * set up of the UART failed in OpenSBI * no UART defined in the device-tree * ... So let us add SBI_ERR_FAILED to the list. Okay, I will add. Best regards Heinrich Regards, Anup
More All Messages By This Member
Ved Shanbhogue #1722 Should we keep this simple in the SBI - only have register based inputs - to send and receive 1 byte in each call? Keeping it a simple out_byte or in_byte - a serial port like interface seems the simplest and most secure. I worry about bugs/security issues that can be caused by M-mode firmware accessing strings in untrusted memory. The API as defined does not say whether the address is a virtual address or a physical address. If it is a virtual address then the SBI call will need to use a MPRV load/store to gather this data and will also need to deal with page fault, access faults, etc. that may occur on such accesses. Based on discussion it did not seem like it needs to be much fancier than this as this is for early OS/VMM code till it has enough functionality to directly interact with a uart. regards ved toggle quoted message Show quoted text On Thu, Jun 2, 2022 at 7:43 AM Anup Patel <apatel@...> wrote: On Thu, Jun 2, 2022 at 2:58 PM Heiko Stübner <heiko@...> wrote: > > Am Donnerstag, 2. Juni 2022, 10:50:56 CEST schrieb Heiko Stübner: > > Am Donnerstag, 2. Juni 2022, 10:47:58 CEST schrieb Anup Patel: > > > On Thu, Jun 2, 2022 at 12:02 AM Heiko Stübner <heiko@...> wrote: > > > > > > > > Hi Anup, > > > > > > > > Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel: > > > > > Hi All, > > > > > > > > > > Below is the draft proposal for SBI Debug Console Extension. > > > > > > > > > > Please review it and provide feedback. > > > > > > > > > > Thanks, > > > > > Anup > > > > > > > > > > Debug Console Extension (EID #0x4442434E "DBCN") > > > > > ================================================ > > > > > > > > > > The debug console extension defines a generic mechanism for boot-time > > > > > early prints from supervisor-mode software which allows users to catch > > > > > boot-time issues in supervisor-mode software. > > > > > > > > > > This extension replaces legacy console putchar (EID #0x01) extension > > > > > and it is better in following ways: > > > > > 1) It follows the new calling convention defined for SBI v1.0 > > > > > (or higher). > > > > > 2) It is based on a shared memory area between SBI implementation > > > > > and supervisor-mode software so multiple characters can be > > > > > printed using a single SBI call. > > > > > > > > > > The supervisor-mode software must set the shared memory area before > > > > > printing characters on the debug console. Also, all HARTs share the > > > > > same shared memory area so only one HART needs to set it at boot-time. > > > > > > > > > > Function: Set Console Area (FID #0) > > > > > ----------------------------------- > > > > > > > > > > struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, > > > > > unsigned long size) > > > > > > > > > > Set the shared memory area specified by `addr_div_by_2` and `size` > > > > > > > > typo in the "div_by_2" (not 4 like below and in the function itself) ? > > > > > > > > > > > > > parameters. The `addr_div_by_4` parameter is base address of the > > > > > shared memory area right shifted by 2 whereas `size` parameter is > > > > > the size of shared memory area in bytes. > > > > > > > > > > The shared memory area should be normal cacheable memory for the > > > > > supervisor-mode software. Also, the shared memory area is global > > > > > across all HARTs so SBI implementation must ensure atomicity in > > > > > setting the shared memory area. > > > > > > > > > > Errors: > > > > > SBI_SUCCESS - Shared memory area set successfully. > > > > > SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by > > > > > `addr_div_by_2` and `size` parameters > > > > > is not normal cacheable memory or not > > > > > accessible to supervisor-mode software. > > > > > > > > > > Function: Console Puts (FID #1) > > > > > ------------------------------- > > > > > > > > > > struct sbiret sbi_debug_console_puts(unsigned long area_offset, > > > > > unsigned long num_chars) > > > > > > > > > > Print the string specified by `area_offset` and `num_chars` on > > > > > the debug console. The `area_offset` parameter is the start of > > > > > string in the shard memory area whereas `num_chars` parameter > > > > > is the number of characters (or bytes) in the string. > > > > > > > > > > This is a blocking SBI call and will only return after printing > > > > > all characters of the string. > > > > > > > > > > Errors: > > > > > SBI_SUCCESS - Characters printed successfully. > > > > > SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. > > > > > `area_offset`) or end of the string > > > > > (i.e. `area_offset + num_chars`) is > > > > > outside shared memory area. > > > > > > > > This will vastly reduce the number of needed ecalls when outputting > > > > characters, so this will probably improve performance quite a bit :-) > > > > > > > > > > > > I guess I still would like to have an _additional_ single-character > > > > putc call. As mentioned in the other thread [0], especially on consumer > > > > hardware [where there is no elaborate debug infrastructure] this can > > > > be a very handy debugging tool even in the earliest stages of a > > > > booting kernel (both before relocation and even inside the startup > > > > assembly). > > > > > > > > I.e. just doing a > > > > li a7, 1 > > > > li a6, 0 > > > > li a0, 36 > > > > ecall > > > > > > > > in any kernel assembly will just output a "$" character right now, without > > > > needing any preparation at all - same with using the current > > > > sbi_console_putchar() directly in c-code. > > > > > > > > This _can_ be very helpful in some cases, so I guess it would be nice > > > > to keep such a functionality around also in the new spec. > > > > > > You can easily create multiple pre-populated strings using ".asciiz" in > > > assembly sources. Just set the base address of pre-populated strings > > > once on boot hart and print from anywhere using usual 4-5 instruction > > > (similar to what posted above). > > > > ok, sounds like a plan as well :-) > > though, how does that relate to the time before MMU setup? > > I.e. in response to Heinrich's mail you talk about svpbmt, so I guess you > expect virtual memory there, so what is the expected value-type before > the mmu is setup in S-mode ? The memory type should be 0 (i.e. PMA) for the shared memory between SBI implementation and supervisor software. Before MMU setup, the memory type is by default 0 (i.e. PMA) so we don't have to mandate any memory type for MMU disabled case. We only have issue on systems with Svpmbt where supervisor software can potentially map the shared memory as non-cacheable or IO (memory type != 0) using PTE memory type bits. In addition to above, a SBI implementation must ensure that the shared memory address provided by supervisor software is a regular memory (not MMIO device). This can be easily achieved in OpenSBI, KVM RISC-V, and various hypervisors. Regards, Anup > > > > > > [0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html > > > > > > > > > > > > > > > > > > Regards, > > > Anup > > > > > > > > > > >
More All Messages By This Member
Anup Patel #1721 On Thu, Jun 2, 2022 at 2:58 PM Heiko Stübner <heiko@...> wrote: Am Donnerstag, 2. Juni 2022, 10:50:56 CEST schrieb Heiko Stübner: Am Donnerstag, 2. Juni 2022, 10:47:58 CEST schrieb Anup Patel: On Thu, Jun 2, 2022 at 12:02 AM Heiko Stübner <heiko@...> wrote: Hi Anup, Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) Set the shared memory area specified by `addr_div_by_2` and `size` typo in the "div_by_2" (not 4 like below and in the function itself) ? parameters. The `addr_div_by_4` parameter is base address of the shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. This will vastly reduce the number of needed ecalls when outputting characters, so this will probably improve performance quite a bit :-) I guess I still would like to have an _additional_ single-character putc call. As mentioned in the other thread [0], especially on consumer hardware [where there is no elaborate debug infrastructure] this can be a very handy debugging tool even in the earliest stages of a booting kernel (both before relocation and even inside the startup assembly). I.e. just doing a li a7, 1 li a6, 0 li a0, 36 ecall in any kernel assembly will just output a "$" character right now, without needing any preparation at all - same with using the current sbi_console_putchar() directly in c-code. This _can_ be very helpful in some cases, so I guess it would be nice to keep such a functionality around also in the new spec. You can easily create multiple pre-populated strings using ".asciiz" in assembly sources. Just set the base address of pre-populated strings once on boot hart and print from anywhere using usual 4-5 instruction (similar to what posted above). ok, sounds like a plan as well :-) though, how does that relate to the time before MMU setup? I.e. in response to Heinrich's mail you talk about svpbmt, so I guess you expect virtual memory there, so what is the expected value-type before the mmu is setup in S-mode ? The memory type should be 0 (i.e. PMA) for the shared memory between SBI implementation and supervisor software. Before MMU setup, the memory type is by default 0 (i.e. PMA) so we don't have to mandate any memory type for MMU disabled case. We only have issue on systems with Svpmbt where supervisor software can potentially map the shared memory as non-cacheable or IO (memory type != 0) using PTE memory type bits. In addition to above, a SBI implementation must ensure that the shared memory address provided by supervisor software is a regular memory (not MMIO device). This can be easily achieved in OpenSBI, KVM RISC-V, and various hypervisors. Regards, Anup [0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html Regards, Anup
More All Messages By This Member
Anup Patel #1720 On Thu, Jun 2, 2022 at 10:08 AM Xiang W <wxjstz@...> wrote: 在 2022-06-01星期三的 21:47 +0530，Anup Patel写道： Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) Set the shared memory area specified by `addr_div_by_2` and `size` parameters. The `addr_div_by_4` parameter is base address of the shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Shared memory can be a single extension. The three interfaces are as follows /* The supervisor hands a piece of physical memory to sbi for shared memory / struct sbiret sbi_shared_memory_extrend(unsigned long addr, unsigned long size); / The supervisor applies for a piece of physical memory from sbi / struct sbiret sbi_shared_memory_alloc(unsigned long size); / The supervisor notifies sbi to release the requested memory */ struct sbiret sbi_shared_memory_free(unsigned long addr); Clearly, if we have a separate SBI extension to manage shared memory then we will end-up with such memory management calls. Memory allocators are generally hard to get it right and this also adds lot of bookkeeping and state management in SBI implementations (e.g. OpenSBI, KVM RISC-V, and various hypervisors). Further, some of the SBI extensions (such as Steal Time Accounting) will have separate shared memory for each HART whereas some (such as Debug Console) will have global shared memory for all HARTs. For clean and modular SBI implementations, I would recommend that each SBI extension define its own shared memory setup API. Regards, Anup Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area.
More All Messages By This Member
Heiko Stuebner <heiko@...> #1718 Am Donnerstag, 2. Juni 2022, 10:50:56 CEST schrieb Heiko Stübner: Am Donnerstag, 2. Juni 2022, 10:47:58 CEST schrieb Anup Patel: On Thu, Jun 2, 2022 at 12:02 AM Heiko Stübner <heiko@...> wrote: Hi Anup, Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) Set the shared memory area specified by `addr_div_by_2` and `size` typo in the "div_by_2" (not 4 like below and in the function itself) ? parameters. The `addr_div_by_4` parameter is base address of the shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. This will vastly reduce the number of needed ecalls when outputting characters, so this will probably improve performance quite a bit :-) I guess I still would like to have an _additional_ single-character putc call. As mentioned in the other thread [0], especially on consumer hardware [where there is no elaborate debug infrastructure] this can be a very handy debugging tool even in the earliest stages of a booting kernel (both before relocation and even inside the startup assembly). I.e. just doing a li a7, 1 li a6, 0 li a0, 36 ecall in any kernel assembly will just output a "$" character right now, without needing any preparation at all - same with using the current sbi_console_putchar() directly in c-code. This _can_ be very helpful in some cases, so I guess it would be nice to keep such a functionality around also in the new spec. You can easily create multiple pre-populated strings using ".asciiz" in assembly sources. Just set the base address of pre-populated strings once on boot hart and print from anywhere using usual 4-5 instruction (similar to what posted above). ok, sounds like a plan as well :-) though, how does that relate to the time before MMU setup? I.e. in response to Heinrich's mail you talk about svpbmt, so I guess you expect virtual memory there, so what is the expected value-type before the mmu is setup in S-mode ? [0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html Regards, Anup
More All Messages By This Member
Heinrich Schuchardt #1717 On 6/2/22 10:44, Anup Patel wrote: On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt <heinrich.schuchardt@...> wrote: On 6/1/22 18:17, Anup Patel wrote: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: Thanks for starting to close this gap. 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. I miss a discussion of the conflicts that can arise if the configuration of the serial console is changed by the caller. Do we need an ecall that closes the SBI console to further access? Usually, the serial port related code in M-mode firmware only uses status and data registers so for most serial ports support the M-mode firmware will adapt to serial port configuration changes. In fact, this is why we never had a special SBI call for serial port reconfiguration in legacy SBI v0.1 as well. In case of virtualization, the serial port (or console) is emulated so the special SBI call is not useful for virtualization. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Isn't it M-mode software that has to program the MMU to allow all harts in M-mode and S-mode access to the memory area? What is the S-mode software to do about the memory area prior to calling sbi_debug_console_set_area()? Actually, it's the S-mode software which is voluntarily giving a portion of its memory to be used as shared memory. The proposal only mandates that whatever memory is selected by S-mode software should be a regular cacheable memory (not IO memory). Also, if Svpbmt is available then S-mode software should only use memory type 0 in the PTEs. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) The console output is needed on the very start of the S-mode software, before setting up anything. Can we avoid this extra function? Can we simply assume that the caller of sbi_debug_console_puts() provides a physical address pointer to a memory area that is suitable? Theoretically, we can avoid the extra function to set shared memory area but it will complicate things in future when we have supervisor software encrypting it's own memory (using special ISA support) because in this case supervisor software will have to unprotect memory every time the sbi_debug_console_puts() is called and protect it again after the call. Currently this function is just a nop(). It is not needed in this revision of the extension. The function might be called repeatedly by different threads with different values. How do you want to keep track of all of these different areas? Memory shared between different security realms will arise in many different scenarios. As this is not console specific it should be in a separate extension. That extension should be defined once we have clarity about how security realms are managed. Set the shared memory area specified by `addr_div_by_2` and `size` %s/addr_div_by_2/addr_div_by_4/ Okay, I will update. parameters. The `addr_div_by_4` parameter is base address of the %s/is base/is the base/ Okay, I will update. shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. Why shifting the address? I would prefer to keep it simple for the caller. If the alignment is not suitable, return an error. But why is an alignment needed here at all? And why 4 aligned? For RV32 S-mode, the physical address space is 34bits wide but "unsigned long" is 32bits wide. This is because Sv32 PTEs allow 34bits of PPN. In fact, even instructions such as HFENCE.GVMA have this "address right shift by 2" requirement based on this rationale. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) I would prefer to simply pass a physical address pointer here with no requirements on alignment. And no prior SBI call. sbi_debug_console_set_area() might be called with different values by different threads. An offset is ambiguous as it does not define to which of the different shared areas it relates. Please, use a pointer. Do we need num_chars? Are we expecting to provide binary output? Using 0x00 as terminator would be adequate in most cases. Bare-metal tests (or assembly sources) can print sub-strings from a large per-populated string in shared memory. Assuming that string is always terminated by 0x00 in sbi_debug_console_puts() will break this flexibility. OK What is the requirement on the console? Does it have to support 8bit output to allow for UTF-8? We need to clarify this. Suggestions ? The platform specification would be the right place to require 8-bit support for the console. Do we make any assumptions about encoding? Same as above, this needs more clarification. Suggestions ? We should add to the platform specification that UTF-8 output is assumed on the serial console. I am of the opinion to keep such encoding related assumptions to be minimal. How would we handle a console set up to 7bit + parity if a character > 0x7f is sent? I would consider this to be part of the clarification we add for encoding. We should state if extra bits are ignored or the bytes are not send. The easiest thing is to just ignore the extra bits. So let't state this here. Best regards Heinrich Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter %s/shard/shared/ Okay, I will update. is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. There could be other reasons of failures: * set up of the UART failed in OpenSBI * no UART defined in the device-tree * ... So let us add SBI_ERR_FAILED to the list. Okay, I will add. Best regards Heinrich Regards, Anup
More All Messages By This Member
Heiko Stuebner <heiko@...> #1716 Am Donnerstag, 2. Juni 2022, 10:47:58 CEST schrieb Anup Patel: On Thu, Jun 2, 2022 at 12:02 AM Heiko Stübner <heiko@...> wrote: Hi Anup, Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) Set the shared memory area specified by `addr_div_by_2` and `size` typo in the "div_by_2" (not 4 like below and in the function itself) ? parameters. The `addr_div_by_4` parameter is base address of the shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. This will vastly reduce the number of needed ecalls when outputting characters, so this will probably improve performance quite a bit :-) I guess I still would like to have an _additional_ single-character putc call. As mentioned in the other thread [0], especially on consumer hardware [where there is no elaborate debug infrastructure] this can be a very handy debugging tool even in the earliest stages of a booting kernel (both before relocation and even inside the startup assembly). I.e. just doing a li a7, 1 li a6, 0 li a0, 36 ecall in any kernel assembly will just output a "$" character right now, without needing any preparation at all - same with using the current sbi_console_putchar() directly in c-code. This _can_ be very helpful in some cases, so I guess it would be nice to keep such a functionality around also in the new spec. You can easily create multiple pre-populated strings using ".asciiz" in assembly sources. Just set the base address of pre-populated strings once on boot hart and print from anywhere using usual 4-5 instruction (similar to what posted above). ok, sounds like a plan as well :-) [0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html Regards, Anup
More All Messages By This Member
Anup Patel #1715 On Thu, Jun 2, 2022 at 9:14 AM Xiang W <wxjstz@...> wrote: 在 2022-06-01星期三的 21:47 +0530，Anup Patel写道： Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup The use of these two APIs is too complicated, and the supervisor-mode software needs to create a data structure to manage the Console Area. I recommend using the simpler interface: struct sbiret sbi_debug_console_puts(unsigned long addr, unsigned long num_chars) Heinrich has a similar suggestion so please see my response to his comment. Regards, Anup Regards, Xiang W Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) Set the shared memory area specified by `addr_div_by_2` and `size` parameters. The `addr_div_by_4` parameter is base address of the shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. -- opensbi mailing list opensbi@... http://lists.infradead.org/mailman/listinfo/opensbi
More All Messages By This Member
Anup Patel #1714 On Thu, Jun 2, 2022 at 12:02 AM Heiko Stübner <heiko@...> wrote: Hi Anup, Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) Set the shared memory area specified by `addr_div_by_2` and `size` typo in the "div_by_2" (not 4 like below and in the function itself) ? parameters. The `addr_div_by_4` parameter is base address of the shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. This will vastly reduce the number of needed ecalls when outputting characters, so this will probably improve performance quite a bit :-) I guess I still would like to have an _additional_ single-character putc call. As mentioned in the other thread [0], especially on consumer hardware [where there is no elaborate debug infrastructure] this can be a very handy debugging tool even in the earliest stages of a booting kernel (both before relocation and even inside the startup assembly). I.e. just doing a li a7, 1 li a6, 0 li a0, 36 ecall in any kernel assembly will just output a "$" character right now, without needing any preparation at all - same with using the current sbi_console_putchar() directly in c-code. This _can_ be very helpful in some cases, so I guess it would be nice to keep such a functionality around also in the new spec. You can easily create multiple pre-populated strings using ".asciiz" in assembly sources. Just set the base address of pre-populated strings once on boot hart and print from anywhere using usual 4-5 instruction (similar to what posted above). Thanks Heiko [0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html Regards, Anup
More All Messages By This Member
Anup Patel #1713 On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt <heinrich.schuchardt@...> wrote: On 6/1/22 18:17, Anup Patel wrote: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: Thanks for starting to close this gap. 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. I miss a discussion of the conflicts that can arise if the configuration of the serial console is changed by the caller. Do we need an ecall that closes the SBI console to further access? Usually, the serial port related code in M-mode firmware only uses status and data registers so for most serial ports support the M-mode firmware will adapt to serial port configuration changes. In fact, this is why we never had a special SBI call for serial port reconfiguration in legacy SBI v0.1 as well. In case of virtualization, the serial port (or console) is emulated so the special SBI call is not useful for virtualization. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Isn't it M-mode software that has to program the MMU to allow all harts in M-mode and S-mode access to the memory area? What is the S-mode software to do about the memory area prior to calling sbi_debug_console_set_area()? Actually, it's the S-mode software which is voluntarily giving a portion of its memory to be used as shared memory. The proposal only mandates that whatever memory is selected by S-mode software should be a regular cacheable memory (not IO memory). Also, if Svpbmt is available then S-mode software should only use memory type 0 in the PTEs. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) The console output is needed on the very start of the S-mode software, before setting up anything. Can we avoid this extra function? Can we simply assume that the caller of sbi_debug_console_puts() provides a physical address pointer to a memory area that is suitable? Theoretically, we can avoid the extra function to set shared memory area but it will complicate things in future when we have supervisor software encrypting it's own memory (using special ISA support) because in this case supervisor software will have to unprotect memory every time the sbi_debug_console_puts() is called and protect it again after the call. Set the shared memory area specified by `addr_div_by_2` and `size` %s/addr_div_by_2/addr_div_by_4/ Okay, I will update. parameters. The `addr_div_by_4` parameter is base address of the %s/is base/is the base/ Okay, I will update. shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. Why shifting the address? I would prefer to keep it simple for the caller. If the alignment is not suitable, return an error. But why is an alignment needed here at all? And why 4 aligned? For RV32 S-mode, the physical address space is 34bits wide but "unsigned long" is 32bits wide. This is because Sv32 PTEs allow 34bits of PPN. In fact, even instructions such as HFENCE.GVMA have this "address right shift by 2" requirement based on this rationale. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) I would prefer to simply pass a physical address pointer here with no requirements on alignment. And no prior SBI call. Do we need num_chars? Are we expecting to provide binary output? Using 0x00 as terminator would be adequate in most cases. Bare-metal tests (or assembly sources) can print sub-strings from a large per-populated string in shared memory. Assuming that string is always terminated by 0x00 in sbi_debug_console_puts() will break this flexibility. What is the requirement on the console? Does it have to support 8bit output to allow for UTF-8? We need to clarify this. Suggestions ? Do we make any assumptions about encoding? Same as above, this needs more clarification. Suggestions ? I am of the opinion to keep such encoding related assumptions to be minimal. How would we handle a console set up to 7bit + parity if a character > 0x7f is sent? I would consider this to be part of the clarification we add for encoding. Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter %s/shard/shared/ Okay, I will update. is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. There could be other reasons of failures: * set up of the UART failed in OpenSBI * no UART defined in the device-tree * ... So let us add SBI_ERR_FAILED to the list. Okay, I will add. Best regards Heinrich Regards, Anup
More All Messages By This Member
Heiko Stuebner <heiko@...> #1711 Hi Anup, Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) Set the shared memory area specified by `addr_div_by_2` and `size` typo in the "div_by_2" (not 4 like below and in the function itself) ? parameters. The `addr_div_by_4` parameter is base address of the shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. This will vastly reduce the number of needed ecalls when outputting characters, so this will probably improve performance quite a bit :-) I guess I still would like to have an _additional_ single-character putc call. As mentioned in the other thread [0], especially on consumer hardware [where there is no elaborate debug infrastructure] this can be a very handy debugging tool even in the earliest stages of a booting kernel (both before relocation and even inside the startup assembly). I.e. just doing a li a7, 1 li a6, 0 li a0, 36 ecall in any kernel assembly will just output a "$" character right now, without needing any preparation at all - same with using the current sbi_console_putchar() directly in c-code. This _can_ be very helpful in some cases, so I guess it would be nice to keep such a functionality around also in the new spec. Thanks Heiko [0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html
More All Messages By This Member
Heinrich Schuchardt #1710 On 6/1/22 18:17, Anup Patel wrote: Hi All, Below is the draft proposal for SBI Debug Console Extension. Please review it and provide feedback. Thanks, Anup Debug Console Extension (EID #0x4442434E "DBCN") ================================================ The debug console extension defines a generic mechanism for boot-time early prints from supervisor-mode software which allows users to catch boot-time issues in supervisor-mode software. This extension replaces legacy console putchar (EID #0x01) extension and it is better in following ways: Thanks for starting to close this gap. 1) It follows the new calling convention defined for SBI v1.0 (or higher). 2) It is based on a shared memory area between SBI implementation and supervisor-mode software so multiple characters can be printed using a single SBI call. I miss a discussion of the conflicts that can arise if the configuration of the serial console is changed by the caller. Do we need an ecall that closes the SBI console to further access? The supervisor-mode software must set the shared memory area before printing characters on the debug console. Also, all HARTs share the same shared memory area so only one HART needs to set it at boot-time. Isn't it M-mode software that has to program the MMU to allow all harts in M-mode and S-mode access to the memory area? What is the S-mode software to do about the memory area prior to calling sbi_debug_console_set_area()? Function: Set Console Area (FID #0) ----------------------------------- struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4, unsigned long size) The console output is needed on the very start of the S-mode software, before setting up anything. Can we avoid this extra function? Can we simply assume that the caller of sbi_debug_console_puts() provides a physical address pointer to a memory area that is suitable? Set the shared memory area specified by `addr_div_by_2` and `size` %s/addr_div_by_2/addr_div_by_4/ parameters. The `addr_div_by_4` parameter is base address of the %s/is base/is the base/ shared memory area right shifted by 2 whereas `size` parameter is the size of shared memory area in bytes. Why shifting the address? I would prefer to keep it simple for the caller. If the alignment is not suitable, return an error. But why is an alignment needed here at all? And why 4 aligned? The shared memory area should be normal cacheable memory for the supervisor-mode software. Also, the shared memory area is global across all HARTs so SBI implementation must ensure atomicity in setting the shared memory area. Errors: SBI_SUCCESS - Shared memory area set successfully. SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by `addr_div_by_2` and `size` parameters is not normal cacheable memory or not accessible to supervisor-mode software. Function: Console Puts (FID #1) ------------------------------- struct sbiret sbi_debug_console_puts(unsigned long area_offset, unsigned long num_chars) I would prefer to simply pass a physical address pointer here with no requirements on alignment. And no prior SBI call. Do we need num_chars? Are we expecting to provide binary output? Using 0x00 as terminator would be adequate in most cases. What is the requirement on the console? Does it have to support 8bit output to allow for UTF-8? Do we make any assumptions about encoding? How would we handle a console set up to 7bit + parity if a character > 0x7f is sent? Print the string specified by `area_offset` and `num_chars` on the debug console. The `area_offset` parameter is the start of string in the shard memory area whereas `num_chars` parameter %s/shard/shared/ is the number of characters (or bytes) in the string. This is a blocking SBI call and will only return after printing all characters of the string. Errors: SBI_SUCCESS - Characters printed successfully. SBI_ERR_INVALID_ADDRESS - The start of the string (i.e. `area_offset`) or end of the string (i.e. `area_offset + num_chars`) is outside shared memory area. There could be other reasons of failures: * set up of the UART failed in OpenSBI * no UART defined in the device-tree * ... So let us add SBI_ERR_FAILED to the list. Best regards Heinrich
More All Messages By This Member

1 - 20 of 21

Previous Topic Next Topic

Home Hashtags Subgroups Terms