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.

Sign up for GitHub

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

Jump to bottom

fix compilation for i386 and ppc64 #709

Merged
merged 18 commits into from
Oct 4, 2022
Merged

fix compilation for i386 and ppc64 #709

merged 18 commits into from
Oct 4, 2022

Conversation

tmakatos
Copy link
Member

@tmakatos tmakatos commented Sep 2, 2022

Signed-off-by: Thanos Makatos thanos.makatos@nutanix.com

@tmakatos tmakatos mentioned this pull request Sep 2, 2022
Closed
@jlevon
Copy link
Collaborator

jlevon commented Sep 3, 2022

I really hate PRix64 and friends, they make code so unreadable. Can't we just use %llx and cast the arg instead?

Also, if we want to support these (I'm surprised i686 is still relevant), we need some CI to at least build + test them.

@tmakatos
Copy link
Member Author

tmakatos commented Sep 5, 2022 •
edited
Loading

I really hate PRix64 and friends, they make code so unreadable. Can't we just use %llx and cast the arg instead?

Sure.

Also, if we want to support these (I'm surprised i686 is still relevant), we need some CI to at least build + test them.

Indeed.

@jlevon
Copy link
Collaborator

jlevon commented Sep 5, 2022

BTW, someone pointed me to:
https://lore.kernel.org/lkml/alpine.DEB.2.22.394.2208251435370.104368@digraph.polyomino.org.uk/

Looking forward to that!!

@tmakatos tmakatos force-pushed the issues/707 branch 5 times, most recently from 3c990b3 to 9549fb3 Compare September 5, 2022 13:14
@jlevon
Copy link
Collaborator

jlevon commented Sep 5, 2022

why do we need our own vfio_user_region_info?

@tmakatos
Copy link
Member Author

tmakatos commented Sep 5, 2022

why do we need our own vfio_user_region_info?

We don't, that was from the first version where I was working around the different typedef for u64 between x86_64 and i386.

@tmakatos tmakatos force-pushed the issues/707 branch 2 times, most recently from 025ddb5 to e579109 Compare September 5, 2022 13:40
@tmakatos
Copy link
Member Author

tmakatos commented Sep 5, 2022 •
edited
Loading

Also, if we want to support these (I'm surprised i686 is still relevant), we need some CI to at least build + test them.

Indeed.

From a quick look I had at https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners, it doesn't look like GitHub provides a 32-bit VM. Not sure we can/want to provide our own-hosted 32-bit runner. Alternatively we'd have to enable cross-compilation somehow.

@tmakatos
Copy link
Member Author

tmakatos commented Sep 5, 2022

Next, I'll test on PPC and then will mark PR ready for review.

@tmakatos
fix compilation on i386 and ppc64
2fd80c6 
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
Reported-by: Eduardo Lima <eblima@gmail.com>
@tmakatos tmakatos changed the title fix compilation for i686 fix compilation for i386 and ppc64 Sep 16, 2022
@tmakatos tmakatos marked this pull request as ready for review September 16, 2022 10:18
@jlevon
Copy link
Collaborator

jlevon commented Sep 18, 2022

Cross-compilation doesn't help too much, since we couldn't run tests.

jlevon
jlevon reviewed Sep 18, 2022
View reviewed changes
test/py/libvfio_user.py Outdated Show resolved Hide resolved
lib/dma.c Outdated Show resolved Hide resolved
@tmakatos
restore VFIO_REGION_INFO_FLAG_
5cf20c8
@tmakatos
fix compilation on i386
f1d1020 
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
Reported-by: Eduardo Lima <eblima@gmail.com>
@tmakatos
fix unit tests on i386
9261600 
Some unit tests don't work, looks like a ctypes bug,
I've disabled them for now on i386.

Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
@tmakatos
fix unit tests on i386
dfea6bb 
Some unit tests don't work, looks like a ctypes bug,
I've disabled them for now on i386.

Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
@tmakatos
Copy link
Member Author

tmakatos commented Sep 21, 2022 •
edited
Loading

I've added a few more fixes

nutanix and others added 6 commits September 22, 2022 12:15
don't assume page size is 4096
3b8438f 
This also fixes tests on PPC64.

Signed-off-by: nutanix <thanos.makatos@ntuanix.com>
@tmakatos
fix compilation on i386
2d05d03 
Signed-off-by: Thanos Makatos <thanos.makatos@ntuanix.com>
@tmakatos
fix flake errors
fe3dc78 
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
@tmakatos
fix more flake errors
aaf77d6 
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
@tmakatos
fix invalid syntax
4f2f1dd 
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
@tmakatos
remove forgotten debug print
2aecd32 
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
@tmakatos tmakatos self-assigned this Sep 22, 2022
@tmakatos
FIXME
bc5096b 
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
@tmakatos
don't use alloca
f4f9d8f 
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
@tmakatos
restore VFIO_REGION_INFO_FLAG_
58e3f18 
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
@tmakatos
restore VFIO_REGION_INFO_FLAG_
ed501c6 
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
@tmakatos
introduce ull_t for casting to unsigned long long
87aaab8 
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
@tmakatos
introduce ull_t for casting to unsigned long long
5bfff59 
Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
@tmakatos
Merge remote-tracking branch 'origin/master' into issues/707
545c2b1
jlevon
jlevon approved these changes Oct 4, 2022
View reviewed changes
Copy link
Collaborator

@jlevon jlevon left a comment

Choose a reason for hiding this comment

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

thanks

@tmakatos tmakatos merged commit aa19ba9 into master Oct 4, 2022
@tmakatos tmakatos deleted the issues/707 branch October 4, 2022 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlevon jlevon jlevon approved these changes

@swapnili swapnili Awaiting requested review from swapnili

Assignees

@tmakatos tmakatos

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tmakatos @jlevon