* Re: [v13 4/5] ext4: adds FS_IOC_FSSETXATTR/FS_IOC_FSGETXATTR interface support
From: Jan Kara @ 2015-04-21 12:52 UTC (permalink / raw)
To: Li Xi
Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-ext4-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, tytso-3s7WtUTddSA,
adilger-m1MBpc4rdrD3fQ9qLvQP4Q, jack-AlSwsSmVLrQ,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, hch-wEGCiKHe2LqWVfeAwA7xHQ,
dmonakhov-GEFAQzZX7r8dnm+yROfE0A
In-Reply-To: <1429493988-16819-5-git-send-email-lixi-LfVdkaOWEx8@public.gmane.org>
On Mon 20-04-15 10:39:47, Li Xi wrote:
> This patch adds FS_IOC_FSSETXATTR/FS_IOC_FSGETXATTR ioctl interface
> support for ext4. The interface is kept consistent with
> XFS_IOC_FSGETXATTR/XFS_IOC_FSGETXATTR.
Thanks for the patch! Besides what Andreas wrote, the patch looks good to
me.
Honza
>
> Signed-off-by: Li Xi <lixi-LfVdkaOWEx8@public.gmane.org>
> ---
> fs/ext4/ext4.h | 9 ++
> fs/ext4/ioctl.c | 364 +++++++++++++++++++++++++++++++++++-----------
> fs/xfs/xfs_fs.h | 47 +++----
> include/uapi/linux/fs.h | 32 ++++
> 4 files changed, 335 insertions(+), 117 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index a7acf10..2d5a097 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -384,6 +384,13 @@ struct flex_groups {
> #define EXT4_FL_USER_VISIBLE 0x304BDFFF /* User visible flags */
> #define EXT4_FL_USER_MODIFIABLE 0x204380FF /* User modifiable flags */
>
> +#define EXT4_FL_XFLAG_VISIBLE (EXT4_SYNC_FL | \
> + EXT4_IMMUTABLE_FL | \
> + EXT4_APPEND_FL | \
> + EXT4_NODUMP_FL | \
> + EXT4_NOATIME_FL | \
> + EXT4_PROJINHERIT_FL)
> +
> /* Flags that should be inherited by new inodes from their parent. */
> #define EXT4_FL_INHERITED (EXT4_SECRM_FL | EXT4_UNRM_FL | EXT4_COMPR_FL |\
> EXT4_SYNC_FL | EXT4_NODUMP_FL | EXT4_NOATIME_FL |\
> @@ -606,6 +613,8 @@ enum {
> #define EXT4_IOC_RESIZE_FS _IOW('f', 16, __u64)
> #define EXT4_IOC_SWAP_BOOT _IO('f', 17)
> #define EXT4_IOC_PRECACHE_EXTENTS _IO('f', 18)
> +#define EXT4_IOC_FSGETXATTR FS_IOC_FSGETXATTR
> +#define EXT4_IOC_FSSETXATTR FS_IOC_FSSETXATTR
>
> #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
> /*
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index f58a0d1..27e50a8 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -14,6 +14,8 @@
> #include <linux/compat.h>
> #include <linux/mount.h>
> #include <linux/file.h>
> +#include <linux/quotaops.h>
> +#include <linux/quota.h>
> #include <asm/uaccess.h>
> #include "ext4_jbd2.h"
> #include "ext4.h"
> @@ -196,6 +198,225 @@ journal_err_out:
> return err;
> }
>
> +static int ext4_ioctl_setflags(struct inode *inode,
> + unsigned int flags)
> +{
> + struct ext4_inode_info *ei = EXT4_I(inode);
> + handle_t *handle = NULL;
> + int err = EPERM, migrate = 0;
> + struct ext4_iloc iloc;
> + unsigned int oldflags, mask, i;
> + unsigned int jflag;
> +
> + /* Is it quota file? Do not allow user to mess with it */
> + if (IS_NOQUOTA(inode))
> + goto flags_out;
> +
> + oldflags = ei->i_flags;
> +
> + /* The JOURNAL_DATA flag is modifiable only by root */
> + jflag = flags & EXT4_JOURNAL_DATA_FL;
> +
> + /*
> + * The IMMUTABLE and APPEND_ONLY flags can only be changed by
> + * the relevant capability.
> + *
> + * This test looks nicer. Thanks to Pauline Middelink
> + */
> + if ((flags ^ oldflags) & (EXT4_APPEND_FL | EXT4_IMMUTABLE_FL)) {
> + if (!capable(CAP_LINUX_IMMUTABLE))
> + goto flags_out;
> + }
> +
> + /*
> + * The JOURNAL_DATA flag can only be changed by
> + * the relevant capability.
> + */
> + if ((jflag ^ oldflags) & (EXT4_JOURNAL_DATA_FL)) {
> + if (!capable(CAP_SYS_RESOURCE))
> + goto flags_out;
> + }
> + if ((flags ^ oldflags) & EXT4_EXTENTS_FL)
> + migrate = 1;
> +
> + if (flags & EXT4_EOFBLOCKS_FL) {
> + /* we don't support adding EOFBLOCKS flag */
> + if (!(oldflags & EXT4_EOFBLOCKS_FL)) {
> + err = -EOPNOTSUPP;
> + goto flags_out;
> + }
> + } else if (oldflags & EXT4_EOFBLOCKS_FL)
> + ext4_truncate(inode);
> +
> + handle = ext4_journal_start(inode, EXT4_HT_INODE, 1);
> + if (IS_ERR(handle)) {
> + err = PTR_ERR(handle);
> + goto flags_out;
> + }
> + if (IS_SYNC(inode))
> + ext4_handle_sync(handle);
> + err = ext4_reserve_inode_write(handle, inode, &iloc);
> + if (err)
> + goto flags_err;
> +
> + for (i = 0, mask = 1; i < 32; i++, mask <<= 1) {
> + if (!(mask & EXT4_FL_USER_MODIFIABLE))
> + continue;
> + if (mask & flags)
> + ext4_set_inode_flag(inode, i);
> + else
> + ext4_clear_inode_flag(inode, i);
> + }
> +
> + ext4_set_inode_flags(inode);
> + inode->i_ctime = ext4_current_time(inode);
> +
> + err = ext4_mark_iloc_dirty(handle, inode, &iloc);
> +flags_err:
> + ext4_journal_stop(handle);
> + if (err)
> + goto flags_out;
> +
> + if ((jflag ^ oldflags) & (EXT4_JOURNAL_DATA_FL))
> + err = ext4_change_inode_journal_flag(inode, jflag);
> + if (err)
> + goto flags_out;
> + if (migrate) {
> + if (flags & EXT4_EXTENTS_FL)
> + err = ext4_ext_migrate(inode);
> + else
> + err = ext4_ind_migrate(inode);
> + }
> +
> +flags_out:
> + return err;
> +}
> +
> +static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
> +{
> + struct inode *inode = file_inode(filp);
> + struct super_block *sb = inode->i_sb;
> + struct ext4_inode_info *ei = EXT4_I(inode);
> + int err;
> + handle_t *handle;
> + kprojid_t kprojid;
> + struct ext4_iloc iloc;
> + struct ext4_inode *raw_inode;
> +
> + struct dquot *transfer_to[EXT4_MAXQUOTAS] = { };
> +
> + if (!EXT4_HAS_RO_COMPAT_FEATURE(sb,
> + EXT4_FEATURE_RO_COMPAT_PROJECT)) {
> + BUG_ON(__kprojid_val(EXT4_I(inode)->i_projid)
> + != EXT4_DEF_PROJID);
> + if (projid != EXT4_DEF_PROJID)
> + return -EOPNOTSUPP;
> + else
> + return 0;
> + }
> +
> + kprojid = make_kprojid(&init_user_ns, (projid_t)projid);
> +
> + if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
> + return 0;
> +
> + err = mnt_want_write_file(filp);
> + if (err)
> + return err;
> +
> + err = -EPERM;
> + mutex_lock(&inode->i_mutex);
> + /* Is it quota file? Do not allow user to mess with it */
> + if (IS_NOQUOTA(inode))
> + goto project_out;
> +
> + dquot_initialize(inode);
> +
> + handle = ext4_journal_start(inode, EXT4_HT_QUOTA,
> + EXT4_QUOTA_INIT_BLOCKS(sb) +
> + EXT4_QUOTA_DEL_BLOCKS(sb) + 3);
> + if (IS_ERR(handle)) {
> + err = PTR_ERR(handle);
> + goto project_out;
> + }
> +
> + err = ext4_reserve_inode_write(handle, inode, &iloc);
> + if (err)
> + goto project_stop;
> +
> + raw_inode = ext4_raw_inode(&iloc);
> + if (EXT4_INODE_SIZE(sb) <= EXT4_GOOD_OLD_INODE_SIZE) {
> + err = -EOPNOTSUPP;
> + goto project_stop;
> + }
> +
> + if (!EXT4_FITS_IN_INODE(raw_inode, ei, i_projid)) {
> + err = -EOVERFLOW;
> + goto project_stop;
> + }
> +
> + transfer_to[PRJQUOTA] = dqget(sb, make_kqid_projid(kprojid));
> + if (!transfer_to[PRJQUOTA])
> + goto project_set;
> +
> + err = __dquot_transfer(inode, transfer_to);
> + dqput(transfer_to[PRJQUOTA]);
> + if (err)
> + goto project_stop;
> +
> +project_set:
> + EXT4_I(inode)->i_projid = kprojid;
> + inode->i_ctime = ext4_current_time(inode);
> + err = ext4_mark_iloc_dirty(handle, inode, &iloc);
> +project_stop:
> + ext4_journal_stop(handle);
> +project_out:
> + mutex_unlock(&inode->i_mutex);
> + mnt_drop_write_file(filp);
> + return err;
> +}
> +
> +/* Transfer internal flags to xflags */
> +static inline __u32 ext4_iflags_to_xflags(unsigned long iflags)
> +{
> + __u32 xflags = 0;
> +
> + if (iflags & EXT4_SYNC_FL)
> + xflags |= FS_XFLAG_SYNC;
> + if (iflags & EXT4_IMMUTABLE_FL)
> + xflags |= FS_XFLAG_IMMUTABLE;
> + if (iflags & EXT4_APPEND_FL)
> + xflags |= FS_XFLAG_APPEND;
> + if (iflags & EXT4_NODUMP_FL)
> + xflags |= FS_XFLAG_NODUMP;
> + if (iflags & EXT4_NOATIME_FL)
> + xflags |= FS_XFLAG_NOATIME;
> + if (iflags & EXT4_PROJINHERIT_FL)
> + xflags |= FS_XFLAG_PROJINHERIT;
> + return xflags;
> +}
> +
> +/* Transfer xflags flags to internal */
> +static inline unsigned long ext4_xflags_to_iflags(__u32 xflags)
> +{
> + unsigned long iflags = 0;
> +
> + if (xflags & FS_XFLAG_SYNC)
> + iflags |= EXT4_SYNC_FL;
> + if (xflags & FS_XFLAG_IMMUTABLE)
> + iflags |= EXT4_IMMUTABLE_FL;
> + if (xflags & FS_XFLAG_APPEND)
> + iflags |= EXT4_APPEND_FL;
> + if (xflags & FS_XFLAG_NODUMP)
> + iflags |= EXT4_NODUMP_FL;
> + if (xflags & FS_XFLAG_NOATIME)
> + iflags |= EXT4_NOATIME_FL;
> + if (xflags & FS_XFLAG_PROJINHERIT)
> + iflags |= EXT4_PROJINHERIT_FL;
> +
> + return iflags;
> +}
> +
> long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> struct inode *inode = file_inode(filp);
> @@ -211,11 +432,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> flags = ei->i_flags & EXT4_FL_USER_VISIBLE;
> return put_user(flags, (int __user *) arg);
> case EXT4_IOC_SETFLAGS: {
> - handle_t *handle = NULL;
> - int err, migrate = 0;
> - struct ext4_iloc iloc;
> - unsigned int oldflags, mask, i;
> - unsigned int jflag;
> + int err;
>
> if (!inode_owner_or_capable(inode))
> return -EACCES;
> @@ -229,89 +446,8 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>
> flags = ext4_mask_flags(inode->i_mode, flags);
>
> - err = -EPERM;
> mutex_lock(&inode->i_mutex);
> - /* Is it quota file? Do not allow user to mess with it */
> - if (IS_NOQUOTA(inode))
> - goto flags_out;
> -
> - oldflags = ei->i_flags;
> -
> - /* The JOURNAL_DATA flag is modifiable only by root */
> - jflag = flags & EXT4_JOURNAL_DATA_FL;
> -
> - /*
> - * The IMMUTABLE and APPEND_ONLY flags can only be changed by
> - * the relevant capability.
> - *
> - * This test looks nicer. Thanks to Pauline Middelink
> - */
> - if ((flags ^ oldflags) & (EXT4_APPEND_FL | EXT4_IMMUTABLE_FL)) {
> - if (!capable(CAP_LINUX_IMMUTABLE))
> - goto flags_out;
> - }
> -
> - /*
> - * The JOURNAL_DATA flag can only be changed by
> - * the relevant capability.
> - */
> - if ((jflag ^ oldflags) & (EXT4_JOURNAL_DATA_FL)) {
> - if (!capable(CAP_SYS_RESOURCE))
> - goto flags_out;
> - }
> - if ((flags ^ oldflags) & EXT4_EXTENTS_FL)
> - migrate = 1;
> -
> - if (flags & EXT4_EOFBLOCKS_FL) {
> - /* we don't support adding EOFBLOCKS flag */
> - if (!(oldflags & EXT4_EOFBLOCKS_FL)) {
> - err = -EOPNOTSUPP;
> - goto flags_out;
> - }
> - } else if (oldflags & EXT4_EOFBLOCKS_FL)
> - ext4_truncate(inode);
> -
> - handle = ext4_journal_start(inode, EXT4_HT_INODE, 1);
> - if (IS_ERR(handle)) {
> - err = PTR_ERR(handle);
> - goto flags_out;
> - }
> - if (IS_SYNC(inode))
> - ext4_handle_sync(handle);
> - err = ext4_reserve_inode_write(handle, inode, &iloc);
> - if (err)
> - goto flags_err;
> -
> - for (i = 0, mask = 1; i < 32; i++, mask <<= 1) {
> - if (!(mask & EXT4_FL_USER_MODIFIABLE))
> - continue;
> - if (mask & flags)
> - ext4_set_inode_flag(inode, i);
> - else
> - ext4_clear_inode_flag(inode, i);
> - }
> -
> - ext4_set_inode_flags(inode);
> - inode->i_ctime = ext4_current_time(inode);
> -
> - err = ext4_mark_iloc_dirty(handle, inode, &iloc);
> -flags_err:
> - ext4_journal_stop(handle);
> - if (err)
> - goto flags_out;
> -
> - if ((jflag ^ oldflags) & (EXT4_JOURNAL_DATA_FL))
> - err = ext4_change_inode_journal_flag(inode, jflag);
> - if (err)
> - goto flags_out;
> - if (migrate) {
> - if (flags & EXT4_EXTENTS_FL)
> - err = ext4_ext_migrate(inode);
> - else
> - err = ext4_ind_migrate(inode);
> - }
> -
> -flags_out:
> + err = ext4_ioctl_setflags(inode, flags);
> mutex_unlock(&inode->i_mutex);
> mnt_drop_write_file(filp);
> return err;
> @@ -615,7 +751,61 @@ resizefs_out:
> }
> case EXT4_IOC_PRECACHE_EXTENTS:
> return ext4_ext_precache(inode);
> + case EXT4_IOC_FSGETXATTR:
> + {
> + struct fsxattr fa;
> +
> + memset(&fa, 0, sizeof(struct fsxattr));
>
> + ext4_get_inode_flags(ei);
> + fa.fsx_xflags = ext4_iflags_to_xflags(ei->i_flags & EXT4_FL_USER_VISIBLE);
> +
> + if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> + EXT4_FEATURE_RO_COMPAT_PROJECT)) {
> + fa.fsx_projid = (__u32)from_kprojid(&init_user_ns,
> + EXT4_I(inode)->i_projid);
> + }
> +
> + if (copy_to_user((struct fsxattr __user *)arg,
> + &fa, sizeof(fa)))
> + return -EFAULT;
> + return 0;
> + }
> + case EXT4_IOC_FSSETXATTR:
> + {
> + struct fsxattr fa;
> + int err;
> +
> + if (copy_from_user(&fa, (struct fsxattr __user *)arg,
> + sizeof(fa)))
> + return -EFAULT;
> +
> + /* Make sure caller has proper permission */
> + if (!inode_owner_or_capable(inode))
> + return -EACCES;
> +
> + err = mnt_want_write_file(filp);
> + if (err)
> + return err;
> +
> + flags = ext4_xflags_to_iflags(fa.fsx_xflags);
> + flags = ext4_mask_flags(inode->i_mode, flags);
> +
> + mutex_lock(&inode->i_mutex);
> + flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
> + (flags & EXT4_FL_XFLAG_VISIBLE);
> + err = ext4_ioctl_setflags(inode, flags);
> + mutex_unlock(&inode->i_mutex);
> + mnt_drop_write_file(filp);
> + if (err)
> + return err;
> +
> + err = ext4_ioctl_setproject(filp, fa.fsx_projid);
> + if (err)
> + return err;
> +
> + return 0;
> + }
> default:
> return -ENOTTY;
> }
> diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
> index 18dc721..64c7ae6 100644
> --- a/fs/xfs/xfs_fs.h
> +++ b/fs/xfs/xfs_fs.h
> @@ -36,38 +36,25 @@ struct dioattr {
> #endif
>
> /*
> - * Structure for XFS_IOC_FSGETXATTR[A] and XFS_IOC_FSSETXATTR.
> - */
> -#ifndef HAVE_FSXATTR
> -struct fsxattr {
> - __u32 fsx_xflags; /* xflags field value (get/set) */
> - __u32 fsx_extsize; /* extsize field value (get/set)*/
> - __u32 fsx_nextents; /* nextents field value (get) */
> - __u32 fsx_projid; /* project identifier (get/set) */
> - unsigned char fsx_pad[12];
> -};
> -#endif
> -
> -/*
> * Flags for the bs_xflags/fsx_xflags field
> * There should be a one-to-one correspondence between these flags and the
> * XFS_DIFLAG_s.
> */
> -#define XFS_XFLAG_REALTIME 0x00000001 /* data in realtime volume */
> -#define XFS_XFLAG_PREALLOC 0x00000002 /* preallocated file extents */
> -#define XFS_XFLAG_IMMUTABLE 0x00000008 /* file cannot be modified */
> -#define XFS_XFLAG_APPEND 0x00000010 /* all writes append */
> -#define XFS_XFLAG_SYNC 0x00000020 /* all writes synchronous */
> -#define XFS_XFLAG_NOATIME 0x00000040 /* do not update access time */
> -#define XFS_XFLAG_NODUMP 0x00000080 /* do not include in backups */
> -#define XFS_XFLAG_RTINHERIT 0x00000100 /* create with rt bit set */
> -#define XFS_XFLAG_PROJINHERIT 0x00000200 /* create with parents projid */
> -#define XFS_XFLAG_NOSYMLINKS 0x00000400 /* disallow symlink creation */
> -#define XFS_XFLAG_EXTSIZE 0x00000800 /* extent size allocator hint */
> -#define XFS_XFLAG_EXTSZINHERIT 0x00001000 /* inherit inode extent size */
> -#define XFS_XFLAG_NODEFRAG 0x00002000 /* do not defragment */
> -#define XFS_XFLAG_FILESTREAM 0x00004000 /* use filestream allocator */
> -#define XFS_XFLAG_HASATTR 0x80000000 /* no DIFLAG for this */
> +#define XFS_XFLAG_REALTIME FS_XFLAG_REALTIME /* data in realtime volume */
> +#define XFS_XFLAG_PREALLOC FS_XFLAG_PREALLOC /* preallocated file extents */
> +#define XFS_XFLAG_IMMUTABLE FS_XFLAG_IMMUTABLE /* file cannot be modified */
> +#define XFS_XFLAG_APPEND FS_XFLAG_APPEND /* all writes append */
> +#define XFS_XFLAG_SYNC FS_XFLAG_SYNC /* all writes synchronous */
> +#define XFS_XFLAG_NOATIME FS_XFLAG_NOATIME /* do not update access time */
> +#define XFS_XFLAG_NODUMP FS_XFLAG_NODUMP /* do not include in backups */
> +#define XFS_XFLAG_RTINHERIT FS_XFLAG_RTINHERIT /* create with rt bit set */
> +#define XFS_XFLAG_PROJINHERIT FS_XFLAG_PROJINHERIT /* create with parents projid */
> +#define XFS_XFLAG_NOSYMLINKS FS_XFLAG_NOSYMLINKS /* disallow symlink creation */
> +#define XFS_XFLAG_EXTSIZE FS_XFLAG_EXTSIZE /* extent size allocator hint */
> +#define XFS_XFLAG_EXTSZINHERIT FS_XFLAG_EXTSZINHERIT /* inherit inode extent size */
> +#define XFS_XFLAG_NODEFRAG FS_XFLAG_NODEFRAG /* do not defragment */
> +#define XFS_XFLAG_FILESTREAM FS_XFLAG_FILESTREAM /* use filestream allocator */
> +#define XFS_XFLAG_HASATTR FS_XFLAG_HASATTR /* no DIFLAG for this */
>
> /*
> * Structure for XFS_IOC_GETBMAP.
> @@ -503,8 +490,8 @@ typedef struct xfs_swapext
> #define XFS_IOC_ALLOCSP _IOW ('X', 10, struct xfs_flock64)
> #define XFS_IOC_FREESP _IOW ('X', 11, struct xfs_flock64)
> #define XFS_IOC_DIOINFO _IOR ('X', 30, struct dioattr)
> -#define XFS_IOC_FSGETXATTR _IOR ('X', 31, struct fsxattr)
> -#define XFS_IOC_FSSETXATTR _IOW ('X', 32, struct fsxattr)
> +#define XFS_IOC_FSGETXATTR FS_IOC_FSGETXATTR
> +#define XFS_IOC_FSSETXATTR FS_IOC_FSSETXATTR
> #define XFS_IOC_ALLOCSP64 _IOW ('X', 36, struct xfs_flock64)
> #define XFS_IOC_FREESP64 _IOW ('X', 37, struct xfs_flock64)
> #define XFS_IOC_GETBMAP _IOWR('X', 38, struct getbmap)
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index fcbf647..69dda62 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -58,6 +58,36 @@ struct inodes_stat_t {
> long dummy[5]; /* padding for sysctl ABI compatibility */
> };
>
> +/*
> + * Structure for FS_IOC_FSGETXATTR and FS_IOC_FSSETXATTR.
> + */
> +struct fsxattr {
> + __u32 fsx_xflags; /* xflags field value (get/set) */
> + __u32 fsx_extsize; /* extsize field value (get/set)*/
> + __u32 fsx_nextents; /* nextents field value (get) */
> + __u32 fsx_projid; /* project identifier (get/set) */
> + unsigned char fsx_pad[12];
> +};
> +
> +/*
> + * Flags for the fsx_xflags field
> + */
> +#define FS_XFLAG_REALTIME 0x00000001 /* data in realtime volume */
> +#define FS_XFLAG_PREALLOC 0x00000002 /* preallocated file extents */
> +#define FS_XFLAG_IMMUTABLE 0x00000008 /* file cannot be modified */
> +#define FS_XFLAG_APPEND 0x00000010 /* all writes append */
> +#define FS_XFLAG_SYNC 0x00000020 /* all writes synchronous */
> +#define FS_XFLAG_NOATIME 0x00000040 /* do not update access time */
> +#define FS_XFLAG_NODUMP 0x00000080 /* do not include in backups */
> +#define FS_XFLAG_RTINHERIT 0x00000100 /* create with rt bit set */
> +#define FS_XFLAG_PROJINHERIT 0x00000200 /* create with parents projid */
> +#define FS_XFLAG_NOSYMLINKS 0x00000400 /* disallow symlink creation */
> +#define FS_XFLAG_EXTSIZE 0x00000800 /* extent size allocator hint */
> +#define FS_XFLAG_EXTSZINHERIT 0x00001000 /* inherit inode extent size */
> +#define FS_XFLAG_NODEFRAG 0x00002000 /* do not defragment */
> +#define FS_XFLAG_FILESTREAM 0x00004000 /* use filestream allocator */
> +#define FS_XFLAG_HASATTR 0x80000000 /* no DIFLAG for this */
> +
>
> #define NR_FILE 8192 /* this can well be larger on a larger system */
>
> @@ -163,6 +193,8 @@ struct inodes_stat_t {
> #define FS_IOC_GETVERSION _IOR('v', 1, long)
> #define FS_IOC_SETVERSION _IOW('v', 2, long)
> #define FS_IOC_FIEMAP _IOWR('f', 11, struct fiemap)
> +#define FS_IOC_FSGETXATTR _IOR('X', 31, struct fsxattr)
> +#define FS_IOC_FSSETXATTR _IOW('X', 32, struct fsxattr)
> #define FS_IOC32_GETFLAGS _IOR('f', 1, int)
> #define FS_IOC32_SETFLAGS _IOW('f', 2, int)
> #define FS_IOC32_GETVERSION _IOR('v', 1, int)
> --
> 1.7.1
>
--
Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 1/2] added stream id write support
From: Jeff Moyer @ 2015-04-21 13:44 UTC (permalink / raw)
To: kwan.huen
Cc: Matthew Wilcox, Keith Busch, Jens Axboe, Dimitri John Ledkov,
linux-nvme-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1429580863-3451-2-git-send-email-kwan.huen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
"kwan.huen" <kwan.huen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> writes:
> ---
> drivers/block/nvme-core.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> index 85b8036..332341a 100644
> --- a/drivers/block/nvme-core.c
> +++ b/drivers/block/nvme-core.c
> @@ -769,6 +769,9 @@ static int nvme_submit_iod(struct nvme_queue *nvmeq, struct nvme_iod *iod,
> if (req->cmd_flags & REQ_RAHEAD)
> dsmgmt |= NVME_RW_DSM_FREQ_PREFETCH;
>
> + if (rq_data_dir(req))
> + dsmgmt |= bio_get_streamid(req->bio) << 8;
> +
There's no public specification for this, yet. How many bits are set
aside for the stream id? Do you need to do bounds checking/input
validation? What happens on adapters with older firmware when these
bits are set?
Cheers,
Jeff
^ permalink raw reply
* Re: [PATCH v4 7/8] vhost: feature to set the vring endianness
From: Michael S. Tsirkin @ 2015-04-21 14:04 UTC (permalink / raw)
To: Greg Kurz
Cc: Rusty Russell, Cornelia Huck, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, kvm-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <20150410101627.31843.21710.stgit-GiB8zCg7hOfDOqzlkpFKJg@public.gmane.org>
On Fri, Apr 10, 2015 at 12:19:16PM +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).
>
> The vq->is_le boolean field is added to cache the endianness to be
> used for ring accesses. It defaults to native endian, as expected
> by legacy virtio devices. When the ring gets active, we force little
> endian if the device is modern. When the ring is deactivated, we
> revert to the native endian default.
>
> If cross-endian was compiled in, a vq->user_be boolean field is added
> so that userspace may request a specific endianness. This field is
> used to override the default when activating the ring of a legacy
> device. It has no effect on modern devices.
>
> Signed-off-by: Greg Kurz <gkurz-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
> drivers/vhost/Kconfig | 10 ++++++
> drivers/vhost/vhost.c | 76 +++++++++++++++++++++++++++++++++++++++++++-
> drivers/vhost/vhost.h | 12 +++++--
> include/uapi/linux/vhost.h | 9 +++++
> 4 files changed, 103 insertions(+), 4 deletions(-)
>
> Changes since v3:
> - VHOST_SET_VRING_ENDIAN_LEGACY ioctl renamed to VHOST_SET_VRING_BIG_ENDIAN
> - ioctl API is now: 0 for le, 1 for be, other values are EINVAL
> - ioctl doesn't filter out modern devices
> - ioctl stubs return ENOIOCTLCMD
> - forbid endianness changes when vring is active
> - logic now handled with vq->is_le and vq->user_be according to device
> start/stop as suggested by Michael
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 017a1e8..0aec88c 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
I'd prefer namin this VHOST_CROSS_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).
and is only useful on a few platforms (powerpc and arm).
"it" seems to refer to "overhead", which is rarely needed.
needed is a bit too strong, you can always e.g. run virtio
in userspace.
> +
> + If unsure, say "N".
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 2ee2826..3eb756b 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -199,6 +199,10 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> vq->call = NULL;
> vq->log_ctx = NULL;
> vq->memory = NULL;
> + vq->is_le = virtio_legacy_is_little_endian();
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> + vq->user_be = !vq->is_le;
> +#endif
add a wrapper for this too?
> }
>
> static int vhost_worker(void *data)
> @@ -630,6 +634,53 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> return 0;
> }
>
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> +static long vhost_set_vring_big_endian(struct vhost_virtqueue *vq,
> + int __user *argp)
> +{
> + struct vhost_vring_state s;
> +
> + if (vq->private_data)
> + return -EBUSY;
> +
> + if (copy_from_user(&s, argp, sizeof(s)))
> + return -EFAULT;
> +
> + if (s.num && s.num != 1)
s.num & ~0x1
> + return -EINVAL;
> +
> + vq->user_be = s.num;
> +
> + return 0;
> +}
> +
> +static long vhost_get_vring_big_endian(struct vhost_virtqueue *vq, u32 idx,
> + int __user *argp)
> +{
> + struct vhost_vring_state s = {
> + .index = idx,
> + .num = vq->user_be
> + };
> +
> + if (copy_to_user(argp, &s, sizeof(s)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +#else
> +static long vhost_set_vring_big_endian(struct vhost_virtqueue *vq,
> + int __user *argp)
> +{
> + return -ENOIOCTLCMD;
> +}
> +
> +static long vhost_get_vring_big_endian(struct vhost_virtqueue *vq, u32 idx,
> + int __user *argp)
> +{
> + return -ENOIOCTLCMD;
> +}
> +#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */
> +
> long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> {
> struct file *eventfp, *filep = NULL;
> @@ -806,6 +857,12 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> } else
> filep = eventfp;
> break;
> + case VHOST_SET_VRING_BIG_ENDIAN:
> + r = vhost_set_vring_big_endian(vq, argp);
> + break;
> + case VHOST_GET_VRING_BIG_ENDIAN:
> + r = vhost_get_vring_big_endian(vq, idx, argp);
> + break;
> default:
> r = -ENOIOCTLCMD;
> }
> @@ -1040,12 +1097,29 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
> return 0;
> }
>
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> +static void vhost_init_is_le(struct vhost_virtqueue *vq)
> +{
> + vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be;
> +}
> +#else
> +static void vhost_init_is_le(struct vhost_virtqueue *vq)
> +{
> + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> + vq->is_le = true;
> +}
> +#endif
> +
I'd prefer localizing ifdefery somewhere near top of file.
> int vhost_init_used(struct vhost_virtqueue *vq)
> {
> __virtio16 last_used_idx;
> int r;
> - if (!vq->private_data)
> + if (!vq->private_data) {
> + vq->is_le = virtio_legacy_is_little_endian();
> return 0;
> + }
> +
> + vhost_init_is_le(vq);
>
> r = vhost_update_used_flags(vq);
> if (r)
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 4e9a186..04b2add 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -106,6 +106,14 @@ struct vhost_virtqueue {
> /* Log write descriptors */
> void __user *log_base;
> struct vhost_log *log;
> +
> + /* Ring endianness. Defaults to legacy native endianness.
> + * Set to true when starting a modern virtio device. */
> + bool is_le;
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> + /* Ring endianness requested by userspace for cross-endian support. */
> + bool user_be;
> +#endif
> };
>
> struct vhost_dev {
> @@ -175,9 +183,7 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
>
> static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> {
> - if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> - return true;
> - return virtio_legacy_is_little_endian();
> + return vq->is_le;
> }
>
> /* Memory accessors */
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index bb6a5b4..5cdebbc 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -103,6 +103,15 @@ 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 the vring byte order in num. This is a legacy only API that is simply
> + * ignored when VIRTIO_F_VERSION_1 is set.
> + * 0 to set to little-endian
> + * 1 to set to big-endian
How about defines for these?
> + * other values return EINVAL.
> + */
> +#define VHOST_SET_VRING_BIG_ENDIAN _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state)
> +#define VHOST_GET_VRING_BIG_ENDIAN _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
> +
> /* The following ioctls use eventfd file descriptors to signal and poll
> * for events. */
>
^ permalink raw reply
* Re: [PATCH v4 8/8] macvtap/tun: add VNET_BE flag
From: Michael S. Tsirkin @ 2015-04-21 14:06 UTC (permalink / raw)
To: Greg Kurz
Cc: Rusty Russell, Cornelia Huck, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, kvm-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <20150410101923.31843.66853.stgit-GiB8zCg7hOfDOqzlkpFKJg@public.gmane.org>
On Fri, Apr 10, 2015 at 12:20:21PM +0200, Greg Kurz wrote:
> The VNET_LE flag was introduced to fix accesses to virtio 1.0 headers
> that are always little-endian. It can also be used to handle the special
> case of a legacy little-endian device implemented by a big-endian host.
>
> Let's add a flag and ioctls for big-endian devices as well. If both flags
> are set, little-endian wins.
>
> Since this is isn't a common usecase, the feature is controlled by a kernel
> config option (not set by default).
>
> Both macvtap and tun are covered by this patch since they share the same
> API with userland.
>
> Signed-off-by: Greg Kurz <gkurz-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
> drivers/net/Kconfig | 12 ++++++++
> drivers/net/macvtap.c | 60 +++++++++++++++++++++++++++++++++++++++++-
> drivers/net/tun.c | 62 ++++++++++++++++++++++++++++++++++++++++++-
> include/uapi/linux/if_tun.h | 2 +
> 4 files changed, 134 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index df51d60..f0e23a0 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -244,6 +244,18 @@ config TUN
>
> If you don't know what to use this for, you don't need it.
>
> +config TUN_VNET_BE
> + bool "Support for big-endian vnet headers"
> + default n
> + ---help---
> + This option allows TUN/TAP and MACVTAP device drivers to parse
> + vnet headers that are in big-endian byte order. It is useful
> + when the headers come from a big-endian legacy virtio driver and
> + the host is little-endian.
> +
> + Unless you have a little-endian system hosting a big-endian virtual
> + machine with a virtio NIC, you should say N.
> +
should mention cross-endian, not big-endian, right?
> config VETH
> tristate "Virtual ethernet pair device"
> ---help---
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index 0a03a66..e0ab1b7 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -48,12 +48,27 @@ struct macvtap_queue {
> #define MACVTAP_FEATURES (IFF_VNET_HDR | IFF_MULTI_QUEUE)
>
> #define MACVTAP_VNET_LE 0x80000000
> +#define MACVTAP_VNET_BE 0x40000000
> +
> +#ifdef CONFIG_TUN_VNET_BE
> +static inline bool macvtap_legacy_is_little_endian(struct macvtap_queue *q)
> +{
> + if (q->flags & MACVTAP_VNET_BE)
> + return false;
> + return virtio_legacy_is_little_endian();
> +}
> +#else
> +static inline bool macvtap_legacy_is_little_endian(struct macvtap_queue *q)
> +{
> + return virtio_legacy_is_little_endian();
> +}
> +#endif
>
> static inline bool macvtap_is_little_endian(struct macvtap_queue *q)
> {
> if (q->flags & MACVTAP_VNET_LE)
> return true;
> - return virtio_legacy_is_little_endian();
> + return macvtap_legacy_is_little_endian(q);
> }
>
> static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)
> @@ -1000,6 +1015,43 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
> return 0;
> }
>
> +#ifdef CONFIG_TUN_VNET_BE
> +static long macvtap_get_vnet_be(struct macvtap_queue *q, int __user *sp)
> +{
> + int s = !!(q->flags & MACVTAP_VNET_BE);
> +
> + if (put_user(s, sp))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static long macvtap_set_vnet_be(struct macvtap_queue *q, int __user *sp)
> +{
> + int s;
> +
> + if (get_user(s, sp))
> + return -EFAULT;
> +
> + if (s)
> + q->flags |= MACVTAP_VNET_BE;
> + else
> + q->flags &= ~MACVTAP_VNET_BE;
> +
> + return 0;
> +}
> +#else
> +static long macvtap_get_vnet_be(struct macvtap_queue *q, int __user *argp)
> +{
> + return -EINVAL;
> +}
> +
> +static long macvtap_set_vnet_be(struct macvtap_queue *q, int __user *argp)
> +{
> + return -EINVAL;
> +}
> +#endif /* CONFIG_TUN_VNET_BE */
> +
> /*
> * provide compatibility with generic tun/tap interface
> */
> @@ -1097,6 +1149,12 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
> q->flags &= ~MACVTAP_VNET_LE;
> return 0;
>
> + case TUNGETVNETBE:
> + return macvtap_get_vnet_be(q, sp);
> +
> + case TUNSETVNETBE:
> + return macvtap_set_vnet_be(q, sp);
> +
> case TUNSETOFFLOAD:
> /* let the user check for future flags */
> if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 053f9b6..4e12488 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -111,6 +111,7 @@ do { \
> #define TUN_FASYNC IFF_ATTACH_QUEUE
> /* High bits in flags field are unused. */
> #define TUN_VNET_LE 0x80000000
> +#define TUN_VNET_BE 0x40000000
>
> #define TUN_FEATURES (IFF_NO_PI | IFF_ONE_QUEUE | IFF_VNET_HDR | \
> IFF_MULTI_QUEUE)
> @@ -206,11 +207,25 @@ struct tun_struct {
> u32 flow_count;
> };
>
> +#ifdef CONFIG_TUN_VNET_BE
> +static inline bool tun_legacy_is_little_endian(struct tun_struct *tun)
> +{
> + if (tun->flags & TUN_VNET_BE)
> + return false;
> + return virtio_legacy_is_little_endian();
> +}
> +#else
> +static inline bool tun_legacy_is_little_endian(struct tun_struct *tun)
> +{
> + return virtio_legacy_is_little_endian();
> +}
> +#endif
> +
> static inline bool tun_is_little_endian(struct tun_struct *tun)
> {
> if (tun->flags & TUN_VNET_LE)
> return true;
> - return virtio_legacy_is_little_endian();
> + return tun_legacy_is_little_endian(tun);
> }
>
> static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
> @@ -1836,6 +1851,43 @@ unlock:
> return ret;
> }
>
> +#ifdef CONFIG_TUN_VNET_BE
> +static long tun_get_vnet_be(struct tun_struct *tun, int __user *argp)
> +{
> + int be = !!(tun->flags & TUN_VNET_BE);
> +
> + if (put_user(be, argp))
> + return EFAULT;
> +
> + return 0;
> +}
> +
> +static long tun_set_vnet_be(struct tun_struct *tun, int __user *argp)
> +{
> + int be;
> +
> + if (get_user(be, argp))
> + return -EFAULT;
> +
> + if (be)
> + tun->flags |= TUN_VNET_BE;
> + else
> + tun->flags &= ~TUN_VNET_BE;
> +
> + return 0;
> +}
> +#else
> +static long tun_get_vnet_be(struct tun_struct *tun, int __user *argp)
> +{
> + return -EINVAL;
> +}
> +
> +static long tun_set_vnet_be(struct tun_struct *tun, int __user *argp)
> +{
> + return -EINVAL;
> +}
> +#endif /* CONFIG_TUN_VNET_BE */
> +
> static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg, int ifreq_len)
> {
> @@ -2065,6 +2117,14 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
> tun->flags &= ~TUN_VNET_LE;
> break;
>
> + case TUNGETVNETBE:
> + ret = tun_get_vnet_be(tun, argp);
> + break;
> +
> + case TUNSETVNETBE:
> + ret = tun_set_vnet_be(tun, argp);
> + break;
> +
> case TUNATTACHFILTER:
> /* Can be set only for TAPs */
> ret = -EINVAL;
> diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h
> index 50ae243..bcac4c0 100644
> --- a/include/uapi/linux/if_tun.h
> +++ b/include/uapi/linux/if_tun.h
> @@ -50,6 +50,8 @@
> #define TUNGETFILTER _IOR('T', 219, struct sock_fprog)
> #define TUNSETVNETLE _IOW('T', 220, int)
> #define TUNGETVNETLE _IOR('T', 221, int)
> +#define TUNSETVNETBE _IOW('T', 222, int)
> +#define TUNGETVNETBE _IOR('T', 223, int)
>
> /* TUNSETIFF ifr flags */
> #define IFF_TUN 0x0001
^ permalink raw reply
* Re: [PATCH v4 6/8] virtio: add explicit big-endian support to memory accessors
From: Michael S. Tsirkin @ 2015-04-21 14:09 UTC (permalink / raw)
To: Greg Kurz
Cc: Rusty Russell, Cornelia Huck, linux-api, linux-kernel, kvm,
virtualization
In-Reply-To: <20150410101616.31843.51000.stgit@bahia.local>
On Fri, Apr 10, 2015 at 12:16:20PM +0200, Greg Kurz wrote:
> The current memory accessors logic is:
> - little endian if little_endian
> - native endian (i.e. no byteswap) if !little_endian
>
> If we want to fully support cross-endian vhost, we also need to be
> able to convert to big endian.
>
> Instead of changing the little_endian argument to some 3-value enum, this
> patch changes the logic to:
> - little endian if little_endian
> - big endian if !little_endian
>
> The native endian case is handled by all users with a trivial helper. This
> patch doesn't change any functionality, nor it does add overhead.
>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
OK overall. Style comment:
> drivers/net/macvtap.c | 4 +++-
> drivers/net/tun.c | 4 +++-
> drivers/vhost/vhost.h | 4 +++-
> include/linux/virtio_byteorder.h | 24 ++++++++++++++----------
> include/linux/virtio_config.h | 4 +++-
> include/linux/vringh.h | 4 +++-
> 6 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index a2f2958..0a03a66 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -51,7 +51,9 @@ struct macvtap_queue {
>
> static inline bool macvtap_is_little_endian(struct macvtap_queue *q)
> {
> - return q->flags & MACVTAP_VNET_LE;
> + if (q->flags & MACVTAP_VNET_LE)
> + return true;
> + return virtio_legacy_is_little_endian();
> }
>
I'd prefer a bit more symmetry:
+ if (q->flags & MACVTAP_VNET_LE)
+ return true;
+ else
+ return virtio_legacy_is_little_endian();
Or better just:
return (q->flags & MACVTAP_VNET_LE) ? true : virtio_legacy_is_little_endian();
might make line long, but your follow-up patch makes it short again,
so that's ok.
> static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 3c3d6c0..053f9b6 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -208,7 +208,9 @@ struct tun_struct {
>
> static inline bool tun_is_little_endian(struct tun_struct *tun)
> {
> - return tun->flags & TUN_VNET_LE;
> + if (tun->flags & TUN_VNET_LE)
> + return true;
> + return virtio_legacy_is_little_endian();
> }
>
> static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 6a49960..4e9a186 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -175,7 +175,9 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
>
> static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> {
> - return vhost_has_feature(vq, VIRTIO_F_VERSION_1);
> + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> + return true;
> + return virtio_legacy_is_little_endian();
> }
>
> /* Memory accessors */
> diff --git a/include/linux/virtio_byteorder.h b/include/linux/virtio_byteorder.h
> index 51865d0..ce63a2c 100644
> --- a/include/linux/virtio_byteorder.h
> +++ b/include/linux/virtio_byteorder.h
> @@ -3,17 +3,21 @@
> #include <linux/types.h>
> #include <uapi/linux/virtio_types.h>
>
> -/*
> - * Low-level memory accessors for handling virtio in modern little endian and in
> - * compatibility native endian format.
> - */
> +static inline bool virtio_legacy_is_little_endian(void)
> +{
> +#ifdef __LITTLE_ENDIAN
> + return true;
> +#else
> + return false;
> +#endif
> +}
>
> static inline u16 __virtio16_to_cpu(bool little_endian, __virtio16 val)
> {
> if (little_endian)
> return le16_to_cpu((__force __le16)val);
> else
> - return (__force u16)val;
> + return be16_to_cpu((__force __be16)val);
> }
>
> static inline __virtio16 __cpu_to_virtio16(bool little_endian, u16 val)
> @@ -21,7 +25,7 @@ static inline __virtio16 __cpu_to_virtio16(bool little_endian, u16 val)
> if (little_endian)
> return (__force __virtio16)cpu_to_le16(val);
> else
> - return (__force __virtio16)val;
> + return (__force __virtio16)cpu_to_be16(val);
> }
>
> static inline u32 __virtio32_to_cpu(bool little_endian, __virtio32 val)
> @@ -29,7 +33,7 @@ static inline u32 __virtio32_to_cpu(bool little_endian, __virtio32 val)
> if (little_endian)
> return le32_to_cpu((__force __le32)val);
> else
> - return (__force u32)val;
> + return be32_to_cpu((__force __be32)val);
> }
>
> static inline __virtio32 __cpu_to_virtio32(bool little_endian, u32 val)
> @@ -37,7 +41,7 @@ static inline __virtio32 __cpu_to_virtio32(bool little_endian, u32 val)
> if (little_endian)
> return (__force __virtio32)cpu_to_le32(val);
> else
> - return (__force __virtio32)val;
> + return (__force __virtio32)cpu_to_be32(val);
> }
>
> static inline u64 __virtio64_to_cpu(bool little_endian, __virtio64 val)
> @@ -45,7 +49,7 @@ static inline u64 __virtio64_to_cpu(bool little_endian, __virtio64 val)
> if (little_endian)
> return le64_to_cpu((__force __le64)val);
> else
> - return (__force u64)val;
> + return be64_to_cpu((__force __be64)val);
> }
>
> static inline __virtio64 __cpu_to_virtio64(bool little_endian, u64 val)
> @@ -53,7 +57,7 @@ static inline __virtio64 __cpu_to_virtio64(bool little_endian, u64 val)
> if (little_endian)
> return (__force __virtio64)cpu_to_le64(val);
> else
> - return (__force __virtio64)val;
> + return (__force __virtio64)cpu_to_be64(val);
> }
>
> #endif /* _LINUX_VIRTIO_BYTEORDER */
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index bd1a582..36a6daa 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -207,7 +207,9 @@ int virtqueue_set_affinity(struct virtqueue *vq, int cpu)
>
> static inline bool virtio_is_little_endian(struct virtio_device *vdev)
> {
> - return virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
> + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> + return true;
> + return virtio_legacy_is_little_endian();
> }
>
> /* Memory accessors */
> diff --git a/include/linux/vringh.h b/include/linux/vringh.h
> index 3ed62ef..d786c2d 100644
> --- a/include/linux/vringh.h
> +++ b/include/linux/vringh.h
> @@ -228,7 +228,9 @@ static inline void vringh_notify(struct vringh *vrh)
>
> static inline bool vringh_is_little_endian(const struct vringh *vrh)
> {
> - return vrh->little_endian;
> + if (vrh->little_endian)
> + return true;
> + return virtio_legacy_is_little_endian();
> }
>
> static inline u16 vringh16_to_cpu(const struct vringh *vrh, __virtio16 val)
^ permalink raw reply
* Re: [PATCH v4 0/8] vhost: support for cross endian guests
From: Michael S. Tsirkin @ 2015-04-21 14:10 UTC (permalink / raw)
To: Greg Kurz
Cc: Rusty Russell, Cornelia Huck, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, kvm-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <20150410101500.31843.61248.stgit-GiB8zCg7hOfDOqzlkpFKJg@public.gmane.org>
On Fri, Apr 10, 2015 at 12:15:00PM +0200, Greg Kurz wrote:
> Hi,
>
> This patchset allows vhost to be used with legacy virtio when guest and host
> have a different endianness.
>
> Patch 7 got rewritten according to Cornelia's and Michael's comments. I have
> also introduced patch 8 that brings BE vnet headers support to tun/macvtap.
>
> This series is enough to have vhost_net working flawlessly. I could
> succesfully reboot guests from ppc64 to ppc64le and vice-versa on ppc64
> and ppc64le hosts.
Looks good overall.
A couple of style comments.
Thanks!
> ---
>
> Greg Kurz (8):
> 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
> macvtap/tun: add VNET_BE flag
>
>
> drivers/net/Kconfig | 12 ++++++
> drivers/net/macvtap.c | 69 ++++++++++++++++++++++++++++++++++-
> drivers/net/tun.c | 71 +++++++++++++++++++++++++++++++++++-
> drivers/vhost/Kconfig | 10 +++++
> drivers/vhost/vhost.c | 76 ++++++++++++++++++++++++++++++++++++++
> drivers/vhost/vhost.h | 25 ++++++++++---
> include/linux/virtio_byteorder.h | 24 +++++++-----
> include/linux/virtio_config.h | 19 +++++++---
> include/linux/vringh.h | 19 +++++++---
> include/uapi/linux/if_tun.h | 2 +
> include/uapi/linux/vhost.h | 9 +++++
> 11 files changed, 303 insertions(+), 33 deletions(-)
>
> --
> Greg
^ permalink raw reply
* Re: [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
From: Jean Delvare @ 2015-04-21 14:24 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, roy.franz
In-Reply-To: <1429525187-3376-3-git-send-email-ivan.khoronzhuk@globallogic.com>
Hi Ivan,
On Mon, 20 Apr 2015 13:19:46 +0300, Ivan Khoronzhuk wrote:
> Some utils, like dmidecode and smbios, need to access SMBIOS entry
> table area in order to get information like SMBIOS version, size, etc.
> Currently it's done via /dev/mem. But for situation when /dev/mem
> usage is disabled, the utils have to use dmi sysfs instead, which
> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
> access for table is needed.
>
> So this patch creates dmi/tables and adds SMBIOS entry point to allow
> utils in question to work correctly without /dev/mem. Also patch adds
> raw dmi table to simplify dmi table processing in user space, as
> proposed by Jean Delvare.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> ---
> .../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
> drivers/firmware/dmi-sysfs.c | 17 +++--
> drivers/firmware/dmi_scan.c | 82 ++++++++++++++++++++++
> include/linux/dmi.h | 2 +
> 4 files changed, 114 insertions(+), 9 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
> (...)
> +static int __init dmi_init(void)
> +{
> + int ret;
> + u8 *dmi_table = NULL;
> + struct kobject *tables_kobj = NULL;
> +
> + if (!dmi_available) {
> + ret = -ENODATA;
> + goto err;
> + }
> +
> + /*
> + * Set up dmi directory at /sys/firmware/dmi. This entry should stay
> + * even after farther error, as it can be used by other modules like
> + * dmi-sysfs.
> + */
> + dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> + tables_kobj = kobject_create_and_add("tables", dmi_kobj);
I'm afraid you can't do that. kobject_create_and_add() doesn't check if
the parent is NULL and will happily create "tables" at the root of
sysfs if for any reason the previous call to kobject_create_and_add()
failed and returned NULL. I agree it is unlikely and would be cleaned
up immediately, but still, instantiating an object at the wrong place,
even temporarily, is wrong.
> + if (!(dmi_kobj && tables_kobj)) {
> + ret = -ENOMEM;
> + goto err;
> + }
I'd rather go with:
if (!(dmi_kobj = kobject_create_and_add("dmi", firmware_kobj))
|| !(tables_kobj = kobject_create_and_add("tables", dmi_kobj))) {
I know that checkpatch complains about this construct, but in many
cases it is the right thing to do.
Another possible approach is simply:
dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
if (dmi_kobj)
tables_kobj = kobject_create_and_add("tables", dmi_kobj);
if (!tables_kobj) {
> +
> + bin_attr_smbios_entry_point.size = smbios_entry_point_size;
> + bin_attr_smbios_entry_point.private = smbios_entry_point;
> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_smbios_entry_point);
> + if (ret)
> + goto err;
> +
> + dmi_table = dmi_remap(dmi_base, dmi_len);
> + if (!dmi_table) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + bin_attr_DMI.size = dmi_len;
> + bin_attr_DMI.private = dmi_table;
> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
> + if (!ret)
> + return 0;
> +
> +err:
> + pr_err("dmi: Firmware registration failed.\n");
> +
> + if (tables_kobj) {
> + sysfs_remove_bin_file(tables_kobj,
> + &bin_attr_smbios_entry_point);
> + kobject_del(tables_kobj);
> + kobject_put(tables_kobj);
> + }
> +
> + if (dmi_table)
> + dmi_unmap(dmi_table);
I'm not happy with this single error label. This forces you to
initialize all your pointers to NULL and then to check for them to find
out what needs to be done in the error path. With multiple error labels,
you would know exactly what needs to be done, this is more efficient.
> +
> + return ret;
> +}
Everything else looks good. No need to resend, I'll fix up the above
myself in place or as an incremental patch.
Thanks,
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply
* Re: [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
From: Ivan.khoronzhuk @ 2015-04-21 15:26 UTC (permalink / raw)
To: Jean Delvare
Cc: linux-kernel, matt.fleming, ard.biesheuvel, grant.likely,
linux-api, linux-doc, mikew, dmidecode-devel, leif.lindholm,
msalter, roy.franz
In-Reply-To: <20150421162427.21c358a5@endymion.delvare>
Hi Jean,
On 21.04.15 17:24, Jean Delvare wrote:
> Hi Ivan,
>
> On Mon, 20 Apr 2015 13:19:46 +0300, Ivan Khoronzhuk wrote:
>> Some utils, like dmidecode and smbios, need to access SMBIOS entry
>> table area in order to get information like SMBIOS version, size, etc.
>> Currently it's done via /dev/mem. But for situation when /dev/mem
>> usage is disabled, the utils have to use dmi sysfs instead, which
>> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
>> access for table is needed.
>>
>> So this patch creates dmi/tables and adds SMBIOS entry point to allow
>> utils in question to work correctly without /dev/mem. Also patch adds
>> raw dmi table to simplify dmi table processing in user space, as
>> proposed by Jean Delvare.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
>> ---
>> .../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
>> drivers/firmware/dmi-sysfs.c | 17 +++--
>> drivers/firmware/dmi_scan.c | 82 ++++++++++++++++++++++
>> include/linux/dmi.h | 2 +
>> 4 files changed, 114 insertions(+), 9 deletions(-)
>> create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> (...)
>> +static int __init dmi_init(void)
>> +{
>> + int ret;
>> + u8 *dmi_table = NULL;
>> + struct kobject *tables_kobj = NULL;
>> +
>> + if (!dmi_available) {
>> + ret = -ENODATA;
>> + goto err;
>> + }
>> +
>> + /*
>> + * Set up dmi directory at /sys/firmware/dmi. This entry should stay
>> + * even after farther error, as it can be used by other modules like
>> + * dmi-sysfs.
>> + */
>> + dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> + tables_kobj = kobject_create_and_add("tables", dmi_kobj);
> I'm afraid you can't do that. kobject_create_and_add() doesn't check if
> the parent is NULL and will happily create "tables" at the root of
> sysfs if for any reason the previous call to kobject_create_and_add()
> failed and returned NULL. I agree it is unlikely and would be cleaned
> up immediately, but still, instantiating an object at the wrong place,
> even temporarily, is wrong.
>
>> + if (!(dmi_kobj && tables_kobj)) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
> I'd rather go with:
>
> if (!(dmi_kobj = kobject_create_and_add("dmi", firmware_kobj))
> || !(tables_kobj = kobject_create_and_add("tables", dmi_kobj))) {
>
> I know that checkpatch complains about this construct, but in many
> cases it is the right thing to do.
>
> Another possible approach is simply:
>
> dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> if (dmi_kobj)
> tables_kobj = kobject_create_and_add("tables", dmi_kobj);
> if (!tables_kobj) {
>
>> +
>> + bin_attr_smbios_entry_point.size = smbios_entry_point_size;
>> + bin_attr_smbios_entry_point.private = smbios_entry_point;
>> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_smbios_entry_point);
>> + if (ret)
>> + goto err;
>> +
>> + dmi_table = dmi_remap(dmi_base, dmi_len);
>> + if (!dmi_table) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + bin_attr_DMI.size = dmi_len;
>> + bin_attr_DMI.private = dmi_table;
>> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
>> + if (!ret)
>> + return 0;
>> +
>> +err:
>> + pr_err("dmi: Firmware registration failed.\n");
>> +
>> + if (tables_kobj) {
>> + sysfs_remove_bin_file(tables_kobj,
>> + &bin_attr_smbios_entry_point);
>> + kobject_del(tables_kobj);
>> + kobject_put(tables_kobj);
>> + }
>> +
>> + if (dmi_table)
>> + dmi_unmap(dmi_table);
> I'm not happy with this single error label. This forces you to
> initialize all your pointers to NULL and then to check for them to find
> out what needs to be done in the error path. With multiple error labels,
> you would know exactly what needs to be done, this is more efficient.
>
>> +
>> + return ret;
>> +}
> Everything else looks good. No need to resend, I'll fix up the above
> myself in place or as an incremental patch.
>
> Thanks,
Don't bother, I'll send corrected version.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
From: Jean Delvare @ 2015-04-21 15:36 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: dmidecode-devel, matt.fleming, linux-doc, linux-api,
ard.biesheuvel, linux-kernel, leif.lindholm, mikew, roy.franz,
msalter, grant.likely
In-Reply-To: <1429525187-3376-3-git-send-email-ivan.khoronzhuk@globallogic.com>
Hi again Ivan,
On Mon, 20 Apr 2015 13:19:46 +0300, Ivan Khoronzhuk wrote:
> + bin_attr_DMI.size = dmi_len;
> + bin_attr_DMI.private = dmi_table;
> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
> + if (!ret)
> + return 0;
I just found that more work is needed here for the SMBIOS v3 entry
point case. These entry points do not specify the exact length of the
table, but only its maximum. The real world sample I have access to
indeed specifies a maximum length of 6419 bytes, but the actual table
only spans over 2373 bytes. It is properly terminated with a type 127
DMI structure, so the kernel table parser ignores the garbage after it.
The garbage is however exported to user-space above.
I taught dmidecode to ignore the garbage, but there are two problem
left here. First problem is a waste of memory. Minor issue I suppose,
who cares about a few kilobytes these days.
Second problem is a security problem. We are leaking the contents of
physical memory to user-space. In my case it's filled with 0xffs so no
big deal. But what if actual data happens to be stored there? It
definitely shouldn't go to user-space.
So dmi_len needs to be trimmed to the actual table size before the
attribute above is created. I have an idea how this could be
implemented easily, let me give it a try.
Maybe we should trim the length for previous implementations, too.
There is no reason to walk past a type 127 structure anyway, ever.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply
* Re: [PATCH v4 7/8] vhost: feature to set the vring endianness
From: Greg Kurz @ 2015-04-21 15:48 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, linux-api, linux-kernel, virtualization
In-Reply-To: <20150421155753-mutt-send-email-mst@redhat.com>
On Tue, 21 Apr 2015 16:04:23 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Apr 10, 2015 at 12:19:16PM +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).
> >
> > The vq->is_le boolean field is added to cache the endianness to be
> > used for ring accesses. It defaults to native endian, as expected
> > by legacy virtio devices. When the ring gets active, we force little
> > endian if the device is modern. When the ring is deactivated, we
> > revert to the native endian default.
> >
> > If cross-endian was compiled in, a vq->user_be boolean field is added
> > so that userspace may request a specific endianness. This field is
> > used to override the default when activating the ring of a legacy
> > device. It has no effect on modern devices.
> >
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> > drivers/vhost/Kconfig | 10 ++++++the
> > drivers/vhost/vhost.c | 76 +++++++++++++++++++++++++++++++++++++++++++-
> > drivers/vhost/vhost.h | 12 +++++--
> > include/uapi/linux/vhost.h | 9 +++++
> > 4 files changed, 103 insertions(+), 4 deletions(-)
> >
> > Changes since v3:
> > - VHOST_SET_VRING_ENDIAN_LEGACY ioctl renamed to VHOST_SET_VRING_BIG_ENDIAN
> > - ioctl API is now: 0 for le, 1 for be, other values are EINVAL
> > - ioctl doesn't filter out modern devices
> > - ioctl stubs return ENOIOCTLCMD
> > - forbid endianness changes when vring is active
> > - logic now handled with vq->is_le and vq->user_be according to device
> > start/stop as suggested by Michael
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index 017a1e8..0aec88c 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
>
> I'd prefer namin this VHOST_CROSS_ENDIAN_LEGACY
>
Yes, makes sense. I'll make sure most of the cross-endian changes are
easy to grep.
> > + 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).
>
> and is only useful on a few platforms (powerpc and arm).
>
> "it" seems to refer to "overhead", which is rarely needed.
> needed is a bit too strong, you can always e.g. run virtio
> in userspace.
>
My poor English again... it seems you understood what I wanted to
write though :)
> > +
> > + If unsure, say "N".
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 2ee2826..3eb756b 100644the
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -199,6 +199,10 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> > vq->call = NULL;
> > vq->log_ctx = NULL;
> > vq->memory = NULL;
> > + vq->is_le = virtio_legacy_is_little_endian();
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > + vq->user_be = !vq->is_le;
> > +#endif
>
> add a wrapper for this too?
>
Will do.
> > }
> >
> > static int vhost_worker(void *data)
> > @@ -630,6 +634,53 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > +static long vhost_set_vring_big_endian(struct vhost_virtqueue *vq,
> > + int __user *argp)
> > +{
> > + struct vhost_vring_state s;
> > +
> > + if (vq->private_data)
> > + return -EBUSY;
> > +
> > + if (copy_from_user(&s, argp, sizeof(s)))
> > + return -EFAULT;
> > +
> > + if (s.num && s.num != 1)
>
> s.num & ~0x1
>
Since s.num is unsigned and I assume this won't change, what about
s.num > 1 as suggested by Cornelia ?
>
> > + return -EINVAL;
> > +
> > + vq->user_be = s.num;
> > +
> > + return 0;
> > +}
> > +
> > +static long vhost_get_vring_big_endian(struct vhost_virtqueue *vq, u32 idx,
> > + int __user *argp)
> > +{
> > + struct vhost_vring_state s = {
> > + .index = idx,
> > + .num = vq->user_be
> > + };
> > +
> > + if (copy_to_user(argp, &s, sizeof(s)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +#else
> > +static long vhost_set_vring_big_endian(struct vhost_virtqueue *vq,
> > + int __user *argp)
> > +{
> > + return -ENOIOCTLCMD;
> > +}
> > +
> > +static long vhost_get_vring_big_endian(struct vhost_virtqueue *vq, u32 idx,
> > + int __user *argp)
> > +{
> > + return -ENOIOCTLCMD;
> > +}
> > +#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */
> > +
> > long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> > {
> > struct file *eventfp, *filep = NULL;
> > @@ -806,6 +857,12 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> > } else
> > filep = eventfp;the
> > break;
> > + case VHOST_SET_VRING_BIG_ENDIAN:
> > + r = vhost_set_vring_big_endian(vq, argp);
> > + break;
> > + case VHOST_GET_VRING_BIG_ENDIAN:
> > + r = vhost_get_vring_big_endian(vq, idx, argp);
> > + break;
> > default:
> > r = -ENOIOCTLCMD;
> > }
> > @@ -1040,12 +1097,29 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > +static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > +{
> > + vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be;
> > +}
> > +#else
> > +static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > +{
> > + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > + vq->is_le = true;
> > +}
> > +#endif
> > +
>
> I'd prefer localizing ifdefery somewhere near top of file.
>
Will do.
> > int vhost_init_used(struct vhost_virtqueue *vq)
> > {
> > __virtio16 last_used_idx;
> > int r;
> > - if (!vq->private_data)
> > + if (!vq->private_data) {
> > + vq->is_le = virtio_legacy_is_little_endian();
> > return 0;
> > + }
> > +
> > + vhost_init_is_le(vq);
> >
> > r = vhost_update_used_flags(vq);
> > if (r)
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 4e9a186..04b2add 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -106,6 +106,14 @@ struct vhost_virtqueue {
> > /* Log write descriptors */
> > void __user *log_base;
> > struct vhost_log *log;
> > +
> > + /* Ring endianness. Defaults to legacy native endianness.
> > + * Set to true when starting a modern virtio device. */
> > + bool is_le;
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > + /* Ring endianness requested by userspace for cross-endian support. */
> > + bool user_be;
> > +#endif
> > };
> >
> > struct vhost_dev {
> > @@ -175,9 +183,7 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> >
> > static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> > {
> > - if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > - return true;
> > - return virtio_legacy_is_little_endian();
> > + return vq->is_le;
> > }
> >
> > /* Memory accessors */
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index bb6a5b4..5cdebbc 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -103,6 +103,15 @@ 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 the vring byte order in num. This is a legacy only API that is simply
> > + * ignored when VIRTIO_F_VERSION_1 is set.
> > + * 0 to set to little-endian
> > + * 1 to set to big-endian
>
> How about defines for these?
>
Ok. I'll put the defines here so that all the cross-endian stuff
lies in the same hunk. Is it ok for you ?
> > + * other values return EINVAL.
> > + */
> > +#define VHOST_SET_VRING_BIG_ENDIAN _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state)
> > +#define VHOST_GET_VRING_BIG_ENDIAN _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
> > +
> > /* The following ioctls use eventfd file descriptors to signal and poll
> > * for events. */
> >
>
^ permalink raw reply
* [Patch v3 0/3] firmware: dmi_scan: add SBMIOS entry point and DMI tables
From: Ivan Khoronzhuk @ 2015-04-21 15:52 UTC (permalink / raw)
To: linux-kernel, matt.fleming, jdelvare, ard.biesheuvel,
grant.likely, linux-api, linux-doc, mikew
Cc: dmidecode-devel, leif.lindholm, msalter, roy.franz,
Ivan Khoronzhuk
This series adds SMBIOS entry point table and DMI table under
/sys/firmware/dmi/tables in order to use as an alternative to utilities
reading them from /dev/mem.
Based on linux next + 2 patches from Jean Delvare.
The patches can be obtained with links:
http://jdelvare.nerim.net/devel/linux-3/jdelvare-dmi/firmware-dmi-03-simplify-displayed-version.patch
http://jdelvare.nerim.net/devel/linux-3/jdelvare-dmi/firmware-dmi-04-fix-product-uuid.patch
Link on V2:
https://lkml.org/lkml/2015/4/20/218
Changes since V2
- use more labels in error path
Changes since V1
- correct error path in dmi-sysfs
- don't use globally dmi_table var
- use "DMI" in attribute name
- correct error path in dmi_init
- leave dmi_kobj even in case of error
- include linux/kobject.h in header
Ivan Khoronzhuk (3):
firmware: dmi_scan: rename dmi_table to dmi_decode_table
firmware: dmi_scan: add SBMIOS entry and DMI tables
Documentation: ABI: sysfs-firmware-dmi: add -entries suffix to file
name
...sfs-firmware-dmi => sysfs-firmware-dmi-entries} | 2 +-
.../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
drivers/firmware/dmi-sysfs.c | 17 ++---
drivers/firmware/dmi_scan.c | 88 ++++++++++++++++++++--
include/linux/dmi.h | 2 +
5 files changed, 116 insertions(+), 15 deletions(-)
rename Documentation/ABI/testing/{sysfs-firmware-dmi => sysfs-firmware-dmi-entries} (99%)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
--
1.9.1
^ permalink raw reply
* [Patch v3 1/3] firmware: dmi_scan: rename dmi_table to dmi_decode_table
From: Ivan Khoronzhuk @ 2015-04-21 15:52 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
matt.fleming-ral2JQCrhuEAvxtiuMwx3w, jdelvare-l3A5Bk7waGM,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, mikew-hpIqsD4AKlfQT0dZR+AlfA
Cc: dmidecode-devel-qX2TKyscuCcdnm+yROfE0A,
leif.lindholm-QSEj5FYQhm4dnm+yROfE0A,
msalter-H+wXaHxf7aLQT0dZR+AlfA, roy.franz-QSEj5FYQhm4dnm+yROfE0A,
Ivan Khoronzhuk
In-Reply-To: <1429631560-31359-1-git-send-email-ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
The "dmi_table" function looks like data instance, but it does DMI
table decode. This patch renames it to "dmi_decode_table" name as
more appropriate. That allows us to use "dmi_table" name for correct
purposes.
Reviewed-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
---
drivers/firmware/dmi_scan.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 97b1616..a864a6b 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -80,9 +80,9 @@ static const char * __init dmi_string(const struct dmi_header *dm, u8 s)
* We have to be cautious here. We have seen BIOSes with DMI pointers
* pointing to completely the wrong place for example
*/
-static void dmi_table(u8 *buf,
- void (*decode)(const struct dmi_header *, void *),
- void *private_data)
+static void dmi_decode_table(u8 *buf,
+ void (*decode)(const struct dmi_header *, void *),
+ void *private_data)
{
u8 *data = buf;
int i = 0;
@@ -130,7 +130,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
if (buf == NULL)
return -1;
- dmi_table(buf, decode, NULL);
+ dmi_decode_table(buf, decode, NULL);
add_device_randomness(buf, dmi_len);
@@ -897,7 +897,7 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
if (buf == NULL)
return -1;
- dmi_table(buf, decode, private_data);
+ dmi_decode_table(buf, decode, private_data);
dmi_unmap(buf);
return 0;
--
1.9.1
^ permalink raw reply related
* [Patch v3 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
From: Ivan Khoronzhuk @ 2015-04-21 15:52 UTC (permalink / raw)
To: linux-kernel, matt.fleming, jdelvare, ard.biesheuvel,
grant.likely, linux-api, linux-doc, mikew
Cc: dmidecode-devel, leif.lindholm, msalter, roy.franz,
Ivan Khoronzhuk
In-Reply-To: <1429631560-31359-1-git-send-email-ivan.khoronzhuk@globallogic.com>
Some utils, like dmidecode and smbios, need to access SMBIOS entry
table area in order to get information like SMBIOS version, size, etc.
Currently it's done via /dev/mem. But for situation when /dev/mem
usage is disabled, the utils have to use dmi sysfs instead, which
doesn't represent SMBIOS entry and adds code/delay redundancy when direct
access for table is needed.
So this patch creates dmi/tables and adds SMBIOS entry point to allow
utils in question to work correctly without /dev/mem. Also patch adds
raw dmi table to simplify dmi table processing in user space, as
proposed by Jean Delvare.
Tested-by: Roy Franz <roy.franz@linaro.org>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
---
.../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
drivers/firmware/dmi-sysfs.c | 17 +++--
drivers/firmware/dmi_scan.c | 78 ++++++++++++++++++++++
include/linux/dmi.h | 2 +
4 files changed, 110 insertions(+), 9 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
new file mode 100644
index 0000000..ff3cac8
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
@@ -0,0 +1,22 @@
+What: /sys/firmware/dmi/tables/
+Date: April 2015
+Contact: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
+Description:
+ The firmware provides DMI structures as a packed list of
+ data referenced by a SMBIOS table entry point. The SMBIOS
+ entry point contains general information, like SMBIOS
+ version, DMI table size, etc. The structure, content and
+ size of SMBIOS entry point is dependent on SMBIOS version.
+ The format of SMBIOS entry point and DMI structures
+ can be read in SMBIOS specification.
+
+ The dmi/tables provides raw SMBIOS entry point and DMI tables
+ through sysfs as an alternative to utilities reading them
+ from /dev/mem. The raw SMBIOS entry point and DMI table are
+ presented as binary attributes and are accessible via:
+
+ /sys/firmware/dmi/tables/smbios_entry_point
+ /sys/firmware/dmi/tables/DMI
+
+ The complete DMI information can be obtained using these two
+ tables.
diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index e0f1cb3..ef76e5e 100644
--- a/drivers/firmware/dmi-sysfs.c
+++ b/drivers/firmware/dmi-sysfs.c
@@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
.default_attrs = dmi_sysfs_entry_attrs,
};
-static struct kobject *dmi_kobj;
static struct kset *dmi_kset;
/* Global count of all instances seen. Only for setup */
@@ -648,17 +647,20 @@ static void cleanup_entry_list(void)
static int __init dmi_sysfs_init(void)
{
- int error = -ENOMEM;
+ int error;
int val;
- /* Set up our directory */
- dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
- if (!dmi_kobj)
+ if (!dmi_kobj) {
+ pr_err("dmi-sysfs: dmi entry is absent.\n");
+ error = -ENODATA;
goto err;
+ }
dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
- if (!dmi_kset)
+ if (!dmi_kset) {
+ error = -ENOMEM;
goto err;
+ }
val = 0;
error = dmi_walk(dmi_sysfs_register_handle, &val);
@@ -675,7 +677,6 @@ static int __init dmi_sysfs_init(void)
err:
cleanup_entry_list();
kset_unregister(dmi_kset);
- kobject_put(dmi_kobj);
return error;
}
@@ -685,8 +686,6 @@ static void __exit dmi_sysfs_exit(void)
pr_debug("dmi-sysfs: unloading.\n");
cleanup_entry_list();
kset_unregister(dmi_kset);
- kobject_del(dmi_kobj);
- kobject_put(dmi_kobj);
}
module_init(dmi_sysfs_init);
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index a864a6b..389f8e9 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -10,6 +10,9 @@
#include <asm/dmi.h>
#include <asm/unaligned.h>
+struct kobject *dmi_kobj;
+EXPORT_SYMBOL_GPL(dmi_kobj);
+
/*
* DMI stands for "Desktop Management Interface". It is part
* of and an antecedent to, SMBIOS, which stands for System
@@ -20,6 +23,9 @@ static const char dmi_empty_string[] = " ";
static u32 dmi_ver __initdata;
static u32 dmi_len;
static u16 dmi_num;
+static u8 smbios_entry_point[32];
+static int smbios_entry_point_size;
+
/*
* Catch too early calls to dmi_check_system():
*/
@@ -478,6 +484,8 @@ static int __init dmi_present(const u8 *buf)
if (memcmp(buf, "_SM_", 4) == 0 &&
buf[5] < 32 && dmi_checksum(buf, buf[5])) {
smbios_ver = get_unaligned_be16(buf + 6);
+ smbios_entry_point_size = buf[5];
+ memcpy(smbios_entry_point, buf, smbios_entry_point_size);
/* Some BIOS report weird SMBIOS version, fix that up */
switch (smbios_ver) {
@@ -512,6 +520,9 @@ static int __init dmi_present(const u8 *buf)
pr_info("SMBIOS %d.%d present.\n",
dmi_ver >> 8, dmi_ver & 0xFF);
} else {
+ smbios_entry_point_size = 15;
+ memcpy(smbios_entry_point, buf,
+ smbios_entry_point_size);
pr_info("Legacy DMI %d.%d present.\n",
dmi_ver >> 8, dmi_ver & 0xFF);
}
@@ -538,6 +549,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
dmi_num = 0; /* No longer specified */
dmi_len = get_unaligned_le32(buf + 12);
dmi_base = get_unaligned_le64(buf + 16);
+ smbios_entry_point_size = buf[6];
+ memcpy(smbios_entry_point, buf, smbios_entry_point_size);
if (dmi_walk_early(dmi_decode) == 0) {
pr_info("SMBIOS %d.%d.%d present.\n",
@@ -629,6 +642,71 @@ void __init dmi_scan_machine(void)
dmi_initialized = 1;
}
+static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf,
+ loff_t pos, size_t count)
+{
+ memcpy(buf, attr->private + pos, count);
+ return count;
+}
+
+static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
+static BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
+
+static int __init dmi_init(void)
+{
+ u8 *dmi_table;
+ int ret = -ENOMEM;
+ struct kobject *tables_kobj;
+
+ if (!dmi_available) {
+ ret = -ENODATA;
+ goto err;
+ }
+
+ /*
+ * Set up dmi directory at /sys/firmware/dmi. This entry should stay
+ * even after farther error, as it can be used by other modules like
+ * dmi-sysfs.
+ */
+ dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
+ if (!dmi_kobj)
+ goto err;
+
+ tables_kobj = kobject_create_and_add("tables", dmi_kobj);
+ if (!tables_kobj)
+ goto err;
+
+ dmi_table = dmi_remap(dmi_base, dmi_len);
+ if (!dmi_table)
+ goto err_tables;
+
+ bin_attr_smbios_entry_point.size = smbios_entry_point_size;
+ bin_attr_smbios_entry_point.private = smbios_entry_point;
+ ret = sysfs_create_bin_file(tables_kobj, &bin_attr_smbios_entry_point);
+ if (ret)
+ goto err_remap;
+
+ bin_attr_DMI.size = dmi_len;
+ bin_attr_DMI.private = dmi_table;
+ ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
+ if (!ret)
+ return 0;
+
+ sysfs_remove_bin_file(tables_kobj,
+ &bin_attr_smbios_entry_point);
+err_remap:
+ dmi_unmap(dmi_table);
+err_tables:
+ kobject_del(tables_kobj);
+ kobject_put(tables_kobj);
+err:
+ pr_err("dmi: Firmware registration failed.\n");
+
+ return ret;
+}
+subsys_initcall(dmi_init);
+
/**
* dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
*
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index f820f0a..2f9f988 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -2,6 +2,7 @@
#define __DMI_H__
#include <linux/list.h>
+#include <linux/kobject.h>
#include <linux/mod_devicetable.h>
/* enum dmi_field is in mod_devicetable.h */
@@ -93,6 +94,7 @@ struct dmi_dev_onboard {
int devfn;
};
+extern struct kobject *dmi_kobj;
extern int dmi_check_system(const struct dmi_system_id *list);
const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
extern const char * dmi_get_system_info(int field);
--
1.9.1
^ permalink raw reply related
* [Patch v3 3/3] Documentation: ABI: sysfs-firmware-dmi: add -entries suffix to file name
From: Ivan Khoronzhuk @ 2015-04-21 15:52 UTC (permalink / raw)
To: linux-kernel, matt.fleming, jdelvare, ard.biesheuvel,
grant.likely, linux-api, linux-doc, mikew
Cc: dmidecode-devel, leif.lindholm, msalter, roy.franz,
Ivan Khoronzhuk
In-Reply-To: <1429631560-31359-1-git-send-email-ivan.khoronzhuk@globallogic.com>
The dmi-sysfs module adds DMI table structures entries under
/sys/firmware/dmi/entries only, so rename documentation file to
sysfs-firmware-dmi-entries as more appropriate. Without renaming it's
confusing to differ this from sysfs-firmware-dmi-tables that adds raw
DMI table and actually adds "dmi" kobject.
Reviewed-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
---
.../ABI/testing/{sysfs-firmware-dmi => sysfs-firmware-dmi-entries} | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
rename Documentation/ABI/testing/{sysfs-firmware-dmi => sysfs-firmware-dmi-entries} (99%)
diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi-entries
similarity index 99%
rename from Documentation/ABI/testing/sysfs-firmware-dmi
rename to Documentation/ABI/testing/sysfs-firmware-dmi-entries
index c78f9ab..210ad44 100644
--- a/Documentation/ABI/testing/sysfs-firmware-dmi
+++ b/Documentation/ABI/testing/sysfs-firmware-dmi-entries
@@ -1,4 +1,4 @@
-What: /sys/firmware/dmi/
+What: /sys/firmware/dmi/entries/
Date: February 2011
Contact: Mike Waychison <mikew@google.com>
Description:
--
1.9.1
^ permalink raw reply related
* Re: [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
From: Ivan.khoronzhuk @ 2015-04-21 16:03 UTC (permalink / raw)
To: Jean Delvare
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
matt.fleming-ral2JQCrhuEAvxtiuMwx3w,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, mikew-hpIqsD4AKlfQT0dZR+AlfA,
dmidecode-devel-qX2TKyscuCcdnm+yROfE0A,
leif.lindholm-QSEj5FYQhm4dnm+yROfE0A,
msalter-H+wXaHxf7aLQT0dZR+AlfA, roy.franz-QSEj5FYQhm4dnm+yROfE0A
In-Reply-To: <20150421173613.6601da15-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org>
Jean,
On 21.04.15 18:36, Jean Delvare wrote:
> Hi again Ivan,
>
> On Mon, 20 Apr 2015 13:19:46 +0300, Ivan Khoronzhuk wrote:
>> + bin_attr_DMI.size = dmi_len;
>> + bin_attr_DMI.private = dmi_table;
>> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
>> + if (!ret)
>> + return 0;
> I just found that more work is needed here for the SMBIOS v3 entry
> point case. These entry points do not specify the exact length of the
> table, but only its maximum. The real world sample I have access to
> indeed specifies a maximum length of 6419 bytes, but the actual table
> only spans over 2373 bytes. It is properly terminated with a type 127
> DMI structure, so the kernel table parser ignores the garbage after it.
> The garbage is however exported to user-space above.
>
> I taught dmidecode to ignore the garbage, but there are two problem
> left here. First problem is a waste of memory. Minor issue I suppose,
> who cares about a few kilobytes these days.
>
> Second problem is a security problem. We are leaking the contents of
> physical memory to user-space. In my case it's filled with 0xffs so no
> big deal. But what if actual data happens to be stored there? It
> definitely shouldn't go to user-space.
>
> So dmi_len needs to be trimmed to the actual table size before the
> attribute above is created. I have an idea how this could be
> implemented easily, let me give it a try.
>
> Maybe we should trim the length for previous implementations, too.
> There is no reason to walk past a type 127 structure anyway, ever.
>
It can happen of-cause, I've also thought about that sometime ago,
but forget...).
I've sent the updated series already.
Let me know when your fix will be ready and I will re-base the
series if it has conflicts.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH v4 8/8] macvtap/tun: add VNET_BE flag
From: Greg Kurz @ 2015-04-21 16:22 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, Cornelia Huck, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, kvm-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <20150421160538-mutt-send-email-mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Tue, 21 Apr 2015 16:06:33 +0200
"Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, Apr 10, 2015 at 12:20:21PM +0200, Greg Kurz wrote:
> > The VNET_LE flag was introduced to fix accesses to virtio 1.0 headers
> > that are always little-endian. It can also be used to handle the special
> > case of a legacy little-endian device implemented by a big-endian host.
> >
> > Let's add a flag and ioctls for big-endian devices as well. If both flags
> > are set, little-endian wins.
> >
> > Since this is isn't a common usecase, the feature is controlled by a kernel
> > config option (not set by default).
> >
> > Both macvtap and tun are covered by this patch since they share the same
> > API with userland.
> >
> > Signed-off-by: Greg Kurz <gkurz-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > ---
> > drivers/net/Kconfig | 12 ++++++++
> > drivers/net/macvtap.c | 60 +++++++++++++++++++++++++++++++++++++++++-
> > drivers/net/tun.c | 62 ++++++++++++++++++++++++++++++++++++++++++-
> > include/uapi/linux/if_tun.h | 2 +
> > 4 files changed, 134 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > index df51d60..f0e23a0 100644
> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -244,6 +244,18 @@ config TUN
> >
> > If you don't know what to use this for, you don't need it.
> >
> > +config TUN_VNET_BE
> > + bool "Support for big-endian vnet headers"
> > + default n
> > + ---help---
> > + This option allows TUN/TAP and MACVTAP device drivers to parse
> > + vnet headers that are in big-endian byte order. It is useful
> > + when the headers come from a big-endian legacy virtio driver and
> > + the host is little-endian.
> > +
> > + Unless you have a little-endian system hosting a big-endian virtual
> > + machine with a virtio NIC, you should say N.
> > +
>
> should mention cross-endian, not big-endian, right?
>
The current TUN_VNET_LE related code is already doing cross-endian: without
this patch, one can already run a LE guest on a BE host... wouldn't it be
confusing to mention cross-endian only when the guest is BE ?
What about having a completely distinct implementation for cross-endian that
don't reuse the existing code and defines then ?
> > config VETH
> > tristate "Virtual ethernet pair device"
> > ---help---
> > diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> > index 0a03a66..e0ab1b7 100644
> > --- a/drivers/net/macvtap.c
> > +++ b/drivers/net/macvtap.c
> > @@ -48,12 +48,27 @@ struct macvtap_queue {
> > #define MACVTAP_FEATURES (IFF_VNET_HDR | IFF_MULTI_QUEUE)
> >
> > #define MACVTAP_VNET_LE 0x80000000
> > +#define MACVTAP_VNET_BE 0x40000000
> > +
> > +#ifdef CONFIG_TUN_VNET_BE
> > +static inline bool macvtap_legacy_is_little_endian(struct macvtap_queue *q)
> > +{
> > + if (q->flags & MACVTAP_VNET_BE)
> > + return false;
> > + return virtio_legacy_is_little_endian();
> > +}
> > +#else
> > +static inline bool macvtap_legacy_is_little_endian(struct macvtap_queue *q)
> > +{
> > + return virtio_legacy_is_little_endian();
> > +}
> > +#endif
> >
> > static inline bool macvtap_is_little_endian(struct macvtap_queue *q)
> > {
> > if (q->flags & MACVTAP_VNET_LE)
> > return true;
> > - return virtio_legacy_is_little_endian();
> > + return macvtap_legacy_is_little_endian(q);
> > }
> >
> > static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)
> > @@ -1000,6 +1015,43 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_TUN_VNET_BE
> > +static long macvtap_get_vnet_be(struct macvtap_queue *q, int __user *sp)
> > +{
> > + int s = !!(q->flags & MACVTAP_VNET_BE);
> > +
> > + if (put_user(s, sp))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > +static long macvtap_set_vnet_be(struct macvtap_queue *q, int __user *sp)
> > +{
> > + int s;
> > +
> > + if (get_user(s, sp))
> > + return -EFAULT;
> > +
> > + if (s)
> > + q->flags |= MACVTAP_VNET_BE;
> > + else
> > + q->flags &= ~MACVTAP_VNET_BE;
> > +
> > + return 0;
> > +}
> > +#else
> > +static long macvtap_get_vnet_be(struct macvtap_queue *q, int __user *argp)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +static long macvtap_set_vnet_be(struct macvtap_queue *q, int __user *argp)
> > +{
> > + return -EINVAL;
> > +}
> > +#endif /* CONFIG_TUN_VNET_BE */
> > +
> > /*
> > * provide compatibility with generic tun/tap interface
> > */
> > @@ -1097,6 +1149,12 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
> > q->flags &= ~MACVTAP_VNET_LE;
> > return 0;
> >
> > + case TUNGETVNETBE:
> > + return macvtap_get_vnet_be(q, sp);
> > +
> > + case TUNSETVNETBE:
> > + return macvtap_set_vnet_be(q, sp);
> > +
> > case TUNSETOFFLOAD:
> > /* let the user check for future flags */
> > if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 053f9b6..4e12488 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -111,6 +111,7 @@ do { \
> > #define TUN_FASYNC IFF_ATTACH_QUEUE
> > /* High bits in flags field are unused. */
> > #define TUN_VNET_LE 0x80000000
> > +#define TUN_VNET_BE 0x40000000
> >
> > #define TUN_FEATURES (IFF_NO_PI | IFF_ONE_QUEUE | IFF_VNET_HDR | \
> > IFF_MULTI_QUEUE)
> > @@ -206,11 +207,25 @@ struct tun_struct {
> > u32 flow_count;
> > };
> >
> > +#ifdef CONFIG_TUN_VNET_BE
> > +static inline bool tun_legacy_is_little_endian(struct tun_struct *tun)
> > +{
> > + if (tun->flags & TUN_VNET_BE)
> > + return false;
> > + return virtio_legacy_is_little_endian();
> > +}
> > +#else
> > +static inline bool tun_legacy_is_little_endian(struct tun_struct *tun)
> > +{
> > + return virtio_legacy_is_little_endian();
> > +}
> > +#endif
> > +
> > static inline bool tun_is_little_endian(struct tun_struct *tun)
> > {
> > if (tun->flags & TUN_VNET_LE)
> > return true;
> > - return virtio_legacy_is_little_endian();
> > + return tun_legacy_is_little_endian(tun);
> > }
> >
> > static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
> > @@ -1836,6 +1851,43 @@ unlock:
> > return ret;
> > }
> >
> > +#ifdef CONFIG_TUN_VNET_BE
> > +static long tun_get_vnet_be(struct tun_struct *tun, int __user *argp)
> > +{
> > + int be = !!(tun->flags & TUN_VNET_BE);
> > +
> > + if (put_user(be, argp))
> > + return EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > +static long tun_set_vnet_be(struct tun_struct *tun, int __user *argp)
> > +{
> > + int be;
> > +
> > + if (get_user(be, argp))
> > + return -EFAULT;
> > +
> > + if (be)
> > + tun->flags |= TUN_VNET_BE;
> > + else
> > + tun->flags &= ~TUN_VNET_BE;
> > +
> > + return 0;
> > +}
> > +#else
> > +static long tun_get_vnet_be(struct tun_struct *tun, int __user *argp)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +static long tun_set_vnet_be(struct tun_struct *tun, int __user *argp)
> > +{
> > + return -EINVAL;
> > +}
> > +#endif /* CONFIG_TUN_VNET_BE */
> > +
> > static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
> > unsigned long arg, int ifreq_len)
> > {
> > @@ -2065,6 +2117,14 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
> > tun->flags &= ~TUN_VNET_LE;
> > break;
> >
> > + case TUNGETVNETBE:
> > + ret = tun_get_vnet_be(tun, argp);
> > + break;
> > +
> > + case TUNSETVNETBE:
> > + ret = tun_set_vnet_be(tun, argp);
> > + break;
> > +
> > case TUNATTACHFILTER:
> > /* Can be set only for TAPs */
> > ret = -EINVAL;
> > diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h
> > index 50ae243..bcac4c0 100644
> > --- a/include/uapi/linux/if_tun.h
> > +++ b/include/uapi/linux/if_tun.h
> > @@ -50,6 +50,8 @@
> > #define TUNGETFILTER _IOR('T', 219, struct sock_fprog)
> > #define TUNSETVNETLE _IOW('T', 220, int)
> > #define TUNGETVNETLE _IOR('T', 221, int)
> > +#define TUNSETVNETBE _IOW('T', 222, int)
> > +#define TUNGETVNETBE _IOR('T', 223, int)
> >
> > /* TUNSETIFF ifr flags */
> > #define IFF_TUN 0x0001
>
^ permalink raw reply
* Re: [PATCH v4 6/8] virtio: add explicit big-endian support to memory accessors
From: Greg Kurz @ 2015-04-21 16:23 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, linux-api, linux-kernel, virtualization
In-Reply-To: <20150421160652-mutt-send-email-mst@redhat.com>
On Tue, 21 Apr 2015 16:09:44 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Apr 10, 2015 at 12:16:20PM +0200, Greg Kurz wrote:
> > The current memory accessors logic is:
> > - little endian if little_endian
> > - native endian (i.e. no byteswap) if !little_endian
> >
> > If we want to fully support cross-endian vhost, we also need to be
> > able to convert to big endian.
> >
> > Instead of changing the little_endian argument to some 3-value enum, this
> > patch changes the logic to:
> > - little endian if little_endian
> > - big endian if !little_endian
> >
> > The native endian case is handled by all users with a trivial helper. This
> > patch doesn't change any functionality, nor it does add overhead.
> >
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
>
> OK overall. Style comment:
>
> > drivers/net/macvtap.c | 4 +++-
> > drivers/net/tun.c | 4 +++-
> > drivers/vhost/vhost.h | 4 +++-
> > include/linux/virtio_byteorder.h | 24 ++++++++++++++----------
> > include/linux/virtio_config.h | 4 +++-
> > include/linux/vringh.h | 4 +++-
> > 6 files changed, 29 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> > index a2f2958..0a03a66 100644
> > --- a/drivers/net/macvtap.c
> > +++ b/drivers/net/macvtap.c
> > @@ -51,7 +51,9 @@ struct macvtap_queue {
> >
> > static inline bool macvtap_is_little_endian(struct macvtap_queue *q)
> > {
> > - return q->flags & MACVTAP_VNET_LE;
> > + if (q->flags & MACVTAP_VNET_LE)
> > + return true;
> > + return virtio_legacy_is_little_endian();
> > }
> >
>
> I'd prefer a bit more symmetry:
>
> + if (q->flags & MACVTAP_VNET_LE)
> + return true;
> + else
> + return virtio_legacy_is_little_endian();
>
> Or better just:
> return (q->flags & MACVTAP_VNET_LE) ? true : virtio_legacy_is_little_endian();
>
> might make line long, but your follow-up patch makes it short again,
> so that's ok.
>
Will do.
>
> > static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 3c3d6c0..053f9b6 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -208,7 +208,9 @@ struct tun_struct {
> >
> > static inline bool tun_is_little_endian(struct tun_struct *tun)
> > {
> > - return tun->flags & TUN_VNET_LE;
> > + if (tun->flags & TUN_VNET_LE)
> > + return true;
> > + return virtio_legacy_is_little_endian();
> > }
> >
> > static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 6a49960..4e9a186 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -175,7 +175,9 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> >
> > static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> > {
> > - return vhost_has_feature(vq, VIRTIO_F_VERSION_1);
> > + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > + return true;
> > + return virtio_legacy_is_little_endian();
> > }
> >
> > /* Memory accessors */
> > diff --git a/include/linux/virtio_byteorder.h b/include/linux/virtio_byteorder.h
> > index 51865d0..ce63a2c 100644
> > --- a/include/linux/virtio_byteorder.h
> > +++ b/include/linux/virtio_byteorder.h
> > @@ -3,17 +3,21 @@
> > #include <linux/types.h>
> > #include <uapi/linux/virtio_types.h>
> >
> > -/*
> > - * Low-level memory accessors for handling virtio in modern little endian and in
> > - * compatibility native endian format.
> > - */
> > +static inline bool virtio_legacy_is_little_endian(void)
> > +{
> > +#ifdef __LITTLE_ENDIAN
> > + return true;
> > +#else
> > + return false;
> > +#endif
> > +}
> >
> > static inline u16 __virtio16_to_cpu(bool little_endian, __virtio16 val)
> > {
> > if (little_endian)
> > return le16_to_cpu((__force __le16)val);
> > else
> > - return (__force u16)val;
> > + return be16_to_cpu((__force __be16)val);
> > }
> >
> > static inline __virtio16 __cpu_to_virtio16(bool little_endian, u16 val)
> > @@ -21,7 +25,7 @@ static inline __virtio16 __cpu_to_virtio16(bool little_endian, u16 val)
> > if (little_endian)
> > return (__force __virtio16)cpu_to_le16(val);
> > else
> > - return (__force __virtio16)val;
> > + return (__force __virtio16)cpu_to_be16(val);
> > }
> >
> > static inline u32 __virtio32_to_cpu(bool little_endian, __virtio32 val)
> > @@ -29,7 +33,7 @@ static inline u32 __virtio32_to_cpu(bool little_endian, __virtio32 val)
> > if (little_endian)
> > return le32_to_cpu((__force __le32)val);
> > else
> > - return (__force u32)val;
> > + return be32_to_cpu((__force __be32)val);
> > }
> >
> > static inline __virtio32 __cpu_to_virtio32(bool little_endian, u32 val)
> > @@ -37,7 +41,7 @@ static inline __virtio32 __cpu_to_virtio32(bool little_endian, u32 val)
> > if (little_endian)
> > return (__force __virtio32)cpu_to_le32(val);
> > else
> > - return (__force __virtio32)val;
> > + return (__force __virtio32)cpu_to_be32(val);
> > }
> >
> > static inline u64 __virtio64_to_cpu(bool little_endian, __virtio64 val)
> > @@ -45,7 +49,7 @@ static inline u64 __virtio64_to_cpu(bool little_endian, __virtio64 val)
> > if (little_endian)
> > return le64_to_cpu((__force __le64)val);
> > else
> > - return (__force u64)val;
> > + return be64_to_cpu((__force __be64)val);
> > }
> >
> > static inline __virtio64 __cpu_to_virtio64(bool little_endian, u64 val)
> > @@ -53,7 +57,7 @@ static inline __virtio64 __cpu_to_virtio64(bool little_endian, u64 val)
> > if (little_endian)
> > return (__force __virtio64)cpu_to_le64(val);
> > else
> > - return (__force __virtio64)val;
> > + return (__force __virtio64)cpu_to_be64(val);
> > }
> >
> > #endif /* _LINUX_VIRTIO_BYTEORDER */
> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > index bd1a582..36a6daa 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -207,7 +207,9 @@ int virtqueue_set_affinity(struct virtqueue *vq, int cpu)
> >
> > static inline bool virtio_is_little_endian(struct virtio_device *vdev)
> > {
> > - return virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
> > + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> > + return true;
> > + return virtio_legacy_is_little_endian();
> > }
> >
> > /* Memory accessors */
> > diff --git a/include/linux/vringh.h b/include/linux/vringh.h
> > index 3ed62ef..d786c2d 100644
> > --- a/include/linux/vringh.h
> > +++ b/include/linux/vringh.h
> > @@ -228,7 +228,9 @@ static inline void vringh_notify(struct vringh *vrh)
> >
> > static inline bool vringh_is_little_endian(const struct vringh *vrh)
> > {
> > - return vrh->little_endian;
> > + if (vrh->little_endian)
> > + return true;
> > + return virtio_legacy_is_little_endian();
> > }
> >
> > static inline u16 vringh16_to_cpu(const struct vringh *vrh, __virtio16 val)
>
^ permalink raw reply
* Re: [PATCH v4 0/8] vhost: support for cross endian guests
From: Greg Kurz @ 2015-04-21 16:24 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, linux-api, linux-kernel, virtualization
In-Reply-To: <20150421160954-mutt-send-email-mst@redhat.com>
On Tue, 21 Apr 2015 16:10:18 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Apr 10, 2015 at 12:15:00PM +0200, Greg Kurz wrote:
> > Hi,
> >
> > This patchset allows vhost to be used with legacy virtio when guest and host
> > have a different endianness.
> >
> > Patch 7 got rewritten according to Cornelia's and Michael's comments. I have
> > also introduced patch 8 that brings BE vnet headers support to tun/macvtap.
> >
> > This series is enough to have vhost_net working flawlessly. I could
> > succesfully reboot guests from ppc64 to ppc64le and vice-versa on ppc64
> > and ppc64le hosts.
>
> Looks good overall.
> A couple of style comments.
>
> Thanks!
>
Thanks for your time Michael.
> > ---
> >
> > Greg Kurz (8):
> > 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
> > macvtap/tun: add VNET_BE flag
> >
> >
> > drivers/net/Kconfig | 12 ++++++
> > drivers/net/macvtap.c | 69 ++++++++++++++++++++++++++++++++++-
> > drivers/net/tun.c | 71 +++++++++++++++++++++++++++++++++++-
> > drivers/vhost/Kconfig | 10 +++++
> > drivers/vhost/vhost.c | 76 ++++++++++++++++++++++++++++++++++++++
> > drivers/vhost/vhost.h | 25 ++++++++++---
> > include/linux/virtio_byteorder.h | 24 +++++++-----
> > include/linux/virtio_config.h | 19 +++++++---
> > include/linux/vringh.h | 19 +++++++---
> > include/uapi/linux/if_tun.h | 2 +
> > include/uapi/linux/vhost.h | 9 +++++
> > 11 files changed, 303 insertions(+), 33 deletions(-)
> >
> > --
> > Greg
>
^ permalink raw reply
* [PATCH 0/2] clone: Support passing tls argument via C rather than pt_regs magic
From: Josh Triplett @ 2015-04-21 17:46 UTC (permalink / raw)
To: Andy Lutomirski, Ingo Molnar, H. Peter Anvin, Peter Zijlstra,
Thomas Gleixner, Linus Torvalds, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, x86-DgEjT+Ai2ygdnm+yROfE0A
clone has some of the quirkiest syscall handling in the kernel, with a pile of
special cases, historical curiosities, and architecture-specific calling
conventions. In particular, clone with CLONE_SETTLS accepts a parameter "tls"
that the C entry point completely ignores and some assembly entry points
overwrite; instead, the low-level arch-specific code pulls the tls parameter
out of the arch-specific register captured as part of pt_regs on entry to the
kernel. That's a massive hack, and it makes the arch-specific code only work
when called via the specific existing syscall entry points; because of this
hack, any new clone-like system call would have to accept an identical tls
argument in exactly the same arch-specific position, rather than providing a
unified system call entry point across architectures.
The first patch allows architectures to handle the tls argument via normal C
parameter passing, if they opt in by selecting HAVE_COPY_THREAD_TLS. The
second patch makes 32-bit and 64-bit x86 opt into this.
These two patches came out of the clone4 series, which isn't ready for this
merge window, but these first two cleanup patches were entirely uncontroversial
and have acks. I'd like to go ahead and submit these two so that other
architectures can begin building on top of this and opting into
HAVE_COPY_THREAD_TLS. However, I'm also happy to wait and send these through
the next merge window (along with v3 of clone4) if anyone would prefer that.
Josh Triplett (2):
clone: Support passing tls argument via C rather than pt_regs magic
x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit
arch/Kconfig | 7 ++++++
arch/x86/Kconfig | 1 +
arch/x86/ia32/ia32entry.S | 2 +-
arch/x86/kernel/process_32.c | 6 ++---
arch/x86/kernel/process_64.c | 8 +++----
include/linux/sched.h | 14 +++++++++++
include/linux/syscalls.h | 6 ++---
kernel/fork.c | 55 +++++++++++++++++++++++++++++---------------
8 files changed, 69 insertions(+), 30 deletions(-)
--
2.1.4
^ permalink raw reply
* [PATCH 1/2] clone: Support passing tls argument via C rather than pt_regs magic
From: Josh Triplett @ 2015-04-21 17:47 UTC (permalink / raw)
To: Andy Lutomirski, Ingo Molnar, H. Peter Anvin, Peter Zijlstra,
Thomas Gleixner, Linus Torvalds, linux-api, linux-kernel, x86
clone with CLONE_SETTLS accepts an argument to set the thread-local
storage area for the new thread. sys_clone declares an int argument
tls_val in the appropriate point in the argument list (based on the
various CLONE_BACKWARDS variants), but doesn't actually use or pass
along that argument. Instead, sys_clone calls do_fork, which calls
copy_process, which calls the arch-specific copy_thread, and copy_thread
pulls the corresponding syscall argument out of the pt_regs captured at
kernel entry (knowing what argument of clone that architecture passes
tls in).
Apart from being awful and inscrutable, that also only works because
only one code path into copy_thread can pass the CLONE_SETTLS flag, and
that code path comes from sys_clone with its architecture-specific
argument-passing order. This prevents introducing a new version of the
clone system call without propagating the same architecture-specific
position of the tls argument.
However, there's no reason to pull the argument out of pt_regs when
sys_clone could just pass it down via C function call arguments.
Introduce a new CONFIG_HAVE_COPY_THREAD_TLS for architectures to opt
into, and a new copy_thread_tls that accepts the tls parameter as an
additional unsigned long (syscall-argument-sized) argument.
Change sys_clone's tls argument to an unsigned long (which does
not change the ABI), and pass that down to copy_thread_tls.
Architectures that don't opt into copy_thread_tls will continue to
ignore the C argument to sys_clone in favor of the pt_regs captured at
kernel entry, and thus will be unable to introduce new versions of the
clone syscall.
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
Acked-by: Andy Lutomirski <luto@kernel.org>
---
arch/Kconfig | 7 ++++++
include/linux/sched.h | 14 ++++++++++++
include/linux/syscalls.h | 6 +++---
kernel/fork.c | 55 +++++++++++++++++++++++++++++++-----------------
4 files changed, 60 insertions(+), 22 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 05d7a8a..4834a58 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -484,6 +484,13 @@ config HAVE_IRQ_EXIT_ON_IRQ_STACK
This spares a stack switch and improves cache usage on softirq
processing.
+config HAVE_COPY_THREAD_TLS
+ bool
+ help
+ Architecture provides copy_thread_tls to accept tls argument via
+ normal C parameter passing, rather than extracting the syscall
+ argument from pt_regs.
+
#
# ABI hall of shame
#
diff --git a/include/linux/sched.h b/include/linux/sched.h
index a419b65..2cc88c6 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2480,8 +2480,22 @@ extern struct mm_struct *mm_access(struct task_struct *task, unsigned int mode);
/* Remove the current tasks stale references to the old mm_struct */
extern void mm_release(struct task_struct *, struct mm_struct *);
+#ifdef CONFIG_HAVE_COPY_THREAD_TLS
+extern int copy_thread_tls(unsigned long, unsigned long, unsigned long,
+ struct task_struct *, unsigned long);
+#else
extern int copy_thread(unsigned long, unsigned long, unsigned long,
struct task_struct *);
+
+/* Architectures that haven't opted into copy_thread_tls get the tls argument
+ * via pt_regs, so ignore the tls argument passed via C. */
+static inline int copy_thread_tls(
+ unsigned long clone_flags, unsigned long sp, unsigned long arg,
+ struct task_struct *p, unsigned long tls)
+{
+ return copy_thread(clone_flags, sp, arg, p);
+}
+#endif
extern void flush_thread(void);
extern void exit_thread(void);
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 76d1e38..bb51bec 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -827,15 +827,15 @@ asmlinkage long sys_syncfs(int fd);
asmlinkage long sys_fork(void);
asmlinkage long sys_vfork(void);
#ifdef CONFIG_CLONE_BACKWARDS
-asmlinkage long sys_clone(unsigned long, unsigned long, int __user *, int,
+asmlinkage long sys_clone(unsigned long, unsigned long, int __user *, unsigned long,
int __user *);
#else
#ifdef CONFIG_CLONE_BACKWARDS3
asmlinkage long sys_clone(unsigned long, unsigned long, int, int __user *,
- int __user *, int);
+ int __user *, unsigned long);
#else
asmlinkage long sys_clone(unsigned long, unsigned long, int __user *,
- int __user *, int);
+ int __user *, unsigned long);
#endif
#endif
diff --git a/kernel/fork.c b/kernel/fork.c
index cf65139..b3dadf4 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1192,7 +1192,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_size,
int __user *child_tidptr,
struct pid *pid,
- int trace)
+ int trace,
+ unsigned long tls)
{
int retval;
struct task_struct *p;
@@ -1401,7 +1402,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
retval = copy_io(clone_flags, p);
if (retval)
goto bad_fork_cleanup_namespaces;
- retval = copy_thread(clone_flags, stack_start, stack_size, p);
+ retval = copy_thread_tls(clone_flags, stack_start, stack_size, p, tls);
if (retval)
goto bad_fork_cleanup_io;
@@ -1613,7 +1614,7 @@ static inline void init_idle_pids(struct pid_link *links)
struct task_struct *fork_idle(int cpu)
{
struct task_struct *task;
- task = copy_process(CLONE_VM, 0, 0, NULL, &init_struct_pid, 0);
+ task = copy_process(CLONE_VM, 0, 0, NULL, &init_struct_pid, 0, 0);
if (!IS_ERR(task)) {
init_idle_pids(task->pids);
init_idle(task, cpu);
@@ -1628,11 +1629,13 @@ struct task_struct *fork_idle(int cpu)
* It copies the process, and if successful kick-starts
* it and waits for it to finish using the VM if required.
*/
-long do_fork(unsigned long clone_flags,
- unsigned long stack_start,
- unsigned long stack_size,
- int __user *parent_tidptr,
- int __user *child_tidptr)
+static long _do_fork(
+ unsigned long clone_flags,
+ unsigned long stack_start,
+ unsigned long stack_size,
+ int __user *parent_tidptr,
+ int __user *child_tidptr,
+ unsigned long tls)
{
struct task_struct *p;
int trace = 0;
@@ -1657,7 +1660,7 @@ long do_fork(unsigned long clone_flags,
}
p = copy_process(clone_flags, stack_start, stack_size,
- child_tidptr, NULL, trace);
+ child_tidptr, NULL, trace, tls);
/*
* Do this prior waking up the new thread - the thread pointer
* might get invalid after that point, if the thread exits quickly.
@@ -1698,20 +1701,34 @@ long do_fork(unsigned long clone_flags,
return nr;
}
+#ifndef CONFIG_HAVE_COPY_THREAD_TLS
+/* For compatibility with architectures that call do_fork directly rather than
+ * using the syscall entry points below. */
+long do_fork(unsigned long clone_flags,
+ unsigned long stack_start,
+ unsigned long stack_size,
+ int __user *parent_tidptr,
+ int __user *child_tidptr)
+{
+ return _do_fork(clone_flags, stack_start, stack_size,
+ parent_tidptr, child_tidptr, 0);
+}
+#endif
+
/*
* Create a kernel thread.
*/
pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long flags)
{
- return do_fork(flags|CLONE_VM|CLONE_UNTRACED, (unsigned long)fn,
- (unsigned long)arg, NULL, NULL);
+ return _do_fork(flags|CLONE_VM|CLONE_UNTRACED, (unsigned long)fn,
+ (unsigned long)arg, NULL, NULL, 0);
}
#ifdef __ARCH_WANT_SYS_FORK
SYSCALL_DEFINE0(fork)
{
#ifdef CONFIG_MMU
- return do_fork(SIGCHLD, 0, 0, NULL, NULL);
+ return _do_fork(SIGCHLD, 0, 0, NULL, NULL, 0);
#else
/* can not support in nommu mode */
return -EINVAL;
@@ -1722,8 +1739,8 @@ SYSCALL_DEFINE0(fork)
#ifdef __ARCH_WANT_SYS_VFORK
SYSCALL_DEFINE0(vfork)
{
- return do_fork(CLONE_VFORK | CLONE_VM | SIGCHLD, 0,
- 0, NULL, NULL);
+ return _do_fork(CLONE_VFORK | CLONE_VM | SIGCHLD, 0,
+ 0, NULL, NULL, 0);
}
#endif
@@ -1731,27 +1748,27 @@ SYSCALL_DEFINE0(vfork)
#ifdef CONFIG_CLONE_BACKWARDS
SYSCALL_DEFINE5(clone, unsigned long, clone_flags, unsigned long, newsp,
int __user *, parent_tidptr,
- int, tls_val,
+ unsigned long, tls,
int __user *, child_tidptr)
#elif defined(CONFIG_CLONE_BACKWARDS2)
SYSCALL_DEFINE5(clone, unsigned long, newsp, unsigned long, clone_flags,
int __user *, parent_tidptr,
int __user *, child_tidptr,
- int, tls_val)
+ unsigned long, tls)
#elif defined(CONFIG_CLONE_BACKWARDS3)
SYSCALL_DEFINE6(clone, unsigned long, clone_flags, unsigned long, newsp,
int, stack_size,
int __user *, parent_tidptr,
int __user *, child_tidptr,
- int, tls_val)
+ unsigned long, tls)
#else
SYSCALL_DEFINE5(clone, unsigned long, clone_flags, unsigned long, newsp,
int __user *, parent_tidptr,
int __user *, child_tidptr,
- int, tls_val)
+ unsigned long, tls)
#endif
{
- return do_fork(clone_flags, newsp, 0, parent_tidptr, child_tidptr);
+ return _do_fork(clone_flags, newsp, 0, parent_tidptr, child_tidptr, tls);
}
#endif
--
2.1.4
^ permalink raw reply related
* [PATCH 2/2] x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit
From: Josh Triplett @ 2015-04-21 17:47 UTC (permalink / raw)
To: Andy Lutomirski, Ingo Molnar, H. Peter Anvin, Peter Zijlstra,
Thomas Gleixner, Linus Torvalds, linux-api, linux-kernel, x86
For 32-bit userspace on a 64-bit kernel, this requires modifying
stub32_clone to actually swap the appropriate arguments to match
CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls
broken.
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
Acked-by: Andy Lutomirski <luto@kernel.org>
---
arch/x86/Kconfig | 1 +
arch/x86/ia32/ia32entry.S | 2 +-
arch/x86/kernel/process_32.c | 6 +++---
arch/x86/kernel/process_64.c | 8 ++++----
4 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b7d31ca..4960b0d 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -124,6 +124,7 @@ config X86
select MODULES_USE_ELF_REL if X86_32
select MODULES_USE_ELF_RELA if X86_64
select CLONE_BACKWARDS if X86_32
+ select HAVE_COPY_THREAD_TLS
select ARCH_USE_BUILTIN_BSWAP
select ARCH_USE_QUEUE_RWLOCK
select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION
diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index 156ebca..0286735 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -487,7 +487,7 @@ GLOBAL(\label)
ALIGN
GLOBAL(stub32_clone)
leaq sys_clone(%rip),%rax
- mov %r8, %rcx
+ xchg %r8, %rcx
jmp ia32_ptregs_common
ALIGN
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 603c4f9..ead28ff 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -129,8 +129,8 @@ void release_thread(struct task_struct *dead_task)
release_vm86_irqs(dead_task);
}
-int copy_thread(unsigned long clone_flags, unsigned long sp,
- unsigned long arg, struct task_struct *p)
+int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
+ unsigned long arg, struct task_struct *p, unsigned long tls)
{
struct pt_regs *childregs = task_pt_regs(p);
struct task_struct *tsk;
@@ -185,7 +185,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
*/
if (clone_flags & CLONE_SETTLS)
err = do_set_thread_area(p, -1,
- (struct user_desc __user *)childregs->si, 0);
+ (struct user_desc __user *)tls, 0);
if (err && p->thread.io_bitmap_ptr) {
kfree(p->thread.io_bitmap_ptr);
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 67fcc43..c69cabc 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -151,8 +151,8 @@ static inline u32 read_32bit_tls(struct task_struct *t, int tls)
return get_desc_base(&t->thread.tls_array[tls]);
}
-int copy_thread(unsigned long clone_flags, unsigned long sp,
- unsigned long arg, struct task_struct *p)
+int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
+ unsigned long arg, struct task_struct *p, unsigned long tls)
{
int err;
struct pt_regs *childregs;
@@ -209,10 +209,10 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
#ifdef CONFIG_IA32_EMULATION
if (test_thread_flag(TIF_IA32))
err = do_set_thread_area(p, -1,
- (struct user_desc __user *)childregs->si, 0);
+ (struct user_desc __user *)tls, 0);
else
#endif
- err = do_arch_prctl(p, ARCH_SET_FS, childregs->r8);
+ err = do_arch_prctl(p, ARCH_SET_FS, tls);
if (err)
goto out;
}
--
2.1.4
^ permalink raw reply related
* Re: [PATCH v4 7/8] vhost: feature to set the vring endianness
From: Michael S. Tsirkin @ 2015-04-21 18:25 UTC (permalink / raw)
To: Greg Kurz
Cc: Rusty Russell, Cornelia Huck, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, kvm-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <20150421174820.78c150b7-GiB8zCg7hOfDOqzlkpFKJg@public.gmane.org>
On Tue, Apr 21, 2015 at 05:48:20PM +0200, Greg Kurz wrote:
> On Tue, 21 Apr 2015 16:04:23 +0200
> "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> > On Fri, Apr 10, 2015 at 12:19:16PM +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).
> > >
> > > The vq->is_le boolean field is added to cache the endianness to be
> > > used for ring accesses. It defaults to native endian, as expected
> > > by legacy virtio devices. When the ring gets active, we force little
> > > endian if the device is modern. When the ring is deactivated, we
> > > revert to the native endian default.
> > >
> > > If cross-endian was compiled in, a vq->user_be boolean field is added
> > > so that userspace may request a specific endianness. This field is
> > > used to override the default when activating the ring of a legacy
> > > device. It has no effect on modern devices.
> > >
> > > Signed-off-by: Greg Kurz <gkurz-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > > ---
> > > drivers/vhost/Kconfig | 10 ++++++the
> > > drivers/vhost/vhost.c | 76 +++++++++++++++++++++++++++++++++++++++++++-
> > > drivers/vhost/vhost.h | 12 +++++--
> > > include/uapi/linux/vhost.h | 9 +++++
> > > 4 files changed, 103 insertions(+), 4 deletions(-)
> > >
> > > Changes since v3:
> > > - VHOST_SET_VRING_ENDIAN_LEGACY ioctl renamed to VHOST_SET_VRING_BIG_ENDIAN
> > > - ioctl API is now: 0 for le, 1 for be, other values are EINVAL
> > > - ioctl doesn't filter out modern devices
> > > - ioctl stubs return ENOIOCTLCMD
> > > - forbid endianness changes when vring is active
> > > - logic now handled with vq->is_le and vq->user_be according to device
> > > start/stop as suggested by Michael
> > >
> > > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > > index 017a1e8..0aec88c 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
> >
> > I'd prefer namin this VHOST_CROSS_ENDIAN_LEGACY
> >
>
> Yes, makes sense. I'll make sure most of the cross-endian changes are
> easy to grep.
>
> > > + 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).
> >
> > and is only useful on a few platforms (powerpc and arm).
> >
> > "it" seems to refer to "overhead", which is rarely needed.
> > needed is a bit too strong, you can always e.g. run virtio
> > in userspace.
> >
>
> My poor English again... it seems you understood what I wanted to
> write though :)
>
> > > +
> > > + If unsure, say "N".
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index 2ee2826..3eb756b 100644the
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -199,6 +199,10 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> > > vq->call = NULL;
> > > vq->log_ctx = NULL;
> > > vq->memory = NULL;
> > > + vq->is_le = virtio_legacy_is_little_endian();
> > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > > + vq->user_be = !vq->is_le;
> > > +#endif
> >
> > add a wrapper for this too?
> >
>
> Will do.
>
> > > }
> > >
> > > static int vhost_worker(void *data)
> > > @@ -630,6 +634,53 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> > > return 0;
> > > }
> > >
> > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > > +static long vhost_set_vring_big_endian(struct vhost_virtqueue *vq,
> > > + int __user *argp)
> > > +{
> > > + struct vhost_vring_state s;
> > > +
> > > + if (vq->private_data)
> > > + return -EBUSY;
> > > +
> > > + if (copy_from_user(&s, argp, sizeof(s)))
> > > + return -EFAULT;
> > > +
> > > + if (s.num && s.num != 1)
> >
> > s.num & ~0x1
> >
>
> Since s.num is unsigned and I assume this won't change, what about
> s.num > 1 as suggested by Cornelia ?
I just tried and gcc optimizes
s.num != 0 && s.num != 1 to s.num > 1
The former will be more readable once we
replace 0 and 1 with defines.
So ignore my advice, keep code as is but use defines.
> >
> > > + return -EINVAL;
> > > +
> > > + vq->user_be = s.num;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static long vhost_get_vring_big_endian(struct vhost_virtqueue *vq, u32 idx,
> > > + int __user *argp)
> > > +{
> > > + struct vhost_vring_state s = {
> > > + .index = idx,
> > > + .num = vq->user_be
> > > + };
> > > +
> > > + if (copy_to_user(argp, &s, sizeof(s)))
> > > + return -EFAULT;
> > > +
> > > + return 0;
> > > +}
> > > +#else
> > > +static long vhost_set_vring_big_endian(struct vhost_virtqueue *vq,
> > > + int __user *argp)
> > > +{
> > > + return -ENOIOCTLCMD;
> > > +}
> > > +
> > > +static long vhost_get_vring_big_endian(struct vhost_virtqueue *vq, u32 idx,
> > > + int __user *argp)
> > > +{
> > > + return -ENOIOCTLCMD;
> > > +}
> > > +#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */
> > > +
> > > long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> > > {
> > > struct file *eventfp, *filep = NULL;
> > > @@ -806,6 +857,12 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> > > } else
> > > filep = eventfp;the
> > > break;
> > > + case VHOST_SET_VRING_BIG_ENDIAN:
> > > + r = vhost_set_vring_big_endian(vq, argp);
> > > + break;
> > > + case VHOST_GET_VRING_BIG_ENDIAN:
> > > + r = vhost_get_vring_big_endian(vq, idx, argp);
> > > + break;
> > > default:
> > > r = -ENOIOCTLCMD;
> > > }
> > > @@ -1040,12 +1097,29 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
> > > return 0;
> > > }
> > >
> > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > > +static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > > +{
> > > + vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be;
> > > +}
> > > +#else
> > > +static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > > +{
> > > + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > > + vq->is_le = true;
> > > +}
> > > +#endif
> > > +
> >
> > I'd prefer localizing ifdefery somewhere near top of file.
> >
>
> Will do.
>
> > > int vhost_init_used(struct vhost_virtqueue *vq)
> > > {
> > > __virtio16 last_used_idx;
> > > int r;
> > > - if (!vq->private_data)
> > > + if (!vq->private_data) {
> > > + vq->is_le = virtio_legacy_is_little_endian();
> > > return 0;
> > > + }
> > > +
> > > + vhost_init_is_le(vq);
> > >
> > > r = vhost_update_used_flags(vq);
> > > if (r)
> > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > > index 4e9a186..04b2add 100644
> > > --- a/drivers/vhost/vhost.h
> > > +++ b/drivers/vhost/vhost.h
> > > @@ -106,6 +106,14 @@ struct vhost_virtqueue {
> > > /* Log write descriptors */
> > > void __user *log_base;
> > > struct vhost_log *log;
> > > +
> > > + /* Ring endianness. Defaults to legacy native endianness.
> > > + * Set to true when starting a modern virtio device. */
> > > + bool is_le;
> > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > > + /* Ring endianness requested by userspace for cross-endian support. */
> > > + bool user_be;
> > > +#endif
> > > };
> > >
> > > struct vhost_dev {
> > > @@ -175,9 +183,7 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> > >
> > > static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> > > {
> > > - if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > > - return true;
> > > - return virtio_legacy_is_little_endian();
> > > + return vq->is_le;
> > > }
> > >
> > > /* Memory accessors */
> > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > index bb6a5b4..5cdebbc 100644
> > > --- a/include/uapi/linux/vhost.h
> > > +++ b/include/uapi/linux/vhost.h
> > > @@ -103,6 +103,15 @@ 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 the vring byte order in num. This is a legacy only API that is simply
> > > + * ignored when VIRTIO_F_VERSION_1 is set.
> > > + * 0 to set to little-endian
> > > + * 1 to set to big-endian
> >
> > How about defines for these?
> >
>
> Ok. I'll put the defines here so that all the cross-endian stuff
> lies in the same hunk. Is it ok for you ?
Fine.
> > > + * other values return EINVAL.
Pls also add a note saying that not all kernel configurations support this ioctl,
but all configurations that support SET also support GET.
> > > + */
> > > +#define VHOST_SET_VRING_BIG_ENDIAN _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state)
> > > +#define VHOST_GET_VRING_BIG_ENDIAN _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
> > > +
> > > /* The following ioctls use eventfd file descriptors to signal and poll
> > > * for events. */
> > >
> >
I'm inclined to think VHOST_SET_VRING_ENDIAN is a slightly better name.
What do you think?
--
MST
^ permalink raw reply
* Re: [PATCH v4 8/8] macvtap/tun: add VNET_BE flag
From: Michael S. Tsirkin @ 2015-04-21 18:30 UTC (permalink / raw)
To: Greg Kurz
Cc: Rusty Russell, Cornelia Huck, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, kvm-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <20150421182220.33257bd6-GiB8zCg7hOfDOqzlkpFKJg@public.gmane.org>
On Tue, Apr 21, 2015 at 06:22:20PM +0200, Greg Kurz wrote:
> On Tue, 21 Apr 2015 16:06:33 +0200
> "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> > On Fri, Apr 10, 2015 at 12:20:21PM +0200, Greg Kurz wrote:
> > > The VNET_LE flag was introduced to fix accesses to virtio 1.0 headers
> > > that are always little-endian. It can also be used to handle the special
> > > case of a legacy little-endian device implemented by a big-endian host.
> > >
> > > Let's add a flag and ioctls for big-endian devices as well. If both flags
> > > are set, little-endian wins.
> > >
> > > Since this is isn't a common usecase, the feature is controlled by a kernel
> > > config option (not set by default).
> > >
> > > Both macvtap and tun are covered by this patch since they share the same
> > > API with userland.
> > >
> > > Signed-off-by: Greg Kurz <gkurz-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > > ---
> > > drivers/net/Kconfig | 12 ++++++++
> > > drivers/net/macvtap.c | 60 +++++++++++++++++++++++++++++++++++++++++-
> > > drivers/net/tun.c | 62 ++++++++++++++++++++++++++++++++++++++++++-
> > > include/uapi/linux/if_tun.h | 2 +
> > > 4 files changed, 134 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > > index df51d60..f0e23a0 100644
> > > --- a/drivers/net/Kconfig
> > > +++ b/drivers/net/Kconfig
> > > @@ -244,6 +244,18 @@ config TUN
> > >
> > > If you don't know what to use this for, you don't need it.
> > >
> > > +config TUN_VNET_BE
> > > + bool "Support for big-endian vnet headers"
> > > + default n
> > > + ---help---
> > > + This option allows TUN/TAP and MACVTAP device drivers to parse
> > > + vnet headers that are in big-endian byte order. It is useful
> > > + when the headers come from a big-endian legacy virtio driver and
> > > + the host is little-endian.
> > > +
> > > + Unless you have a little-endian system hosting a big-endian virtual
> > > + machine with a virtio NIC, you should say N.
> > > +
> >
> > should mention cross-endian, not big-endian, right?
> >
>
> The current TUN_VNET_LE related code is already doing cross-endian: without
> this patch, one can already run a LE guest on a BE host... wouldn't it be
> confusing to mention cross-endian only when the guest is BE ?
Hmm I think no - LE is also useful for virtio 1 - this is what it was
intended for after all.
> What about having a completely distinct implementation for cross-endian that
> don't reuse the existing code and defines then ?
I think implementation and interface are fine, just the documentation
can be improved a bit.
How about:
"Support for cross-endian vnet headers on little-endian kernels".
Accordingly CONFIG_TUN_VNET_CROSS_LE
?
--
MST
^ permalink raw reply
* RE: [PATCH 1/2] added stream id write support
From: Hingkwan Huen @ 2015-04-21 19:03 UTC (permalink / raw)
To: 'Jeff Moyer'
Cc: 'Matthew Wilcox', 'Keith Busch',
'Jens Axboe', 'Dimitri John Ledkov',
linux-nvme-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <x49y4ll8uk3.fsf-RRHT56Q3PSP4kTEheFKJxxDDeQx5vsVwAInAS/Ez/D0@public.gmane.org>
> -----Original Message-----
> From: Jeff Moyer [mailto:jmoyer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> Sent: Tuesday, April 21, 2015 6:45 AM
> To: kwan.huen
> Cc: Matthew Wilcox; Keith Busch; Jens Axboe; Dimitri John Ledkov; linux-
> nvme-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 1/2] added stream id write support
>
> "kwan.huen" <kwan.huen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> writes:
>
> > ---
> > drivers/block/nvme-core.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> > index 85b8036..332341a 100644
> > --- a/drivers/block/nvme-core.c
> > +++ b/drivers/block/nvme-core.c
> > @@ -769,6 +769,9 @@ static int nvme_submit_iod(struct nvme_queue
> *nvmeq, struct nvme_iod *iod,
> > if (req->cmd_flags & REQ_RAHEAD)
> > dsmgmt |= NVME_RW_DSM_FREQ_PREFETCH;
> >
> > + if (rq_data_dir(req))
> > + dsmgmt |= bio_get_streamid(req->bio) << 8;
> > +
>
> There's no public specification for this, yet. How many bits are set
aside for
> the stream id? Do you need to do bounds checking/input validation? What
> happens on adapters with older firmware when these bits are set?
>
> Cheers,
> Jeff
Thanks Jeff for the review!
The patch depends on Jens' recent io-streamid patch set, and the new NVMe
spec we are trying to get approved.
The patch is probably still too early and we'll push this again when Jens'
patches are released and the new spec goes public.
Thanks,
kwan
^ permalink raw reply
* Re: [PATCH v4 7/8] vhost: feature to set the vring endianness
From: Greg Kurz @ 2015-04-22 9:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, Cornelia Huck, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, kvm-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <20150421201636-mutt-send-email-mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Tue, 21 Apr 2015 20:25:03 +0200
"Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
[ ... ]
> > > > @@ -630,6 +634,53 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> > > > return 0;
> > > > }
> > > >
> > > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > > > +static long vhost_set_vring_big_endian(struct vhost_virtqueue *vq,
> > > > + int __user *argp)
> > > > +{
> > > > + struct vhost_vring_state s;
> > > > +
> > > > + if (vq->private_data)
> > > > + return -EBUSY;
> > > > +
> > > > + if (copy_from_user(&s, argp, sizeof(s)))
> > > > + return -EFAULT;
> > > > +
> > > > + if (s.num && s.num != 1)
> > >
> > > s.num & ~0x1
> > >
> >
> > Since s.num is unsigned and I assume this won't change, what about
> > s.num > 1 as suggested by Cornelia ?
>
> I just tried and gcc optimizes
> s.num != 0 && s.num != 1 to s.num > 1
>
> The former will be more readable once we
> replace 0 and 1 with defines.
>
> So ignore my advice, keep code as is but use defines.
>
Ok.
[ ... ]
> > > > --- a/include/uapi/linux/vhost.h
> > > > +++ b/include/uapi/linux/vhost.h
> > > > @@ -103,6 +103,15 @@ 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 the vring byte order in num. This is a legacy only API that is simply
> > > > + * ignored when VIRTIO_F_VERSION_1 is set.
> > > > + * 0 to set to little-endian
> > > > + * 1 to set to big-endian
> > >
> > > How about defines for these?
> > >
> >
> > Ok. I'll put the defines here so that all the cross-endian stuff
> > lies in the same hunk. Is it ok for you ?
>
> Fine.
>
> > > > + * other values return EINVAL.
>
> Pls also add a note saying that not all kernel configurations support this ioctl,
> but all configurations that support SET also support GET.
>
Ok.
> > > > + */
> > > > +#define VHOST_SET_VRING_BIG_ENDIAN _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state)
> > > > +#define VHOST_GET_VRING_BIG_ENDIAN _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
> > > > +
> > > > /* The following ioctls use eventfd file descriptors to signal and poll
> > > > * for events. */
> > > >
> > >
>
> I'm inclined to think VHOST_SET_VRING_ENDIAN is a slightly better name.
> What do you think?
>
Or VHOST_SET_VRING_CROSS_ENDIAN ? I like the idea to keep a hint that this
API is for cross-endian only... like the rest of this series.
--
Greg
^ 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