Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Physical Memory Protection Extension #37

Closed
wants to merge 1 commit into from

Conversation

lbmeng
Copy link
Contributor

@lbmeng lbmeng commented Mar 7, 2020

S-mode software needs a way to know memory used by SBI firmware so
that it can correctly mark such memory as reserved.

Related discussion:
riscv-software-src/opensbi#103

Signed-off-by: Bin Meng bmeng.cn@gmail.com

Copy link

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This information is already provided through other means (devicetree something, I’m not sure exactly), or else conventional OS would not function correctly.

@lbmeng
Copy link
Contributor Author

lbmeng commented Mar 7, 2020

This information is already provided through other means (devicetree something, I’m not sure exactly), or else conventional OS would not function correctly.

No, this is not provided in devicetree as of today. Linux today is hardcoded to ignore memory below 2MB (64-bit).

With this new extension, U-Boot can query SBI firmware and insert a reserved memory node in the DT.

@aswaterman
Copy link

aswaterman commented Mar 7, 2020 via email

@lbmeng
Copy link
Contributor Author

lbmeng commented Mar 7, 2020

If that’s true, it’s a regression. I can say so with certainty, because I designed the first mechanism to communicate to the kernel what the SBI exclusion was. The original mechanism was via the initial page tables. When we got rid of initial page tables, we started using exclusions in the DT to communicate the same thing. Is it possible the DT you’re using is wrong?

I am using the same DT used by U-Boot/Linux (arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts in the kernel tree). There is only one memory node:

24        memory@80000000 {
25                device_type = "memory";
26                reg = <0x0 0x80000000 0x2 0x00000000>;
27        };

And U-Boot does not fix up anything (because there is no way to know that currently) before loading the kernel.

IIRC, @terpstra worked on the DT exclusion stuff, so I hope he can comment. Regardless, I think this API is too narrow. There’s no reason to believe the SBI exclusion should be contiguous. That’s why we expressed it using the existing standard mechanism in DT.

I think we can add another function to query how many contiguous memory regions are protected, and pass region number to sbi_get_fw_prot_{base, size} functions?

@aswaterman
Copy link

aswaterman commented Mar 7, 2020 via email

@lbmeng
Copy link
Contributor Author

lbmeng commented Mar 7, 2020

I don’t know why we would invent a new set of SBI calls to do what DT is already quite capable of doing.

Of course, we can just manually add a reserved memory node in DT and pass that to kernel. But I don't think that's a clean way.

Currently in my view how this extension should be used:

  • S-mode U-Boot queries M-mode SBI firmware to get such information, and fix-up the DT by inserting reserved memory node before loading the S-mode OS. S-mode OS does not need to do anything special to use this extension unless it is not booted via a bootloader that already does such job.
  • In the OpenSBI case, OpenSBI can fix-up DT directly if DT is available and pass the modified DT to its payload, e.g.: in the QEMU 'virt' or 'sifive_u' case, booting Linux directly via "-bios default -kernel Image".

@fintelia
Copy link
Contributor

fintelia commented Mar 7, 2020

The device tree spec provides the "Reserved Memory Block" for exactly this purpose. In the binary representation (FDT) this block as actually separate from the tree part of the data structure, so it should be easy quite easy to add entries without needing code to manipulate the nodes of the tree.

@avpatel
Copy link
Contributor

avpatel commented Mar 7, 2020

We have to use "reserved memory" device tree bindings for memory reservation. Please refer Linux device tree bindings documentation.

Regards,
Anup

@fintelia
Copy link
Contributor

fintelia commented Mar 7, 2020

@avpatel Could you link to the relevant documentation? I found this page which seems to be describing a way to use device tree nodes to reserve memory regions for special uses within the Linux kernel, but I didn't see anything about the "Memory Reservation Block" from section 5.3 of the devicetree spec which is meant to be used to protect boot program code/data. Does Linux ignore the Memory Reservation Block? If so, why?

@avpatel
Copy link
Contributor

avpatel commented Mar 7, 2020

Here's the link to "reserved memory" device tree bindings documentation:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt?h=v5.6-rc4

Any OS kernel or Hypervisor can implement this bindings to reserve memory.

OpenSBI can certainly update DTB as per "reserved memory" bindings and pass modified DTB to next stage (U-Boot or EDK2). If most people agree then we can go this route.

Regards,
Anup

@avpatel
Copy link
Contributor

avpatel commented Mar 7, 2020

Before "reserved memory" device tree bindings, there was "/memreserve/" device tree syntax used for memory reservation but it is not preferred anymore because other device DT nodes cannot refer reserved memory described using "/memreserve/" device tree syntax.

Regards,
Anup

@atishp04
Copy link
Collaborator

atishp04 commented Mar 7, 2020

I agree with DT node approach. There was a recent discussion about this on UEFI patches as well. I think OpenSBI should add this node so that U-Boot and Linux kernel doesn't accidentally stomp on that.

@lbmeng
Copy link
Contributor Author

lbmeng commented Mar 8, 2020

Here's the link to "reserved memory" device tree bindings documentation:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt?h=v5.6-rc4

Any OS kernel or Hypervisor can implement this bindings to reserve memory.

OpenSBI can certainly update DTB as per "reserved memory" bindings and pass modified DTB to next stage (U-Boot or EDK2). If most people agree then we can go this route.

Yes, that's my intention to propose this PMP extension. Please note U-Boot S-mode on FU540 does not use OpenSBI's FDT, instead it uses the kernel's FDT directly, that's why we need a way to query SBI firmware to provide such information, so that U-Boot (not OpenSBI in this case) can fix up the DT.

@lbmeng
Copy link
Contributor Author

lbmeng commented Mar 8, 2020

I agree with DT node approach. There was a recent discussion about this on UEFI patches as well. I think OpenSBI should add this node so that U-Boot and Linux kernel doesn't accidentally stomp on that.

Eventually the protected memory region information WILL be encoded in DT, and that's what I wanted as well. But we need a way to query that so that either OpenSBI or S-mode U-Boot can insert the node to the DT!

I need to repeat again (below is my previous reply to Andrew, but it looks to me this was ignored).

Currently in my view how this extension should be used:

  • S-mode U-Boot queries M-mode SBI firmware to get such information, and fix-up the DT by inserting reserved memory node before loading the S-mode OS. S-mode OS does not need to do anything special to use this extension unless it is not booted via a bootloader that already does such job.
  • In the OpenSBI case, OpenSBI can fix-up DT directly if DT is available and pass the modified DT to its payload, e.g.: in the QEMU 'virt' or 'sifive_u' case, booting Linux directly via "-bios default -kernel Image".

@aswaterman
Copy link

aswaterman commented Mar 8, 2020 via email

@lbmeng
Copy link
Contributor Author

lbmeng commented Mar 8, 2020

That suggests to me that M-mode U-boot should add the exclusions to the DT it passes to S-mode.

There is no M-mode U-Boot on FU540 as of today. The mainline OpenSBI/U-Boot workflow is:

  • OpenSBI loads S-mode U-Boot, then loads other operating systems
  • S-mode U-Boot does NOT use the DT passed from OpenSBI, but uses its own version of DT which is from Linux kernel

@avpatel
Copy link
Contributor

avpatel commented Mar 8, 2020

@aswaterman , the U-Boot SPL (M-mode) does not have all the information about amount of memory runtime firmware (i.e. OpenSBI) will use so runtime firmware should add its own reserved memory entries.

I think @lbmeng has valid concern here because U-Boot can always override the DTB passed by runtime firmware (i.e. OpenSBI).

I suggest that any booting stage (U-Boot SPL, OpenSBI or U-Boot S-mode) can update entries in reserved memory DT node but each stage should ensure that reserved entries done by previous booting stage preserved.

To address @lbmeng concern, the U-Boot S-mode should parse and preserve contents of reserved memory DT node from DTB passed by OpenSBI. Now U-Boot S-mode can put back the reserved memory entries passed by previous booting stage (along with any additional reservation if any) in the DTB that is being passed to Linux kernel.

Regards,
Anup

@avpatel
Copy link
Contributor

avpatel commented Mar 8, 2020

I forgot to mention that in case of virtualization the SBI calls are provided by Hypervisor (Xvisor RISC-V or KVM RISC-V) and there is no M-mode runtime firmware. In this case, no memory reservation is done by Hypervisors. This means U-Boot S-mode running inside Guest/VM will get DTB (passed by Hypervisor) which does not have any reserved memory DT node.

Regards,
Anup

@lbmeng
Copy link
Contributor Author

lbmeng commented Mar 8, 2020

@aswaterman , the U-Boot SPL (M-mode) does not have all the information about amount of memory runtime firmware (i.e. OpenSBI) will use so runtime firmware should add its own reserved memory entries.

I think @lbmeng has valid concern here because U-Boot can always override the DTB passed by runtime firmware (i.e. OpenSBI).

Correct. For U-Boot, the U-Boot SPL (M-mode) and U-Boot proper (S-mode) does not actually use the same DT, in fact SPL uses a much smaller DT, so fixing up the DT from SPL (M-mode) does not help here. The reason why U-Boot SPL was designed like this, is because SPL usually executes in a memory constrained environment and code footprint matters. Not to mention in some SPL implementation, FDT support might be completely missing (i.e.: CONFIG_OF_PLATDATA has to be used).

I suggest that any booting stage (U-Boot SPL, OpenSBI or U-Boot S-mode) can update entries in reserved memory DT node but each stage should ensure that reserved entries done by previous booting stage preserved.

To address @lbmeng concern, the U-Boot S-mode should parse and preserve contents of reserved memory DT node from DTB passed by OpenSBI. Now U-Boot S-mode can put back the reserved memory entries passed by previous booting stage (along with any additional reservation if any) in the DTB that is being passed to Linux kernel.

It looks complicated to do this way on FU540:

  • Current mainline OpenSBI has to be built with FW_PAYLOAD_FDT_PATH=hifive-unleashed-a00.dtb, and mainline S-mode U-Boot can be built in 2 ways:

    • If U-Boot is built with its own DT (CONFIG_OF_SEPARATE), U-Boot needs to preserve reserved memory information from DTB passed by OpenSBI, and then fix up the DT before passing it to OS
    • If U-Boot is built with previous stage DT (CONFIG_OF_PRIOR_STAGE), use the reserved memory information directly
  • If U-Boot SPL (M-mode) is to be added in the future, then:

    • U-Boot SPL may get PMP information directly by probing PMP register settings, or read reserved memory information from DTB passed by OpenSBI (require OpenSBI to be built with FW_PAYLOAD_FDT_PATH), then pass a special hand-off information to U-Boot proper (S-mode)

For QEMU 'virt' or 'sifive_u' machine, OpenSBI always have FDT available (QEMU built-in), but the workflow is almost similar as FU540.

These seem to need quite amount of of work to do. If we could provide PMP extension, it would be much easier.

@avpatel
Copy link
Contributor

avpatel commented Mar 8, 2020

Yes, indeed it will certainly turn-out to be very complicated for U-Boot considering OF_SEPERATE and OF_PRIOR.

Thinking about this more even EDK2 can override the DTB passed by previous booting stage. Also, we are only discussing device tree here and not thinking about ACPI as well. If we take DT centric approach now then later things will be complicated for ACPI based systems.

An SBI extension for listing memory regions will certainly simplify things for both U-Boot and EDK2. Any other use-case where this SBI extension can be useful ?

Also, SBI calls proposed by this PR can be reduced to just two SBI calls:

  1. first SBI call which will return size of memory (in bytes) required to describe memory regions
  2. second SBI call where S-mode software will just provide physical address of RAM where memory region information is to be written.

Regards,
Anup

@avpatel
Copy link
Contributor

avpatel commented Mar 8, 2020

To be clear, we will still need the DT reservation in runtime firmware (i.e. OpenSBI) so that we can support direct Linux booting as payload.

Regards,
Anup

@lbmeng
Copy link
Contributor Author

lbmeng commented Mar 8, 2020

Yes, indeed it will certainly turn-out to be very complicated for U-Boot considering OF_SEPERATE and OF_PRIOR.

Thinking about this more even EDK2 can override the DTB passed by previous booting stage. Also, we are only discussing device tree here and not thinking about ACPI as well. If we take DT centric approach now then later things will be complicated for ACPI based systems.

Yes, agreed that we should not focus on DT only.

An SBI extension for listing memory regions will certainly simplify things for both U-Boot and EDK2. Any other use-case where this SBI extension can be useful ?

Also, SBI calls proposed by this PR can be reduced to just two SBI calls:

  1. first SBI call which will return size of memory (in bytes) required to describe memory regions
  2. second SBI call where S-mode software will just provide physical address of RAM where memory region information is to be written.

Yep, this is simpler, and reduces the number of SBI calls. If no one objects, I will update the proposal in the next version.

@lbmeng
Copy link
Contributor Author

lbmeng commented Mar 8, 2020

To be clear, we will still need the DT reservation in runtime firmware (i.e. OpenSBI) so that we can support direct Linux booting as payload.

Yes, definitely!

@atishp04
Copy link
Collaborator

While I am not opposed to the new SBI extension, I think this can be solved without it as well.
I am currently working on a PoC patches for both OpenSBI and U-Boot. I will send it once I am done.

Here is my current approach for U-Boot implementation.

  1. Platforms with OF_PRIOR_STAGE
    Solution:
    No changes needed as U-Boot depend on DT provided by OpenSBI anyways.
    Example:
    Qemu/sifive_u, Andes ae350
  2. Platforms with OF_SEPARTE
    a. Any stage prior to OpenSBI (U-Boot SPL/FSBL/Coreboot) provides the DT:
    b. Any stage prior to OpenSBI (U-Boot SPL/FSBL/Coreboot) doesn't provide the DT:
    - OpenSBI has to be built with FW_PAYLOAD_FDT_PATH in this case.
    Now, the DT address from previous stage can be saved in global data and CONFIG_OF_BOARD_FIXUP can be used to update the final DT used from U-Boot sources.

AFAIK, EDK2 doesn't touch the FDT. So it has to rely on FDT modification provided by OpenSBI only.

@lbmeng Can you send the SBI patch to the Unix platform working group mailing list so that we can continue the discussion there? We can weigh the pros and cons of both method and pick the best suitable one.

@aswaterman
Copy link

I hope you are able to use @atishp04's PoC approach, rather than expanding the SBI interface for this one particular case.

@avpatel
Copy link
Contributor

avpatel commented Mar 10, 2020

Like I mentioned previously, we can certainly get this working for DT because all booting stages use DT but if we start supporting ACPI in-future then DT only approach won't work. Because of this reason we have kept DT related code confined to platform code in OpenSBI.

@lbmeng
Copy link
Contributor Author

lbmeng commented Mar 10, 2020

While I am not opposed to the new SBI extension, I think this can be solved without it as well.
I am currently working on a PoC patches for both OpenSBI and U-Boot. I will send it once I am done.

Here is my current approach for U-Boot implementation.

  1. Platforms with OF_PRIOR_STAGE
    Solution:
    No changes needed as U-Boot depend on DT provided by OpenSBI anyways.
    Example:
    Qemu/sifive_u, Andes ae350
  2. Platforms with OF_SEPARTE
    a. Any stage prior to OpenSBI (U-Boot SPL/FSBL/Coreboot) provides the DT:
    b. Any stage prior to OpenSBI (U-Boot SPL/FSBL/Coreboot) doesn't provide the DT:
    • OpenSBI has to be built with FW_PAYLOAD_FDT_PATH in this case.
      Now, the DT address from previous stage can be saved in global data and CONFIG_OF_BOARD_FIXUP can be used to update the final DT used from U-Boot sources.

I have already pointed out this possible solution before, and this is way too complicated for U-Boot to handle. I am not in favor of this solution hence the proposal.

AFAIK, EDK2 doesn't touch the FDT. So it has to rely on FDT modification provided by OpenSBI only.

As @avpatel pointed out, we need also consider ACPI here.

@lbmeng Can you send the SBI patch to the Unix platform working group mailing list so that we can continue the discussion there? We can weigh the pros and cons of both method and pick the best suitable one.

Do you mean sending this SBI extension patch in this PR? Is the Unix platform working group mailing list this one: tech-unixplatformspec@lists.riscv.org ? Looks it is not a public mailing list.

@avpatel
Copy link
Contributor

avpatel commented Mar 10, 2020

Yes, @lbmeng send the patch in PR to tech-unixplatformspec@lists.riscv.org mailing list. It is a public read-only mailing list. Only RISC-V foundation members can join and post on this mailing list.

@atishp04
Copy link
Collaborator

U-Boot will add equivalent reserved-memory entry to ACPI anyways. Whenever we have ACPI, we may still need to update the ACPI in OpenSBI (assuming we use OpenSBI for ACPI based platforms as well) for direct Linux boot.

@lbmeng You need to be a member of RISC-V foundation. IIRC, individual membership is free.

@lbmeng
Copy link
Contributor Author

lbmeng commented Mar 10, 2020

@lbmeng Yes. Any new SBI extension has to discussed and approved by Unix platform specification working group.

Email was sent to the Unix platform specification working group mailing list.

I have a concern if such proposal needs to be discussed and approved on a members only mailing list, it seems to be against the openness of the RISC-V architecture.

@fintelia
Copy link
Contributor

The discussions on the mailing list are public:

https://lists.riscv.org/g/tech-unixplatformspec/topics

@atishp04
Copy link
Collaborator

@lbmeng SBI is a RISC-V specific standard. The process for SBI specification is same as any other RISC-V specification. Every specification in RISC-V is driven by a working group. Every working group follows the same principle i.e. all the discussions are available to public but voting rights are only reserved for members.

JohnAZoidberg referenced this pull request in changab/edk2-staging-riscv Mar 12, 2020
The Linux EFISTUB reads the FDT from the EFI system configuration table.
Before installing the FDT needs to be patched with the booting hartid, because
the kernel in S-Mode cannot determine it.

Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com>
@JohnAZoidberg
Copy link
Contributor

@atishp04

AFAIK, EDK2 doesn't touch the FDT. So it has to rely on FDT modification provided by OpenSBI only.

Actually we do touch the FDT in EDK2. Just like U-Boot we need to add the boot-hartid to start the EFISTUB kernel.

@avpatel

Thinking about this more even EDK2 can override the DTB passed by previous booting stage.

EDK2 is not a stage after OpenSBI. EDK2 is the first code to run, then calls sbi_init and uses OpenSBI as a library. We call sbi_init very early, so we'd prefer not to have OpenSBI modify the DT, but EDK2 be able to make the modifications in a later stage.

Since the stages of EDK2 run first in M-Mode and then later in S-Mode, we don't directly need this extension or the proposed trap-n-emulate for PMP CSRs.
cc @changab

@JohnAZoidberg
Copy link
Contributor

Maybe I'm not getting something but what is the point of restricting mvendorid, marchid, mimpid and PMP CSRs to M-Mode if we provide an unfiltered way to access them from S-Mode?

I guess PMP CSRs shouldn't be writable by S-Mode. But couldn't we make them read-write in M-Mode and readonly in S-Mode? Of course this would need modifications in the privileged spec.

@changab
Copy link
Contributor

changab commented Mar 12, 2020

EDK2 has many memory types for the memory blocks maintained in its memory manager and also provides GetMemoryMap for ACPI-capable OS to get the memory region type and attributes before OS invokes ExitBootService. So for edk2+ACPI OS, we will provide enough information to the next UEFI-aware boot stage through boot service GetMemoryMap(). But we (RISC-V edk2 port) still can modify DT as Daniel said to add the reserver memory region if DT aware boot stage needs it. RISC-V edk2 port will have DT in EFI configuration table and EFI GetMemoryMap simultaneously for reporting memory information.

If my understanding is correct, this proposal is to get PMP settings to know the reserved memory region, right? However, PMP is the memory region configuration and can not exactly reflect which memory region is reserved. A R/W-able memory region could be reserved for some reason. Maybe I get it wrong with the proposal been discussing here.

@changab
Copy link
Contributor

changab commented Mar 12, 2020

I guess PMP CSRs shouldn't be writable by S-Mode. But couldn't we make them read-write in M-Mode and readonly in S-Mode? Of course this would need modifications in the privileged spec.

There is a proposal in TEE TG(?) The sPMP, which Smode can have its own PMP set. These sPMP registers are definitely R/W-able. Not sure the latest proposal of sPMP though.

@fintelia
Copy link
Contributor

@JohnAZoidberg wrote...

Maybe I'm not getting something but what is the point of restricting mvendorid, marchid, mimpid and PMP CSRs to M-Mode if we provide an unfiltered way to access them from S-Mode?

The SBI provider is allowed to lie. Most of the time it probably wouldn't, but there are cases like having code running inside a VM where it might be useful to lie about exactly what hardware is being used.

@atishp04
Copy link
Collaborator

There is a proposal in TEE TG(?) The sPMP, which Smode can have its own PMP set. These sPMP registers are definitely R/W-able. Not sure the latest proposal of sPMP though.

As per my understanding, sPMP is in addition to existing PMP. sPMP allows S-mode to create physical address protection from U-Mode.

@atishp04
Copy link
Collaborator

EDK2 has many memory types for the memory blocks maintained in its memory manager and also provides GetMemoryMap for ACPI-capable OS to get the memory region type and attributes before OS invokes ExitBootService. So for edk2+ACPI OS, we will provide enough information to the next UEFI-aware boot stage through boot service GetMemoryMap(). But we (RISC-V edk2 port) still can modify DT as Daniel said to add the reserver memory region if DT aware boot stage needs it. RISC-V edk2 port will have DT in EFI configuration table and EFI GetMemoryMap simultaneously for reporting memory information.

@changab If I understand you correctly, EDK2 doesn't need any SBI call to update the ACPI tables.
As PEI phase of EDK2 also runs in M-mode, it can read the PMP information and set the EFI configuration tables/update DT as needed.

OpenSBI still need to modify the DT to allow direct booting of Linux without any real boot loader or firmware. It's up to the EDK2 to reuse the information updated by OpenSBI or just read the PMP config registers directly in M-mode and update the DT by itself.

If my understanding is correct, this proposal is to get PMP settings to know the reserved memory region, right? However, PMP is the memory region configuration and can not exactly reflect which memory region is reserved. A R/W-able memory region could be reserved for some reason. Maybe I get it wrong with the proposal been discussing here.

I think "reserved" here means that any region that doesn't have R/W permission. It ensures that S-mode software do not try to read/write from that region.

@changab
Copy link
Contributor

changab commented Mar 13, 2020

@changab If I understand you correctly, EDK2 doesn't need any SBI call to update the ACPI tables.
As PEI phase of EDK2 also runs in M-mode, it can read the PMP information and set the EFI configuration tables/update DT as needed.

Right. We can report the initial EFI memory map according to PMP setting in the very early boot phase (M-Mode PEI phase) and do whatever we need before entering OS.

However... I can't say EDK2 doesn't need SBI to read/write PMP. In our proposal of RISC-V edk2 port, we design the mechanism that allows load EFI SMode driver or EFI MMode driver in POST. We also defined the new EFI_MEMORY_TYPE for SMode accessible memory region and MMode accessible memory region for security reasons. This may requrie to manipulate PMPs in runtime during POST. We also evaluate the implementation of RISC-V version Management Mode, this may also need to update PMP in runtime. That would be good if OpenSBI provides the API for accessing PMPs, although we originally plan to have similar APIs in EFI_OPEN_SBI_PROTOCOL for EFI DXE phase driver.

OpenSBI still need to modify the DT to allow direct booting of Linux without any real boot loader or
firmware. It's up to the EDK2 to reuse the information updated by OpenSBI or just read the PMP >config registers directly in M-mode and update the DT by itself.

Right. but it would be good if OpenSBI defines a standard process for updating DT either for boot hart or reserved memory region. Then firmware could have a standard to follow.

I think "reserved" here means that any region that doesn't have R/W permission. It ensures that S-
mode software do not try to read/write from that region.

Ok.

lbmeng added a commit to lbmeng/opensbi that referenced this pull request Mar 13, 2020
This calls fdt_reserved_memory_fixup() helper in the platform's
final_init() routine.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Series-version: 2
Series-prefix: RESEND
Cover-letter:
lib: Fix up device tree to insert a reserved memory node
Currently there is no way for S-mode payload to know the information
of the memory region used by OpenSBI firmware. The on-chip Physical
Memory Proction CSR has been programmed to protect this memory region
from buggy S-mode software. We have to find a way to tell such info
to S-mode software, and fortunately the device tree specification
in the Linux kernel [1] allows us to convey it via a reserved memory
node in the DT.

This adds a helper API to do the reserved memory node fix-up. Platform
codes are updated to call the helper in their final_init().

Tested on QEMU 'sifive_u' machine, booting to S-mode U-Boot, then print
the device tree from the shell:

