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

IOMMU cleanup #76

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rossphilipson
Copy link
Collaborator

Some IOMMU code cleanup:

  • Remove redundant IOMMU header file.
  • Move IOMMU setup to a common function to allow it to be called on the Xen launch path too.
  • Move memory protection disabling logic to its own function in iommu.c.

Two IOMMU headers are not really needed. Move the contents of
iommu_defs.h into iommu.h.

Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
The code to disable mem protection and setup the IOMMU should
be called on both the Linux and Multiboot2 launch paths. Both
paths now call this common function.

Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
This cleans up the code in main.c a bit and makes future changes
to this functionality cleaner too.

Signed-off-by: Ross Philipson <ross.philipson@oracle.com>

#define MEMPROT_CR 0x384

#define MEMPROT_EN 1<<0
Copy link
Member

Choose a reason for hiding this comment

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

This needs brackets, otherwise ~MEMPROT_EN becomes (~1)<<0, same for DEV_CR_SL_DEV_EN_MASK above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, I copied the problem to the new MEMPROT_EN define. So DEV_CR_SL_DEV_EN_MASK was already had the issue so how was this working? The disabling of DEV on the APU systems should have failed.

Copy link
Member

Choose a reason for hiding this comment

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

It was explicitly bracketed in the point it was used: https://github.com/TrenchBoot/landing-zone/pull/76/files#diff-06fe7dc01e61244778e4a79b20e826b83073d3dc18d726b5fa7a1ec5cf87a156R157

Doing it in the header file instead is safer, too many parentheses only hurt eyes but not the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes I see. I guess I should make everything consistent.

Comment on lines +394 to +399
/*
* Now in 64b mode, paging is setup. This is the launching point. We can
* now do what we want. First order of business is to setup IOMMU to cover
* all memory. At the end, trampoline to the PM entry point which will
* include the Secure Launch stub.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is obsolete. Amongst other things, we might be in 32bit mode. Seeing as the hunk has moved by virtue of the introduction of iommu_setup(), I'd just drop it.

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.

3 participants