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

kata: use restrictive default policy #759

Merged
merged 2 commits into from
Jul 24, 2024
Merged

kata: use restrictive default policy #759

merged 2 commits into from
Jul 24, 2024

Conversation

burgerdev
Copy link
Contributor

@burgerdev burgerdev commented Jul 23, 2024

There's a bug in patch 0003 which causes wrong arguments for QEMU and thus prevents starting VMs - patch 0004 in this PR fixes that bug. Since 0003 corresponds to an open upstream PR, I'd rather not integrate the patch here into 0003, so that we have a clear history of the modifications.

There was another bug in the upstream PR, which assumed hex encoding for MRCONFIGID where it should have been base64.

I amended 0003 to fix these, and subsequently changed to a restrictive default policy.

@burgerdev burgerdev requested a review from katexochen as a code owner July 23, 2024 14:02
@burgerdev burgerdev requested review from msanft and Freax13 and removed request for katexochen July 23, 2024 14:02
@burgerdev burgerdev added the no changelog PRs not listed in the release notes label Jul 23, 2024
@katexochen katexochen self-requested a review July 23, 2024 14:07
@katexochen
Copy link
Member

iirc the 0003 patch is already a modified version of the upstream PR, isn't it?

@burgerdev
Copy link
Contributor Author

It sure looks different :) Should I then just merge the two?

@katexochen
Copy link
Member

katexochen commented Jul 23, 2024

idk, both fine for me. Costs nothing to add a patch and provide some history, might also be easier to roll-back if needed. Whatever you do, please document it properly (would also be nice if you could try to improve the comment of the existing patch), as it is obviously not covering all relevant points.

Copy link
Contributor

@msanft msanft left a comment

Choose a reason for hiding this comment

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

Having the new patch seems reasonable to me. Didn't test this yet on m50-ganondorf.

@Freax13
Copy link
Contributor

Freax13 commented Jul 24, 2024

iirc the 0003 patch is already a modified version of the upstream PR, isn't it?

Yes, I had to resolve some merge conflicts, so the bug was introduced by me.

@burgerdev
Copy link
Contributor Author

Everything addressed. @katexochen please check the patch docstring.

Copy link
Member

@katexochen katexochen left a comment

Choose a reason for hiding this comment

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

Doc comment looks good, thanks! (didn't test the change)

@burgerdev burgerdev merged commit 6ff4f9d into main Jul 24, 2024
9 checks passed
@burgerdev burgerdev deleted the burgerdev/tdx branch July 24, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants