| Message ID | 20230425102545.162888-1-sunilvl@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none" | expand |
Hi Alistair, On Tue, Apr 25, 2023 at 03:55:45PM +0530, Sunil V L wrote: > Currently, virt machine supports two pflash instances each with > 32MB size. However, the first pflash is always assumed to > contain M-mode firmware and reset vector is set to this if > enabled. Hence, for S-mode payloads like EDK2, only one pflash > instance is available for use. This means both code and NV variables > of EDK2 will need to use the same pflash. > > The OS distros keep the EDK2 FW code as readonly. When non-volatile > variables also need to share the same pflash, it is not possible > to keep it as readonly since variables need write access. > > To resolve this issue, the code and NV variables need to be separated. > But in that case we need an extra flash. Hence, modify the convention > such that pflash0 will contain the M-mode FW only when "-bios none" > option is used. Otherwise, pflash0 will contain the S-mode payload FW. > This enables both pflash instances available for EDK2 use. > > Example usage: > 1) pflash0 containing M-mode FW > qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt > or > qemu-system-riscv64 -bios none \ > -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt > > 2) pflash0 containing S-mode payload like EDK2 > qemu-system-riscv64 -pflash <smode_fw_vars> -pflash <smode_fw_code> -machine virt > or > qemu-system-riscv64 -bios <opensbi_fw> \ > -pflash <smode_fw_vars> \ > -pflash <smode_fw_code> \ > -machine virt > or > qemu-system-riscv64 -bios <opensbi_fw> \ > -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=0 \ > -drive file=<smode_fw_code>,if=pflash,format=raw,unit=1,readonly=on \ > -machine virt > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > The issue is reported at > https://salsa.debian.org/qemu-team/edk2/-/commit/c345655a0149f64c5020bfc1e53c619ce60587f6 > > The patch is based on Alistair's riscv-to-apply.next branch. > > Changes since v1: > 1) Simplified the fix such that it doesn't break current EDK2. > Could you please help reviewing this patch? This was sent when you were out of office. Thanks, Sunil > hw/riscv/virt.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index 4e3efbee16..ca445d3d02 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -1296,10 +1296,11 @@ static void virt_machine_done(Notifier *notifier, void *data) > kernel_entry = 0; > } > > - if (drive_get(IF_PFLASH, 0, 0)) { > + if (drive_get(IF_PFLASH, 0, 0) && > + machine->firmware && !strcmp(machine->firmware, "none")) { > /* > - * Pflash was supplied, let's overwrite the address we jump to after > - * reset to the base of the flash. > + * Pflash0 was supplied with "-bios none", let's overwrite the address > + * we jump to after reset to the base of the flash. > */ > start_addr = virt_memmap[VIRT_FLASH].base; > } > -- > 2.34.1 >
On 4/25/23 12:25, Sunil V L wrote: > Currently, virt machine supports two pflash instances each with > 32MB size. However, the first pflash is always assumed to > contain M-mode firmware and reset vector is set to this if > enabled. Hence, for S-mode payloads like EDK2, only one pflash > instance is available for use. This means both code and NV variables > of EDK2 will need to use the same pflash. > > The OS distros keep the EDK2 FW code as readonly. When non-volatile > variables also need to share the same pflash, it is not possible > to keep it as readonly since variables need write access. > > To resolve this issue, the code and NV variables need to be separated. > But in that case we need an extra flash. Hence, modify the convention > such that pflash0 will contain the M-mode FW only when "-bios none" > option is used. Otherwise, pflash0 will contain the S-mode payload FW. > This enables both pflash instances available for EDK2 use. > > Example usage: > 1) pflash0 containing M-mode FW > qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt > or > qemu-system-riscv64 -bios none \ > -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt > > 2) pflash0 containing S-mode payload like EDK2 > qemu-system-riscv64 -pflash <smode_fw_vars> -pflash <smode_fw_code> -machine virt > or > qemu-system-riscv64 -bios <opensbi_fw> \ > -pflash <smode_fw_vars> \ > -pflash <smode_fw_code> \ On amd64 and arm64 unit=0 is used for code and unit=1 is used for variables. Shouldn't riscv64 do the same? Best regards Heinrich > -machine virt > or > qemu-system-riscv64 -bios <opensbi_fw> \ > -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=0 \ > -drive file=<smode_fw_code>,if=pflash,format=raw,unit=1,readonly=on \ > -machine virt > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > The issue is reported at > https://salsa.debian.org/qemu-team/edk2/-/commit/c345655a0149f64c5020bfc1e53c619ce60587f6 > > The patch is based on Alistair's riscv-to-apply.next branch. > > Changes since v1: > 1) Simplified the fix such that it doesn't break current EDK2. > > hw/riscv/virt.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index 4e3efbee16..ca445d3d02 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -1296,10 +1296,11 @@ static void virt_machine_done(Notifier *notifier, void *data) > kernel_entry = 0; > } > > - if (drive_get(IF_PFLASH, 0, 0)) { > + if (drive_get(IF_PFLASH, 0, 0) && > + machine->firmware && !strcmp(machine->firmware, "none")) { > /* > - * Pflash was supplied, let's overwrite the address we jump to after > - * reset to the base of the flash. > + * Pflash0 was supplied with "-bios none", let's overwrite the address > + * we jump to after reset to the base of the flash. > */ > start_addr = virt_memmap[VIRT_FLASH].base; > }
On Mon, May 08, 2023 at 07:37:23AM +0200, Heinrich Schuchardt wrote: > On 4/25/23 12:25, Sunil V L wrote: > > Currently, virt machine supports two pflash instances each with > > 32MB size. However, the first pflash is always assumed to > > contain M-mode firmware and reset vector is set to this if > > enabled. Hence, for S-mode payloads like EDK2, only one pflash > > instance is available for use. This means both code and NV variables > > of EDK2 will need to use the same pflash. > > > > The OS distros keep the EDK2 FW code as readonly. When non-volatile > > variables also need to share the same pflash, it is not possible > > to keep it as readonly since variables need write access. > > > > To resolve this issue, the code and NV variables need to be separated. > > But in that case we need an extra flash. Hence, modify the convention > > such that pflash0 will contain the M-mode FW only when "-bios none" > > option is used. Otherwise, pflash0 will contain the S-mode payload FW. > > This enables both pflash instances available for EDK2 use. > > > > Example usage: > > 1) pflash0 containing M-mode FW > > qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt > > or > > qemu-system-riscv64 -bios none \ > > -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt > > > > 2) pflash0 containing S-mode payload like EDK2 > > qemu-system-riscv64 -pflash <smode_fw_vars> -pflash <smode_fw_code> -machine virt > > or > > qemu-system-riscv64 -bios <opensbi_fw> \ > > -pflash <smode_fw_vars> \ > > -pflash <smode_fw_code> \ > > On amd64 and arm64 unit=0 is used for code and unit=1 is used for variables. > Shouldn't riscv64 do the same? > Hi Heinrich, Is that a requirement from distros perspective? That was my original v1 design. But the reason why I kept unit0 for variables, it helps in keeping current EDK2 usage model work. Otherwise, current EDK2 will break if we change the code to unit 0. Second, since unit 0 for RISC-V is currently assumed to start in M-mode fw which is secure, I think it makes sense to keep variables also in unit 0. Thanks! Sunil
On Mon, May 08, 2023 at 11:37:43AM +0530, Sunil V L wrote: > On Mon, May 08, 2023 at 07:37:23AM +0200, Heinrich Schuchardt wrote: > > On 4/25/23 12:25, Sunil V L wrote: > > > Currently, virt machine supports two pflash instances each with > > > 32MB size. However, the first pflash is always assumed to > > > contain M-mode firmware and reset vector is set to this if > > > enabled. Hence, for S-mode payloads like EDK2, only one pflash > > > instance is available for use. This means both code and NV variables > > > of EDK2 will need to use the same pflash. > > > > > > The OS distros keep the EDK2 FW code as readonly. When non-volatile > > > variables also need to share the same pflash, it is not possible > > > to keep it as readonly since variables need write access. > > > > > > To resolve this issue, the code and NV variables need to be separated. > > > But in that case we need an extra flash. Hence, modify the convention > > > such that pflash0 will contain the M-mode FW only when "-bios none" > > > option is used. Otherwise, pflash0 will contain the S-mode payload FW. > > > This enables both pflash instances available for EDK2 use. > > > > > > Example usage: > > > 1) pflash0 containing M-mode FW > > > qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt > > > or > > > qemu-system-riscv64 -bios none \ > > > -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt > > > > > > 2) pflash0 containing S-mode payload like EDK2 > > > qemu-system-riscv64 -pflash <smode_fw_vars> -pflash <smode_fw_code> -machine virt > > > or > > > qemu-system-riscv64 -bios <opensbi_fw> \ > > > -pflash <smode_fw_vars> \ > > > -pflash <smode_fw_code> \ > > > > On amd64 and arm64 unit=0 is used for code and unit=1 is used for variables. > > Shouldn't riscv64 do the same? Good catch, I had missed that! > Is that a requirement from distros perspective? That was my original v1 > design. > > But the reason why I kept unit0 for variables, it helps in keeping current > EDK2 usage model work. Otherwise, current EDK2 will break if we change > the code to unit 0. I think that it's more important to align with other architectures. The number of people currently running edk2 on RISC-V is probably vanishingly small, and in my opinion requiring them to tweak their command lines a bit is a fair price to pay to avoid having to carry a subtle difference between architectures for years to come. With that in mind, my preference would be to go back to v1. > Second, since unit 0 for RISC-V is currently assumed to start in M-mode fw > which is secure, I think it makes sense to keep variables also in unit > 0. If you're storing variables rather than code in pflash0, does it even make sense to talk about M-mode and S-mode? Taking a step back, what is even the use case for having M-mode code in pflash0? If you want to use an M-mode firmware, can't you just use -bios instead? In other words, can we change the behavior so that pflash being present always mean loading S-mode firmware off it?
On Mon, May 08, 2023 at 03:00:02AM -0700, Andrea Bolognani wrote: > On Mon, May 08, 2023 at 11:37:43AM +0530, Sunil V L wrote: > > On Mon, May 08, 2023 at 07:37:23AM +0200, Heinrich Schuchardt wrote: > > > On 4/25/23 12:25, Sunil V L wrote: > > > > Currently, virt machine supports two pflash instances each with > > > > 32MB size. However, the first pflash is always assumed to > > > > contain M-mode firmware and reset vector is set to this if > > > > enabled. Hence, for S-mode payloads like EDK2, only one pflash > > > > instance is available for use. This means both code and NV variables > > > > of EDK2 will need to use the same pflash. > > > > > > > > The OS distros keep the EDK2 FW code as readonly. When non-volatile > > > > variables also need to share the same pflash, it is not possible > > > > to keep it as readonly since variables need write access. > > > > > > > > To resolve this issue, the code and NV variables need to be separated. > > > > But in that case we need an extra flash. Hence, modify the convention > > > > such that pflash0 will contain the M-mode FW only when "-bios none" > > > > option is used. Otherwise, pflash0 will contain the S-mode payload FW. > > > > This enables both pflash instances available for EDK2 use. > > > > > > > > Example usage: > > > > 1) pflash0 containing M-mode FW > > > > qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt > > > > or > > > > qemu-system-riscv64 -bios none \ > > > > -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt > > > > > > > > 2) pflash0 containing S-mode payload like EDK2 > > > > qemu-system-riscv64 -pflash <smode_fw_vars> -pflash <smode_fw_code> -machine virt > > > > or > > > > qemu-system-riscv64 -bios <opensbi_fw> \ > > > > -pflash <smode_fw_vars> \ > > > > -pflash <smode_fw_code> \ > > > > > > On amd64 and arm64 unit=0 is used for code and unit=1 is used for variables. > > > Shouldn't riscv64 do the same? > > Good catch, I had missed that! > > > Is that a requirement from distros perspective? That was my original v1 > > design. > > > > But the reason why I kept unit0 for variables, it helps in keeping current > > EDK2 usage model work. Otherwise, current EDK2 will break if we change > > the code to unit 0. > > I think that it's more important to align with other architectures. > > The number of people currently running edk2 on RISC-V is probably > vanishingly small, and in my opinion requiring them to tweak their > command lines a bit is a fair price to pay to avoid having to carry a > subtle difference between architectures for years to come. > It is not just tweaking the command line. The current EDK2 will not work anymore if code is moved to plfash 0 since EDK2 assumed its entry point is in pflash1. I agree there may not be too many users but if we have to align with other archs, there will be combinations of qemu and edk2 versions which won't work. > With that in mind, my preference would be to go back to v1. > Thanks!. If this is the preference, we can request people to use proper versions of EDK2 with different qemu versions. > > Second, since unit 0 for RISC-V is currently assumed to start in M-mode fw > > which is secure, I think it makes sense to keep variables also in unit > > 0. > > If you're storing variables rather than code in pflash0, does it even > make sense to talk about M-mode and S-mode? > > > Taking a step back, what is even the use case for having M-mode code > in pflash0? If you want to use an M-mode firmware, can't you just use > -bios instead? In other words, can we change the behavior so that > pflash being present always mean loading S-mode firmware off it? > TBH, I don't know. I am sure Alistair would know since it was added in https://github.com/qemu/qemu/commit/1c20d3ff6004b600336c52cbef9f134fad3ccd94 I don't think opensbi can be launched from pflash. So, it may be some other use case which I am now aware of. I will be happy if this can be avoided by using -bios. Thanks, Sunil
On Mon, May 08, 2023 at 04:53:46PM +0530, Sunil V L wrote: > On Mon, May 08, 2023 at 03:00:02AM -0700, Andrea Bolognani wrote: > > I think that it's more important to align with other architectures. > > > > The number of people currently running edk2 on RISC-V is probably > > vanishingly small, and in my opinion requiring them to tweak their > > command lines a bit is a fair price to pay to avoid having to carry a > > subtle difference between architectures for years to come. > > It is not just tweaking the command line. The current EDK2 will not work > anymore if code is moved to plfash 0 since EDK2 assumed its entry point > is in pflash1. I agree there may not be too many users but if we have > to align with other archs, there will be combinations of qemu and > edk2 versions which won't work. Right. > > With that in mind, my preference would be to go back to v1. > > Thanks!. If this is the preference, we can request people to use proper > versions of EDK2 with different qemu versions. Yeah, in the (not so) long run this will just not matter, as the versions of edk2 and QEMU available to people will all implement the new behavior. Better to optimize for the long future ahead of us rather than causing ongoing pain for the sake of the few users of a work-in-progress board. > > Taking a step back, what is even the use case for having M-mode code > > in pflash0? If you want to use an M-mode firmware, can't you just use > > -bios instead? In other words, can we change the behavior so that > > pflash being present always mean loading S-mode firmware off it? > > TBH, I don't know. I am sure Alistair would know since it was added in > https://github.com/qemu/qemu/commit/1c20d3ff6004b600336c52cbef9f134fad3ccd94 > I don't think opensbi can be launched from pflash. So, it may be some > other use case which I am now aware of. > > I will be happy if this can be avoided by using -bios. The actual commit would be [1], from late 2019. Things might have changed in the intervening ~3.5 years. Let's wait to hear from Alistair :) [1] https://github.com/qemu/qemu/commit/2738b3b555efaf206b814677966e8e3510c64a8a
On Mon, May 8, 2023 at 9:45 PM Andrea Bolognani <abologna@redhat.com> wrote: > > On Mon, May 08, 2023 at 04:53:46PM +0530, Sunil V L wrote: > > On Mon, May 08, 2023 at 03:00:02AM -0700, Andrea Bolognani wrote: > > > I think that it's more important to align with other architectures. That's true, ideally we want to match what people are already doing. > > > > > > The number of people currently running edk2 on RISC-V is probably > > > vanishingly small, and in my opinion requiring them to tweak their > > > command lines a bit is a fair price to pay to avoid having to carry a > > > subtle difference between architectures for years to come. > > > > It is not just tweaking the command line. The current EDK2 will not work > > anymore if code is moved to plfash 0 since EDK2 assumed its entry point > > is in pflash1. I agree there may not be too many users but if we have > > to align with other archs, there will be combinations of qemu and > > edk2 versions which won't work. > > Right. > > > > With that in mind, my preference would be to go back to v1. > > > > Thanks!. If this is the preference, we can request people to use proper > > versions of EDK2 with different qemu versions. > > Yeah, in the (not so) long run this will just not matter, as the > versions of edk2 and QEMU available to people will all implement the > new behavior. Better to optimize for the long future ahead of us > rather than causing ongoing pain for the sake of the few users of a > work-in-progress board. > > > > Taking a step back, what is even the use case for having M-mode code > > > in pflash0? If you want to use an M-mode firmware, can't you just use > > > -bios instead? In other words, can we change the behavior so that > > > pflash being present always mean loading S-mode firmware off it? It was originally added to support Oreboot (the Rust version of Coreboot). The idea was that Oreboot (ROM) would be in flash and then go from there. It also applies to other ROM code that a user might want to test that runs before OpenSBI. > > > > TBH, I don't know. I am sure Alistair would know since it was added in > > https://github.com/qemu/qemu/commit/1c20d3ff6004b600336c52cbef9f134fad3ccd94 > > I don't think opensbi can be launched from pflash. So, it may be some > > other use case which I am now aware of. > > > > I will be happy if this can be avoided by using -bios. > > The actual commit would be [1], from late 2019. Things might have > changed in the intervening ~3.5 years. Let's wait to hear from > Alistair :) Overall for this patch I don't feel strongly about following what ARM does or continuing with what we already have. I would prefer to match other archs if we can though. Also, either way we should update the documentation in docs/system/riscv/virt.rst to describe what happens. Alistair > > > [1] https://github.com/qemu/qemu/commit/2738b3b555efaf206b814677966e8e3510c64a8a > -- > Andrea Bolognani / Red Hat / Virtualization > >
On Mon, May 08, 2023 at 04:44:22AM -0700, Andrea Bolognani wrote: > On Mon, May 08, 2023 at 04:53:46PM +0530, Sunil V L wrote: > > On Mon, May 08, 2023 at 03:00:02AM -0700, Andrea Bolognani wrote: > > > I think that it's more important to align with other architectures. > > > > > > The number of people currently running edk2 on RISC-V is probably > > > vanishingly small, and in my opinion requiring them to tweak their > > > command lines a bit is a fair price to pay to avoid having to carry a > > > subtle difference between architectures for years to come. > > > > It is not just tweaking the command line. The current EDK2 will not work > > anymore if code is moved to plfash 0 since EDK2 assumed its entry point > > is in pflash1. I agree there may not be too many users but if we have > > to align with other archs, there will be combinations of qemu and > > edk2 versions which won't work. > > Right. > > > > With that in mind, my preference would be to go back to v1. > > > > Thanks!. If this is the preference, we can request people to use proper > > versions of EDK2 with different qemu versions. > > Yeah, in the (not so) long run this will just not matter, as the > versions of edk2 and QEMU available to people will all implement the > new behavior. Better to optimize for the long future ahead of us > rather than causing ongoing pain for the sake of the few users of a > work-in-progress board. > > > > Taking a step back, what is even the use case for having M-mode code > > > in pflash0? If you want to use an M-mode firmware, can't you just use > > > -bios instead? In other words, can we change the behavior so that > > > pflash being present always mean loading S-mode firmware off it? > > > > TBH, I don't know. I am sure Alistair would know since it was added in > > https://github.com/qemu/qemu/commit/1c20d3ff6004b600336c52cbef9f134fad3ccd94 > > I don't think opensbi can be launched from pflash. So, it may be some > > other use case which I am now aware of. > > > > I will be happy if this can be avoided by using -bios. > > The actual commit would be [1], from late 2019. Things might have > changed in the intervening ~3.5 years. Let's wait to hear from > Alistair :) > > > [1] https://github.com/qemu/qemu/commit/2738b3b555efaf206b814677966e8e3510c64a8a > -- Hi Alistair, Could you please provide your inputs on whether we can remove this pflash0 check completely and assume pflash will always have S-mode payload? I realized you responded to similar patch from Yong at [1] which I missed since qemu-riscv was not copied. My v2 patch is similar to Yong's patch but the feedback from distro experts is that, it better we align with other architectures. Based on your feedback, I will modify the patch and send v3. [1] - https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04023.html Thanks Sunil
On Wed, May 17, 2023 at 02:57:12PM +1000, Alistair Francis wrote: > On Mon, May 8, 2023 at 9:45 PM Andrea Bolognani <abologna@redhat.com> wrote: > > > > On Mon, May 08, 2023 at 04:53:46PM +0530, Sunil V L wrote: > > > On Mon, May 08, 2023 at 03:00:02AM -0700, Andrea Bolognani wrote: > > > > I think that it's more important to align with other architectures. > > That's true, ideally we want to match what people are already doing. > > > > > > > > > The number of people currently running edk2 on RISC-V is probably > > > > vanishingly small, and in my opinion requiring them to tweak their > > > > command lines a bit is a fair price to pay to avoid having to carry a > > > > subtle difference between architectures for years to come. > > > > > > It is not just tweaking the command line. The current EDK2 will not work > > > anymore if code is moved to plfash 0 since EDK2 assumed its entry point > > > is in pflash1. I agree there may not be too many users but if we have > > > to align with other archs, there will be combinations of qemu and > > > edk2 versions which won't work. > > > > Right. > > > > > > With that in mind, my preference would be to go back to v1. > > > > > > Thanks!. If this is the preference, we can request people to use proper > > > versions of EDK2 with different qemu versions. > > > > Yeah, in the (not so) long run this will just not matter, as the > > versions of edk2 and QEMU available to people will all implement the > > new behavior. Better to optimize for the long future ahead of us > > rather than causing ongoing pain for the sake of the few users of a > > work-in-progress board. > > > > > > Taking a step back, what is even the use case for having M-mode code > > > > in pflash0? If you want to use an M-mode firmware, can't you just use > > > > -bios instead? In other words, can we change the behavior so that > > > > pflash being present always mean loading S-mode firmware off it? > > It was originally added to support Oreboot (the Rust version of > Coreboot). The idea was that Oreboot (ROM) would be in flash and then > go from there. > > It also applies to other ROM code that a user might want to test that > runs before OpenSBI. > > > > > > > TBH, I don't know. I am sure Alistair would know since it was added in > > > https://github.com/qemu/qemu/commit/1c20d3ff6004b600336c52cbef9f134fad3ccd94 > > > I don't think opensbi can be launched from pflash. So, it may be some > > > other use case which I am now aware of. > > > > > > I will be happy if this can be avoided by using -bios. > > > > The actual commit would be [1], from late 2019. Things might have > > changed in the intervening ~3.5 years. Let's wait to hear from > > Alistair :) > > Overall for this patch I don't feel strongly about following what ARM > does or continuing with what we already have. I would prefer to match > other archs if we can though. > > Also, either way we should update the documentation in > docs/system/riscv/virt.rst to describe what happens. > Thanks! Alistair. My reminder mail was sent just before seeing this response. Sorry about that. Let me go back to v1 and also update the virt.rst and send v3. Thanks! Sunil
On Wed, May 17, 2023 at 02:57:12PM +1000, Alistair Francis wrote: > On Mon, May 8, 2023 at 9:45 PM Andrea Bolognani <abologna@redhat.com> wrote: > > > > Taking a step back, what is even the use case for having M-mode code > > > > in pflash0? If you want to use an M-mode firmware, can't you just use > > > > -bios instead? In other words, can we change the behavior so that > > > > pflash being present always mean loading S-mode firmware off it? > > It was originally added to support Oreboot (the Rust version of > Coreboot). The idea was that Oreboot (ROM) would be in flash and then > go from there. > > It also applies to other ROM code that a user might want to test that > runs before OpenSBI. Is there a reason why these would have to be loaded into pflash instead of being passed to -bios? From a quick look at the documentation for oreboot[1], it looks like they're doing the latter. Either way, assuming that there's a genuine reason why pflash must be used, I think the behavior implemented in v1 (pflash0 is M-mode when -bios none is used, S-mode otherwise) maps very well conceptually, and results in behavior matching that of other architectures out of the box. That's good enough for me :) I was just wondering whether we could keep things even simpler. [1] https://github.com/oreboot/oreboot/blob/main/src/mainboard/emulation/qemu-riscv/QEMU.md
On 8/5/23 12:00, Andrea Bolognani wrote: > On Mon, May 08, 2023 at 11:37:43AM +0530, Sunil V L wrote: >> On Mon, May 08, 2023 at 07:37:23AM +0200, Heinrich Schuchardt wrote: >>> On 4/25/23 12:25, Sunil V L wrote: >>>> Currently, virt machine supports two pflash instances each with >>>> 32MB size. However, the first pflash is always assumed to >>>> contain M-mode firmware and reset vector is set to this if >>>> enabled. Hence, for S-mode payloads like EDK2, only one pflash >>>> instance is available for use. This means both code and NV variables >>>> of EDK2 will need to use the same pflash. >>>> >>>> The OS distros keep the EDK2 FW code as readonly. When non-volatile >>>> variables also need to share the same pflash, it is not possible >>>> to keep it as readonly since variables need write access. >>>> >>>> To resolve this issue, the code and NV variables need to be separated. >>>> But in that case we need an extra flash. Hence, modify the convention >>>> such that pflash0 will contain the M-mode FW only when "-bios none" >>>> option is used. Otherwise, pflash0 will contain the S-mode payload FW. >>>> This enables both pflash instances available for EDK2 use. >>>> >>>> Example usage: >>>> 1) pflash0 containing M-mode FW >>>> qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt >>>> or >>>> qemu-system-riscv64 -bios none \ >>>> -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt >>>> >>>> 2) pflash0 containing S-mode payload like EDK2 >>>> qemu-system-riscv64 -pflash <smode_fw_vars> -pflash <smode_fw_code> -machine virt >>>> or >>>> qemu-system-riscv64 -bios <opensbi_fw> \ >>>> -pflash <smode_fw_vars> \ >>>> -pflash <smode_fw_code> \ >>> >>> On amd64 and arm64 unit=0 is used for code and unit=1 is used for variables. >>> Shouldn't riscv64 do the same? > > Good catch, I had missed that! This is a design mistake spreading. What EDK2 maintainers want is one Read-Only + Exec region for CODE and one Read-Write + NoExec region for VARS. QEMU never implemented correctly pflash bank (multiple sectors) write protected. When EDK2 (x86, OVMF) was tried on QEMU, QEMU was using a single pflash. To separate CODE/VARS, a second pflash was added, the first one being "locked" into Read-Only mode. Using a pflash allowed the firmware to identify device size using pflash CFI commands. Then this design was copied to the ARM virt board for EDK2 needs. In retrospective, this design was declared a mistake, since a simple ROM region for the CODE is sufficient, and much simpler [*]. Thankfully the Loongarch64 virt machine started cleanly avoiding the previous design flaw. It provides a ROM for CODE and pflash for VARS. Unfortunately the riscv virt machine copied the x86/arm virt machines. [*] Having 2 distinct pflash is useful for non-virt machines where the firmware might want to (re)program the CODE region, in the "capsule update" scenario. This scenario is irrelevant for virt machines, since a guest will never update its CODE. CODE is updated by the host. >> Is that a requirement from distros perspective? That was my original v1 >> design. >> >> But the reason why I kept unit0 for variables, it helps in keeping current >> EDK2 usage model work. Otherwise, current EDK2 will break if we change >> the code to unit 0. > > I think that it's more important to align with other architectures. > > The number of people currently running edk2 on RISC-V is probably > vanishingly small, and in my opinion requiring them to tweak their > command lines a bit is a fair price to pay to avoid having to carry a > subtle difference between architectures for years to come. > > With that in mind, my preference would be to go back to v1. > >> Second, since unit 0 for RISC-V is currently assumed to start in M-mode fw >> which is secure, I think it makes sense to keep variables also in unit >> 0. > > If you're storing variables rather than code in pflash0, does it even > make sense to talk about M-mode and S-mode? > > > Taking a step back, what is even the use case for having M-mode code > in pflash0? If you want to use an M-mode firmware, can't you just use > -bios instead? In other words, can we change the behavior so that > pflash being present always mean loading S-mode firmware off it? >
On Wed, May 17, 2023 at 6:45 PM Andrea Bolognani <abologna@redhat.com> wrote: > > On Wed, May 17, 2023 at 02:57:12PM +1000, Alistair Francis wrote: > > On Mon, May 8, 2023 at 9:45 PM Andrea Bolognani <abologna@redhat.com> wrote: > > > > > Taking a step back, what is even the use case for having M-mode code > > > > > in pflash0? If you want to use an M-mode firmware, can't you just use > > > > > -bios instead? In other words, can we change the behavior so that > > > > > pflash being present always mean loading S-mode firmware off it? > > > > It was originally added to support Oreboot (the Rust version of > > Coreboot). The idea was that Oreboot (ROM) would be in flash and then > > go from there. > > > > It also applies to other ROM code that a user might want to test that > > runs before OpenSBI. > > Is there a reason why these would have to be loaded into pflash > instead of being passed to -bios? From a quick look at the > documentation for oreboot[1], it looks like they're doing the latter. At one point we loaded Oreboot in in flash and booted from that. I think Oreboot then loaded OpenSBI into memory. The idea was to mimic what a physical board would do, so we could allow testing. It doesn't look like it's used any more. > > Either way, assuming that there's a genuine reason why pflash must be > used, I think the behavior implemented in v1 (pflash0 is M-mode when > -bios none is used, S-mode otherwise) maps very well conceptually, > and results in behavior matching that of other architectures out of > the box. That's good enough for me :) I was just wondering whether we > could keep things even simpler. I don't see a reason to remove the boot from pflash if no -bios argument is supplied. If there is a good reason to, I think we can (via deprecation) but the current functionality seems fine to me. Alistair > > > [1] https://github.com/oreboot/oreboot/blob/main/src/mainboard/emulation/qemu-riscv/QEMU.md > -- > Andrea Bolognani / Red Hat / Virtualization >
On Wed, May 17, 2023 at 10:48 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 8/5/23 12:00, Andrea Bolognani wrote: > > On Mon, May 08, 2023 at 11:37:43AM +0530, Sunil V L wrote: > >> On Mon, May 08, 2023 at 07:37:23AM +0200, Heinrich Schuchardt wrote: > >>> On 4/25/23 12:25, Sunil V L wrote: > >>>> Currently, virt machine supports two pflash instances each with > >>>> 32MB size. However, the first pflash is always assumed to > >>>> contain M-mode firmware and reset vector is set to this if > >>>> enabled. Hence, for S-mode payloads like EDK2, only one pflash > >>>> instance is available for use. This means both code and NV variables > >>>> of EDK2 will need to use the same pflash. > >>>> > >>>> The OS distros keep the EDK2 FW code as readonly. When non-volatile > >>>> variables also need to share the same pflash, it is not possible > >>>> to keep it as readonly since variables need write access. > >>>> > >>>> To resolve this issue, the code and NV variables need to be separated. > >>>> But in that case we need an extra flash. Hence, modify the convention > >>>> such that pflash0 will contain the M-mode FW only when "-bios none" > >>>> option is used. Otherwise, pflash0 will contain the S-mode payload FW. > >>>> This enables both pflash instances available for EDK2 use. > >>>> > >>>> Example usage: > >>>> 1) pflash0 containing M-mode FW > >>>> qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt > >>>> or > >>>> qemu-system-riscv64 -bios none \ > >>>> -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt > >>>> > >>>> 2) pflash0 containing S-mode payload like EDK2 > >>>> qemu-system-riscv64 -pflash <smode_fw_vars> -pflash <smode_fw_code> -machine virt > >>>> or > >>>> qemu-system-riscv64 -bios <opensbi_fw> \ > >>>> -pflash <smode_fw_vars> \ > >>>> -pflash <smode_fw_code> \ > >>> > >>> On amd64 and arm64 unit=0 is used for code and unit=1 is used for variables. > >>> Shouldn't riscv64 do the same? > > > > Good catch, I had missed that! > > This is a design mistake spreading. > > What EDK2 maintainers want is one Read-Only + Exec region for CODE and > one Read-Write + NoExec region for VARS. > > QEMU never implemented correctly pflash bank (multiple sectors) write > protected. > > When EDK2 (x86, OVMF) was tried on QEMU, QEMU was using a single pflash. > To separate CODE/VARS, a second pflash was added, the first one being > "locked" into Read-Only mode. Using a pflash allowed the firmware to > identify device size using pflash CFI commands. > > Then this design was copied to the ARM virt board for EDK2 needs. > > In retrospective, this design was declared a mistake, since a simple > ROM region for the CODE is sufficient, and much simpler [*]. It seems like we are making changes to the whole flash setup. Is it worth adding the ROM region now as well, so we can migrate to the simpler approach. We would keep two pflash regions, hopefully in a way that doesn't break any existing users. Alistair > > Thankfully the Loongarch64 virt machine started cleanly avoiding the > previous design flaw. It provides a ROM for CODE and pflash for VARS. > > Unfortunately the riscv virt machine copied the x86/arm virt machines. > > > [*] Having 2 distinct pflash is useful for non-virt machines where the > firmware might want to (re)program the CODE region, in the "capsule > update" scenario. This scenario is irrelevant for virt machines, > since a guest will never update its CODE. CODE is updated by the > host. > > >> Is that a requirement from distros perspective? That was my original v1 > >> design. > >> > >> But the reason why I kept unit0 for variables, it helps in keeping current > >> EDK2 usage model work. Otherwise, current EDK2 will break if we change > >> the code to unit 0. > > > > I think that it's more important to align with other architectures. > > > > The number of people currently running edk2 on RISC-V is probably > > vanishingly small, and in my opinion requiring them to tweak their > > command lines a bit is a fair price to pay to avoid having to carry a > > subtle difference between architectures for years to come. > > > > With that in mind, my preference would be to go back to v1. > > > >> Second, since unit 0 for RISC-V is currently assumed to start in M-mode fw > >> which is secure, I think it makes sense to keep variables also in unit > >> 0. > > > > If you're storing variables rather than code in pflash0, does it even > > make sense to talk about M-mode and S-mode? > > > > > > Taking a step back, what is even the use case for having M-mode code > > in pflash0? If you want to use an M-mode firmware, can't you just use > > -bios instead? In other words, can we change the behavior so that > > pflash being present always mean loading S-mode firmware off it? > > > >
On Thu, May 18, 2023 at 02:55:16PM +1000, Alistair Francis wrote: > On Wed, May 17, 2023 at 10:48 PM Philippe Mathieu-Daudé > <philmd@linaro.org> wrote: > > > > On 8/5/23 12:00, Andrea Bolognani wrote: > > > On Mon, May 08, 2023 at 11:37:43AM +0530, Sunil V L wrote: > > >> On Mon, May 08, 2023 at 07:37:23AM +0200, Heinrich Schuchardt wrote: > > >>> On 4/25/23 12:25, Sunil V L wrote: > > >>>> Currently, virt machine supports two pflash instances each with > > >>>> 32MB size. However, the first pflash is always assumed to > > >>>> contain M-mode firmware and reset vector is set to this if > > >>>> enabled. Hence, for S-mode payloads like EDK2, only one pflash > > >>>> instance is available for use. This means both code and NV variables > > >>>> of EDK2 will need to use the same pflash. > > >>>> > > >>>> The OS distros keep the EDK2 FW code as readonly. When non-volatile > > >>>> variables also need to share the same pflash, it is not possible > > >>>> to keep it as readonly since variables need write access. > > >>>> > > >>>> To resolve this issue, the code and NV variables need to be separated. > > >>>> But in that case we need an extra flash. Hence, modify the convention > > >>>> such that pflash0 will contain the M-mode FW only when "-bios none" > > >>>> option is used. Otherwise, pflash0 will contain the S-mode payload FW. > > >>>> This enables both pflash instances available for EDK2 use. > > >>>> > > >>>> Example usage: > > >>>> 1) pflash0 containing M-mode FW > > >>>> qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt > > >>>> or > > >>>> qemu-system-riscv64 -bios none \ > > >>>> -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt > > >>>> > > >>>> 2) pflash0 containing S-mode payload like EDK2 > > >>>> qemu-system-riscv64 -pflash <smode_fw_vars> -pflash <smode_fw_code> -machine virt > > >>>> or > > >>>> qemu-system-riscv64 -bios <opensbi_fw> \ > > >>>> -pflash <smode_fw_vars> \ > > >>>> -pflash <smode_fw_code> \ > > >>> > > >>> On amd64 and arm64 unit=0 is used for code and unit=1 is used for variables. > > >>> Shouldn't riscv64 do the same? > > > > > > Good catch, I had missed that! > > > > This is a design mistake spreading. > > > > What EDK2 maintainers want is one Read-Only + Exec region for CODE and > > one Read-Write + NoExec region for VARS. > > > > QEMU never implemented correctly pflash bank (multiple sectors) write > > protected. > > > > When EDK2 (x86, OVMF) was tried on QEMU, QEMU was using a single pflash. > > To separate CODE/VARS, a second pflash was added, the first one being > > "locked" into Read-Only mode. Using a pflash allowed the firmware to > > identify device size using pflash CFI commands. > > > > Then this design was copied to the ARM virt board for EDK2 needs. > > > > In retrospective, this design was declared a mistake, since a simple > > ROM region for the CODE is sufficient, and much simpler [*]. > > It seems like we are making changes to the whole flash setup. Is it > worth adding the ROM region now as well, so we can migrate to the > simpler approach. > ROM is used for FW image used for -bios option in RISC-V. It appears that loongarch uses -bios to boot EDK2 also as per docs/system/loongarch/virt.rst. But in RISC-V, EDK2 is S-mode payload and hence -bios can not be used. If we have to create ROM for S-mode payload, do we need to add a new qemu option? Also, I don't see enough space in the memmap of RISC-V virt machine for PAYLOAD_ROM region. So, I am not sure how to keep 2 flashes and add ROM region for S-mode payload. Let me know if I am missing some thing. > We would keep two pflash regions, hopefully in a way that doesn't > break any existing users. > Agree. While I agree with Philippe, I think we better solve the current problem by going back to v1 of the patch. Thanks, Sunil
On Wed, May 17, 2023 at 02:47:42PM +0200, Philippe Mathieu-Daudé wrote: > > > On Mon, May 08, 2023 at 07:37:23AM +0200, Heinrich Schuchardt wrote: > > > > On amd64 and arm64 unit=0 is used for code and unit=1 is used for variables. > > > > Shouldn't riscv64 do the same? > > > > Good catch, I had missed that! > > This is a design mistake spreading. > > What EDK2 maintainers want is one Read-Only + Exec region for CODE and > one Read-Write + NoExec region for VARS. > > QEMU never implemented correctly pflash bank (multiple sectors) write > protected. Does setting -drive if=pflash,unit=0,readonly=on not do the trick? > When EDK2 (x86, OVMF) was tried on QEMU, QEMU was using a single pflash. > To separate CODE/VARS, a second pflash was added, the first one being > "locked" into Read-Only mode. Using a pflash allowed the firmware to > identify device size using pflash CFI commands. > > Then this design was copied to the ARM virt board for EDK2 needs. > > In retrospective, this design was declared a mistake, since a simple > ROM region for the CODE is sufficient, and much simpler [*]. > > Thankfully the Loongarch64 virt machine started cleanly avoiding the > previous design flaw. It provides a ROM for CODE and pflash for VARS. Based on the documentation both in QEMU and edk2, it looks like a single file is used? I haven't seen an example where the pflash is used to provide a R/W area for VARS. Note that the current version of the firmware.json standard doesn't include a way to describe builds that have to be consumed by loading the CODE via -bios and the VARS via pflash. This is likely an artifact of codifying existing usage (x86/arm) and could probably be fixed, but the point remains that there is currently no way to represent such a build in a way that makes it possible for consumers such as libvirt to automatically pick it up. > [*] Having 2 distinct pflash is useful for non-virt machines where the > firmware might want to (re)program the CODE region, in the "capsule > update" scenario. This scenario is irrelevant for virt machines, > since a guest will never update its CODE. CODE is updated by the > host. Yeah, this makes sense. But I don't understand what's wrong with using a R/O pflash for CODE as opposed to -bios? What makes that approach so problematic? You're still going to need to use pflash for VARS anyway...
On Thu, May 18, 2023 at 02:53:05PM +1000, Alistair Francis wrote: > On Wed, May 17, 2023 at 6:45 PM Andrea Bolognani <abologna@redhat.com> wrote: > > On Wed, May 17, 2023 at 02:57:12PM +1000, Alistair Francis wrote: > At one point we loaded Oreboot in in flash and booted from that. I > think Oreboot then loaded OpenSBI into memory. The idea was to mimic > what a physical board would do, so we could allow testing. > > It doesn't look like it's used any more. > > > Either way, assuming that there's a genuine reason why pflash must be > > used, I think the behavior implemented in v1 (pflash0 is M-mode when > > -bios none is used, S-mode otherwise) maps very well conceptually, > > and results in behavior matching that of other architectures out of > > the box. That's good enough for me :) I was just wondering whether we > > could keep things even simpler. > > I don't see a reason to remove the boot from pflash if no -bios > argument is supplied. If there is a good reason to, I think we can > (via deprecation) but the current functionality seems fine to me. Aligning with other architectures. In the short term, we cause some minor pain to existing users, although as noted above for the oreboot case there might not even be any people still depending on this functionality at this point. In the long run, we guarantee smooth operation across the stack by not being different for being different's sake (e.g. requiring pflash0 and pflash1 to be swapped compared to x86 and Arm, which is a configuration that falls outside what can be described by the firmware.json standard and thus can't be automatically applied by libvirt).
On Tue, Apr 25, 2023 at 03:55:45PM +0530, Sunil V L wrote: > qemu-system-riscv64 -bios <opensbi_fw> \ > -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=0 \ > -drive file=<smode_fw_code>,if=pflash,format=raw,unit=1,readonly=on \ > -machine virt I've noticed that edk2 for RISC-V, at least in the form it is currently packaged for Fedora, doesn't seem to have separate CODE and VARS files: $ ls /usr/share/edk2/riscv/* | cat /usr/share/edk2/riscv/RISCV_VIRT.fd /usr/share/edk2/riscv/RISCV_VIRT.raw Is that something that needs to be addressed in upstream edk2? If so, will you be looking into it?
On Fri, May 19, 2023 at 09:11:37AM -0700, Andrea Bolognani wrote: > On Tue, Apr 25, 2023 at 03:55:45PM +0530, Sunil V L wrote: > > qemu-system-riscv64 -bios <opensbi_fw> \ > > -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=0 \ > > -drive file=<smode_fw_code>,if=pflash,format=raw,unit=1,readonly=on \ > > -machine virt > > I've noticed that edk2 for RISC-V, at least in the form it is > currently packaged for Fedora, doesn't seem to have separate CODE and > VARS files: > > $ ls /usr/share/edk2/riscv/* | cat > /usr/share/edk2/riscv/RISCV_VIRT.fd > /usr/share/edk2/riscv/RISCV_VIRT.raw > > Is that something that needs to be addressed in upstream edk2? If so, > will you be looking into it? > Hi Andrea, Yes, I have changes ready but before I submit EDK2 patches, I need qemu changes merged which enables EDK2 to use both pflash. Thanks, Sunil
On 18/5/23 08:03, Sunil V L wrote: > On Thu, May 18, 2023 at 02:55:16PM +1000, Alistair Francis wrote: >> On Wed, May 17, 2023 at 10:48 PM Philippe Mathieu-Daudé >> <philmd@linaro.org> wrote: >>> >>> On 8/5/23 12:00, Andrea Bolognani wrote: >>>> On Mon, May 08, 2023 at 11:37:43AM +0530, Sunil V L wrote: >>>>> On Mon, May 08, 2023 at 07:37:23AM +0200, Heinrich Schuchardt wrote: >>>>>> On 4/25/23 12:25, Sunil V L wrote: >>>>>>> Currently, virt machine supports two pflash instances each with >>>>>>> 32MB size. However, the first pflash is always assumed to >>>>>>> contain M-mode firmware and reset vector is set to this if >>>>>>> enabled. Hence, for S-mode payloads like EDK2, only one pflash >>>>>>> instance is available for use. This means both code and NV variables >>>>>>> of EDK2 will need to use the same pflash. >>>>>>> >>>>>>> The OS distros keep the EDK2 FW code as readonly. When non-volatile >>>>>>> variables also need to share the same pflash, it is not possible >>>>>>> to keep it as readonly since variables need write access. >>>>>>> >>>>>>> To resolve this issue, the code and NV variables need to be separated. >>>>>>> But in that case we need an extra flash. Hence, modify the convention >>>>>>> such that pflash0 will contain the M-mode FW only when "-bios none" >>>>>>> option is used. Otherwise, pflash0 will contain the S-mode payload FW. >>>>>>> This enables both pflash instances available for EDK2 use. >>>>>>> >>>>>>> Example usage: >>>>>>> 1) pflash0 containing M-mode FW >>>>>>> qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt >>>>>>> or >>>>>>> qemu-system-riscv64 -bios none \ >>>>>>> -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt >>>>>>> >>>>>>> 2) pflash0 containing S-mode payload like EDK2 >>>>>>> qemu-system-riscv64 -pflash <smode_fw_vars> -pflash <smode_fw_code> -machine virt >>>>>>> or >>>>>>> qemu-system-riscv64 -bios <opensbi_fw> \ >>>>>>> -pflash <smode_fw_vars> \ >>>>>>> -pflash <smode_fw_code> \ >>>>>> >>>>>> On amd64 and arm64 unit=0 is used for code and unit=1 is used for variables. >>>>>> Shouldn't riscv64 do the same? >>>> >>>> Good catch, I had missed that! >>> >>> This is a design mistake spreading. >>> >>> What EDK2 maintainers want is one Read-Only + Exec region for CODE and >>> one Read-Write + NoExec region for VARS. >>> >>> QEMU never implemented correctly pflash bank (multiple sectors) write >>> protected. >>> >>> When EDK2 (x86, OVMF) was tried on QEMU, QEMU was using a single pflash. >>> To separate CODE/VARS, a second pflash was added, the first one being >>> "locked" into Read-Only mode. Using a pflash allowed the firmware to >>> identify device size using pflash CFI commands. >>> >>> Then this design was copied to the ARM virt board for EDK2 needs. >>> >>> In retrospective, this design was declared a mistake, since a simple >>> ROM region for the CODE is sufficient, and much simpler [*]. >> >> It seems like we are making changes to the whole flash setup. Is it >> worth adding the ROM region now as well, so we can migrate to the >> simpler approach. >> > > ROM is used for FW image used for -bios option in RISC-V. It appears > that loongarch uses -bios to boot EDK2 also as per > docs/system/loongarch/virt.rst. But in RISC-V, EDK2 is S-mode payload > and hence -bios can not be used. Personally I'd try to not use -bios at all, as I see it as a magic / confusing machine-specific command line option, doing some open-coded "load this blob somewhere in memory". I prefer using explicit hardware device model. -bios usually call the "hw/loader.h" API which eventually model a ROM, but do a lot of file format parsing magic. If you pass a plain ROM file, no need for all this magic. > If we have to create ROM for S-mode > payload, do we need to add a new qemu option? Also, I don't see > enough space in the memmap of RISC-V virt machine for PAYLOAD_ROM > region. So, I am not sure how to keep 2 flashes and add ROM region for > S-mode payload. Let me know if I am missing some thing. > >> We would keep two pflash regions, hopefully in a way that doesn't >> break any existing users. >> > Agree. While I agree with Philippe, I think we better solve the current > problem by going back to v1 of the patch. > > Thanks, > Sunil
On 19/5/23 18:34, Philippe Mathieu-Daudé wrote: > On 18/5/23 08:03, Sunil V L wrote: >> On Thu, May 18, 2023 at 02:55:16PM +1000, Alistair Francis wrote: >>> On Wed, May 17, 2023 at 10:48 PM Philippe Mathieu-Daudé >>> <philmd@linaro.org> wrote: >>>> >>>> On 8/5/23 12:00, Andrea Bolognani wrote: >>>>> On Mon, May 08, 2023 at 11:37:43AM +0530, Sunil V L wrote: >>>>>> On Mon, May 08, 2023 at 07:37:23AM +0200, Heinrich Schuchardt wrote: >>>>>>> On 4/25/23 12:25, Sunil V L wrote: >>>>>>>> Currently, virt machine supports two pflash instances each with >>>>>>>> 32MB size. However, the first pflash is always assumed to >>>>>>>> contain M-mode firmware and reset vector is set to this if >>>>>>>> enabled. Hence, for S-mode payloads like EDK2, only one pflash >>>>>>>> instance is available for use. This means both code and NV >>>>>>>> variables >>>>>>>> of EDK2 will need to use the same pflash. >>>>>>>> >>>>>>>> The OS distros keep the EDK2 FW code as readonly. When non-volatile >>>>>>>> variables also need to share the same pflash, it is not possible >>>>>>>> to keep it as readonly since variables need write access. >>>>>>>> >>>>>>>> To resolve this issue, the code and NV variables need to be >>>>>>>> separated. >>>>>>>> But in that case we need an extra flash. Hence, modify the >>>>>>>> convention >>>>>>>> such that pflash0 will contain the M-mode FW only when "-bios none" >>>>>>>> option is used. Otherwise, pflash0 will contain the S-mode >>>>>>>> payload FW. >>>>>>>> This enables both pflash instances available for EDK2 use. >>>>>>>> >>>>>>>> Example usage: >>>>>>>> 1) pflash0 containing M-mode FW >>>>>>>> qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt >>>>>>>> or >>>>>>>> qemu-system-riscv64 -bios none \ >>>>>>>> -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt >>>>>>>> >>>>>>>> 2) pflash0 containing S-mode payload like EDK2 >>>>>>>> qemu-system-riscv64 -pflash <smode_fw_vars> -pflash >>>>>>>> <smode_fw_code> -machine virt >>>>>>>> or >>>>>>>> qemu-system-riscv64 -bios <opensbi_fw> \ >>>>>>>> -pflash <smode_fw_vars> \ >>>>>>>> -pflash <smode_fw_code> \ >>>>>>> >>>>>>> On amd64 and arm64 unit=0 is used for code and unit=1 is used for >>>>>>> variables. >>>>>>> Shouldn't riscv64 do the same? >>>>> >>>>> Good catch, I had missed that! >>>> >>>> This is a design mistake spreading. >>>> >>>> What EDK2 maintainers want is one Read-Only + Exec region for CODE and >>>> one Read-Write + NoExec region for VARS. >>>> >>>> QEMU never implemented correctly pflash bank (multiple sectors) write >>>> protected. >>>> >>>> When EDK2 (x86, OVMF) was tried on QEMU, QEMU was using a single >>>> pflash. >>>> To separate CODE/VARS, a second pflash was added, the first one being >>>> "locked" into Read-Only mode. Using a pflash allowed the firmware to >>>> identify device size using pflash CFI commands. >>>> >>>> Then this design was copied to the ARM virt board for EDK2 needs. >>>> >>>> In retrospective, this design was declared a mistake, since a simple >>>> ROM region for the CODE is sufficient, and much simpler [*]. >>> >>> It seems like we are making changes to the whole flash setup. Is it >>> worth adding the ROM region now as well, so we can migrate to the >>> simpler approach. >>> >> >> ROM is used for FW image used for -bios option in RISC-V. It appears >> that loongarch uses -bios to boot EDK2 also as per >> docs/system/loongarch/virt.rst. But in RISC-V, EDK2 is S-mode payload >> and hence -bios can not be used. > > Personally I'd try to not use -bios at all, as I see it as a magic / > confusing machine-specific command line option, doing some open-coded > "load this blob somewhere in memory". I prefer using explicit hardware > device model. > > -bios usually call the "hw/loader.h" API which eventually model a ROM, > but do a lot of file format parsing magic. If you pass a plain ROM file, > no need for all this magic. > >> If we have to create ROM for S-mode >> payload, do we need to add a new qemu option? Also, I don't see >> enough space in the memmap of RISC-V virt machine for PAYLOAD_ROM >> region. So, I am not sure how to keep 2 flashes and add ROM region for >> S-mode payload. Let me know if I am missing some thing. >> >>> We would keep two pflash regions, hopefully in a way that doesn't >>> break any existing users. >>> >> Agree. While I agree with Philippe, I think we better solve the current >> problem by going back to v1 of the patch. BTW clarifying, I'm not rejecting this particular patch; I was just trying to correct the idea than "doing what other architectures do" is always the right approach ;)
On Fri, May 19, 2023 at 06:40:37PM +0200, Philippe Mathieu-Daudé wrote: > > On 18/5/23 08:03, Sunil V L wrote: > > > Agree. While I agree with Philippe, I think we better solve the current > > > problem by going back to v1 of the patch. > > BTW clarifying, I'm not rejecting this particular patch; I was just > trying to correct the idea than "doing what other architectures do" > is always the right approach ;) Definitely agree with that :) But there's a cost associated with going off the beaten path, which the benefits must then outweight. There seems to be rough consensus of that not being the case here. So, can we get v1 merged?
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 4e3efbee16..ca445d3d02 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1296,10 +1296,11 @@ static void virt_machine_done(Notifier *notifier, void *data) kernel_entry = 0; } - if (drive_get(IF_PFLASH, 0, 0)) { + if (drive_get(IF_PFLASH, 0, 0) && + machine->firmware && !strcmp(machine->firmware, "none")) { /* - * Pflash was supplied, let's overwrite the address we jump to after - * reset to the base of the flash. + * Pflash0 was supplied with "-bios none", let's overwrite the address + * we jump to after reset to the base of the flash. */ start_addr = virt_memmap[VIRT_FLASH].base; }
Currently, virt machine supports two pflash instances each with 32MB size. However, the first pflash is always assumed to contain M-mode firmware and reset vector is set to this if enabled. Hence, for S-mode payloads like EDK2, only one pflash instance is available for use. This means both code and NV variables of EDK2 will need to use the same pflash. The OS distros keep the EDK2 FW code as readonly. When non-volatile variables also need to share the same pflash, it is not possible to keep it as readonly since variables need write access. To resolve this issue, the code and NV variables need to be separated. But in that case we need an extra flash. Hence, modify the convention such that pflash0 will contain the M-mode FW only when "-bios none" option is used. Otherwise, pflash0 will contain the S-mode payload FW. This enables both pflash instances available for EDK2 use. Example usage: 1) pflash0 containing M-mode FW qemu-system-riscv64 -bios none -pflash <mmode_fw> -machine virt or qemu-system-riscv64 -bios none \ -drive file=<mmode_fw>,if=pflash,format=raw,unit=0 -machine virt 2) pflash0 containing S-mode payload like EDK2 qemu-system-riscv64 -pflash <smode_fw_vars> -pflash <smode_fw_code> -machine virt or qemu-system-riscv64 -bios <opensbi_fw> \ -pflash <smode_fw_vars> \ -pflash <smode_fw_code> \ -machine virt or qemu-system-riscv64 -bios <opensbi_fw> \ -drive file=<smode_fw_vars>,if=pflash,format=raw,unit=0 \ -drive file=<smode_fw_code>,if=pflash,format=raw,unit=1,readonly=on \ -machine virt Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- The issue is reported at https://salsa.debian.org/qemu-team/edk2/-/commit/c345655a0149f64c5020bfc1e53c619ce60587f6 The patch is based on Alistair's riscv-to-apply.next branch. Changes since v1: 1) Simplified the fix such that it doesn't break current EDK2. hw/riscv/virt.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)