Peter Xu <pet...@redhat.com> wrote:
> On Fri, Feb 03, 2023 at 10:01:04PM +0100, Juan Quintela wrote:
>> Peter Xu <pet...@redhat.com> wrote:
>> > On Thu, Feb 02, 2023 at 11:52:21AM +0100, Juan Quintela wrote:
>> >> Peter Xu <pet...@redhat.com> wrote:
>> >> > Teach QEMU to use /dev/userfaultfd when it existed and fallback to the
>> >> > system call if either it's not there or doesn't have enough permission.
>> >> >
>> >> > Firstly, as long as the app has permission to access /dev/userfaultfd,
>> >> > it
>> >> > always have the ability to trap kernel faults which QEMU mostly wants.
>> >> > Meanwhile, in some context (e.g. containers) the userfaultfd syscall
>> >> > can be
>> >> > forbidden, so it can be the major way to use postcopy in a restricted
>> >> > environment with strict seccomp setup.
>> >> >
>> >> > Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
>> >> > Signed-off-by: Peter Xu <pet...@redhat.com>
>> >>
>> >>
>> >> Hi
>> >
>> > Hi, Juan,
>>
>>
>> >> static int open_userfaultd(void)
>> >> {
>> >> /*
>> >> * Make /dev/userfaultfd the default approach because it has better
>> >> * permission controls, meanwhile allows kernel faults without any
>> >> * privilege requirement (e.g. SYS_CAP_PTRACE).
>> >> */
>> >> int uffd = open("/dev/userfaultfd", O_RDWR | O_CLOEXEC);
>> >> if (uffd >= 0) {
>> >> return uffd;
>> >> }
>> >> return -1;
>> >> }
>> >>
>> >> int uffd_open(int flags)
>> >> {
>> >> #if defined(__linux__) && defined(__NR_userfaultfd)
>>
>> Just an incise, checkpatch don't liue that you use __linux__
>>
>> This file is compiled under CONFIG_LINUX, so you can drop it.
>
> Yes indeed. I'll drop it.
>
>>
>> >> static int uffd = -2;
>> >> if (uffd == -2) {
>> >> uffd = open_userfaultd();
>> >> }
>> >> if (uffd >= 0) {
>> >> return ioctl(uffd, USERFAULTFD_IOC_NEW, flags);
>> >> }
>> >> return syscall(__NR_userfaultfd, flags);
>> >> #else
>> >> return -EINVAL;
>> >>
>> >> 27 lines vs 42
>> >>
>> >> No need for enum type
>> >> No need for global variable
>> >>
>> >> What do you think?
>> >
>> > Yes, as I used to reply to Phil I think it can be simplified. I did this
>> > major for (1) better readability, and (2) being crystal clear on which way
>> > we used to open /dev/userfaultfd, then guarantee we're keeping using it. so
>> > at least I prefer keeping things like trace_uffd_detect_open_mode().
>>
>> The trace is ok for me. I just forgot to copy it on the rework, sorry.
>>
>> > I also plan to add another mode when fd-mode is there even if it'll reuse
>> > the same USERFAULTFD_IOC_NEW; they can be useful information when a failure
>> > happens.
>>
>> The other fd mode will change the uffd.
>>
>> What I *kind* of object is:
>> - Using a global variable when it is not needed
>> i.e. for me using a global variable means that anything else is worse.
>> Not the case IMHO.
>
> IMHO globals are evil when they're used in multiple places; that's bad to
> readability. Here it's not the case because it's set once and for
> all.
That is part of the problem.
int foo;
I need to search all the code to see where it is used.
int bar(...)
{
static int foo;
....
}
I am really sure that:
- foo value is preserved between calls
- it is not used anywhere else
without a single grep through the code.
> I
> wanted to have an easy and clear way to peek what's the mode chosen even
> without tracing enabled (e.g. from a dump or a live process).
I haven't thought about this. But you want something different than this?
(fada)$ cat /tmp/kk.c
int bar(void)
{
static int foo = 42;
return foo++;
}
int main(void)
{
int a = 7 + 1;
return a + bar();
}
(fada)$ gcc -Wall /tmp/kk.c -o /tmp/kkk -g
(fada)$ gdb /tmp/kkk
(gdb) b main
Breakpoint 1 at 0x401123: file /tmp/kk.c, line 10.
(gdb) p bar::foo
$1 = 42
(gdb)
And yes, I have to search how this is done O:-)
>> - Call uffd_open_mode() for every call, when we know that it can change,
>> it is going to return always the same value, so cache it.
>
> uffd_detect_open_mode() caches the result already? Or maybe you meant
> something else?
What I did. Only call the equilavent function once. You are calling it
every time that uffd_open() is called.
>
>>
>> > Though if you insist, I can switch to the simple version too.
>>
>> I always told that the person who did the patch has the last word on
>> style. I preffer my version, but it is up to you to take it or not.
>
> Thanks,
Later, Juan.
