[RTL] Fixes for issue number 10 and 11

zero-day-labs / riscv-iommu

IOMMU IP compliant with the RISC-V IOMMU Specification v1.0

Apache License 2.0

66 stars 12 forks source link

[RTL] Fixes for issue number 10 and 11 #12

Closed mhayat-10xe closed 2 months ago

mhayat-10xe commented 2 months ago

Description

This PR has the fixes for issue number 10 and 11.

Changes

Fixed issue number 10 by using else if instead of if in module rv_iommu_cdw, this way it will give priority to the first if block and then go to the next else block.
Fixed issue number 11 by adding the condition that whenever InclPC == 0 and dc.tc.pdtv == 1, the cause_code must be "TRANS_TYPE_DISALLOWED".

malejo97 commented 2 months ago

@mhayat-10xe Thanks for creating the PR with the changes to address the referred issues!

I need you to perform some modifications before merging the PR into main:

It's true that it does not makes sense to cache a DC into the DDTC if DC.tc.pdtv==1 and the IOMMU does not support Process Contexts (InclPC == 0). Actually, this condition is checked (and the error generated) here, after getting a hit in the DDTC. In this sense, I would appreciate if you could modify your commits to remove this check, as with your changes it would be impossible to have DCs with this misconfiguration cached in the DDTC.
Could you rewrite the commit history to unify the three commits made to fix Issue #11 into a single commit?
Also, we require all commits to be signed in order to merge the PR. When rewriting the history, please sign off the commits :)

mhayat-10xe commented 2 months ago

Ok, I am removing this fix for issue 11 and only sending the fix for issue 10 by signing off the commit. So actually error is generated when pdtv is 1 but after updating the DC cache it means only the DC cache updating logic is wrong.

malejo97 commented 2 months ago

Ok, I am removing this fix for issue 11 and only sending the fix for issue 10 by signing off the commit. So actually error is generated when pdtv is 1 but after updating the DC cache it means only the DC cache updating logic is wrong.

@mhayat-10xe What I was saying is to keep the fixes for both issues, but removing the check after updating the DDTC (indicated here), as it is not necessary. Your fix for Issue #11 does makes sense :)

mhayat-10xe commented 2 months ago

Done, I have removed the check you mentioned, fixed issue 11 in a single commit, and signed off the commit.

malejo97 commented 2 months ago

Done, I have removed the check you mentioned, fixed issue 11 in a single commit, and signed off the commit.

@mhayat-10xe Thanks a lot!