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

QEMU Apple Silicon: Find BIOS FD wherever #11453

Merged

Conversation

jonpspri
Copy link
Contributor

@jonpspri jonpspri commented Sep 5, 2021

QEmu normally install BIOS images under /usr/local prefix, but
Homebrew installs them under /opt/homebrew. This change searches both
locations and then puts back to an unpathed name if it doesn't find the
BIOS. (I imitated other architectures' implemenations in that failback
behavior.)

[NO TESTS NEEDED]

Signed-off-by: Jonathan Springer [email protected]

@vtolstov
Copy link

vtolstov commented Sep 5, 2021

LGTM (i'm using macbook air m1)

@mheon
Copy link
Member

mheon commented Sep 5, 2021

Can you squash the commits down into one, and ensure it has a signoff?
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 5, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonpspri, mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2021
@jonpspri
Copy link
Contributor Author

jonpspri commented Sep 5, 2021

Yep, will do. Still getting the hang of the contributors guidelines.

@jonpspri jonpspri force-pushed the qemu-apple-silicon-bios-fd branch 2 times, most recently from 0731349 to e356374 Compare September 5, 2021 23:09
@mheon
Copy link
Member

mheon commented Sep 6, 2021

Gofmt is complaining about trailing whitespace in the comment you added.

Otherwise LGTM

@@ -15,7 +16,7 @@ func (v *MachineVM) addArchOptions() []string {
"-accel", "hvf",
"-cpu", "cortex-a57",
"-M", "virt,highmem=off",
"-drive", "file=/usr/local/share/qemu/edk2-aarch64-code.fd,if=pflash,format=raw,readonly=on",
"-drive", "file=" + getEdk2CodeFd("edk2-aarch64-code.fd") + ",if=pflash,format=raw,readonly=on",
Copy link
Member

Choose a reason for hiding this comment

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

Does the Leading " + " matter here?
Wouldn't this be different then the existing code?

Copy link
Contributor Author

@jonpspri jonpspri Sep 6, 2021

Choose a reason for hiding this comment

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

Not sure what you mean by "The leading plus". getEdk2CodeFd will return either /usr/local/share/qemu/edk2-aarch64-code.fd or /opt/homebrew/share/qemu/edk2-aarch64-code.fd. Those get concatenated in where the full filename used to be. If it makes things clearer, I can switch to using a formatted string.

Unless you mean the diff plus... in which case it very definitely matters :).

return fullpath
}
}
return name
Copy link
Collaborator

Choose a reason for hiding this comment

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

In case if nothing is found inside loop or stat call fails, it should default to return filepath.Join(dirs[0], name) instead of return name at line no 56. Otherwise default return would be edk2-aarch64-code.fd which is not a valid path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the convention established in options_linux_arm64.go. I believe the rationale is to return a naked filename to pass to qemu and let it deal with trying to find it or erroring out. I considered adding go-style error returns across the package but decided instead to follow already-established patterns.

@rhatdan
Copy link
Member

rhatdan commented Sep 7, 2021

Looks like you need to run gofmt on your code to get by the test.

@jonpspri
Copy link
Contributor Author

jonpspri commented Sep 7, 2021

@rhatdan Yes. Waiting for your response on the review question you raised about the '+' up above. Want to be sure I didn't miss something before I kick off another squash.

@jonpspri jonpspri force-pushed the qemu-apple-silicon-bios-fd branch from e356374 to b95d065 Compare September 7, 2021 11:45
QEmu normally install BIOS images under `/usr/local` prefix, but
Homebrew installs them under `/opt/homebrew`.  This change searches both
locations and then puts back to an unpathed name if it doesn't find the
BIOS.  (I imitated other architectures' implemenations in that failback
behavior.)

[NO TESTS NEEDED]

Signed-off-by: Jonathan Springer <[email protected]>
@jonpspri jonpspri force-pushed the qemu-apple-silicon-bios-fd branch from b95d065 to 8b4f99a Compare September 7, 2021 13:01
@jonpspri
Copy link
Contributor Author

jonpspri commented Sep 7, 2021

Rebase...

@rhatdan
Copy link
Member

rhatdan commented Sep 7, 2021

I am fine with the explanation of the " + "
/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2021
@Luap99
Copy link
Member

Luap99 commented Sep 8, 2021

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2021
@openshift-merge-robot openshift-merge-robot merged commit cd43cf8 into containers:main Sep 8, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants