riscv-non-isa / riscv-iommu

RISC-V IOMMU Specification
https://jira.riscv.org/browse/RVG-55
Creative Commons Attribution 4.0 International
82 stars 16 forks source link

Clarification updates to IOMMU v1.0.0 #243

Open ved-rivos opened 1 year ago

ved-rivos commented 1 year ago

This PR is opened to accumulate clarification updates to IOMMU v1.0 specification.

ved-rivos commented 1 year ago

@pperesse

lyctw commented 1 year ago

hi, there are duplicate entries of PPN and PRI in the Glossary section

ved-rivos commented 1 year ago

Thanks. Will add that to the list.

baimengwei commented 1 year ago

If it is convenient for you, can you update the meaning of this sentence below? this sentence is not very easy to understand.

The only PTE update supported by the IOMMU without first clearing the V bit in the
original PTE and executing a appropriate `IOTINVAL` command is to do a page size
promotion or demotion.

https://github.com/riscv-non-isa/riscv-iommu/blob/main/iommu_sw_guidelines.adoc?plain=1#L266 I will think that: If I want to change the attributes of a PTE, a correct way is as below:

  1. modify the attribution position of the PTE
  2. set the V position of the PTE to 0
  3. execute the IOTINVAL instruction
  4. set the V position of the PTE to 1
  5. execute the IOTINVAL instruction

However, my usual steps is as follows. This steps is determined by the implementation, and there is no guarantee that these steps will be effective for IOMMUs implemented by other designers.

  1. modify the attribution position of the PTE
  2. execute the IOTINVAL instruction

Is this understanding correct?

thank you very much.

ved-rivos commented 1 year ago

There does not seem a need to make any updates to that sentence. Doing a page size promotion or demotion may be done without marking the PTE invalid. Doing any other changes needs a break-before-make sequence - make PTE invalid, invalidate IOATC, update PTE, and mark valid.

baimengwei commented 1 year ago

There does not seem a need to make any updates to that sentence. Doing a page size promotion or demotion may be done without marking the PTE invalid. Doing any other changes needs a break-before-make sequence - make PTE invalid, invalidate IOATC, update PTE, and mark valid.

Is the software using such an operation below because the hardware may also modify the PTE, causing the software to fail to modify the attributes of the PTE?

make PTE invalid, invalidate IOATC, update PTE, and mark valid.

The following is a scenario:

  1. the software want to write 0 to the position D of the PTE, so the software write 0 to the position D of the PTE.
  2. the hardware implicitly modifies the position D of the PTE to 1.
  3. software executes IOTINVAL, the position D in the PTE becomes 1 instead of 0, causing the operation to fail, but using the method make PTE invalid, invalidate IOATC seems to be ok to avoid failure.

If this is the case, why not check bit 24 of the capabilities register, if this bit is 0, it means that the hardware will not be modified, and then the software only needs to directly modify the PTE and execute the IOTINVAL instruction?

baimengwei commented 1 year ago

There are some jumps about chapter 2.1.3.6 in the PDF, but the pdf itself does not mark these chapters.

Could you please mark them with number for easy browsing?

image

image

ved-rivos commented 1 year ago

That is covered by the first paragraph of that section - changing A/D bits is allowed without needing PTEs to be marked invalid. The text referenced is in the section about promoting or demoting page size.

baimengwei commented 1 year ago

That is covered by the first paragraph of that section - changing A/D bits is allowed without needing PTEs to be marked invalid. The text referenced is in the section about promoting or demoting page size.

I really can't find the reason for this process to first clear the V position in PTE, then invalidate the IOATC, thenmodify PTE, then set the V position to 1, and finally execute invalidate the IOATC again. My prior knowledge is that one invalidate the IOATC instruction seems to be enough even for promoting or demoting page size, just as mmu operation in core.😳

ved-rivos commented 1 year ago

"and finally execute invalidate the IOATC again." where do you see that mentioned?

baimengwei commented 1 year ago

"and finally execute invalidate the IOATC again." where do you see that mentioned?

It was not mentioned, and my understanding of the previous use about mmu is incorrect. I am sorry to bother you. These are just regular operations.

There does not seem a need to make any updates to that sentence. Doing a page size promotion or demotion may be done without marking the PTE invalid. Doing any other changes needs a break-before-make sequence - make PTE invalid, invalidate IOATC, update PTE, and mark valid.

baimengwei commented 1 year ago

is this a typo? If yes, please modify it. Thanks.

original:

When an IOMMU is transitioned to Bare of Off state, the IOMMU may retain information ...

expect:

When an IOMMU is transitioned to Bare or Off state, the IOMMU may retain information ...
ved-rivos commented 1 year ago

thanks. Its included in the PR now.

baimengwei commented 1 year ago

It seems that there is an extra S in IPSR_FIELDS, which leads to a jump failure in PDF.

...
[[IPSR_FIELD]]
...

If the conditions to set that bit are still present (See <<IPSR_FIELDS>>) or if
...

image

ved-rivos commented 1 year ago

Thanks. FIxed IPSR_FIELDS->IPSR_FIELD.

ved-rivos commented 1 year ago

Could you please mark them with number for easy browsing?

This is included in the PR.

jones-drew commented 3 months ago

Section 3.1 lists the opcode 3 as IOTDIR, but then later refers to that opcode as IODIR in section 3.1.3.

ved-rivos commented 3 months ago

@jones-drew that correction is already in this PR

jones-drew commented 3 months ago

@jones-drew that correction is already in this PR

Thanks and sorry I didn't check the full diff first!