=> fdt print
/ {
        #address-cells = <0x00000002>;
        #size-cells = <0x00000002>;
        compatible = "sifive,hifive-unleashed-a00";
        model = "SiFive HiFive Unleashed A00";
        reserved-memory {
                #address-cells = <0x00000002>;
                #size-cells = <0x00000002>;
                ranges;
                opensbi@80000000 {
                        reg = <0x00000000 0x80000000 0x00000000 0x0001b000>;
                        no-map;
                };
        };

Tested on QEMU 'sifive_u' machine, booting to Linux kernel v5.5 directly.

Note there is still an on-going discussion [2] about whether we need
introduce a new SBI extension for converying such information directly
to S-mode payload.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
[2] riscv-non-isa/riscv-sbi-doc#37
END
@atishp04
Copy link
Collaborator

@changab If I understand you correctly, EDK2 doesn't need any SBI call to update the ACPI tables.
As PEI phase of EDK2 also runs in M-mode, it can read the PMP information and set the EFI configuration tables/update DT as needed.

Right. We can report the initial EFI memory map according to PMP setting in the very early boot phase (M-Mode PEI phase) and do whatever we need before entering OS.

However... I can't say EDK2 doesn't need SBI to read/write PMP. In our proposal of RISC-V edk2 port, we design the mechanism that allows load EFI SMode driver or EFI MMode driver in POST. We also defined the new EFI_MEMORY_TYPE for SMode accessible memory region and MMode accessible memory region for security reasons. This may requrie to manipulate PMPs in runtime during POST. We also evaluate the implementation of RISC-V version Management Mode, this may also need to update PMP in runtime. That would be good if OpenSBI provides the API for accessing PMPs, although we originally plan to have similar APIs in EFI_OPEN_SBI_PROTOCOL for EFI DXE phase driver.

S-mode can't manipulate PMP configuration. If EDK2 wants manipulate the PMP configuration in M-mode, you don't need SBI extension for that. I am okay with OpenSBI providing an API to read/write PMP configurations so that EDK2 can use it in M-mode. That is completely different from a SBI extension being discussed here.

Let's continue the discussion in unix platform mailing list as more folks are watching that.

OpenSBI still need to modify the DT to allow direct booting of Linux without any real boot loader or
firmware. It's up to the EDK2 to reuse the information updated by OpenSBI or just read the PMP >config registers directly in M-mode and update the DT by itself.

Right. but it would be good if OpenSBI defines a standard process for updating DT either for boot hart or reserved memory region. Then firmware could have a standard to follow.

Yes. That's what we are trying to do it. As I said above, we can continue discussing the best approach to solve the problem in unix platform mailing list.

I think "reserved" here means that any region that doesn't have R/W permission. It ensures that S-
mode software do not try to read/write from that region.

Ok.

@changab
Copy link
Contributor

changab commented Mar 14, 2020

S-mode can't manipulate PMP configuration. If EDK2 wants manipulate the PMP configuration in M-mode, you don't need SBI extension for that.

Most of EDK2 POST time is in SMode and it has to switch to Mmode for configuring PMPs. Thus I said above it would be good if OpenSBI could provide SBI if those SBIs are not just benefiting to edk2.

I am okay with OpenSBI providing an API to read/write PMP configurations so that EDK2 can use it in M-mode. That is completely different from a SBI extension being discussed here.

Let's continue the discussion in unix platform mailing list as more folks are watching that.

sure.

@fintelia
Copy link
Contributor

Perhaps this should be a broader question about the scope of SBI, but shouldn't it only cover interfaces between the SBI provider and the operating system? I'd expect what happens before Linux starts to be irrelevant from the perspective of this standard. That would include ways to gather information to be included in the device tree, like this proposal to query reserved memory regions

lbmeng added a commit to lbmeng/opensbi that referenced this pull request Mar 17, 2020
This calls fdt_reserved_memory_fixup() helper in the platform's
final_init() routine.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Series-version: 4
Cover-letter:
lib: Fix up device tree to insert a reserved memory node
Currently there is no way for S-mode payload to know the information
of the memory region used by OpenSBI firmware. The on-chip Physical
Memory Proction CSR has been programmed to protect this memory region
from buggy S-mode software. We have to find a way to tell such info
to S-mode software, and fortunately the device tree specification
in the Linux kernel [1] allows us to convey it via a reserved memory
node in the DT.

This adds several helper APIs to do various device tree fix-ups. This
includes a generic CPU node "status" property fix-up per hart status,
PLIC "interrupts-extended" fix-up, adding reserved memory node and a
general fdt_fixups() for convenience. Platform codes are updated to
call these helpers in their final_init().

Tested on QEMU 'sifive_u' machine, booting to S-mode U-Boot, then print
the device tree from the shell:

=> fdt print
/ {
        #address-cells = <0x00000002>;
        #size-cells = <0x00000002>;
        compatible = "sifive,hifive-unleashed-a00";
        model = "SiFive HiFive Unleashed A00";
        reserved-memory {
                #address-cells = <0x00000002>;
                #size-cells = <0x00000002>;
                ranges;
                mmode_pmp0@80000000 {
                        reg = <0x00000000 0x80000000 0x00000000 0x00020000>;
                        no-map;
                };
        };

Tested on QEMU 'sifive_u' machine, booting to Linux kernel v5.5 directly.

Note there is still an on-going discussion [2] about whether we need
introduce a new SBI extension for converying such information directly
to S-mode payload.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
[2] riscv-non-isa/riscv-sbi-doc#37
END
mhorne added a commit to mhorne/freebsd that referenced this pull request Mar 22, 2020
Currently, there is no mechanism through which the SBI firmware can
communicate the details of its reserved memory region(s) to a supervisor
payload. There has been some discussion recently on how this can be
achieved [1], and it seems that it will happen via the device-tree, a
new set of SBI calls, or some combination of both.

This hasn't caused any issues for us yet, since we exclude all physical
memory below the kernel's load address from being allocated, and on all
currently supported platforms this covers the SBI firmware region.

OpenSBI has begun specifying its section of memory under the
"reserved-memory" device-tree property [2], which we can parse easily.
If this property is present, we know exactly which region of physical
memory the kernel should ignore.

In the case that no "reserved-memory" property is found, exclude the
lowest 2MB of memory from allocation. This isn't a perfect solution, but
it is consistent with how the currently supported platforms are set up,
allowing us to handle this case with BBL or the current release of
OpenSBI.

[1] riscv-non-isa/riscv-sbi-doc#37
[2] https://lists.infradead.org/pipermail/opensbi/2020-March/001544.html
mhorne added a commit to mhorne/freebsd that referenced this pull request Mar 29, 2020
Currently, there is no mechanism through which the SBI firmware can
communicate the details of its reserved memory region(s) to a supervisor
payload. There has been some discussion recently on how this can be
achieved [1], and it seems that it will happen via the device-tree, a
new set of SBI calls, or some combination of both.

This hasn't caused any issues for us yet, since we exclude all physical
memory below the kernel's load address from being allocated, and on all
currently supported platforms this covers the SBI firmware region.

OpenSBI has begun specifying its section of memory under the
"reserved-memory" device-tree property [2], which we can parse easily.
If this property is present, we know exactly which region of physical
memory the kernel should ignore.

In the case that no "reserved-memory" property is found, exclude the
lowest 2MB of memory from allocation. This isn't a perfect solution, but
it is consistent with how the currently supported platforms are set up,
allowing us to handle this case with BBL or the current release of
OpenSBI.

[1] riscv-non-isa/riscv-sbi-doc#37
[2] https://lists.infradead.org/pipermail/opensbi/2020-March/001544.html
mhorne added a commit to mhorne/freebsd that referenced this pull request Mar 29, 2020
Currently, there is no mechanism through which the SBI firmware can
communicate the details of its reserved memory region(s) to a supervisor
payload. There has been some discussion recently on how this can be
achieved [1], and it seems that it will happen via the device-tree, a
new set of SBI calls, or some combination of both.

This hasn't caused any issues for us yet, since we exclude all physical
memory below the kernel's load address from being allocated, and on all
currently supported platforms this covers the SBI firmware region.

OpenSBI has begun specifying its section of memory under the
"reserved-memory" device-tree property [2], which we can parse easily.
If this property is present, we know exactly which region of physical
memory the kernel should ignore.

In the case that no "reserved-memory" property is found, exclude the
lowest 2MB of memory from allocation. This isn't a perfect solution, but
it is consistent with how the currently supported platforms are set up,
allowing us to handle this case with BBL or the current release of
OpenSBI.

[1] riscv-non-isa/riscv-sbi-doc#37
[2] https://lists.infradead.org/pipermail/opensbi/2020-March/001544.html
mhorne added a commit to mhorne/freebsd that referenced this pull request Apr 4, 2020
Currently, there is no mechanism through which the SBI firmware can
communicate the details of its reserved memory region(s) to a supervisor
payload. There has been some discussion recently on how this can be
achieved [1], and it seems that it will happen via the device-tree, a
new set of SBI calls, or some combination of both.

This hasn't caused any issues for us yet, since we exclude all physical
memory below the kernel's load address from being allocated, and on all
currently supported platforms this covers the SBI firmware region.

OpenSBI has begun specifying its section of memory under the
"reserved-memory" device-tree property [2], which we can parse easily.
If this property is present, we know exactly which region of physical
memory the kernel should ignore.

In the case that no "reserved-memory" property is found, exclude the
lowest 2MB of memory from allocation. This isn't a perfect solution, but
it is consistent with how the currently supported platforms are set up,
allowing us to handle this case with BBL or the current release of
OpenSBI.

[1] riscv-non-isa/riscv-sbi-doc#37
[2] https://lists.infradead.org/pipermail/opensbi/2020-March/001544.html
mhorne added a commit to mhorne/freebsd that referenced this pull request Apr 17, 2020
Currently, there is no mechanism through which the SBI firmware can
communicate the details of its reserved memory region(s) to a supervisor
payload. There has been some discussion recently on how this can be
achieved [1], and it seems that it will happen via the device-tree, a
new set of SBI calls, or some combination of both.

This hasn't caused any issues for us yet, since we exclude all physical
memory below the kernel's load address from being allocated, and on all
currently supported platforms this covers the SBI firmware region.

OpenSBI has begun specifying its section of memory under the
"reserved-memory" device-tree property [2], which we can parse easily.
If this property is present, we know exactly which region of physical
memory the kernel should ignore.

In the case that no "reserved-memory" property is found, exclude the
lowest 2MB of memory from allocation. This isn't a perfect solution, but
it is consistent with how the currently supported platforms are set up,
allowing us to handle this case with BBL or the current release of
OpenSBI.

[1] riscv-non-isa/riscv-sbi-doc#37
[2] https://lists.infradead.org/pipermail/opensbi/2020-March/001544.html
mhorne added a commit to mhorne/freebsd that referenced this pull request Apr 17, 2020
Currently, there is no mechanism through which the SBI firmware can
communicate the details of its reserved memory region(s) to a supervisor
payload. There has been some discussion recently on how this can be
achieved [1], and it seems that it will happen via the device-tree, a
new set of SBI calls, or some combination of both.

This hasn't caused any issues for us yet, since we exclude all physical
memory below the kernel's load address from being allocated, and on all
currently supported platforms this covers the SBI firmware region.

OpenSBI has begun specifying its section of memory under the
"reserved-memory" device-tree property [2], which we can parse easily.
If this property is present, we know exactly which region of physical
memory the kernel should ignore.

In the case that no "reserved-memory" property is found, exclude the
lowest 2MB of memory from allocation. This isn't a perfect solution, but
it is consistent with how the currently supported platforms are set up,
allowing us to handle this case with BBL or the current release of
OpenSBI.

[1] riscv-non-isa/riscv-sbi-doc#37
[2] https://lists.infradead.org/pipermail/opensbi/2020-March/001544.html
uqs pushed a commit to freebsd/freebsd-src that referenced this pull request Apr 19, 2020
The device tree may contain a "reserved-memory" node, whose purpose is
to communicate sections of physical memory that should not be used for
general allocations. Add the logic to parse and exclude these regions.

The particular motivation for this is protection of the SBI runtime
firmware. Currently, there is no mechanism through which the SBI
can communicate the details of its reserved memory region(s) to
a supervisor payload. There has been some discussion recently on how
this can be achieved [1], and it seems that the path going forward
will be to add an entry to the reserved-memory node.

This hasn't caused any issues for us yet, since we exclude all physical
memory below the kernel's load address from being allocated, and on all
currently supported platforms this covers the SBI firmware region. This
will change in another commit, so as a safety measure, ensure that the
lowest 2MB of memory is excluded if this region has not been reported.

[1] riscv-non-isa/riscv-sbi-doc#37

Reviewed by:	markj, nick (older version)
Differential Revision:	https://reviews.freebsd.org/D24155


git-svn-id: svn+ssh://svn.freebsd.org/base/head@360084 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f
uqs pushed a commit to freebsd/freebsd-src that referenced this pull request Apr 19, 2020
The device tree may contain a "reserved-memory" node, whose purpose is
to communicate sections of physical memory that should not be used for
general allocations. Add the logic to parse and exclude these regions.

The particular motivation for this is protection of the SBI runtime
firmware. Currently, there is no mechanism through which the SBI
can communicate the details of its reserved memory region(s) to
a supervisor payload. There has been some discussion recently on how
this can be achieved [1], and it seems that the path going forward
will be to add an entry to the reserved-memory node.

This hasn't caused any issues for us yet, since we exclude all physical
memory below the kernel's load address from being allocated, and on all
currently supported platforms this covers the SBI firmware region. This
will change in another commit, so as a safety measure, ensure that the
lowest 2MB of memory is excluded if this region has not been reported.

[1] riscv-non-isa/riscv-sbi-doc#37

Reviewed by:	markj, nick (older version)
Differential Revision:	https://reviews.freebsd.org/D24155
brooksdavis pushed a commit to CTSRD-CHERI/cheribsd that referenced this pull request May 1, 2020
The device tree may contain a "reserved-memory" node, whose purpose is
to communicate sections of physical memory that should not be used for
general allocations. Add the logic to parse and exclude these regions.

The particular motivation for this is protection of the SBI runtime
firmware. Currently, there is no mechanism through which the SBI
can communicate the details of its reserved memory region(s) to
a supervisor payload. There has been some discussion recently on how
this can be achieved [1], and it seems that the path going forward
will be to add an entry to the reserved-memory node.

This hasn't caused any issues for us yet, since we exclude all physical
memory below the kernel's load address from being allocated, and on all
currently supported platforms this covers the SBI firmware region. This
will change in another commit, so as a safety measure, ensure that the
lowest 2MB of memory is excluded if this region has not been reported.

[1] riscv-non-isa/riscv-sbi-doc#37

Reviewed by:	markj, nick (older version)
Differential Revision:	https://reviews.freebsd.org/D24155
@atishp04
Copy link
Collaborator

As the proposed features are implemented via DT, we can close this PR.

@atishp04 atishp04 closed this May 18, 2020
zxombie pushed a commit to CTSRD-CHERI/freebsd-morello that referenced this pull request May 28, 2020
The device tree may contain a "reserved-memory" node, whose purpose is
to communicate sections of physical memory that should not be used for
general allocations. Add the logic to parse and exclude these regions.

The particular motivation for this is protection of the SBI runtime
firmware. Currently, there is no mechanism through which the SBI
can communicate the details of its reserved memory region(s) to
a supervisor payload. There has been some discussion recently on how
this can be achieved [1], and it seems that the path going forward
will be to add an entry to the reserved-memory node.

This hasn't caused any issues for us yet, since we exclude all physical
memory below the kernel's load address from being allocated, and on all
currently supported platforms this covers the SBI firmware region. This
will change in another commit, so as a safety measure, ensure that the
lowest 2MB of memory is excluded if this region has not been reported.

[1] riscv-non-isa/riscv-sbi-doc#37

Reviewed by:	markj, nick (older version)
Differential Revision:	https://reviews.freebsd.org/D24155
bdrewery pushed a commit to bdrewery/freebsd that referenced this pull request May 31, 2020
The device tree may contain a "reserved-memory" node, whose purpose is
to communicate sections of physical memory that should not be used for
general allocations. Add the logic to parse and exclude these regions.

The particular motivation for this is protection of the SBI runtime
firmware. Currently, there is no mechanism through which the SBI
can communicate the details of its reserved memory region(s) to
a supervisor payload. There has been some discussion recently on how
this can be achieved [1], and it seems that the path going forward
will be to add an entry to the reserved-memory node.

This hasn't caused any issues for us yet, since we exclude all physical
memory below the kernel's load address from being allocated, and on all
currently supported platforms this covers the SBI firmware region. This
will change in another commit, so as a safety measure, ensure that the
lowest 2MB of memory is excluded if this region has not been reported.

[1] riscv-non-isa/riscv-sbi-doc#37

Reviewed by:	markj, nick (older version)
Differential Revision:	https://reviews.freebsd.org/D24155


git-svn-id: svn+ssh://svn.freebsd.org/base/head@360084 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants