* [PATCH v3 2/3] toshiba_acpi: Fix pr_* messages from USB Sleep Functions
From: Azael Avalos @ 2015-04-03 1:27 UTC (permalink / raw)
To: Darren Hart, platform-driver-x86, linux-kernel, linux-api; +Cc: Azael Avalos
This patch fixes the messages displayed by the USB Sleep Functions,
they were printing wrong messages not associated to the feature
currently queried.
Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
---
drivers/platform/x86/toshiba_acpi.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 37f5f64..f624dd5 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -965,11 +965,11 @@ static int toshiba_usb_rapid_charge_get(struct toshiba_acpi_dev *dev,
status = tci_raw(dev, in, out);
sci_close(dev);
if (ACPI_FAILURE(status) || out[0] == TOS_FAILURE) {
- pr_err("ACPI call to get USB S&C battery level failed\n");
+ pr_err("ACPI call to get USB Rapid Charge failed\n");
return -EIO;
} else if (out[0] == TOS_NOT_SUPPORTED ||
out[0] == TOS_INPUT_DATA_ERROR) {
- pr_info("USB Sleep and Charge not supported\n");
+ pr_info("USB Rapid Charge not supported\n");
return -ENODEV;
}
@@ -993,10 +993,10 @@ static int toshiba_usb_rapid_charge_set(struct toshiba_acpi_dev *dev,
status = tci_raw(dev, in, out);
sci_close(dev);
if (ACPI_FAILURE(status) || out[0] == TOS_FAILURE) {
- pr_err("ACPI call to set USB S&C battery level failed\n");
+ pr_err("ACPI call to set USB Rapid Charge failed\n");
return -EIO;
} else if (out[0] == TOS_NOT_SUPPORTED) {
- pr_info("USB Sleep and Charge not supported\n");
+ pr_info("USB Rapid Charge not supported\n");
return -ENODEV;
} else if (out[0] == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -1015,10 +1015,10 @@ static int toshiba_usb_sleep_music_get(struct toshiba_acpi_dev *dev, u32 *state)
result = sci_read(dev, SCI_USB_SLEEP_MUSIC, state);
sci_close(dev);
if (result == TOS_FAILURE) {
- pr_err("ACPI call to set USB S&C mode failed\n");
+ pr_err("ACPI call to get Sleep and Music failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("USB Sleep and Charge not supported\n");
+ pr_info("Sleep and Music not supported\n");
return -ENODEV;
} else if (result == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -1037,10 +1037,10 @@ static int toshiba_usb_sleep_music_set(struct toshiba_acpi_dev *dev, u32 state)
result = sci_write(dev, SCI_USB_SLEEP_MUSIC, state);
sci_close(dev);
if (result == TOS_FAILURE) {
- pr_err("ACPI call to set USB S&C mode failed\n");
+ pr_err("ACPI call to set Sleep and Music failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("USB Sleep and Charge not supported\n");
+ pr_info("Sleep and Music not supported\n");
return -ENODEV;
} else if (result == TOS_INPUT_DATA_ERROR) {
return -EIO;
--
2.3.4
^ permalink raw reply related
* Re: [PATCH] selftests/mount: output error messages when mount test fail
From: Zhang Zhen @ 2015-04-03 1:17 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA, Linux Kernel Mailing List,
shuahkh-JPH+aEBZ4P+UEJcrhfAQsw, mpe-Gsx/Oe8HsFggBc27wqDAHg
In-Reply-To: <87r3s2qrzc.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
On 2015/4/3 2:56, Eric W. Biederman wrote:
> Zhang Zhen <zhenzhang.zhang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> writes:
>
>> Without this patch, if /proc/self/uid_map is not exist,
>> the mount test case will fail and no any prompting.
>
> The intent was not to fail if /proc/self/uid_map is missing but to skip
> the test because it is not applicable.
>
> Making this an error seems wrong. Even if I did mess up the shell
> script error propagation and made it an error by accident.
>
Ok， modify the output messages as follows：
WARN: No /proc/self/uid_map exist, test skipped.
Do you think this modification right?
Best regards!
>> After applied this patch, the case will prompt why it fail.
>> Just as follows:
>> root@kernel-host:/opt/kernel> make -C tools/testing/selftests TARGETS=mount run_tests
>> make: Entering directory `/opt/kernel/tools/testing/selftests'
>> for TARGET in mount; do \
>> make -C $TARGET; \
>> done;
>> make[1]: Entering directory `/opt/kernel/tools/testing/selftests/mount'
>> make[1]: Nothing to be done for `all'.
>> make[1]: Leaving directory `/opt/kernel/tools/testing/selftests/mount'
>> for TARGET in mount; do \
>> make -C $TARGET run_tests; \
>> done;
>> make[1]: Entering directory `/opt/kernel/tools/testing/selftests/mount'
>> ERROR: No /proc/self/uid_map exist
>> make[1]: Leaving directory `/opt/kernel/tools/testing/selftests/mount'
>> make: Leaving directory `/opt/kernel/tools/testing/selftests'
>>
>> Signed-off-by: Zhang Zhen <zhenzhang.zhang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>> tools/testing/selftests/mount/Makefile | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/mount/Makefile b/tools/testing/selftests/mount/Makefile
>> index a5b367f..b3266db 100644
>> --- a/tools/testing/selftests/mount/Makefile
>> +++ b/tools/testing/selftests/mount/Makefile
>> @@ -8,7 +8,12 @@ unprivileged-remount-test: unprivileged-remount-test.c
>> include ../lib.mk
>>
>> TEST_PROGS := unprivileged-remount-test
>> -override RUN_TESTS := if [ -f /proc/self/uid_map ] ; then ./unprivileged-remount-test ; fi
>> +override RUN_TESTS := @if [ -f /proc/self/uid_map ] ; \
>> + then \
>> + ./unprivileged-remount-test ; \
>> + else \
>> + echo "ERROR: No /proc/self/uid_map exist" ; \
>> + fi
>> override EMIT_TESTS := echo "$(RUN_TESTS)"
>>
>> clean:
>
>
^ permalink raw reply
* Re: [patch -mm] mm, doc: cleanup and clarify munmap behavior for hugetlb memory fix
From: Hugh Dickins @ 2015-04-03 1:05 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Hugh Dickins, Jonathan Corbet, Davide Libenzi,
Luiz Capitulino, Shuah Khan, Andrea Arcangeli, Joern Engel,
Jianguo Wu, Eric B Munson, linux-mm, linux-kernel, linux-api,
linux-doc
In-Reply-To: <alpine.DEB.2.10.1504021547330.15536@chino.kir.corp.google.com>
On Thu, 2 Apr 2015, David Rientjes wrote:
> Don't only specify munmap(2) behavior with respect the hugetlb memory, all
> other syscalls get naturally aligned to the native page size of the
> processor. Rather, pick out munmap(2) as a specific example.
>
> Signed-off-by: David Rientjes <rientjes@google.com>
Thanks, yes, good wording: it is best to be a bit vague here,
since each msyscall takes the approach most convenient for it.
Acked-by: Hugh Dickins <hughd@google.com>
> ---
> Documentation/vm/hugetlbpage.txt | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/vm/hugetlbpage.txt b/Documentation/vm/hugetlbpage.txt
> index 1270fb1..030977f 100644
> --- a/Documentation/vm/hugetlbpage.txt
> +++ b/Documentation/vm/hugetlbpage.txt
> @@ -313,8 +313,11 @@ into /proc/sys/vm/hugetlb_shm_group. It is possible for same or different
> applications to use any combination of mmaps and shm* calls, though the mount of
> filesystem will be required for using mmap calls without MAP_HUGETLB.
>
> -When using munmap(2) to unmap hugetlb memory, the length specified must be
> -hugepage aligned, otherwise it will fail with errno set to EINVAL.
> +Syscalls that operate on memory backed by hugetlb pages only have their lengths
> +aligned to the native page size of the processor; they will normally fail with
> +errno set to EINVAL or exclude hugetlb pages that extend beyond the length if
> +not hugepage aligned. For example, munmap(2) will fail if memory is backed by
> +a hugetlb page and the length is smaller than the hugepage size.
>
>
> Examples
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [patch -mm] mm, doc: cleanup and clarify munmap behavior for hugetlb memory fix
From: David Rientjes @ 2015-04-02 22:50 UTC (permalink / raw)
To: Andrew Morton
Cc: Hugh Dickins, Jonathan Corbet, Davide Libenzi, Luiz Capitulino,
Shuah Khan, Andrea Arcangeli, Joern Engel, Jianguo Wu,
Eric B Munson, linux-mm, linux-kernel, linux-api, linux-doc
In-Reply-To: <alpine.DEB.2.10.1504021536210.15536@chino.kir.corp.google.com>
Don't only specify munmap(2) behavior with respect the hugetlb memory, all
other syscalls get naturally aligned to the native page size of the
processor. Rather, pick out munmap(2) as a specific example.
Signed-off-by: David Rientjes <rientjes@google.com>
---
Documentation/vm/hugetlbpage.txt | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/Documentation/vm/hugetlbpage.txt b/Documentation/vm/hugetlbpage.txt
index 1270fb1..030977f 100644
--- a/Documentation/vm/hugetlbpage.txt
+++ b/Documentation/vm/hugetlbpage.txt
@@ -313,8 +313,11 @@ into /proc/sys/vm/hugetlb_shm_group. It is possible for same or different
applications to use any combination of mmaps and shm* calls, though the mount of
filesystem will be required for using mmap calls without MAP_HUGETLB.
-When using munmap(2) to unmap hugetlb memory, the length specified must be
-hugepage aligned, otherwise it will fail with errno set to EINVAL.
+Syscalls that operate on memory backed by hugetlb pages only have their lengths
+aligned to the native page size of the processor; they will normally fail with
+errno set to EINVAL or exclude hugetlb pages that extend beyond the length if
+not hugepage aligned. For example, munmap(2) will fail if memory is backed by
+a hugetlb page and the length is smaller than the hugepage size.
Examples
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [patch 1/2] mm, doc: cleanup and clarify munmap behavior for hugetlb memory
From: David Rientjes @ 2015-04-02 22:40 UTC (permalink / raw)
To: Hugh Dickins
Cc: Andrew Morton, Jonathan Corbet, Davide Libenzi, Luiz Capitulino,
Shuah Khan, Andrea Arcangeli, Joern Engel, Jianguo Wu,
Eric B Munson, linux-mm, linux-kernel, linux-api, linux-doc
In-Reply-To: <alpine.LSU.2.11.1503291801400.1052@eggly.anvils>
On Sun, 29 Mar 2015, Hugh Dickins wrote:
> > munmap(2) of hugetlb memory requires a length that is hugepage aligned,
> > otherwise it may fail. Add this to the documentation.
>
> Thanks for taking this on, David. But although munmap(2) is the one
> Davide called out, it goes beyond that, doesn't it? To mprotect and
> madvise and ...
>
Yes, good point, munmap(2) isn't special in this case, the alignment to
the native page size of the platform should apply to madvise, mbind,
mincore, mlock, mprotect, remap_file_pages, etc.
I'd hesitate to compile any authoritative list on the behavior in
Documentation/vm/hugetlbpage.txt since it would exclude future extensions,
but I'll update it to be more inclusive of other mm syscalls rather than
specify only munmap(2).
^ permalink raw reply
* Re: [PATCH v2] pci: export class IDs from pci_ids.h
From: Bjorn Helgaas @ 2015-04-02 22:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel@vger.kernel.org, Jonathan Corbet, David S. Miller,
Hans Verkuil, Mauro Carvalho Chehab, Alexei Starovoitov,
stephen hemminger, Masahiro Yamada, linux-pci@vger.kernel.org,
linux-doc@vger.kernel.org, linux-api, Greg KH, Martin Mareš
In-Reply-To: <20150402232804-mutt-send-email-mst@redhat.com>
[+cc Martin]
On Thu, Apr 2, 2015 at 4:28 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Apr 02, 2015 at 03:53:47PM -0500, Bjorn Helgaas wrote:
>> On Mon, Mar 30, 2015 at 01:33:28PM +0200, Michael S. Tsirkin wrote:
>> > The basic class ID macros in pci_ids.h are pretty useful for userspace
>> > using the pci sysfs interface, and they aren't fundamentally different
>> > from the constants in pci_regs.h - both are defined in the
>> > pci spec.
>> >
>> > At the moment userspace is forced to duplicate these macros
>> > (e.g. QEMU does this, so do seabios, gpxe, and others), it is better to
>> > expose them in /usr/include/linux/pci_ids.h so everyone can just include
>> > this header.
>>
>> I agree that it would be nice for applications to get these definitions
>> from a single place, but I'm not sure that include/uapi/linux/pci_ids.h
>> needs to be that place.
>>
>> These constants are just copies of what's in the spec, and I don't think
>> you're suggesting that the constants are necessary to use a kernel API.
>>
>> I know the kernel does provide access to values via sysfs "class" files,
>> but the kernel is just passing the values through from the hardware.
>> That's analogous to reading the class with setpci, and I don't think it
>> leads to a requirement that the kernel export all the information about how
>> to interpret the class values.
>>
>> I haven't looked at libpci or libudev, but it sounds like you think those
>> are not good solutions. Is that because they don't currently have this
>> information? People don't want to add dependencies on them?
>
> People don't want to add dependencies on them.
Why not? I'm not a user-space programmer, so it's not obvious to me
what the problems with adding a dependency are. If a package provides
functionality you want, it seems like a *good* thing to use it and
depend on it rather than reimplementing the functionality yourself.
The /usr/include/pci/header.h supplied by libpci-dev (on Ubuntu) looks
like it has most or all of the constants you want.
Bjorn
^ permalink raw reply
* Re: [PATCH v2] pci: export class IDs from pci_ids.h
From: Greg KH @ 2015-04-02 21:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Bjorn Helgaas, linux-kernel, Jonathan Corbet, David S. Miller,
Hans Verkuil, Mauro Carvalho Chehab, Alexei Starovoitov,
stephen hemminger, Masahiro Yamada, linux-pci, linux-doc,
linux-api
In-Reply-To: <20150402232804-mutt-send-email-mst@redhat.com>
On Thu, Apr 02, 2015 at 11:28:19PM +0200, Michael S. Tsirkin wrote:
> On Thu, Apr 02, 2015 at 03:53:47PM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 30, 2015 at 01:33:28PM +0200, Michael S. Tsirkin wrote:
> > > The basic class ID macros in pci_ids.h are pretty useful for userspace
> > > using the pci sysfs interface, and they aren't fundamentally different
> > > from the constants in pci_regs.h - both are defined in the
> > > pci spec.
> > >
> > > At the moment userspace is forced to duplicate these macros
> > > (e.g. QEMU does this, so do seabios, gpxe, and others), it is better to
> > > expose them in /usr/include/linux/pci_ids.h so everyone can just include
> > > this header.
> >
> > I agree that it would be nice for applications to get these definitions
> > from a single place, but I'm not sure that include/uapi/linux/pci_ids.h
> > needs to be that place.
> >
> > These constants are just copies of what's in the spec, and I don't think
> > you're suggesting that the constants are necessary to use a kernel API.
> >
> > I know the kernel does provide access to values via sysfs "class" files,
> > but the kernel is just passing the values through from the hardware.
> > That's analogous to reading the class with setpci, and I don't think it
> > leads to a requirement that the kernel export all the information about how
> > to interpret the class values.
> >
> > I haven't looked at libpci or libudev, but it sounds like you think those
> > are not good solutions. Is that because they don't currently have this
> > information? People don't want to add dependencies on them?
> >
> > Bjorn
>
> People don't want to add dependencies on them.
So you want us to create yet-a-third-location for these values? Both
libpci and libudev are on all systems. And it's not a run-time
dependancy you are talking about here, but a build-time one, which is
quite different. Having a build-time dependancy on packages that ship
with all Linux distributions seems acceptable to me.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2] pci: export class IDs from pci_ids.h
From: Michael S. Tsirkin @ 2015-04-02 21:28 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, Jonathan Corbet, David S. Miller, Hans Verkuil,
Mauro Carvalho Chehab, Alexei Starovoitov, stephen hemminger,
Masahiro Yamada, linux-pci, linux-doc, linux-api, Greg KH
In-Reply-To: <20150402205347.GA10892@google.com>
On Thu, Apr 02, 2015 at 03:53:47PM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 30, 2015 at 01:33:28PM +0200, Michael S. Tsirkin wrote:
> > The basic class ID macros in pci_ids.h are pretty useful for userspace
> > using the pci sysfs interface, and they aren't fundamentally different
> > from the constants in pci_regs.h - both are defined in the
> > pci spec.
> >
> > At the moment userspace is forced to duplicate these macros
> > (e.g. QEMU does this, so do seabios, gpxe, and others), it is better to
> > expose them in /usr/include/linux/pci_ids.h so everyone can just include
> > this header.
>
> I agree that it would be nice for applications to get these definitions
> from a single place, but I'm not sure that include/uapi/linux/pci_ids.h
> needs to be that place.
>
> These constants are just copies of what's in the spec, and I don't think
> you're suggesting that the constants are necessary to use a kernel API.
>
> I know the kernel does provide access to values via sysfs "class" files,
> but the kernel is just passing the values through from the hardware.
> That's analogous to reading the class with setpci, and I don't think it
> leads to a requirement that the kernel export all the information about how
> to interpret the class values.
>
> I haven't looked at libpci or libudev, but it sounds like you think those
> are not good solutions. Is that because they don't currently have this
> information? People don't want to add dependencies on them?
>
> Bjorn
People don't want to add dependencies on them.
--
MST
^ permalink raw reply
* Re: [PATCH v2] pci: export class IDs from pci_ids.h
From: Bjorn Helgaas @ 2015-04-02 20:53 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Jonathan Corbet, David S. Miller, Hans Verkuil,
Mauro Carvalho Chehab, Alexei Starovoitov, stephen hemminger,
Masahiro Yamada, linux-pci, linux-doc, linux-api, Greg KH
In-Reply-To: <1427714755-16873-1-git-send-email-mst@redhat.com>
On Mon, Mar 30, 2015 at 01:33:28PM +0200, Michael S. Tsirkin wrote:
> The basic class ID macros in pci_ids.h are pretty useful for userspace
> using the pci sysfs interface, and they aren't fundamentally different
> from the constants in pci_regs.h - both are defined in the
> pci spec.
>
> At the moment userspace is forced to duplicate these macros
> (e.g. QEMU does this, so do seabios, gpxe, and others), it is better to
> expose them in /usr/include/linux/pci_ids.h so everyone can just include
> this header.
I agree that it would be nice for applications to get these definitions
from a single place, but I'm not sure that include/uapi/linux/pci_ids.h
needs to be that place.
These constants are just copies of what's in the spec, and I don't think
you're suggesting that the constants are necessary to use a kernel API.
I know the kernel does provide access to values via sysfs "class" files,
but the kernel is just passing the values through from the hardware.
That's analogous to reading the class with setpci, and I don't think it
leads to a requirement that the kernel export all the information about how
to interpret the class values.
I haven't looked at libpci or libudev, but it sounds like you think those
are not good solutions. Is that because they don't currently have this
information? People don't want to add dependencies on them?
Bjorn
^ permalink raw reply
* Re: [PATCH] selftests/mount: output error messages when mount test fail
From: Eric W. Biederman @ 2015-04-02 18:56 UTC (permalink / raw)
To: Zhang Zhen; +Cc: linux-api, Linux Kernel Mailing List, shuahkh, mpe
In-Reply-To: <551CE72E.3050303@huawei.com>
Zhang Zhen <zhenzhang.zhang@huawei.com> writes:
> Without this patch, if /proc/self/uid_map is not exist,
> the mount test case will fail and no any prompting.
The intent was not to fail if /proc/self/uid_map is missing but to skip
the test because it is not applicable.
Making this an error seems wrong. Even if I did mess up the shell
script error propagation and made it an error by accident.
> After applied this patch, the case will prompt why it fail.
> Just as follows:
> root@kernel-host:/opt/kernel> make -C tools/testing/selftests TARGETS=mount run_tests
> make: Entering directory `/opt/kernel/tools/testing/selftests'
> for TARGET in mount; do \
> make -C $TARGET; \
> done;
> make[1]: Entering directory `/opt/kernel/tools/testing/selftests/mount'
> make[1]: Nothing to be done for `all'.
> make[1]: Leaving directory `/opt/kernel/tools/testing/selftests/mount'
> for TARGET in mount; do \
> make -C $TARGET run_tests; \
> done;
> make[1]: Entering directory `/opt/kernel/tools/testing/selftests/mount'
> ERROR: No /proc/self/uid_map exist
> make[1]: Leaving directory `/opt/kernel/tools/testing/selftests/mount'
> make: Leaving directory `/opt/kernel/tools/testing/selftests'
>
> Signed-off-by: Zhang Zhen <zhenzhang.zhang@huawei.com>
> ---
> tools/testing/selftests/mount/Makefile | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/mount/Makefile b/tools/testing/selftests/mount/Makefile
> index a5b367f..b3266db 100644
> --- a/tools/testing/selftests/mount/Makefile
> +++ b/tools/testing/selftests/mount/Makefile
> @@ -8,7 +8,12 @@ unprivileged-remount-test: unprivileged-remount-test.c
> include ../lib.mk
>
> TEST_PROGS := unprivileged-remount-test
> -override RUN_TESTS := if [ -f /proc/self/uid_map ] ; then ./unprivileged-remount-test ; fi
> +override RUN_TESTS := @if [ -f /proc/self/uid_map ] ; \
> + then \
> + ./unprivileged-remount-test ; \
> + else \
> + echo "ERROR: No /proc/self/uid_map exist" ; \
> + fi
> override EMIT_TESTS := echo "$(RUN_TESTS)"
>
> clean:
^ permalink raw reply
* Re: [PATCH v2 7/7] vhost: feature to set the vring endianness
From: Michael S. Tsirkin @ 2015-04-02 18:53 UTC (permalink / raw)
To: Greg Kurz; +Cc: Rusty Russell, linux-api, linux-kernel, kvm, virtualization
In-Reply-To: <20150402184527.0bc68efe@bahia.local>
On Thu, Apr 02, 2015 at 06:45:27PM +0200, Greg Kurz wrote:
> On Thu, 2 Apr 2015 16:20:46 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Thu, Apr 02, 2015 at 03:17:13PM +0200, Greg Kurz wrote:
> > > This patch brings cross-endian support to vhost when used to implement
> > > legacy virtio devices. Since it is a relatively rare situation, the feature
> > > availability is controlled by a kernel config option (not set by default).
> > >
> > > If cross-endian support is compiled in, vhost abvertises a new feature
> > > to be negotiated with userspace. If userspace acknowledges the feature,
> > > it can inform vhost about the endianness to use with a new ioctl.
> > >
> > > This feature is mutually exclusive with virtio 1.0. Even if the vhost device
> > > advertises virtio 1.0 and legacy cross-endian features, it cannot receive
> > > aknowledgement for both at the same time.
> > >
> > > Hot paths are being preserved from any penalty when the config option is
> > > disabled or when virtio 1.0 is being used.
> > >
> > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > ---
> > > drivers/vhost/Kconfig | 10 ++++++++++
> > > drivers/vhost/net.c | 5 +++++
> > > drivers/vhost/scsi.c | 4 ++++
> > > drivers/vhost/test.c | 4 ++++
> > > drivers/vhost/vhost.c | 19 +++++++++++++++++++
> > > drivers/vhost/vhost.h | 13 ++++++++++++-
> > > include/uapi/linux/vhost.h | 10 ++++++++++
> > > 7 files changed, 64 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > > index 017a1e8..5bb8da9 100644
> > > --- a/drivers/vhost/Kconfig
> > > +++ b/drivers/vhost/Kconfig
> > > @@ -32,3 +32,13 @@ config VHOST
> > > ---help---
> > > This option is selected by any driver which needs to access
> > > the core of vhost.
> > > +
> > > +config VHOST_SET_ENDIAN_LEGACY
> > > + bool "Cross-endian support for host kernel accelerator"
> > > + default n
> > > + ---help---
> > > + This option allows vhost to support guests with a different byte
> > > + ordering
> >
> > from host
> >
> > >. It is disabled by default since it adds overhead and it
> > > + is only needed by a few platforms (powerpc and arm).
> > > +
> > > + If unsure, say "n".
> >
> > "N"
> >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 2bbfc25..5274a44 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -1011,6 +1011,11 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
> > > vhost_hlen = 0;
> > > sock_hlen = hdr_len;
> > > }
> > > +
> > > + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > > + (1ULL << VIRTIO_F_VERSION_1)))
> > > + return -EINVAL;
> > > +
> > > mutex_lock(&n->dev.mutex);
> > > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > > !vhost_log_access_ok(&n->dev)) {
> > > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > > index 71df240..b53e9c2 100644
> > > --- a/drivers/vhost/scsi.c
> > > +++ b/drivers/vhost/scsi.c
> > > @@ -1544,6 +1544,10 @@ static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features)
> > > if (features & ~VHOST_SCSI_FEATURES)
> > > return -EOPNOTSUPP;
> > >
> > > + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > > + (1ULL << VIRTIO_F_VERSION_1)))
> > > + return -EINVAL;
> > > +
> > > mutex_lock(&vs->dev.mutex);
> > > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > > !vhost_log_access_ok(&vs->dev)) {
> > > diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> > > index d9c501e..cfefdad 100644
> > > --- a/drivers/vhost/test.c
> > > +++ b/drivers/vhost/test.c
> > > @@ -243,6 +243,10 @@ static int vhost_test_set_features(struct vhost_test *n, u64 features)
> > > {
> > > struct vhost_virtqueue *vq;
> > >
> > > + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > > + (1ULL << VIRTIO_F_VERSION_1)))
> > > + return -EINVAL;
> > > +
> > > mutex_lock(&n->dev.mutex);
> > > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > > !vhost_log_access_ok(&n->dev)) {
> >
> > Above seems to prevent users from specifying either
> > VIRTIO_F_VERSION_1 or VHOST_F_SET_ENDIAN_LEGACY.
> > Does it actually work?
> >
>
> Arggg no it doesn't *of course*... I've added these bogus checks lately
> and didn't re-test :(
BTW I would be happier with less ifdefs all over the code,
using some inline wrappers defined differently depending on ifdef.
For example:
#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
bool vhost_vq_legacy_is_little_endian(vq)
{
return !vq->legacy_big_endian;
}
#else
bool vhost_vq_legacy_is_little_endian(vq)
{
return virtio_legacy_is_little_endian();
}
#endif
similarly put VHOST_SET_VRING_ENDIAN_LEGACY handling
in a separate function, with a stub definition
when functionality is configured out.
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index 2ee2826..60a0f32 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -199,6 +199,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> > > vq->call = NULL;
> > > vq->log_ctx = NULL;
> > > vq->memory = NULL;
> > > + vq->legacy_big_endian = false;
> > > }
> > >
> > > static int vhost_worker(void *data)
> > > @@ -806,6 +807,24 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> > > } else
> > > filep = eventfp;
> > > break;
> > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > > + case VHOST_SET_VRING_ENDIAN_LEGACY:
> > > + {
> > > + struct vhost_vring_endian e;
> > > +
> > > + if (!vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY)) {
> > > + r = -EINVAL;
> > > + break;
> > > + }
> > > +
> > > + if (copy_from_user(&e, argp, sizeof(e))) {
> > > + r = -EFAULT;
> > > + break;
> > > + }
> > > + vq->legacy_big_endian = e.is_big_endian;
> > > + break;
> > > + }
> > > +#endif
> > > default:
> > > r = -ENOIOCTLCMD;
> > > }
> > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > > index 4e9a186..d50881d 100644
> > > --- a/drivers/vhost/vhost.h
> > > +++ b/drivers/vhost/vhost.h
> > > @@ -106,6 +106,9 @@ struct vhost_virtqueue {
> > > /* Log write descriptors */
> > > void __user *log_base;
> > > struct vhost_log *log;
> > > +
> > > + /* We need to know the device endianness with legacy virtio. */
> > > + bool legacy_big_endian;
> > > };
> > >
> > > struct vhost_dev {
> > > @@ -165,7 +168,11 @@ enum {
> > > VHOST_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |
> > > (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> > > (1ULL << VIRTIO_RING_F_EVENT_IDX) |
> > > - (1ULL << VHOST_F_LOG_ALL),
> > > + (1ULL << VHOST_F_LOG_ALL) |
> > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > > + (1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > > +#endif
> > > + 0ULL,
> > > };
> > >
> > > static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> > > @@ -177,6 +184,10 @@ static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> > > {
> > > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > > return true;
> > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > > + if (vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY))
> > > + return !vq->legacy_big_endian;
> >
> > why do we need to check the feature bit?
> > How about simple
> > return !vq->legacy_big_endian;
> > here?
>
> This is a runtime feature. Even for powerpc, cross-endian won't be the
> most common scenario. Userspace may have cleared the bit if it doesn't
> need/want it.
Right but if we drop VHOST_F_SET_ENDIAN_LEGACY then
it's simply
#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
return !vq->legacy_big_endian;
#else
return virtio_legacy_is_little_endian();
#endif
> > All you need to do is set legacy_big_endian to
> > !virtio_legacy_is_little_endian() on reset.
> > Maybe rename to legacy_is_little_endian so you don't
> > need to reverse the logic.
> >
>
> Do you mean vhost doesn't need userspace to provide the endianness
> to be used ? Please elaborate on the meaning of "reset" here... I
> am confused.
I refer to vhost_vq_reset here.
> > > +#endif
> > > return virtio_legacy_is_little_endian();
> > > }
> > >
> > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > index bb6a5b4..09d2a48 100644
> > > --- a/include/uapi/linux/vhost.h
> > > +++ b/include/uapi/linux/vhost.h
> > > @@ -47,6 +47,11 @@ struct vhost_vring_addr {
> > > __u64 log_guest_addr;
> > > };
> > >
> > > +struct vhost_vring_endian {
> > > + unsigned int index;
> > > + bool is_big_endian;
> >
> > bool in uapi is a bad idea.
> > Generally, I think you can use vhost_vring_state here.
> >
>
> Ok to turn is_big_endian into an int but why hijacking vhost_vring_state ?
Yes.
>
> > > +};
> > > +
> > > struct vhost_memory_region {
> > > __u64 guest_phys_addr;
> > > __u64 memory_size; /* bytes */
> > > @@ -103,6 +108,9 @@ struct vhost_memory {
> > > /* Get accessor: reads index, writes value in num */
> > > #define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state)
> > >
> > > +/* Set endianness for the ring (legacy virtio only) */
> > > +#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_endian)
> > > +
> > > /* The following ioctls use eventfd file descriptors to signal and poll
> > > * for events. */
> > >
> >
> > You also need a GET ioctl, as a matter of policy.
> >
>
> Ok. Will add.
>
> > > @@ -126,6 +134,8 @@ struct vhost_memory {
> > > #define VHOST_F_LOG_ALL 26
> > > /* vhost-net should add virtio_net_hdr for RX, and strip for TX packets. */
> > > #define VHOST_NET_F_VIRTIO_NET_HDR 27
> > > +/* the vring endianness can be explicitly set (legacy virtio only). */
> > > +#define VHOST_F_SET_ENDIAN_LEGACY 28
> > >
> > > /* VHOST_SCSI specific definitions */
> >
> >
> > VHOST_F_SET_ENDIAN_LEGACY doesn't seem too useful.
> > Is this so userspace can detect kernel configuration?
>
> Yes. Also, it is quite easy to "un-ack" the feature when
> it is not wanted.
>
> > I think probing VHOST_SET_VRING_ENDIAN_LEGACY should
> > be sufficient for this.
> >
>
> I'll get rid of the feature.
>
> Thanks for your time Michael.
>
> --
> Greg
^ permalink raw reply
* Re: [PATCH] selftests/mount: output error messages when mount test fail
From: Shuah Khan @ 2015-04-02 18:03 UTC (permalink / raw)
To: Zhang Zhen, linux-api-u79uwXL29TY76Z2rM5mHXA,
Linux Kernel Mailing List
Cc: mpe-Gsx/Oe8HsFggBc27wqDAHg, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
Shuah Khan
In-Reply-To: <551CE72E.3050303-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
On 04/02/2015 12:52 AM, Zhang Zhen wrote:
> Without this patch, if /proc/self/uid_map is not exist,
> the mount test case will fail and no any prompting.
>
> After applied this patch, the case will prompt why it fail.
> Just as follows:
> root@kernel-host:/opt/kernel> make -C tools/testing/selftests TARGETS=mount run_tests
> make: Entering directory `/opt/kernel/tools/testing/selftests'
> for TARGET in mount; do \
> make -C $TARGET; \
> done;
> make[1]: Entering directory `/opt/kernel/tools/testing/selftests/mount'
> make[1]: Nothing to be done for `all'.
> make[1]: Leaving directory `/opt/kernel/tools/testing/selftests/mount'
> for TARGET in mount; do \
> make -C $TARGET run_tests; \
> done;
> make[1]: Entering directory `/opt/kernel/tools/testing/selftests/mount'
> ERROR: No /proc/self/uid_map exist
> make[1]: Leaving directory `/opt/kernel/tools/testing/selftests/mount'
> make: Leaving directory `/opt/kernel/tools/testing/selftests'
>
> Signed-off-by: Zhang Zhen <zhenzhang.zhang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
> tools/testing/selftests/mount/Makefile | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/mount/Makefile b/tools/testing/selftests/mount/Makefile
> index a5b367f..b3266db 100644
> --- a/tools/testing/selftests/mount/Makefile
> +++ b/tools/testing/selftests/mount/Makefile
> @@ -8,7 +8,12 @@ unprivileged-remount-test: unprivileged-remount-test.c
> include ../lib.mk
>
> TEST_PROGS := unprivileged-remount-test
> -override RUN_TESTS := if [ -f /proc/self/uid_map ] ; then ./unprivileged-remount-test ; fi
> +override RUN_TESTS := @if [ -f /proc/self/uid_map ] ; \
> + then \
> + ./unprivileged-remount-test ; \
> + else \
> + echo "ERROR: No /proc/self/uid_map exist" ; \
> + fi
> override EMIT_TESTS := echo "$(RUN_TESTS)"
>
> clean:
>
This patch introduces syntax error:
Running tests in mount
========================================
./run_kselftest.sh: line 53: syntax error near unexpected token `then'
./run_kselftest.sh: line 53: `@if [ -f /proc/self/uid_map ] ; then
./unprivileged-remount-test ; else echo ERROR: No /proc/self/uid_map
exist ; fi'
You will see it if you run kselftest_install.sh from
tools/testing/selftests and then attempt to run the tests using the
generated
run_kselftest.sh script.
Could you please fix this and re-send the patch.
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org | (970) 217-8978
^ permalink raw reply
* Re: [PATCH net-next] tc: bpf: add checksum helpers
From: Daniel Borkmann @ 2015-04-02 18:01 UTC (permalink / raw)
To: Alexei Starovoitov, Jiri Pirko
Cc: David S. Miller, Jamal Hadi Salim, linux-api, netdev
In-Reply-To: <551D82BA.1010809@plumgrid.com>
On 04/02/2015 07:56 PM, Alexei Starovoitov wrote:
> On 4/2/2015 10:46 AM, Jiri Pirko wrote:
>> Thu, Apr 02, 2015 at 06:34:59PM CEST, ast@plumgrid.com wrote:
>>> On 4/2/2015 8:21 AM, Daniel Borkmann wrote:
>>>> On 04/02/2015 05:15 PM, Jiri Pirko wrote:
>>>>> Thu, Apr 02, 2015 at 05:00:47PM CEST, daniel@iogearbox.net wrote:
>>>>>> On 04/02/2015 02:12 AM, Alexei Starovoitov wrote:
>>>>>>> Commit 608cd71a9c7c ("tc: bpf: generalize pedit action") has added the
>>>>>>> possibility to mangle packet data to BPF programs in the tc pipeline.
>>>>>>> This patch adds two helpers bpf_l3_csum_replace() and
>>>>>>> bpf_l4_csum_replace()
>>>>>>> for fixing up the protocol checksums after the packet mangling.
>>>>>>>
>>>>>>> It also adds 'flags' argument to bpf_skb_store_bytes() helper to avoid
>>>>>>> unnecessary checksum recomputations when BPF programs adjusting l3/l4
>>>>>>> checksums and documents all three helpers in uapi header.
>>>>>>>
>>>>>>> Moreover, a sample program is added to show how BPF programs can
>>>>>>> make use
>>>>>>> of the mangle and csum helpers.
>>>>>>>
>>>>>>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>>>>>>
>>>>>> I think in future we need to find a better place for these helpers,
>>>>>> as they are for cls_bpf and act_bpf.
>>>>>
>>>>> Yeah, makes sense to now have it in net/core/filter.c. How about to
>>>>> create net/bpf dir for these?
>>>>
>>>> I was thinking somewhere under net/sched/, f.e. net/sched/bpf.c
>>>> as probably kernel/bpf/ would end up in too much ifdef pasta. :/
>>>
>>> I would prefer to keep mangle and csum helpers in net/core/filter.c for
>>> now, since it's not clear what networking subsystems beyond TC would
>>> want to use them in the future. Otherwise we'll keep moving them back
>>> and forth.
>>
>> Well, therefore I suggested net/bpf/ dir as a place to store
>> net-subsystem specific bpf stuff.
>
> ahh. misread your earlier statement. yes. makes sense in long term.
> Right now feels too early to create a dir for just 3 functions.
> Eventually half of filter.c probably belongs in there.
Agreed, yeah. I think we should keep that refactoring in mind in
the mid-long term, but I guess a single file somewhere under net/core/
might be enough then.
^ permalink raw reply
* Re: [PATCH net-next] tc: bpf: add checksum helpers
From: Alexei Starovoitov @ 2015-04-02 17:56 UTC (permalink / raw)
To: Jiri Pirko
Cc: Daniel Borkmann, David S. Miller, Jamal Hadi Salim, linux-api,
netdev
In-Reply-To: <20150402174603.GE2613@nanopsycho.orion>
On 4/2/2015 10:46 AM, Jiri Pirko wrote:
> Thu, Apr 02, 2015 at 06:34:59PM CEST, ast@plumgrid.com wrote:
>> On 4/2/2015 8:21 AM, Daniel Borkmann wrote:
>>> On 04/02/2015 05:15 PM, Jiri Pirko wrote:
>>>> Thu, Apr 02, 2015 at 05:00:47PM CEST, daniel@iogearbox.net wrote:
>>>>> On 04/02/2015 02:12 AM, Alexei Starovoitov wrote:
>>>>>> Commit 608cd71a9c7c ("tc: bpf: generalize pedit action") has added the
>>>>>> possibility to mangle packet data to BPF programs in the tc pipeline.
>>>>>> This patch adds two helpers bpf_l3_csum_replace() and
>>>>>> bpf_l4_csum_replace()
>>>>>> for fixing up the protocol checksums after the packet mangling.
>>>>>>
>>>>>> It also adds 'flags' argument to bpf_skb_store_bytes() helper to avoid
>>>>>> unnecessary checksum recomputations when BPF programs adjusting l3/l4
>>>>>> checksums and documents all three helpers in uapi header.
>>>>>>
>>>>>> Moreover, a sample program is added to show how BPF programs can
>>>>>> make use
>>>>>> of the mangle and csum helpers.
>>>>>>
>>>>>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>>>>>
>>>>> I think in future we need to find a better place for these helpers,
>>>>> as they are for cls_bpf and act_bpf.
>>>>
>>>> Yeah, makes sense to now have it in net/core/filter.c. How about to
>>>> create net/bpf dir for these?
>>>
>>> I was thinking somewhere under net/sched/, f.e. net/sched/bpf.c
>>> as probably kernel/bpf/ would end up in too much ifdef pasta. :/
>>
>> I would prefer to keep mangle and csum helpers in net/core/filter.c for
>> now, since it's not clear what networking subsystems beyond TC would
>> want to use them in the future. Otherwise we'll keep moving them back
>> and forth.
>
> Well, therefore I suggested net/bpf/ dir as a place to store
> net-subsystem specific bpf stuff.
ahh. misread your earlier statement. yes. makes sense in long term.
Right now feels too early to create a dir for just 3 functions.
Eventually half of filter.c probably belongs in there.
^ permalink raw reply
* Re: [PATCH net-next] tc: bpf: add checksum helpers
From: Jiri Pirko @ 2015-04-02 17:46 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, David S. Miller, Jamal Hadi Salim, linux-api,
netdev
In-Reply-To: <551D6FB3.7090304@plumgrid.com>
Thu, Apr 02, 2015 at 06:34:59PM CEST, ast@plumgrid.com wrote:
>On 4/2/2015 8:21 AM, Daniel Borkmann wrote:
>>On 04/02/2015 05:15 PM, Jiri Pirko wrote:
>>>Thu, Apr 02, 2015 at 05:00:47PM CEST, daniel@iogearbox.net wrote:
>>>>On 04/02/2015 02:12 AM, Alexei Starovoitov wrote:
>>>>>Commit 608cd71a9c7c ("tc: bpf: generalize pedit action") has added the
>>>>>possibility to mangle packet data to BPF programs in the tc pipeline.
>>>>>This patch adds two helpers bpf_l3_csum_replace() and
>>>>>bpf_l4_csum_replace()
>>>>>for fixing up the protocol checksums after the packet mangling.
>>>>>
>>>>>It also adds 'flags' argument to bpf_skb_store_bytes() helper to avoid
>>>>>unnecessary checksum recomputations when BPF programs adjusting l3/l4
>>>>>checksums and documents all three helpers in uapi header.
>>>>>
>>>>>Moreover, a sample program is added to show how BPF programs can
>>>>>make use
>>>>>of the mangle and csum helpers.
>>>>>
>>>>>Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>>>>
>>>>I think in future we need to find a better place for these helpers,
>>>>as they are for cls_bpf and act_bpf.
>>>
>>>Yeah, makes sense to now have it in net/core/filter.c. How about to
>>>create net/bpf dir for these?
>>
>>I was thinking somewhere under net/sched/, f.e. net/sched/bpf.c
>>as probably kernel/bpf/ would end up in too much ifdef pasta. :/
>
>I would prefer to keep mangle and csum helpers in net/core/filter.c for
>now, since it's not clear what networking subsystems beyond TC would
>want to use them in the future. Otherwise we'll keep moving them back
>and forth.
Well, therefore I suggested net/bpf/ dir as a place to store
net-subsystem specific bpf stuff.
>
^ permalink raw reply
* Re: [PATCH v2 7/7] vhost: feature to set the vring endianness
From: Greg Kurz @ 2015-04-02 16:45 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-api, linux-kernel, kvm, virtualization
In-Reply-To: <20150402142046.GA23121@redhat.com>
On Thu, 2 Apr 2015 16:20:46 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Apr 02, 2015 at 03:17:13PM +0200, Greg Kurz wrote:
> > This patch brings cross-endian support to vhost when used to implement
> > legacy virtio devices. Since it is a relatively rare situation, the feature
> > availability is controlled by a kernel config option (not set by default).
> >
> > If cross-endian support is compiled in, vhost abvertises a new feature
> > to be negotiated with userspace. If userspace acknowledges the feature,
> > it can inform vhost about the endianness to use with a new ioctl.
> >
> > This feature is mutually exclusive with virtio 1.0. Even if the vhost device
> > advertises virtio 1.0 and legacy cross-endian features, it cannot receive
> > aknowledgement for both at the same time.
> >
> > Hot paths are being preserved from any penalty when the config option is
> > disabled or when virtio 1.0 is being used.
> >
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> > drivers/vhost/Kconfig | 10 ++++++++++
> > drivers/vhost/net.c | 5 +++++
> > drivers/vhost/scsi.c | 4 ++++
> > drivers/vhost/test.c | 4 ++++
> > drivers/vhost/vhost.c | 19 +++++++++++++++++++
> > drivers/vhost/vhost.h | 13 ++++++++++++-
> > include/uapi/linux/vhost.h | 10 ++++++++++
> > 7 files changed, 64 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index 017a1e8..5bb8da9 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -32,3 +32,13 @@ config VHOST
> > ---help---
> > This option is selected by any driver which needs to access
> > the core of vhost.
> > +
> > +config VHOST_SET_ENDIAN_LEGACY
> > + bool "Cross-endian support for host kernel accelerator"
> > + default n
> > + ---help---
> > + This option allows vhost to support guests with a different byte
> > + ordering
>
> from host
>
> >. It is disabled by default since it adds overhead and it
> > + is only needed by a few platforms (powerpc and arm).
> > +
> > + If unsure, say "n".
>
> "N"
>
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 2bbfc25..5274a44 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -1011,6 +1011,11 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
> > vhost_hlen = 0;
> > sock_hlen = hdr_len;
> > }
> > +
> > + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > + (1ULL << VIRTIO_F_VERSION_1)))
> > + return -EINVAL;
> > +
> > mutex_lock(&n->dev.mutex);
> > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > !vhost_log_access_ok(&n->dev)) {
> > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > index 71df240..b53e9c2 100644
> > --- a/drivers/vhost/scsi.c
> > +++ b/drivers/vhost/scsi.c
> > @@ -1544,6 +1544,10 @@ static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features)
> > if (features & ~VHOST_SCSI_FEATURES)
> > return -EOPNOTSUPP;
> >
> > + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > + (1ULL << VIRTIO_F_VERSION_1)))
> > + return -EINVAL;
> > +
> > mutex_lock(&vs->dev.mutex);
> > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > !vhost_log_access_ok(&vs->dev)) {
> > diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> > index d9c501e..cfefdad 100644
> > --- a/drivers/vhost/test.c
> > +++ b/drivers/vhost/test.c
> > @@ -243,6 +243,10 @@ static int vhost_test_set_features(struct vhost_test *n, u64 features)
> > {
> > struct vhost_virtqueue *vq;
> >
> > + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > + (1ULL << VIRTIO_F_VERSION_1)))
> > + return -EINVAL;
> > +
> > mutex_lock(&n->dev.mutex);
> > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > !vhost_log_access_ok(&n->dev)) {
>
> Above seems to prevent users from specifying either
> VIRTIO_F_VERSION_1 or VHOST_F_SET_ENDIAN_LEGACY.
> Does it actually work?
>
Arggg no it doesn't *of course*... I've added these bogus checks lately
and didn't re-test :(
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 2ee2826..60a0f32 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -199,6 +199,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> > vq->call = NULL;
> > vq->log_ctx = NULL;
> > vq->memory = NULL;
> > + vq->legacy_big_endian = false;
> > }
> >
> > static int vhost_worker(void *data)
> > @@ -806,6 +807,24 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> > } else
> > filep = eventfp;
> > break;
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > + case VHOST_SET_VRING_ENDIAN_LEGACY:
> > + {
> > + struct vhost_vring_endian e;
> > +
> > + if (!vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY)) {
> > + r = -EINVAL;
> > + break;
> > + }
> > +
> > + if (copy_from_user(&e, argp, sizeof(e))) {
> > + r = -EFAULT;
> > + break;
> > + }
> > + vq->legacy_big_endian = e.is_big_endian;
> > + break;
> > + }
> > +#endif
> > default:
> > r = -ENOIOCTLCMD;
> > }
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 4e9a186..d50881d 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -106,6 +106,9 @@ struct vhost_virtqueue {
> > /* Log write descriptors */
> > void __user *log_base;
> > struct vhost_log *log;
> > +
> > + /* We need to know the device endianness with legacy virtio. */
> > + bool legacy_big_endian;
> > };
> >
> > struct vhost_dev {
> > @@ -165,7 +168,11 @@ enum {
> > VHOST_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |
> > (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> > (1ULL << VIRTIO_RING_F_EVENT_IDX) |
> > - (1ULL << VHOST_F_LOG_ALL),
> > + (1ULL << VHOST_F_LOG_ALL) |
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > + (1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > +#endif
> > + 0ULL,
> > };
> >
> > static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> > @@ -177,6 +184,10 @@ static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> > {
> > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > return true;
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > + if (vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY))
> > + return !vq->legacy_big_endian;
>
> why do we need to check the feature bit?
> How about simple
> return !vq->legacy_big_endian;
> here?
This is a runtime feature. Even for powerpc, cross-endian won't be the
most common scenario. Userspace may have cleared the bit if it doesn't
need/want it.
> All you need to do is set legacy_big_endian to
> !virtio_legacy_is_little_endian() on reset.
> Maybe rename to legacy_is_little_endian so you don't
> need to reverse the logic.
>
Do you mean vhost doesn't need userspace to provide the endianness
to be used ? Please elaborate on the meaning of "reset" here... I
am confused.
> > +#endif
> > return virtio_legacy_is_little_endian();
> > }
> >
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index bb6a5b4..09d2a48 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -47,6 +47,11 @@ struct vhost_vring_addr {
> > __u64 log_guest_addr;
> > };
> >
> > +struct vhost_vring_endian {
> > + unsigned int index;
> > + bool is_big_endian;
>
> bool in uapi is a bad idea.
> Generally, I think you can use vhost_vring_state here.
>
Ok to turn is_big_endian into an int but why hijacking vhost_vring_state ?
> > +};
> > +
> > struct vhost_memory_region {
> > __u64 guest_phys_addr;
> > __u64 memory_size; /* bytes */
> > @@ -103,6 +108,9 @@ struct vhost_memory {
> > /* Get accessor: reads index, writes value in num */
> > #define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state)
> >
> > +/* Set endianness for the ring (legacy virtio only) */
> > +#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_endian)
> > +
> > /* The following ioctls use eventfd file descriptors to signal and poll
> > * for events. */
> >
>
> You also need a GET ioctl, as a matter of policy.
>
Ok. Will add.
> > @@ -126,6 +134,8 @@ struct vhost_memory {
> > #define VHOST_F_LOG_ALL 26
> > /* vhost-net should add virtio_net_hdr for RX, and strip for TX packets. */
> > #define VHOST_NET_F_VIRTIO_NET_HDR 27
> > +/* the vring endianness can be explicitly set (legacy virtio only). */
> > +#define VHOST_F_SET_ENDIAN_LEGACY 28
> >
> > /* VHOST_SCSI specific definitions */
>
>
> VHOST_F_SET_ENDIAN_LEGACY doesn't seem too useful.
> Is this so userspace can detect kernel configuration?
Yes. Also, it is quite easy to "un-ack" the feature when
it is not wanted.
> I think probing VHOST_SET_VRING_ENDIAN_LEGACY should
> be sufficient for this.
>
I'll get rid of the feature.
Thanks for your time Michael.
--
Greg
^ permalink raw reply
* Re: [PATCH net-next] tc: bpf: add checksum helpers
From: Alexei Starovoitov @ 2015-04-02 16:34 UTC (permalink / raw)
To: Daniel Borkmann, Jiri Pirko
Cc: David S. Miller, Jamal Hadi Salim,
linux-api-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <551D5E69.4050704-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
On 4/2/2015 8:21 AM, Daniel Borkmann wrote:
> On 04/02/2015 05:15 PM, Jiri Pirko wrote:
>> Thu, Apr 02, 2015 at 05:00:47PM CEST, daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org wrote:
>>> On 04/02/2015 02:12 AM, Alexei Starovoitov wrote:
>>>> Commit 608cd71a9c7c ("tc: bpf: generalize pedit action") has added the
>>>> possibility to mangle packet data to BPF programs in the tc pipeline.
>>>> This patch adds two helpers bpf_l3_csum_replace() and
>>>> bpf_l4_csum_replace()
>>>> for fixing up the protocol checksums after the packet mangling.
>>>>
>>>> It also adds 'flags' argument to bpf_skb_store_bytes() helper to avoid
>>>> unnecessary checksum recomputations when BPF programs adjusting l3/l4
>>>> checksums and documents all three helpers in uapi header.
>>>>
>>>> Moreover, a sample program is added to show how BPF programs can
>>>> make use
>>>> of the mangle and csum helpers.
>>>>
>>>> Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
>>>
>>> I think in future we need to find a better place for these helpers,
>>> as they are for cls_bpf and act_bpf.
>>
>> Yeah, makes sense to now have it in net/core/filter.c. How about to
>> create net/bpf dir for these?
>
> I was thinking somewhere under net/sched/, f.e. net/sched/bpf.c
> as probably kernel/bpf/ would end up in too much ifdef pasta. :/
I would prefer to keep mangle and csum helpers in net/core/filter.c for
now, since it's not clear what networking subsystems beyond TC would
want to use them in the future. Otherwise we'll keep moving them back
and forth.
^ permalink raw reply
* Re: [PATCH v2 3/4] Add virtio gpu driver.
From: Marc-André Lureau @ 2015-04-02 15:52 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: virtio-dev, Michael S. Tsirkin, open list:ABI/API, Rusty Russell,
open list, dri-devel, open list:VIRTIO CORE, NET..., David Airlie
In-Reply-To: <1427894130-14228-4-git-send-email-kraxel@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 766 bytes --]
Hi Gerd
While doing some testing with your brance, I reached that:
On Wed, Apr 1, 2015 at 3:15 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> +#define MAX_INLINE_CMD_SIZE 96
>
[ 750.446449] kernel BUG at drivers/gpu/drm/virtio/virtgpu_vq.c:132!
[ 750.447005] Call Trace:
[ 750.447005] [<ffffffffa00f3592>] virtio_gpu_alloc_cmd+0x22/0x50
[virtio_gpu]
[ 750.447005] [<ffffffffa00f4580>]
virtio_gpu_cmd_context_create+0x30/0xa0 [virtio_gpu]
struct virtio_gpu_ctx_create {
struct virtio_gpu_ctrl_hdr hdr;
__le32 nlen;
__le32 padding;
char debug_name[64];
};
Is actually 96 bytes.
Perhaps that condition should be changed:
+ BUG_ON(size >= MAX_INLINE_CMD_SIZE);
--
Marc-André Lureau
[-- Attachment #1.2: Type: text/html, Size: 1489 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH net-next] tc: bpf: add checksum helpers
From: Daniel Borkmann @ 2015-04-02 15:21 UTC (permalink / raw)
To: Jiri Pirko
Cc: Alexei Starovoitov, David S. Miller, Jamal Hadi Salim, linux-api,
netdev
In-Reply-To: <20150402151530.GD2613@nanopsycho.orion>
On 04/02/2015 05:15 PM, Jiri Pirko wrote:
> Thu, Apr 02, 2015 at 05:00:47PM CEST, daniel@iogearbox.net wrote:
>> On 04/02/2015 02:12 AM, Alexei Starovoitov wrote:
>>> Commit 608cd71a9c7c ("tc: bpf: generalize pedit action") has added the
>>> possibility to mangle packet data to BPF programs in the tc pipeline.
>>> This patch adds two helpers bpf_l3_csum_replace() and bpf_l4_csum_replace()
>>> for fixing up the protocol checksums after the packet mangling.
>>>
>>> It also adds 'flags' argument to bpf_skb_store_bytes() helper to avoid
>>> unnecessary checksum recomputations when BPF programs adjusting l3/l4
>>> checksums and documents all three helpers in uapi header.
>>>
>>> Moreover, a sample program is added to show how BPF programs can make use
>>> of the mangle and csum helpers.
>>>
>>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>>
>> I think in future we need to find a better place for these helpers,
>> as they are for cls_bpf and act_bpf.
>
> Yeah, makes sense to now have it in net/core/filter.c. How about to
> create net/bpf dir for these?
I was thinking somewhere under net/sched/, f.e. net/sched/bpf.c
as probably kernel/bpf/ would end up in too much ifdef pasta. :/
^ permalink raw reply
* Re: [PATCH net-next] tc: bpf: add checksum helpers
From: Jiri Pirko @ 2015-04-02 15:15 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, David S. Miller, Jamal Hadi Salim,
linux-api-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <551D599F.30408-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
Thu, Apr 02, 2015 at 05:00:47PM CEST, daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org wrote:
>On 04/02/2015 02:12 AM, Alexei Starovoitov wrote:
>>Commit 608cd71a9c7c ("tc: bpf: generalize pedit action") has added the
>>possibility to mangle packet data to BPF programs in the tc pipeline.
>>This patch adds two helpers bpf_l3_csum_replace() and bpf_l4_csum_replace()
>>for fixing up the protocol checksums after the packet mangling.
>>
>>It also adds 'flags' argument to bpf_skb_store_bytes() helper to avoid
>>unnecessary checksum recomputations when BPF programs adjusting l3/l4
>>checksums and documents all three helpers in uapi header.
>>
>>Moreover, a sample program is added to show how BPF programs can make use
>>of the mangle and csum helpers.
>>
>>Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
>
>I think in future we need to find a better place for these helpers,
>as they are for cls_bpf and act_bpf.
Yeah, makes sense to now have it in net/core/filter.c. How about to
create net/bpf dir for these?
>
>Acked-by: Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
^ permalink raw reply
* Re: [Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs
From: Jean Delvare @ 2015-04-02 15:08 UTC (permalink / raw)
To: Ivan.khoronzhuk
Cc: linux-kernel, matt.fleming, ard.biesheuvel, grant.likely,
linux-api, linux-doc, mikew, dmidecode-devel, leif.lindholm,
msalter
In-Reply-To: <551D3DE4.8040409@globallogic.com>
Le Thursday 02 April 2015 à 16:02 +0300, Ivan.khoronzhuk a écrit :
> Hi Jean,
> Sorry for the late reply.
> I've send new series
> "[Patch 0/3] firmware: dmi_scan: add SBMIOS entry point and DMI tables"
> with all last propositions.
Thanks Ivan, no problem. I'll look at it when I have time.
Two related notes:
* On week from April 13th to 17th, this is Hack Week 12 at SUSE. This
time I decided to work on dmidecode. I plan to add full support for
SMBIOS 3.0.0, including the new entry point format. Then I'll work on
integrating your work (the sysfs interface.) Project page:
https://hackweek.suse.com/12/projects/766
If you or anyone else want to participate, you are very welcome. Ideally
we would release dmidecode 2.13 at the end of that week.
* Do you know of any hardware already implementing SMBIOS 3.0 (either
the new entry point format, or the new enumerated values, or both)?
Having a real-world example would make testing much easier.
Thanks,
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply
* Re: [PATCH net-next] tc: bpf: add checksum helpers
From: Daniel Borkmann @ 2015-04-02 15:00 UTC (permalink / raw)
To: Alexei Starovoitov, David S. Miller
Cc: Jiri Pirko, Jamal Hadi Salim, linux-api-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1427933533-14394-1-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
On 04/02/2015 02:12 AM, Alexei Starovoitov wrote:
> Commit 608cd71a9c7c ("tc: bpf: generalize pedit action") has added the
> possibility to mangle packet data to BPF programs in the tc pipeline.
> This patch adds two helpers bpf_l3_csum_replace() and bpf_l4_csum_replace()
> for fixing up the protocol checksums after the packet mangling.
>
> It also adds 'flags' argument to bpf_skb_store_bytes() helper to avoid
> unnecessary checksum recomputations when BPF programs adjusting l3/l4
> checksums and documents all three helpers in uapi header.
>
> Moreover, a sample program is added to show how BPF programs can make use
> of the mangle and csum helpers.
>
> Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
I think in future we need to find a better place for these helpers,
as they are for cls_bpf and act_bpf.
Acked-by: Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
^ permalink raw reply
* Re: [PATCH v2 0/7] vhost: support for cross endian guests
From: Michael S. Tsirkin @ 2015-04-02 14:23 UTC (permalink / raw)
To: Greg Kurz; +Cc: linux-api, linux-kernel, kvm, virtualization
In-Reply-To: <20150402131637.24676.60700.stgit@bahia.lab.toulouse-stg.fr.ibm.com>
On Thu, Apr 02, 2015 at 03:16:37PM +0200, Greg Kurz wrote:
> Hi,
>
> This patchset allows vhost to be used with legacy virtio when guest and host
> have a different endianness. It is a complete rework of my initial post.
>
> Patches 1 to 5 are preliminary work: we move the endianness check out of all
> memory accessors to separate functions.
>
> Patch 6 changes the semantics of the accessors so that they have explicit big
> endian support.
>
> Patch 7 brings the cross-endian feature, with the following changes since v1:
> - conditionnal enablement through a kernel config
> - introduction of a new vhost feature to advertise cross-endian to userspace
I think adding ioctls is sufficient for this, we don't need to burn up a
feature bit.
> The tentative to fix vnet headers was dropped for the moment. As a consequnce,
> vhost_net still fails to work with cross-endian. It will be fixed in another
> patchset I'm currently working on.
I think I'll probably hold off on applying this
patchset until it all actually works.
> ---
>
> Greg Kurz (7):
> virtio: introduce virtio_is_little_endian() helper
> tun: add tun_is_little_endian() helper
> macvtap: introduce macvtap_is_little_endian() helper
> vringh: introduce vringh_is_little_endian() helper
> vhost: introduce vhost_is_little_endian() helper
> virtio: add explicit big-endian support to memory accessors
> vhost: feature to set the vring endianness
>
>
> drivers/net/macvtap.c | 11 +++++++++--
> drivers/net/tun.c | 11 +++++++++--
> drivers/vhost/Kconfig | 10 ++++++++++
> drivers/vhost/net.c | 5 +++++
> drivers/vhost/scsi.c | 4 ++++
> drivers/vhost/test.c | 4 ++++
> drivers/vhost/vhost.c | 19 +++++++++++++++++++
> drivers/vhost/vhost.h | 32 +++++++++++++++++++++++++-------
> include/linux/virtio_byteorder.h | 24 ++++++++++++++----------
> include/linux/virtio_config.h | 19 +++++++++++++------
> include/linux/vringh.h | 19 +++++++++++++------
> include/uapi/linux/vhost.h | 10 ++++++++++
> 12 files changed, 135 insertions(+), 33 deletions(-)
>
> --
> Greg
^ permalink raw reply
* Re: [PATCH] selftests/mount: output error messages when mount test fail
From: Shuah Khan @ 2015-04-02 14:21 UTC (permalink / raw)
To: Zhang Zhen, linux-api, Linux Kernel Mailing List
Cc: mpe, ebiederm, Shuah Khan
In-Reply-To: <551CE72E.3050303@huawei.com>
On 04/02/2015 12:52 AM, Zhang Zhen wrote:
> Without this patch, if /proc/self/uid_map is not exist,
> the mount test case will fail and no any prompting.
>
> After applied this patch, the case will prompt why it fail.
> Just as follows:
> root@kernel-host:/opt/kernel> make -C tools/testing/selftests TARGETS=mount run_tests
> make: Entering directory `/opt/kernel/tools/testing/selftests'
> for TARGET in mount; do \
> make -C $TARGET; \
> done;
> make[1]: Entering directory `/opt/kernel/tools/testing/selftests/mount'
> make[1]: Nothing to be done for `all'.
> make[1]: Leaving directory `/opt/kernel/tools/testing/selftests/mount'
> for TARGET in mount; do \
> make -C $TARGET run_tests; \
> done;
> make[1]: Entering directory `/opt/kernel/tools/testing/selftests/mount'
> ERROR: No /proc/self/uid_map exist
> make[1]: Leaving directory `/opt/kernel/tools/testing/selftests/mount'
> make: Leaving directory `/opt/kernel/tools/testing/selftests'
>
> Signed-off-by: Zhang Zhen <zhenzhang.zhang@huawei.com>
Thanks for fixing this. It will get into 4.1-rc2.
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978
^ permalink raw reply
* Re: [PATCH v2 7/7] vhost: feature to set the vring endianness
From: Michael S. Tsirkin @ 2015-04-02 14:20 UTC (permalink / raw)
To: Greg Kurz; +Cc: linux-api, linux-kernel, kvm, virtualization
In-Reply-To: <20150402131713.24676.9924.stgit@bahia.lab.toulouse-stg.fr.ibm.com>
On Thu, Apr 02, 2015 at 03:17:13PM +0200, Greg Kurz wrote:
> This patch brings cross-endian support to vhost when used to implement
> legacy virtio devices. Since it is a relatively rare situation, the feature
> availability is controlled by a kernel config option (not set by default).
>
> If cross-endian support is compiled in, vhost abvertises a new feature
> to be negotiated with userspace. If userspace acknowledges the feature,
> it can inform vhost about the endianness to use with a new ioctl.
>
> This feature is mutually exclusive with virtio 1.0. Even if the vhost device
> advertises virtio 1.0 and legacy cross-endian features, it cannot receive
> aknowledgement for both at the same time.
>
> Hot paths are being preserved from any penalty when the config option is
> disabled or when virtio 1.0 is being used.
>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> drivers/vhost/Kconfig | 10 ++++++++++
> drivers/vhost/net.c | 5 +++++
> drivers/vhost/scsi.c | 4 ++++
> drivers/vhost/test.c | 4 ++++
> drivers/vhost/vhost.c | 19 +++++++++++++++++++
> drivers/vhost/vhost.h | 13 ++++++++++++-
> include/uapi/linux/vhost.h | 10 ++++++++++
> 7 files changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 017a1e8..5bb8da9 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -32,3 +32,13 @@ config VHOST
> ---help---
> This option is selected by any driver which needs to access
> the core of vhost.
> +
> +config VHOST_SET_ENDIAN_LEGACY
> + bool "Cross-endian support for host kernel accelerator"
> + default n
> + ---help---
> + This option allows vhost to support guests with a different byte
> + ordering
from host
>. It is disabled by default since it adds overhead and it
> + is only needed by a few platforms (powerpc and arm).
> +
> + If unsure, say "n".
"N"
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 2bbfc25..5274a44 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1011,6 +1011,11 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
> vhost_hlen = 0;
> sock_hlen = hdr_len;
> }
> +
> + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> + (1ULL << VIRTIO_F_VERSION_1)))
> + return -EINVAL;
> +
> mutex_lock(&n->dev.mutex);
> if ((features & (1 << VHOST_F_LOG_ALL)) &&
> !vhost_log_access_ok(&n->dev)) {
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 71df240..b53e9c2 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -1544,6 +1544,10 @@ static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features)
> if (features & ~VHOST_SCSI_FEATURES)
> return -EOPNOTSUPP;
>
> + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> + (1ULL << VIRTIO_F_VERSION_1)))
> + return -EINVAL;
> +
> mutex_lock(&vs->dev.mutex);
> if ((features & (1 << VHOST_F_LOG_ALL)) &&
> !vhost_log_access_ok(&vs->dev)) {
> diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> index d9c501e..cfefdad 100644
> --- a/drivers/vhost/test.c
> +++ b/drivers/vhost/test.c
> @@ -243,6 +243,10 @@ static int vhost_test_set_features(struct vhost_test *n, u64 features)
> {
> struct vhost_virtqueue *vq;
>
> + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> + (1ULL << VIRTIO_F_VERSION_1)))
> + return -EINVAL;
> +
> mutex_lock(&n->dev.mutex);
> if ((features & (1 << VHOST_F_LOG_ALL)) &&
> !vhost_log_access_ok(&n->dev)) {
Above seems to prevent users from specifying either
VIRTIO_F_VERSION_1 or VHOST_F_SET_ENDIAN_LEGACY.
Does it actually work?
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 2ee2826..60a0f32 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -199,6 +199,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> vq->call = NULL;
> vq->log_ctx = NULL;
> vq->memory = NULL;
> + vq->legacy_big_endian = false;
> }
>
> static int vhost_worker(void *data)
> @@ -806,6 +807,24 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> } else
> filep = eventfp;
> break;
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> + case VHOST_SET_VRING_ENDIAN_LEGACY:
> + {
> + struct vhost_vring_endian e;
> +
> + if (!vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY)) {
> + r = -EINVAL;
> + break;
> + }
> +
> + if (copy_from_user(&e, argp, sizeof(e))) {
> + r = -EFAULT;
> + break;
> + }
> + vq->legacy_big_endian = e.is_big_endian;
> + break;
> + }
> +#endif
> default:
> r = -ENOIOCTLCMD;
> }
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 4e9a186..d50881d 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -106,6 +106,9 @@ struct vhost_virtqueue {
> /* Log write descriptors */
> void __user *log_base;
> struct vhost_log *log;
> +
> + /* We need to know the device endianness with legacy virtio. */
> + bool legacy_big_endian;
> };
>
> struct vhost_dev {
> @@ -165,7 +168,11 @@ enum {
> VHOST_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |
> (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> (1ULL << VIRTIO_RING_F_EVENT_IDX) |
> - (1ULL << VHOST_F_LOG_ALL),
> + (1ULL << VHOST_F_LOG_ALL) |
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> + (1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> +#endif
> + 0ULL,
> };
>
> static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> @@ -177,6 +184,10 @@ static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> {
> if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> return true;
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> + if (vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY))
> + return !vq->legacy_big_endian;
why do we need to check the feature bit?
How about simple
return !vq->legacy_big_endian;
here?
All you need to do is set legacy_big_endian to
!virtio_legacy_is_little_endian() on reset.
Maybe rename to legacy_is_little_endian so you don't
need to reverse the logic.
> +#endif
> return virtio_legacy_is_little_endian();
> }
>
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index bb6a5b4..09d2a48 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -47,6 +47,11 @@ struct vhost_vring_addr {
> __u64 log_guest_addr;
> };
>
> +struct vhost_vring_endian {
> + unsigned int index;
> + bool is_big_endian;
bool in uapi is a bad idea.
Generally, I think you can use vhost_vring_state here.
> +};
> +
> struct vhost_memory_region {
> __u64 guest_phys_addr;
> __u64 memory_size; /* bytes */
> @@ -103,6 +108,9 @@ struct vhost_memory {
> /* Get accessor: reads index, writes value in num */
> #define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state)
>
> +/* Set endianness for the ring (legacy virtio only) */
> +#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_endian)
> +
> /* The following ioctls use eventfd file descriptors to signal and poll
> * for events. */
>
You also need a GET ioctl, as a matter of policy.
> @@ -126,6 +134,8 @@ struct vhost_memory {
> #define VHOST_F_LOG_ALL 26
> /* vhost-net should add virtio_net_hdr for RX, and strip for TX packets. */
> #define VHOST_NET_F_VIRTIO_NET_HDR 27
> +/* the vring endianness can be explicitly set (legacy virtio only). */
> +#define VHOST_F_SET_ENDIAN_LEGACY 28
>
> /* VHOST_SCSI specific definitions */
VHOST_F_SET_ENDIAN_LEGACY doesn't seem too useful.
Is this so userspace can detect kernel configuration?
I think probing VHOST_SET_VRING_ENDIAN_LEGACY should
be sufficient for this.
--
MST
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox