* [RFC PATCH 18/68] vfs: Convert virtio_balloon to use the new mount API
From: David Howells @ 2019-03-27 23:42 UTC (permalink / raw)
To: viro
Cc: Michael S. Tsirkin, linux-kernel, virtualization, dhowells,
linux-fsdevel
In-Reply-To: <155372999953.7602.13784796495137723805.stgit@warthog.procyon.org.uk>
Convert the virtio_balloon filesystem to the new internal mount API as the old
one will be obsoleted and removed. This allows greater flexibility in
communication of mount parameters between userspace, the VFS and the
filesystem.
See Documentation/filesystems/mount_api.txt for more information.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: "Michael S. Tsirkin" <mst@redhat.com>
cc: Jason Wang <jasowang@redhat.com>
cc: virtualization@lists.linux-foundation.org
---
drivers/virtio/virtio_balloon.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f19061b585a4..89d67c8aa719 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -757,21 +757,22 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
return MIGRATEPAGE_SUCCESS;
}
+#include <linux/fs_context.h>
-static struct dentry *balloon_mount(struct file_system_type *fs_type,
- int flags, const char *dev_name, void *data)
-{
- static const struct dentry_operations ops = {
- .d_dname = simple_dname,
- };
+static const struct dentry_operations balloon_dops = {
+ .d_dname = simple_dname,
+};
- return mount_pseudo(fs_type, "balloon-kvm:", NULL, &ops,
- BALLOON_KVM_MAGIC);
+static int balloon_init_fs_context(struct fs_context *fc)
+{
+ return vfs_init_pseudo_fs_context(fc, "balloon-kvm:",
+ NULL, NULL,
+ &balloon_dops, BALLOON_KVM_MAGIC);
}
static struct file_system_type balloon_fs = {
.name = "balloon-kvm",
- .mount = balloon_mount,
+ .init_fs_context = balloon_init_fs_context,
.kill_sb = kill_anon_super,
};
^ permalink raw reply related
* Re: [PATCH v2 -next] drm/virtio: remove set but not used variable 'vgdev'
From: Gerd Hoffmann @ 2019-03-28 10:32 UTC (permalink / raw)
To: YueHaibing
Cc: David Airlie, kernel-janitors, linux-kernel, dri-devel,
virtualization, Daniel Vetter
In-Reply-To: <20190325092631.152060-1-yuehaibing@huawei.com>
On Mon, Mar 25, 2019 at 09:26:31AM +0000, YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/gpu/drm/virtio/virtgpu_ttm.c: In function 'virtio_gpu_init_mem_type':
> drivers/gpu/drm/virtio/virtgpu_ttm.c:117:28: warning:
> variable 'vgdev' set but not used [-Wunused-but-set-variable]
>
> drivers/gpu/drm/virtio/virtgpu_ttm.c: In function 'virtio_gpu_bo_swap_notify':
> drivers/gpu/drm/virtio/virtgpu_ttm.c:300:28: warning:
> variable 'vgdev' set but not used [-Wunused-but-set-variable]
>
> It is never used since introduction in dc5698e80cf7 ("Add virtio gpu driver.")
Patch queued up.
thanks,
Gerd
^ permalink raw reply
* [PATCH] MAINTAINERS: Update PARAVIRT_OPS_INTERFACE and VMWARE_HYPERVISOR_INTERFACE
From: Thomas Hellstrom via Virtualization @ 2019-03-28 12:06 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org
Cc: Juergen Gross, Thomas Hellstrom, Pv-drivers, x86@kernel.org,
virtualization@lists.linux-foundation.org, mingo@redhat.com,
hpa@zytor.com, tglx@linutronix.de, Alok Kataria
Alok Kataria will be handing over VMware's maintainership of these
interfaces to Thomas Hellström, with pv-drivers as backup contact.
Cc: Juergen Gross <jgross@suse.com>
Cc: virtualization@lists.linux-foundation.org
Cc: x86@kernel.org
Cc: tglx@linutronix.de
Cc: mingo@redhat.com
Cc: hpa@zytor.com
Cc: linux-kernel@vger.kernel.org
Cc: Alok Kataria <akataria@vmware.com>
Cc: <pv-drivers@vmware.com>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Acked-by: Alok Kataria <akataria@vmware.com>
---
MAINTAINERS | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 3e5a5d263f29..160b208b93d6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11711,7 +11711,8 @@ F: Documentation/parport*.txt
PARAVIRT_OPS INTERFACE
M: Juergen Gross <jgross@suse.com>
-M: Alok Kataria <akataria@vmware.com>
+M: Thomas Hellstrom <thellstrom@vmware.com>
+M: "VMware, Inc." <pv-drivers@vmware.com>
L: virtualization@lists.linux-foundation.org
S: Supported
F: Documentation/virtual/paravirt_ops.txt
@@ -16620,7 +16621,8 @@ S: Maintained
F: drivers/misc/vmw_balloon.c
VMWARE HYPERVISOR INTERFACE
-M: Alok Kataria <akataria@vmware.com>
+M: Thomas Hellstrom <thellstrom@vmware.com>
+M: "VMware, Inc." <pv-drivers@vmware.com>
L: virtualization@lists.linux-foundation.org
S: Supported
F: arch/x86/kernel/cpu/vmware.c
--
2.20.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCH] MAINTAINERS: Update PARAVIRT_OPS_INTERFACE and VMWARE_HYPERVISOR_INTERFACE
From: Juergen Gross @ 2019-03-28 14:03 UTC (permalink / raw)
To: Thomas Hellstrom, linux-kernel@vger.kernel.org
Cc: Pv-drivers, x86@kernel.org,
virtualization@lists.linux-foundation.org, mingo@redhat.com,
hpa@zytor.com, Alok Kataria, tglx@linutronix.de
In-Reply-To: <20190328120558.29897-1-thellstrom@vmware.com>
On 28/03/2019 13:06, Thomas Hellstrom wrote:
> Alok Kataria will be handing over VMware's maintainership of these
> interfaces to Thomas Hellström, with pv-drivers as backup contact.
>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: x86@kernel.org
> Cc: tglx@linutronix.de
> Cc: mingo@redhat.com
> Cc: hpa@zytor.com
> Cc: linux-kernel@vger.kernel.org
> Cc: Alok Kataria <akataria@vmware.com>
> Cc: <pv-drivers@vmware.com>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> Acked-by: Alok Kataria <akataria@vmware.com>
Acked-by: Juergen Gross <jgross@suse.com>
Juergen
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net v3] failover: allow name change on IFF_UP slave interfaces
From: Stephen Hemminger @ 2019-03-28 17:14 UTC (permalink / raw)
To: si-wei liu
Cc: Jiri Pirko, mst, kubakici, sridhar.samudrala, alexander.duyck,
virtualization, liran.alon, netdev, boris.ostrovsky, davem
In-Reply-To: <06fa1aec-d9a6-3ca7-8849-a1656349ab83@oracle.com>
On Wed, 27 Mar 2019 16:44:19 -0700
si-wei liu <si-wei.liu@oracle.com> wrote:
> On 3/27/2019 4:11 AM, Jiri Pirko wrote:
> > Wed, Mar 27, 2019 at 12:48:13AM CET, si-wei.liu@oracle.com wrote:
> >> When a netdev appears through hot plug then gets enslaved by a failover
> >> master that is already up and running, the slave will be opened
> >> right away after getting enslaved. Today there's a race that userspace
> >> (udev) may fail to rename the slave if the kernel (net_failover)
> >> opens the slave earlier than when the userspace rename happens.
> >> Unlike bond or team, the primary slave of failover can't be renamed by
> >> userspace ahead of time, since the kernel initiated auto-enslavement is
> >> unable to, or rather, is never meant to be synchronized with the rename
> >> request from userspace.
> >>
> >> As the failover slave interfaces are not designed to be operated
> >> directly by userspace apps: IP configuration, filter rules with
> >> regard to network traffic passing and etc., should all be done on master
> >> interface. In general, userspace apps only care about the
> >> name of master interface, while slave names are less important as long
> >> as admin users can see reliable names that may carry
> >> other information describing the netdev. For e.g., they can infer that
> >> "ens3nsby" is a standby slave of "ens3", while for a
> >> name like "eth0" they can't tell which master it belongs to.
> >>
> >> Historically the name of IFF_UP interface can't be changed because
> >> there might be admin script or management software that is already
> >> relying on such behavior and assumes that the slave name can't be
> >> changed once UP. But failover is special: with the in-kernel
> >> auto-enslavement mechanism, the userspace expectation for device
> >> enumeration and bring-up order is already broken. Previously initramfs
> >> and various userspace config tools were modified to bypass failover
> >> slaves because of auto-enslavement and duplicate MAC address. Similarly,
> >> in case that users care about seeing reliable slave name, the new type
> >> of failover slaves needs to be taken care of specifically in userspace
> >> anyway.
> >>
> >> It's less risky to lift up the rename restriction on failover slave
> >> which is already UP. Although it's possible this change may potentially
> >> break userspace component (most likely configuration scripts or
> >> management software) that assumes slave name can't be changed while
> >> UP, it's relatively a limited and controllable set among all userspace
> >> components, which can be fixed specifically to listen for the rename
> >> and/or link down/up events on failover slaves. Userspace component
> >> interacting with slaves is expected to be changed to operate on failover
> >> master interface instead, as the failover slave is dynamic in nature
> >> which may come and go at any point. The goal is to make the role of
> >> failover slaves less relevant, and userspace components should only
> >> deal with failover master in the long run.
> >>
> >> Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
> >> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> >> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> >>
> >> --
> >> v1 -> v2:
> >> - Drop configurable module parameter (Sridhar)
> >>
> >> v2 -> v3:
> >> - Drop additional IFF_SLAVE_RENAME_OK flag (Sridhar)
> >> - Send down and up events around rename (Michael S. Tsirkin)
> >> ---
> >> net/core/dev.c | 37 ++++++++++++++++++++++++++++++++++---
> >> 1 file changed, 34 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index 722d50d..3e0cd80 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -1171,6 +1171,7 @@ int dev_get_valid_name(struct net *net, struct net_device *dev,
> >> int dev_change_name(struct net_device *dev, const char *newname)
> >> {
> >> unsigned char old_assign_type;
> >> + bool reopen_needed = false;
> >> char oldname[IFNAMSIZ];
> >> int err = 0;
> >> int ret;
> >> @@ -1180,8 +1181,24 @@ int dev_change_name(struct net_device *dev, const char *newname)
> >> BUG_ON(!dev_net(dev));
> >>
> >> net = dev_net(dev);
> >> - if (dev->flags & IFF_UP)
> >> - return -EBUSY;
> >> +
> >> + /* Allow failover slave to rename even when
> >> + * it is up and running.
> >> + *
> >> + * Failover slaves are special, since userspace
> >> + * might rename the slave after the interface
> >> + * has been brought up and running due to
> >> + * auto-enslavement.
> >> + *
> >> + * Failover users don't actually care about slave
> >> + * name change, as they are only expected to operate
> >> + * on master interface directly.
> >> + */
> >> + if (dev->flags & IFF_UP) {
> >> + if (likely(!(dev->priv_flags & IFF_FAILOVER_SLAVE)))
> >> + return -EBUSY;
> >> + reopen_needed = true;
> >> + }
> >>
> >> write_seqcount_begin(&devnet_rename_seq);
> >>
> >> @@ -1198,6 +1215,9 @@ int dev_change_name(struct net_device *dev, const char *newname)
> >> return err;
> >> }
> >>
> >> + if (reopen_needed)
> >> + dev_close(dev);
> > Ugh. Don't dev_close/dev_open on name change.
> See my response to Michael and Stephen. What's your suggestion then?
To a DEV_CHANGE notification instead?
My opinion is that allowing name change is not worth the doing.
Also, the kernel should never do the name change, it is up to userspace.
^ permalink raw reply
* Re: [PATCH net v3] failover: allow name change on IFF_UP slave interfaces
From: kbuild test robot @ 2019-03-28 19:49 UTC (permalink / raw)
Cc: jiri, mst, kubakici, sridhar.samudrala, alexander.duyck,
virtualization, liran.alon, kbuild-all, netdev, si-wei liu,
boris.ostrovsky, davem
In-Reply-To: <1553644093-10917-1-git-send-email-si-wei.liu@oracle.com>
[-- Attachment #1: Type: text/plain, Size: 4836 bytes --]
Hi Si-Wei,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
url: https://github.com/0day-ci/linux/commits/Si-Wei-Liu/failover-allow-name-change-on-IFF_UP-slave-interfaces/20190329-020744
config: openrisc-or1ksim_defconfig (attached as .config)
compiler: or1k-linux-gcc (GCC) 6.0.0 20160327 (experimental)
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=openrisc
All errors (new ones prefixed by >>):
net/core/dev.c: In function 'dev_change_name':
>> net/core/dev.c:1277:9: error: too few arguments to function 'dev_open'
ret = dev_open(dev);
^~~~~~~~
In file included from net/core/dev.c:93:0:
include/linux/netdevice.h:2636:5: note: declared here
int dev_open(struct net_device *dev, struct netlink_ext_ack *extack);
^~~~~~~~
vim +/dev_open +1277 net/core/dev.c
1166
1167 /**
1168 * dev_change_name - change name of a device
1169 * @dev: device
1170 * @newname: name (or format string) must be at least IFNAMSIZ
1171 *
1172 * Change name of a device, can pass format strings "eth%d".
1173 * for wildcarding.
1174 */
1175 int dev_change_name(struct net_device *dev, const char *newname)
1176 {
1177 unsigned char old_assign_type;
1178 bool reopen_needed = false;
1179 char oldname[IFNAMSIZ];
1180 int err = 0;
1181 int ret;
1182 struct net *net;
1183
1184 ASSERT_RTNL();
1185 BUG_ON(!dev_net(dev));
1186
1187 net = dev_net(dev);
1188
1189 /* Allow failover slave to rename even when
1190 * it is up and running.
1191 *
1192 * Failover slaves are special, since userspace
1193 * might rename the slave after the interface
1194 * has been brought up and running due to
1195 * auto-enslavement.
1196 *
1197 * Failover users don't actually care about slave
1198 * name change, as they are only expected to operate
1199 * on master interface directly.
1200 */
1201 if (dev->flags & IFF_UP) {
1202 if (likely(!(dev->priv_flags & IFF_FAILOVER_SLAVE)))
1203 return -EBUSY;
1204 reopen_needed = true;
1205 }
1206
1207 write_seqcount_begin(&devnet_rename_seq);
1208
1209 if (strncmp(newname, dev->name, IFNAMSIZ) == 0) {
1210 write_seqcount_end(&devnet_rename_seq);
1211 return 0;
1212 }
1213
1214 memcpy(oldname, dev->name, IFNAMSIZ);
1215
1216 err = dev_get_valid_name(net, dev, newname);
1217 if (err < 0) {
1218 write_seqcount_end(&devnet_rename_seq);
1219 return err;
1220 }
1221
1222 if (reopen_needed)
1223 dev_close(dev);
1224
1225 if (oldname[0] && !strchr(oldname, '%'))
1226 netdev_info(dev, "renamed from %s\n", oldname);
1227
1228 old_assign_type = dev->name_assign_type;
1229 dev->name_assign_type = NET_NAME_RENAMED;
1230
1231 rollback:
1232 ret = device_rename(&dev->dev, dev->name);
1233 if (ret) {
1234 memcpy(dev->name, oldname, IFNAMSIZ);
1235 dev->name_assign_type = old_assign_type;
1236 write_seqcount_end(&devnet_rename_seq);
1237 if (err >= 0)
1238 err = ret;
1239 goto reopen;
1240 }
1241
1242 write_seqcount_end(&devnet_rename_seq);
1243
1244 netdev_adjacent_rename_links(dev, oldname);
1245
1246 write_lock_bh(&dev_base_lock);
1247 hlist_del_rcu(&dev->name_hlist);
1248 write_unlock_bh(&dev_base_lock);
1249
1250 synchronize_rcu();
1251
1252 write_lock_bh(&dev_base_lock);
1253 hlist_add_head_rcu(&dev->name_hlist, dev_name_hash(net, dev->name));
1254 write_unlock_bh(&dev_base_lock);
1255
1256 ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
1257 ret = notifier_to_errno(ret);
1258
1259 if (ret) {
1260 /* err >= 0 after dev_alloc_name() or stores the first errno */
1261 if (err >= 0) {
1262 err = ret;
1263 write_seqcount_begin(&devnet_rename_seq);
1264 memcpy(dev->name, oldname, IFNAMSIZ);
1265 memcpy(oldname, newname, IFNAMSIZ);
1266 dev->name_assign_type = old_assign_type;
1267 old_assign_type = NET_NAME_RENAMED;
1268 goto rollback;
1269 } else {
1270 pr_err("%s: name change rollback failed: %d\n",
1271 dev->name, ret);
1272 }
1273 }
1274
1275 reopen:
1276 if (reopen_needed) {
> 1277 ret = dev_open(dev);
1278 if (ret) {
1279 pr_err("%s: reopen device failed: %d\n",
1280 dev->name, ret);
1281 }
1282 }
1283
1284 return err;
1285 }
1286
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 8104 bytes --]
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net v4] failover: allow name change on IFF_UP slave interfaces
From: Samudrala, Sridhar @ 2019-03-29 5:55 UTC (permalink / raw)
To: Si-Wei Liu, mst, stephen, davem, kubakici, alexander.duyck, jiri,
netdev, virtualization
Cc: boris.ostrovsky, liran.alon
In-Reply-To: <1553816847-28121-1-git-send-email-si-wei.liu@oracle.com>
On 3/28/2019 4:47 PM, Si-Wei Liu wrote:
> When a netdev appears through hot plug then gets enslaved by a failover
> master that is already up and running, the slave will be opened
> right away after getting enslaved. Today there's a race that userspace
> (udev) may fail to rename the slave if the kernel (net_failover)
> opens the slave earlier than when the userspace rename happens.
> Unlike bond or team, the primary slave of failover can't be renamed by
> userspace ahead of time, since the kernel initiated auto-enslavement is
> unable to, or rather, is never meant to be synchronized with the rename
> request from userspace.
>
> As the failover slave interfaces are not designed to be operated
> directly by userspace apps: IP configuration, filter rules with
> regard to network traffic passing and etc., should all be done on master
> interface. In general, userspace apps only care about the
> name of master interface, while slave names are less important as long
> as admin users can see reliable names that may carry
> other information describing the netdev. For e.g., they can infer that
> "ens3nsby" is a standby slave of "ens3", while for a
> name like "eth0" they can't tell which master it belongs to.
>
> Historically the name of IFF_UP interface can't be changed because
> there might be admin script or management software that is already
> relying on such behavior and assumes that the slave name can't be
> changed once UP. But failover is special: with the in-kernel
> auto-enslavement mechanism, the userspace expectation for device
> enumeration and bring-up order is already broken. Previously initramfs
> and various userspace config tools were modified to bypass failover
> slaves because of auto-enslavement and duplicate MAC address. Similarly,
> in case that users care about seeing reliable slave name, the new type
> of failover slaves needs to be taken care of specifically in userspace
> anyway.
>
> It's less risky to lift up the rename restriction on failover slave
> which is already UP. Although it's possible this change may potentially
> break userspace component (most likely configuration scripts or
> management software) that assumes slave name can't be changed while
> UP, it's relatively a limited and controllable set among all userspace
> components, which can be fixed specifically to listen for the rename
> and/or link down/up events on failover slaves. Userspace component
> interacting with slaves is expected to be changed to operate on failover
> master interface instead, as the failover slave is dynamic in nature
> which may come and go at any point. The goal is to make the role of
> failover slaves less relevant, and userspace components should only
> deal with failover master in the long run.
>
> Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
>
> --
> v1 -> v2:
> - Drop configurable module parameter (Sridhar)
>
> v2 -> v3:
> - Drop additional IFF_SLAVE_RENAME_OK flag (Sridhar)
> - Send down and up events around rename (Michael S. Tsirkin)
>
> v3 -> v4:
> - Simplify notification to be sent (Stephen Hemminger)
> ---
> net/core/dev.c | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 722d50d..6ae5874 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1180,7 +1180,21 @@ int dev_change_name(struct net_device *dev, const char *newname)
> BUG_ON(!dev_net(dev));
>
> net = dev_net(dev);
> - if (dev->flags & IFF_UP)
> +
> + /* Allow failover slave to rename even when
> + * it is up and running.
> + *
> + * Failover slaves are special, since userspace
> + * might rename the slave after the interface
> + * has been brought up and running due to
> + * auto-enslavement.
> + *
> + * Failover users don't actually care about slave
> + * name change, as they are only expected to operate
> + * on master interface directly.
> + */
> + if (dev->flags & IFF_UP &&
> + likely(!(dev->priv_flags & IFF_FAILOVER_SLAVE)))
> return -EBUSY;
>
> write_seqcount_begin(&devnet_rename_seq);
> @@ -1227,6 +1241,14 @@ int dev_change_name(struct net_device *dev, const char *newname)
> hlist_add_head_rcu(&dev->name_hlist, dev_name_hash(net, dev->name));
> write_unlock_bh(&dev_base_lock);
>
> + if (unlikely(dev->flags & IFF_UP)) {
> + struct netdev_notifier_change_info change_info;
> +
> + change_info.flags_changed = 0;
> + call_netdevice_notifiers_info(NETDEV_CHANGE, dev,
> + &change_info.info);
This function no longer takes the dev parameter in the net-next kernel.
Did you consider calling netdev_state_change() although it does send a
RTM_NEWLINK message too. May be just fixing the notifier call should be
fine.
> + }
> +
> ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
> ret = notifier_to_errno(ret);
>
>
^ permalink raw reply
* Re: [PATCH net v4] failover: allow name change on IFF_UP slave interfaces
From: kbuild test robot @ 2019-03-29 13:45 UTC (permalink / raw)
Cc: jiri, mst, kubakici, sridhar.samudrala, alexander.duyck,
virtualization, liran.alon, kbuild-all, netdev, si-wei liu,
boris.ostrovsky, davem
In-Reply-To: <1553816847-28121-1-git-send-email-si-wei.liu@oracle.com>
[-- Attachment #1: Type: text/plain, Size: 5112 bytes --]
Hi Si-Wei,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
url: https://github.com/0day-ci/linux/commits/Si-Wei-Liu/failover-allow-name-change-on-IFF_UP-slave-interfaces/20190329-195445
config: x86_64-lkp (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
net/core/dev.c: In function 'dev_change_name':
>> net/core/dev.c:1252:48: error: passing argument 2 of 'call_netdevice_notifiers_info' from incompatible pointer type [-Werror=incompatible-pointer-types]
call_netdevice_notifiers_info(NETDEV_CHANGE, dev,
^~~
net/core/dev.c:164:12: note: expected 'struct netdev_notifier_info *' but argument is of type 'struct net_device *'
static int call_netdevice_notifiers_info(unsigned long val,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> net/core/dev.c:1252:3: error: too many arguments to function 'call_netdevice_notifiers_info'
call_netdevice_notifiers_info(NETDEV_CHANGE, dev,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/core/dev.c:164:12: note: declared here
static int call_netdevice_notifiers_info(unsigned long val,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/call_netdevice_notifiers_info +1252 net/core/dev.c
1166
1167 /**
1168 * dev_change_name - change name of a device
1169 * @dev: device
1170 * @newname: name (or format string) must be at least IFNAMSIZ
1171 *
1172 * Change name of a device, can pass format strings "eth%d".
1173 * for wildcarding.
1174 */
1175 int dev_change_name(struct net_device *dev, const char *newname)
1176 {
1177 unsigned char old_assign_type;
1178 char oldname[IFNAMSIZ];
1179 int err = 0;
1180 int ret;
1181 struct net *net;
1182
1183 ASSERT_RTNL();
1184 BUG_ON(!dev_net(dev));
1185
1186 net = dev_net(dev);
1187
1188 /* Allow failover slave to rename even when
1189 * it is up and running.
1190 *
1191 * Failover slaves are special, since userspace
1192 * might rename the slave after the interface
1193 * has been brought up and running due to
1194 * auto-enslavement.
1195 *
1196 * Failover users don't actually care about slave
1197 * name change, as they are only expected to operate
1198 * on master interface directly.
1199 */
1200 if (dev->flags & IFF_UP &&
1201 likely(!(dev->priv_flags & IFF_FAILOVER_SLAVE)))
1202 return -EBUSY;
1203
1204 write_seqcount_begin(&devnet_rename_seq);
1205
1206 if (strncmp(newname, dev->name, IFNAMSIZ) == 0) {
1207 write_seqcount_end(&devnet_rename_seq);
1208 return 0;
1209 }
1210
1211 memcpy(oldname, dev->name, IFNAMSIZ);
1212
1213 err = dev_get_valid_name(net, dev, newname);
1214 if (err < 0) {
1215 write_seqcount_end(&devnet_rename_seq);
1216 return err;
1217 }
1218
1219 if (oldname[0] && !strchr(oldname, '%'))
1220 netdev_info(dev, "renamed from %s\n", oldname);
1221
1222 old_assign_type = dev->name_assign_type;
1223 dev->name_assign_type = NET_NAME_RENAMED;
1224
1225 rollback:
1226 ret = device_rename(&dev->dev, dev->name);
1227 if (ret) {
1228 memcpy(dev->name, oldname, IFNAMSIZ);
1229 dev->name_assign_type = old_assign_type;
1230 write_seqcount_end(&devnet_rename_seq);
1231 return ret;
1232 }
1233
1234 write_seqcount_end(&devnet_rename_seq);
1235
1236 netdev_adjacent_rename_links(dev, oldname);
1237
1238 write_lock_bh(&dev_base_lock);
1239 hlist_del_rcu(&dev->name_hlist);
1240 write_unlock_bh(&dev_base_lock);
1241
1242 synchronize_rcu();
1243
1244 write_lock_bh(&dev_base_lock);
1245 hlist_add_head_rcu(&dev->name_hlist, dev_name_hash(net, dev->name));
1246 write_unlock_bh(&dev_base_lock);
1247
1248 if (unlikely(dev->flags & IFF_UP)) {
1249 struct netdev_notifier_change_info change_info;
1250
1251 change_info.flags_changed = 0;
> 1252 call_netdevice_notifiers_info(NETDEV_CHANGE, dev,
1253 &change_info.info);
1254 }
1255
1256 ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
1257 ret = notifier_to_errno(ret);
1258
1259 if (ret) {
1260 /* err >= 0 after dev_alloc_name() or stores the first errno */
1261 if (err >= 0) {
1262 err = ret;
1263 write_seqcount_begin(&devnet_rename_seq);
1264 memcpy(dev->name, oldname, IFNAMSIZ);
1265 memcpy(oldname, newname, IFNAMSIZ);
1266 dev->name_assign_type = old_assign_type;
1267 old_assign_type = NET_NAME_RENAMED;
1268 goto rollback;
1269 } else {
1270 pr_err("%s: name change rollback failed: %d\n",
1271 dev->name, ret);
1272 }
1273 }
1274
1275 return err;
1276 }
1277
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26574 bytes --]
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net v4] failover: allow name change on IFF_UP slave interfaces
From: Stephen Hemminger @ 2019-03-29 15:15 UTC (permalink / raw)
To: Si-Wei Liu
Cc: jiri, mst, kubakici, sridhar.samudrala, alexander.duyck,
virtualization, liran.alon, netdev, boris.ostrovsky, davem
In-Reply-To: <1553816847-28121-1-git-send-email-si-wei.liu@oracle.com>
On Thu, 28 Mar 2019 19:47:27 -0400
Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> + if (unlikely(dev->flags & IFF_UP)) {
> + struct netdev_notifier_change_info change_info;
> +
> + change_info.flags_changed = 0;
Simpler to use structure initialization, which also avoid any chance
of unititialized fields.
struct netdev_notifier_change_info change_info
= { .flags_changed = 0 };
^ permalink raw reply
* Re: [PATCH net v4] failover: allow name change on IFF_UP slave interfaces
From: Jiri Pirko @ 2019-03-29 15:34 UTC (permalink / raw)
To: Stephen Hemminger
Cc: mst, kubakici, sridhar.samudrala, alexander.duyck, virtualization,
liran.alon, netdev, Si-Wei Liu, boris.ostrovsky, davem
In-Reply-To: <20190329081512.2f231508@shemminger-XPS-13-9360>
Fri, Mar 29, 2019 at 04:15:12PM CET, stephen@networkplumber.org wrote:
>On Thu, 28 Mar 2019 19:47:27 -0400
>Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>> + if (unlikely(dev->flags & IFF_UP)) {
>> + struct netdev_notifier_change_info change_info;
>> +
>> + change_info.flags_changed = 0;
>
>Simpler to use structure initialization, which also avoid any chance
>of unititialized fields.
>
> struct netdev_notifier_change_info change_info
> = { .flags_changed = 0 };
In fact, you can do just:
struct netdev_notifier_change_info change_info = {};
to achieve the same.
^ permalink raw reply
* Re: [PATCH net v4] failover: allow name change on IFF_UP slave interfaces
From: Michael S. Tsirkin @ 2019-03-29 22:31 UTC (permalink / raw)
To: si-wei liu
Cc: jiri, kubakici, Samudrala, Sridhar, alexander.duyck,
virtualization, liran.alon, netdev, boris.ostrovsky, davem
In-Reply-To: <bb2239d2-9900-07b3-f969-c188899eb4e6@oracle.com>
On Fri, Mar 29, 2019 at 12:50:25PM -0700, si-wei liu wrote:
>
>
> On 3/28/2019 10:55 PM, Samudrala, Sridhar wrote:
> >
> >
> > On 3/28/2019 4:47 PM, Si-Wei Liu wrote:
> > > When a netdev appears through hot plug then gets enslaved by a failover
> > > master that is already up and running, the slave will be opened
> > > right away after getting enslaved. Today there's a race that userspace
> > > (udev) may fail to rename the slave if the kernel (net_failover)
> > > opens the slave earlier than when the userspace rename happens.
> > > Unlike bond or team, the primary slave of failover can't be renamed by
> > > userspace ahead of time, since the kernel initiated auto-enslavement is
> > > unable to, or rather, is never meant to be synchronized with the rename
> > > request from userspace.
> > >
> > > As the failover slave interfaces are not designed to be operated
> > > directly by userspace apps: IP configuration, filter rules with
> > > regard to network traffic passing and etc., should all be done on master
> > > interface. In general, userspace apps only care about the
> > > name of master interface, while slave names are less important as long
> > > as admin users can see reliable names that may carry
> > > other information describing the netdev. For e.g., they can infer that
> > > "ens3nsby" is a standby slave of "ens3", while for a
> > > name like "eth0" they can't tell which master it belongs to.
> > >
> > > Historically the name of IFF_UP interface can't be changed because
> > > there might be admin script or management software that is already
> > > relying on such behavior and assumes that the slave name can't be
> > > changed once UP. But failover is special: with the in-kernel
> > > auto-enslavement mechanism, the userspace expectation for device
> > > enumeration and bring-up order is already broken. Previously initramfs
> > > and various userspace config tools were modified to bypass failover
> > > slaves because of auto-enslavement and duplicate MAC address. Similarly,
> > > in case that users care about seeing reliable slave name, the new type
> > > of failover slaves needs to be taken care of specifically in userspace
> > > anyway.
> > >
> > > It's less risky to lift up the rename restriction on failover slave
> > > which is already UP. Although it's possible this change may potentially
> > > break userspace component (most likely configuration scripts or
> > > management software) that assumes slave name can't be changed while
> > > UP, it's relatively a limited and controllable set among all userspace
> > > components, which can be fixed specifically to listen for the rename
> > > and/or link down/up events on failover slaves. Userspace component
> > > interacting with slaves is expected to be changed to operate on failover
> > > master interface instead, as the failover slave is dynamic in nature
> > > which may come and go at any point. The goal is to make the role of
> > > failover slaves less relevant, and userspace components should only
> > > deal with failover master in the long run.
> > >
> > > Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
> > > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> > > Reviewed-by: Liran Alon <liran.alon@oracle.com>
> > >
> > > --
> > > v1 -> v2:
> > > - Drop configurable module parameter (Sridhar)
> > >
> > > v2 -> v3:
> > > - Drop additional IFF_SLAVE_RENAME_OK flag (Sridhar)
> > > - Send down and up events around rename (Michael S. Tsirkin)
> > >
> > > v3 -> v4:
> > > - Simplify notification to be sent (Stephen Hemminger)
> > > ---
> > > net/core/dev.c | 24 +++++++++++++++++++++++-
> > > 1 file changed, 23 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 722d50d..6ae5874 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -1180,7 +1180,21 @@ int dev_change_name(struct net_device *dev,
> > > const char *newname)
> > > BUG_ON(!dev_net(dev));
> > > net = dev_net(dev);
> > > - if (dev->flags & IFF_UP)
> > > +
> > > + /* Allow failover slave to rename even when
> > > + * it is up and running.
> > > + *
> > > + * Failover slaves are special, since userspace
> > > + * might rename the slave after the interface
> > > + * has been brought up and running due to
> > > + * auto-enslavement.
> > > + *
> > > + * Failover users don't actually care about slave
> > > + * name change, as they are only expected to operate
> > > + * on master interface directly.
> > > + */
> > > + if (dev->flags & IFF_UP &&
> > > + likely(!(dev->priv_flags & IFF_FAILOVER_SLAVE)))
> > > return -EBUSY;
> > > write_seqcount_begin(&devnet_rename_seq);
> > > @@ -1227,6 +1241,14 @@ int dev_change_name(struct net_device *dev,
> > > const char *newname)
> > > hlist_add_head_rcu(&dev->name_hlist, dev_name_hash(net,
> > > dev->name));
> > > write_unlock_bh(&dev_base_lock);
> > > + if (unlikely(dev->flags & IFF_UP)) {
> > > + struct netdev_notifier_change_info change_info;
> > > +
> > > + change_info.flags_changed = 0;
> > > + call_netdevice_notifiers_info(NETDEV_CHANGE, dev,
> > > + &change_info.info);
> >
> > This function no longer takes the dev parameter in the net-next kernel.
> OK. Will get my patch updated with the latest net-next tree.
>
> >
> > Did you consider calling netdev_state_change() although it does send a
> > RTM_NEWLINK message too. May be just fixing the notifier call should be
> > fine.
> I did look at it and the extra RTM_NEWLINK message was indeed the reason I
> got it rewritten. You see, how the way of initialization is inherited.
>
> Thanks,
> -Siwei
More messages just increase the chance existing scripts will work.
Don't see what the harm is.
> >
> > > + }
> > > +
> > > ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
> > > ret = notifier_to_errno(ret);
> > >
^ permalink raw reply
* [PATCH 2/5] x86: Convert some slow-path static_cpu_has() callers to boot_cpu_has()
From: Borislav Petkov @ 2019-03-30 11:20 UTC (permalink / raw)
To: LKML
Cc: x86, Peter Zijlstra, Sebastian Andrzej Siewior, Dominik Brodowski,
virtualization, Dave Hansen, H. Peter Anvin, Nadav Amit,
Rafael J. Wysocki, Ingo Molnar, Rik van Riel, Thomas Lendacky,
Joerg Roedel, Jann Horn, Aubrey Li, Thomas Gleixner, linux-edac,
Juergen Gross, Tony Luck, Konrad Rzeszutek Wilk, Andy Lutomirski,
Masami Hiramatsu, Kirill A. Shutemov
In-Reply-To: <20190330112022.28888-1-bp@alien8.de>
From: Borislav Petkov <bp@suse.de>
Using static_cpu_has() is pointless on those paths, convert them to the
boot_cpu_has() variant.
No functional changes.
Reported-by: Nadav Amit <nadav.amit@gmail.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Aubrey Li <aubrey.li@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Juergen Gross <jgross@suse.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Thomas Lendacky <Thomas.Lendacky@amd.com>
Cc: linux-edac@vger.kernel.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Rik van Riel <riel@surriel.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: virtualization@lists.linux-foundation.org
Cc: x86@kernel.org
---
arch/x86/include/asm/fpu/internal.h | 7 +++----
arch/x86/kernel/apic/apic_numachip.c | 2 +-
arch/x86/kernel/cpu/aperfmperf.c | 6 +++---
arch/x86/kernel/cpu/common.c | 2 +-
arch/x86/kernel/cpu/mce/inject.c | 2 +-
arch/x86/kernel/cpu/proc.c | 10 +++++-----
arch/x86/kernel/ldt.c | 14 +++++++-------
arch/x86/kernel/paravirt.c | 2 +-
arch/x86/kernel/process.c | 4 ++--
arch/x86/kernel/reboot.c | 2 +-
arch/x86/kernel/vm86_32.c | 2 +-
11 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index fb04a3ded7dd..745a19d34f23 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -253,7 +253,7 @@ static inline void copy_xregs_to_kernel_booting(struct xregs_state *xstate)
WARN_ON(system_state != SYSTEM_BOOTING);
- if (static_cpu_has(X86_FEATURE_XSAVES))
+ if (boot_cpu_has(X86_FEATURE_XSAVES))
XSTATE_OP(XSAVES, xstate, lmask, hmask, err);
else
XSTATE_OP(XSAVE, xstate, lmask, hmask, err);
@@ -275,7 +275,7 @@ static inline void copy_kernel_to_xregs_booting(struct xregs_state *xstate)
WARN_ON(system_state != SYSTEM_BOOTING);
- if (static_cpu_has(X86_FEATURE_XSAVES))
+ if (boot_cpu_has(X86_FEATURE_XSAVES))
XSTATE_OP(XRSTORS, xstate, lmask, hmask, err);
else
XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
@@ -497,8 +497,7 @@ static inline void fpregs_activate(struct fpu *fpu)
* - switch_fpu_finish() restores the new state as
* necessary.
*/
-static inline void
-switch_fpu_prepare(struct fpu *old_fpu, int cpu)
+static inline void switch_fpu_prepare(struct fpu *old_fpu, int cpu)
{
if (static_cpu_has(X86_FEATURE_FPU) && old_fpu->initialized) {
if (!copy_fpregs_to_fpstate(old_fpu))
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index 78778b54f904..a5464b8b6c46 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -175,7 +175,7 @@ static void fixup_cpu_id(struct cpuinfo_x86 *c, int node)
this_cpu_write(cpu_llc_id, node);
/* Account for nodes per socket in multi-core-module processors */
- if (static_cpu_has(X86_FEATURE_NODEID_MSR)) {
+ if (boot_cpu_has(X86_FEATURE_NODEID_MSR)) {
rdmsrl(MSR_FAM10H_NODE_ID, val);
nodes = ((val >> 3) & 7) + 1;
}
diff --git a/arch/x86/kernel/cpu/aperfmperf.c b/arch/x86/kernel/cpu/aperfmperf.c
index 804c49493938..64d5aec24203 100644
--- a/arch/x86/kernel/cpu/aperfmperf.c
+++ b/arch/x86/kernel/cpu/aperfmperf.c
@@ -83,7 +83,7 @@ unsigned int aperfmperf_get_khz(int cpu)
if (!cpu_khz)
return 0;
- if (!static_cpu_has(X86_FEATURE_APERFMPERF))
+ if (!boot_cpu_has(X86_FEATURE_APERFMPERF))
return 0;
aperfmperf_snapshot_cpu(cpu, ktime_get(), true);
@@ -99,7 +99,7 @@ void arch_freq_prepare_all(void)
if (!cpu_khz)
return;
- if (!static_cpu_has(X86_FEATURE_APERFMPERF))
+ if (!boot_cpu_has(X86_FEATURE_APERFMPERF))
return;
for_each_online_cpu(cpu)
@@ -115,7 +115,7 @@ unsigned int arch_freq_get_on_cpu(int cpu)
if (!cpu_khz)
return 0;
- if (!static_cpu_has(X86_FEATURE_APERFMPERF))
+ if (!boot_cpu_has(X86_FEATURE_APERFMPERF))
return 0;
if (aperfmperf_snapshot_cpu(cpu, ktime_get(), true))
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index cb28e98a0659..95a5faf3a6a0 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1668,7 +1668,7 @@ static void setup_getcpu(int cpu)
unsigned long cpudata = vdso_encode_cpunode(cpu, early_cpu_to_node(cpu));
struct desc_struct d = { };
- if (static_cpu_has(X86_FEATURE_RDTSCP))
+ if (boot_cpu_has(X86_FEATURE_RDTSCP))
write_rdtscp_aux(cpudata);
/* Store CPU and node number in limit. */
diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 3f82afd0f46f..a6026170af92 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -526,7 +526,7 @@ static void do_inject(void)
* only on the node base core. Refer to D18F3x44[NbMcaToMstCpuEn] for
* Fam10h and later BKDGs.
*/
- if (static_cpu_has(X86_FEATURE_AMD_DCM) &&
+ if (boot_cpu_has(X86_FEATURE_AMD_DCM) &&
b == 4 &&
boot_cpu_data.x86 < 0x17) {
toggle_nb_mca_mst_cpu(amd_get_nb_id(cpu));
diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index 2c8522a39ed5..cb2e49810d68 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -35,11 +35,11 @@ static void show_cpuinfo_misc(struct seq_file *m, struct cpuinfo_x86 *c)
"fpu_exception\t: %s\n"
"cpuid level\t: %d\n"
"wp\t\t: yes\n",
- static_cpu_has_bug(X86_BUG_FDIV) ? "yes" : "no",
- static_cpu_has_bug(X86_BUG_F00F) ? "yes" : "no",
- static_cpu_has_bug(X86_BUG_COMA) ? "yes" : "no",
- static_cpu_has(X86_FEATURE_FPU) ? "yes" : "no",
- static_cpu_has(X86_FEATURE_FPU) ? "yes" : "no",
+ boot_cpu_has_bug(X86_BUG_FDIV) ? "yes" : "no",
+ boot_cpu_has_bug(X86_BUG_F00F) ? "yes" : "no",
+ boot_cpu_has_bug(X86_BUG_COMA) ? "yes" : "no",
+ boot_cpu_has(X86_FEATURE_FPU) ? "yes" : "no",
+ boot_cpu_has(X86_FEATURE_FPU) ? "yes" : "no",
c->cpuid_level);
}
#else
diff --git a/arch/x86/kernel/ldt.c b/arch/x86/kernel/ldt.c
index 6135ae8ce036..b2463fcb20a8 100644
--- a/arch/x86/kernel/ldt.c
+++ b/arch/x86/kernel/ldt.c
@@ -113,7 +113,7 @@ static void do_sanity_check(struct mm_struct *mm,
* tables.
*/
WARN_ON(!had_kernel_mapping);
- if (static_cpu_has(X86_FEATURE_PTI))
+ if (boot_cpu_has(X86_FEATURE_PTI))
WARN_ON(!had_user_mapping);
} else {
/*
@@ -121,7 +121,7 @@ static void do_sanity_check(struct mm_struct *mm,
* Sync the pgd to the usermode tables.
*/
WARN_ON(had_kernel_mapping);
- if (static_cpu_has(X86_FEATURE_PTI))
+ if (boot_cpu_has(X86_FEATURE_PTI))
WARN_ON(had_user_mapping);
}
}
@@ -156,7 +156,7 @@ static void map_ldt_struct_to_user(struct mm_struct *mm)
k_pmd = pgd_to_pmd_walk(k_pgd, LDT_BASE_ADDR);
u_pmd = pgd_to_pmd_walk(u_pgd, LDT_BASE_ADDR);
- if (static_cpu_has(X86_FEATURE_PTI) && !mm->context.ldt)
+ if (boot_cpu_has(X86_FEATURE_PTI) && !mm->context.ldt)
set_pmd(u_pmd, *k_pmd);
}
@@ -181,7 +181,7 @@ static void map_ldt_struct_to_user(struct mm_struct *mm)
{
pgd_t *pgd = pgd_offset(mm, LDT_BASE_ADDR);
- if (static_cpu_has(X86_FEATURE_PTI) && !mm->context.ldt)
+ if (boot_cpu_has(X86_FEATURE_PTI) && !mm->context.ldt)
set_pgd(kernel_to_user_pgdp(pgd), *pgd);
}
@@ -208,7 +208,7 @@ map_ldt_struct(struct mm_struct *mm, struct ldt_struct *ldt, int slot)
spinlock_t *ptl;
int i, nr_pages;
- if (!static_cpu_has(X86_FEATURE_PTI))
+ if (!boot_cpu_has(X86_FEATURE_PTI))
return 0;
/*
@@ -271,7 +271,7 @@ static void unmap_ldt_struct(struct mm_struct *mm, struct ldt_struct *ldt)
return;
/* LDT map/unmap is only required for PTI */
- if (!static_cpu_has(X86_FEATURE_PTI))
+ if (!boot_cpu_has(X86_FEATURE_PTI))
return;
nr_pages = DIV_ROUND_UP(ldt->nr_entries * LDT_ENTRY_SIZE, PAGE_SIZE);
@@ -311,7 +311,7 @@ static void free_ldt_pgtables(struct mm_struct *mm)
unsigned long start = LDT_BASE_ADDR;
unsigned long end = LDT_END_ADDR;
- if (!static_cpu_has(X86_FEATURE_PTI))
+ if (!boot_cpu_has(X86_FEATURE_PTI))
return;
tlb_gather_mmu(&tlb, mm, start, end);
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index c0e0101133f3..7bbaa6baf37f 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -121,7 +121,7 @@ DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
void __init native_pv_lock_init(void)
{
- if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
+ if (!boot_cpu_has(X86_FEATURE_HYPERVISOR))
static_branch_disable(&virt_spin_lock_key);
}
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 58ac7be52c7a..16a7113e91c5 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -236,7 +236,7 @@ static int get_cpuid_mode(void)
static int set_cpuid_mode(struct task_struct *task, unsigned long cpuid_enabled)
{
- if (!static_cpu_has(X86_FEATURE_CPUID_FAULT))
+ if (!boot_cpu_has(X86_FEATURE_CPUID_FAULT))
return -ENODEV;
if (cpuid_enabled)
@@ -666,7 +666,7 @@ static int prefer_mwait_c1_over_halt(const struct cpuinfo_x86 *c)
if (c->x86_vendor != X86_VENDOR_INTEL)
return 0;
- if (!cpu_has(c, X86_FEATURE_MWAIT) || static_cpu_has_bug(X86_BUG_MONITOR))
+ if (!cpu_has(c, X86_FEATURE_MWAIT) || boot_cpu_has_bug(X86_BUG_MONITOR))
return 0;
return 1;
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 725624b6c0c0..d62ebbc5ec78 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -108,7 +108,7 @@ void __noreturn machine_real_restart(unsigned int type)
write_cr3(real_mode_header->trampoline_pgd);
/* Exiting long mode will fail if CR4.PCIDE is set. */
- if (static_cpu_has(X86_FEATURE_PCID))
+ if (boot_cpu_has(X86_FEATURE_PCID))
cr4_clear_bits(X86_CR4_PCIDE);
#endif
diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
index a092b6b40c6b..6a38717d179c 100644
--- a/arch/x86/kernel/vm86_32.c
+++ b/arch/x86/kernel/vm86_32.c
@@ -369,7 +369,7 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus)
preempt_disable();
tsk->thread.sp0 += 16;
- if (static_cpu_has(X86_FEATURE_SEP)) {
+ if (boot_cpu_has(X86_FEATURE_SEP)) {
tsk->thread.sysenter_cs = 0;
refresh_sysenter_cs(&tsk->thread);
}
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 2/5] x86: Convert some slow-path static_cpu_has() callers to boot_cpu_has()
From: Juergen Gross @ 2019-03-30 12:50 UTC (permalink / raw)
To: Borislav Petkov, LKML
Cc: Rafael J. Wysocki, Peter Zijlstra, Sebastian Andrzej Siewior,
Dominik Brodowski, virtualization, Dave Hansen, H. Peter Anvin,
Nadav Amit, x86, Ingo Molnar, Rik van Riel, Thomas Lendacky,
Joerg Roedel, Jann Horn, Aubrey Li, Thomas Gleixner, linux-edac,
Tony Luck, Konrad Rzeszutek Wilk, Andy Lutomirski,
Masami Hiramatsu, Kirill A. Shutemov
In-Reply-To: <20190330112022.28888-3-bp@alien8.de>
On 30/03/2019 12:20, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
>
> Using static_cpu_has() is pointless on those paths, convert them to the
> boot_cpu_has() variant.
>
> No functional changes.
>
> Reported-by: Nadav Amit <nadav.amit@gmail.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
For the paravirt part: Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
^ permalink raw reply
* Re: [PATCH] x86/paravirt: Guard against invalid cpu # in pv_vcpu_is_preempted()
From: Juergen Gross @ 2019-04-01 6:38 UTC (permalink / raw)
To: Waiman Long, Alok Kataria, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, Peter Zijlstra, Will Deacon
Cc: Paolo Bonzini, x86, linux-kernel, virtualization
In-Reply-To: <5a48f5fb-799e-84f8-1fa7-eda851878d50@redhat.com>
On 25/03/2019 19:03, Waiman Long wrote:
> On 03/25/2019 12:40 PM, Juergen Gross wrote:
>> On 25/03/2019 16:57, Waiman Long wrote:
>>> It was found that passing an invalid cpu number to pv_vcpu_is_preempted()
>>> might panic the kernel in a VM guest. For example,
>>>
>>> [ 2.531077] Oops: 0000 [#1] SMP PTI
>>> :
>>> [ 2.532545] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
>>> [ 2.533321] RIP: 0010:__raw_callee_save___kvm_vcpu_is_preempted+0x0/0x20
>>>
>>> To guard against this kind of kernel panic, check is added to
>>> pv_vcpu_is_preempted() to make sure that no invalid cpu number will
>>> be used.
>>>
>>> Signed-off-by: Waiman Long <longman@redhat.com>
>>> ---
>>> arch/x86/include/asm/paravirt.h | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>>> index c25c38a05c1c..4cfb465dcde4 100644
>>> --- a/arch/x86/include/asm/paravirt.h
>>> +++ b/arch/x86/include/asm/paravirt.h
>>> @@ -671,6 +671,12 @@ static __always_inline void pv_kick(int cpu)
>>>
>>> static __always_inline bool pv_vcpu_is_preempted(long cpu)
>>> {
>>> + /*
>>> + * Guard against invalid cpu number or the kernel might panic.
>>> + */
>>> + if (WARN_ON_ONCE((unsigned long)cpu >= nr_cpu_ids))
>>> + return false;
>>> +
>>> return PVOP_CALLEE1(bool, lock.vcpu_is_preempted, cpu);
>>> }
>> Can this really happen without being a programming error?
>
> This shouldn't happen without a programming error, I think. In my case,
> it was caused by a race condition leading to use-after-free of the cpu
> number. However, my point is that error like that shouldn't cause the
> kernel to panic.
>
>> Basically you'd need to guard all percpu area accesses to foreign cpus
>> this way. Why is this one special?
>
> It depends. If out-of-bound access can only happen with obvious
> programming error, I don't think we need to guard against them. In this
> case, I am not totally sure if the race condition that I found may
> happen with existing code or not. To be prudent, I decide to send this
> patch out.
>
> The race condition that I am looking at is as follows:
>
> CPU 0 CPU 1
> ----- -----
> up_write:
> owner = NULL;
> <release-barrier>
> count = 0;
>
> <rcu-free task structure>
>
> rwsem_can_spin_on_owner:
> rcu_read_lock();
> read owner;
> :
> vcpu_is_preempted(owner->cpu);
> :
> rcu_read_unlock()
>
> When I tried to merge the owner into the count (clear the owner after
> the barrier), I can reproduce the crash 100% when booting up the kernel
> in a VM guest. However, I am not sure if the configuration above is safe
> and is just very hard to reproduce.
>
> Alternatively, I can also do the cpu check before calling
> vcpu_is_preempted().
I think I'd prefer that.
Juergen
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] x86/paravirt: Guard against invalid cpu # in pv_vcpu_is_preempted()
From: Waiman Long @ 2019-04-01 14:01 UTC (permalink / raw)
To: Juergen Gross, Alok Kataria, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, Peter Zijlstra, Will Deacon
Cc: Paolo Bonzini, x86, linux-kernel, virtualization
In-Reply-To: <ad016e8f-acb5-6685-b7b6-de801c002dc6@suse.com>
On 04/01/2019 02:38 AM, Juergen Gross wrote:
> On 25/03/2019 19:03, Waiman Long wrote:
>> On 03/25/2019 12:40 PM, Juergen Gross wrote:
>>> On 25/03/2019 16:57, Waiman Long wrote:
>>>> It was found that passing an invalid cpu number to pv_vcpu_is_preempted()
>>>> might panic the kernel in a VM guest. For example,
>>>>
>>>> [ 2.531077] Oops: 0000 [#1] SMP PTI
>>>> :
>>>> [ 2.532545] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
>>>> [ 2.533321] RIP: 0010:__raw_callee_save___kvm_vcpu_is_preempted+0x0/0x20
>>>>
>>>> To guard against this kind of kernel panic, check is added to
>>>> pv_vcpu_is_preempted() to make sure that no invalid cpu number will
>>>> be used.
>>>>
>>>> Signed-off-by: Waiman Long <longman@redhat.com>
>>>> ---
>>>> arch/x86/include/asm/paravirt.h | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>>>> index c25c38a05c1c..4cfb465dcde4 100644
>>>> --- a/arch/x86/include/asm/paravirt.h
>>>> +++ b/arch/x86/include/asm/paravirt.h
>>>> @@ -671,6 +671,12 @@ static __always_inline void pv_kick(int cpu)
>>>>
>>>> static __always_inline bool pv_vcpu_is_preempted(long cpu)
>>>> {
>>>> + /*
>>>> + * Guard against invalid cpu number or the kernel might panic.
>>>> + */
>>>> + if (WARN_ON_ONCE((unsigned long)cpu >= nr_cpu_ids))
>>>> + return false;
>>>> +
>>>> return PVOP_CALLEE1(bool, lock.vcpu_is_preempted, cpu);
>>>> }
>>> Can this really happen without being a programming error?
>> This shouldn't happen without a programming error, I think. In my case,
>> it was caused by a race condition leading to use-after-free of the cpu
>> number. However, my point is that error like that shouldn't cause the
>> kernel to panic.
>>
>>> Basically you'd need to guard all percpu area accesses to foreign cpus
>>> this way. Why is this one special?
>> It depends. If out-of-bound access can only happen with obvious
>> programming error, I don't think we need to guard against them. In this
>> case, I am not totally sure if the race condition that I found may
>> happen with existing code or not. To be prudent, I decide to send this
>> patch out.
>>
>> The race condition that I am looking at is as follows:
>>
>> CPU 0 CPU 1
>> ----- -----
>> up_write:
>> owner = NULL;
>> <release-barrier>
>> count = 0;
>>
>> <rcu-free task structure>
>>
>> rwsem_can_spin_on_owner:
>> rcu_read_lock();
>> read owner;
>> :
>> vcpu_is_preempted(owner->cpu);
>> :
>> rcu_read_unlock()
>>
>> When I tried to merge the owner into the count (clear the owner after
>> the barrier), I can reproduce the crash 100% when booting up the kernel
>> in a VM guest. However, I am not sure if the configuration above is safe
>> and is just very hard to reproduce.
>>
>> Alternatively, I can also do the cpu check before calling
>> vcpu_is_preempted().
> I think I'd prefer that.
>
>
> Juergen
>
It turns out that it may be caused by a software bug after all. You can
ignore this patch for now.
Thanks,
Longman
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH 1/3] drm/virtio: add missing drm_atomic_helper_shutdown() call.
From: Gerd Hoffmann @ 2019-04-01 14:03 UTC (permalink / raw)
To: dri-devel
Cc: David Airlie, open list, Daniel Vetter,
open list:VIRTIO GPU DRIVER
In-Reply-To: <20190401140306.28063-1-kraxel@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/virtio/virtgpu_display.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
index 653ec7d0bf4d..86843a4d6102 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -385,5 +385,6 @@ void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
for (i = 0 ; i < vgdev->num_scanouts; ++i)
kfree(vgdev->outputs[i].edid);
+ drm_atomic_helper_shutdown(vgdev->ddev);
drm_mode_config_cleanup(vgdev->ddev);
}
--
2.18.1
^ permalink raw reply related
* [PATCH 2/3] drm/bochs: add missing drm_atomic_helper_shutdown() call.
From: Gerd Hoffmann @ 2019-04-01 14:03 UTC (permalink / raw)
To: dri-devel
Cc: David Airlie, open list, Daniel Vetter,
open list:DRM DRIVER FOR BOCHS VIRTUAL GPU
In-Reply-To: <20190401140306.28063-1-kraxel@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/bochs/bochs_kms.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index 9e7cd6b34106..93cb27f93d39 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -293,6 +293,7 @@ int bochs_kms_init(struct bochs_device *bochs)
void bochs_kms_fini(struct bochs_device *bochs)
{
if (bochs->mode_config_initialized) {
+ drm_atomic_helper_shutdown(bochs->dev);
drm_mode_config_cleanup(bochs->dev);
bochs->mode_config_initialized = false;
}
--
2.18.1
^ permalink raw reply related
* [PATCH 3/3] drm/cirrus: add missing drm_helper_force_disable_all() call.
From: Gerd Hoffmann @ 2019-04-01 14:03 UTC (permalink / raw)
To: dri-devel
Cc: David Airlie, open list,
open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE, Daniel Vetter,
Dave Airlie
In-Reply-To: <20190401140306.28063-1-kraxel@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/cirrus/cirrus_mode.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/cirrus/cirrus_mode.c b/drivers/gpu/drm/cirrus/cirrus_mode.c
index 7f9bc32af685..32ce2c1040b4 100644
--- a/drivers/gpu/drm/cirrus/cirrus_mode.c
+++ b/drivers/gpu/drm/cirrus/cirrus_mode.c
@@ -615,6 +615,7 @@ void cirrus_modeset_fini(struct cirrus_device *cdev)
cirrus_fbdev_fini(cdev);
if (cdev->mode_info.mode_config_initialized) {
+ drm_helper_force_disable_all(cdev->dev);
drm_mode_config_cleanup(cdev->dev);
cdev->mode_info.mode_config_initialized = false;
}
--
2.18.1
^ permalink raw reply related
* Re: [PATCH 1/3] drm/virtio: add missing drm_atomic_helper_shutdown() call.
From: Mukesh Ojha @ 2019-04-01 15:08 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: David Airlie, open list, Daniel Vetter,
open list:VIRTIO GPU DRIVER
In-Reply-To: <20190401140306.28063-2-kraxel@redhat.com>
Please atleast mention here why it is required?
-Mukesh
On 4/1/2019 7:33 PM, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> drivers/gpu/drm/virtio/virtgpu_display.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
> index 653ec7d0bf4d..86843a4d6102 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
> @@ -385,5 +385,6 @@ void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
>
> for (i = 0 ; i < vgdev->num_scanouts; ++i)
> kfree(vgdev->outputs[i].edid);
> + drm_atomic_helper_shutdown(vgdev->ddev);
> drm_mode_config_cleanup(vgdev->ddev);
> }
^ permalink raw reply
* Re: [PATCH 2/3] drm/bochs: add missing drm_atomic_helper_shutdown() call.
From: Daniel Vetter @ 2019-04-02 7:54 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: David Airlie, open list, Daniel Vetter, dri-devel,
open list:DRM DRIVER FOR BOCHS VIRTUAL GPU
In-Reply-To: <20190401140306.28063-3-kraxel@redhat.com>
On Mon, Apr 01, 2019 at 04:03:05PM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> drivers/gpu/drm/bochs/bochs_kms.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
> index 9e7cd6b34106..93cb27f93d39 100644
> --- a/drivers/gpu/drm/bochs/bochs_kms.c
> +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> @@ -293,6 +293,7 @@ int bochs_kms_init(struct bochs_device *bochs)
> void bochs_kms_fini(struct bochs_device *bochs)
> {
> if (bochs->mode_config_initialized) {
This mode_config_initialized is hilarious. I think (from looking at git
history and all) this started in radeon, back when kms was an add-on, and
radeon.ko still supported ums. Then it was dutifully copypasted into
cirrus, bochs, mgag200, hisilicon and also amdgpu.
Afaict none of these drivers need this (but I didn't bother to review
amdgpu and radeon fully). Would be nice little cleanup to follow up with.
Anyway, on your 3 patches here:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> + drm_atomic_helper_shutdown(bochs->dev);
> drm_mode_config_cleanup(bochs->dev);
> bochs->mode_config_initialized = false;
> }
> --
> 2.18.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* [PATCH 1/2] drm/bochs: drop mode_config_initialized
From: Gerd Hoffmann @ 2019-04-02 9:04 UTC (permalink / raw)
To: dri-devel
Cc: David Airlie, open list, Daniel Vetter,
open list:DRM DRIVER FOR BOCHS VIRTUAL GPU
In-Reply-To: <20190402090459.12126-1-kraxel@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/bochs/bochs.h | 1 -
drivers/gpu/drm/bochs/bochs_kms.c | 8 ++------
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 03711394f1ed..a7f6723bebdd 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -73,7 +73,6 @@ struct bochs_device {
struct drm_crtc crtc;
struct drm_encoder encoder;
struct drm_connector connector;
- bool mode_config_initialized;
/* ttm */
struct {
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index 93cb27f93d39..485f9cf05e8b 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -267,7 +267,6 @@ const struct drm_mode_config_funcs bochs_mode_funcs = {
int bochs_kms_init(struct bochs_device *bochs)
{
drm_mode_config_init(bochs->dev);
- bochs->mode_config_initialized = true;
bochs->dev->mode_config.max_width = 8192;
bochs->dev->mode_config.max_height = 8192;
@@ -292,9 +291,6 @@ int bochs_kms_init(struct bochs_device *bochs)
void bochs_kms_fini(struct bochs_device *bochs)
{
- if (bochs->mode_config_initialized) {
- drm_atomic_helper_shutdown(bochs->dev);
- drm_mode_config_cleanup(bochs->dev);
- bochs->mode_config_initialized = false;
- }
+ drm_atomic_helper_shutdown(bochs->dev);
+ drm_mode_config_cleanup(bochs->dev);
}
--
2.18.1
^ permalink raw reply related
* [PATCH 2/2] drm/cirrus: drop mode_info.mode_config_initialized
From: Gerd Hoffmann @ 2019-04-02 9:04 UTC (permalink / raw)
To: dri-devel
Cc: David Airlie, open list,
open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE, Daniel Vetter,
Dave Airlie
In-Reply-To: <20190402090459.12126-1-kraxel@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/cirrus/cirrus_drv.h | 1 -
drivers/gpu/drm/cirrus/cirrus_mode.c | 9 ++-------
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.h b/drivers/gpu/drm/cirrus/cirrus_drv.h
index 915709739257..828b150cdb20 100644
--- a/drivers/gpu/drm/cirrus/cirrus_drv.h
+++ b/drivers/gpu/drm/cirrus/cirrus_drv.h
@@ -101,7 +101,6 @@ struct cirrus_crtc {
struct cirrus_fbdev;
struct cirrus_mode_info {
- bool mode_config_initialized;
struct cirrus_crtc *crtc;
/* pointer to fbdev info structure */
struct cirrus_fbdev *gfbdev;
diff --git a/drivers/gpu/drm/cirrus/cirrus_mode.c b/drivers/gpu/drm/cirrus/cirrus_mode.c
index 32ce2c1040b4..b109cd71426f 100644
--- a/drivers/gpu/drm/cirrus/cirrus_mode.c
+++ b/drivers/gpu/drm/cirrus/cirrus_mode.c
@@ -575,7 +575,6 @@ int cirrus_modeset_init(struct cirrus_device *cdev)
int ret;
drm_mode_config_init(cdev->dev);
- cdev->mode_info.mode_config_initialized = true;
cdev->dev->mode_config.max_width = CIRRUS_MAX_FB_WIDTH;
cdev->dev->mode_config.max_height = CIRRUS_MAX_FB_HEIGHT;
@@ -613,10 +612,6 @@ int cirrus_modeset_init(struct cirrus_device *cdev)
void cirrus_modeset_fini(struct cirrus_device *cdev)
{
cirrus_fbdev_fini(cdev);
-
- if (cdev->mode_info.mode_config_initialized) {
- drm_helper_force_disable_all(cdev->dev);
- drm_mode_config_cleanup(cdev->dev);
- cdev->mode_info.mode_config_initialized = false;
- }
+ drm_helper_force_disable_all(cdev->dev);
+ drm_mode_config_cleanup(cdev->dev);
}
--
2.18.1
^ permalink raw reply related
* Re: [PATCH net v5] failover: allow name change on IFF_UP slave interfaces
From: Samudrala, Sridhar @ 2019-04-02 20:03 UTC (permalink / raw)
To: Si-Wei Liu, mst, stephen, davem, kubakici, alexander.duyck, jiri,
netdev, virtualization
Cc: boris.ostrovsky, liran.alon
In-Reply-To: <1554159893-29704-1-git-send-email-si-wei.liu@oracle.com>
On 4/1/2019 4:04 PM, Si-Wei Liu wrote:
> When a netdev appears through hot plug then gets enslaved by a failover
> master that is already up and running, the slave will be opened
> right away after getting enslaved. Today there's a race that userspace
> (udev) may fail to rename the slave if the kernel (net_failover)
> opens the slave earlier than when the userspace rename happens.
> Unlike bond or team, the primary slave of failover can't be renamed by
> userspace ahead of time, since the kernel initiated auto-enslavement is
> unable to, or rather, is never meant to be synchronized with the rename
> request from userspace.
>
> As the failover slave interfaces are not designed to be operated
> directly by userspace apps: IP configuration, filter rules with
> regard to network traffic passing and etc., should all be done on master
> interface. In general, userspace apps only care about the
> name of master interface, while slave names are less important as long
> as admin users can see reliable names that may carry
> other information describing the netdev. For e.g., they can infer that
> "ens3nsby" is a standby slave of "ens3", while for a
> name like "eth0" they can't tell which master it belongs to.
>
> Historically the name of IFF_UP interface can't be changed because
> there might be admin script or management software that is already
> relying on such behavior and assumes that the slave name can't be
> changed once UP. But failover is special: with the in-kernel
> auto-enslavement mechanism, the userspace expectation for device
> enumeration and bring-up order is already broken. Previously initramfs
> and various userspace config tools were modified to bypass failover
> slaves because of auto-enslavement and duplicate MAC address. Similarly,
> in case that users care about seeing reliable slave name, the new type
> of failover slaves needs to be taken care of specifically in userspace
> anyway.
>
> It's less risky to lift up the rename restriction on failover slave
> which is already UP. Although it's possible this change may potentially
> break userspace component (most likely configuration scripts or
> management software) that assumes slave name can't be changed while
> UP, it's relatively a limited and controllable set among all userspace
> components, which can be fixed specifically to listen for the rename
> and/or link down/up events on failover slaves. Userspace component
> interacting with slaves is expected to be changed to operate on failover
> master interface instead, as the failover slave is dynamic in nature
> which may come and go at any point. The goal is to make the role of
> failover slaves less relevant, and userspace components should only
> deal with failover master in the long run.
>
> Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
Acked-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>
> --
> v1 -> v2:
> - Drop configurable module parameter (Sridhar)
>
> v2 -> v3:
> - Drop additional IFF_SLAVE_RENAME_OK flag (Sridhar)
> - Send down and up events around rename (Michael S. Tsirkin)
>
> v3 -> v4:
> - Simplify notification to be sent (Stephen Hemminger)
>
> v4 -> v5:
> - Sync up code with latest net-next (Sridhar)
> - Use proper structure initialization (Stephen, Jiri)
> ---
> net/core/dev.c | 25 ++++++++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 9823b77..b694184 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1185,7 +1185,21 @@ int dev_change_name(struct net_device *dev, const char *newname)
> BUG_ON(!dev_net(dev));
>
> net = dev_net(dev);
> - if (dev->flags & IFF_UP)
> +
> + /* Allow failover slave to rename even when
> + * it is up and running.
> + *
> + * Failover slaves are special, since userspace
> + * might rename the slave after the interface
> + * has been brought up and running due to
> + * auto-enslavement.
> + *
> + * Failover users don't actually care about slave
> + * name change, as they are only expected to operate
> + * on master interface directly.
> + */
> + if (dev->flags & IFF_UP &&
> + likely(!(dev->priv_flags & IFF_FAILOVER_SLAVE)))
> return -EBUSY;
>
> write_seqcount_begin(&devnet_rename_seq);
> @@ -1232,6 +1246,15 @@ int dev_change_name(struct net_device *dev, const char *newname)
> hlist_add_head_rcu(&dev->name_hlist, dev_name_hash(net, dev->name));
> write_unlock_bh(&dev_base_lock);
>
> + if (unlikely(dev->flags & IFF_UP)) {
> + struct netdev_notifier_change_info change_info = {
> + .info.dev = dev,
> + };
> +
> + call_netdevice_notifiers_info(NETDEV_CHANGE,
> + &change_info.info);
> + }
> +
> ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
> ret = notifier_to_errno(ret);
>
>
^ permalink raw reply
* Re: [PATCH net v5] failover: allow name change on IFF_UP slave interfaces
From: Michael S. Tsirkin @ 2019-04-02 21:50 UTC (permalink / raw)
To: Si-Wei Liu
Cc: jiri, kubakici, sridhar.samudrala, alexander.duyck,
virtualization, liran.alon, netdev, boris.ostrovsky, davem
In-Reply-To: <1554159893-29704-1-git-send-email-si-wei.liu@oracle.com>
On Mon, Apr 01, 2019 at 07:04:53PM -0400, Si-Wei Liu wrote:
> When a netdev appears through hot plug then gets enslaved by a failover
> master that is already up and running, the slave will be opened
> right away after getting enslaved. Today there's a race that userspace
> (udev) may fail to rename the slave if the kernel (net_failover)
> opens the slave earlier than when the userspace rename happens.
> Unlike bond or team, the primary slave of failover can't be renamed by
> userspace ahead of time, since the kernel initiated auto-enslavement is
> unable to, or rather, is never meant to be synchronized with the rename
> request from userspace.
>
> As the failover slave interfaces are not designed to be operated
> directly by userspace apps: IP configuration, filter rules with
> regard to network traffic passing and etc., should all be done on master
> interface. In general, userspace apps only care about the
> name of master interface, while slave names are less important as long
> as admin users can see reliable names that may carry
> other information describing the netdev. For e.g., they can infer that
> "ens3nsby" is a standby slave of "ens3", while for a
> name like "eth0" they can't tell which master it belongs to.
>
> Historically the name of IFF_UP interface can't be changed because
> there might be admin script or management software that is already
> relying on such behavior and assumes that the slave name can't be
> changed once UP. But failover is special: with the in-kernel
> auto-enslavement mechanism, the userspace expectation for device
> enumeration and bring-up order is already broken. Previously initramfs
> and various userspace config tools were modified to bypass failover
> slaves because of auto-enslavement and duplicate MAC address. Similarly,
> in case that users care about seeing reliable slave name, the new type
> of failover slaves needs to be taken care of specifically in userspace
> anyway.
>
> It's less risky to lift up the rename restriction on failover slave
> which is already UP. Although it's possible this change may potentially
> break userspace component (most likely configuration scripts or
> management software) that assumes slave name can't be changed while
> UP, it's relatively a limited and controllable set among all userspace
> components, which can be fixed specifically to listen for the rename
> and/or link down/up events on failover slaves. Userspace component
> interacting with slaves is expected to be changed to operate on failover
> master interface instead, as the failover slave is dynamic in nature
> which may come and go at any point. The goal is to make the role of
> failover slaves less relevant, and userspace components should only
> deal with failover master in the long run.
>
> Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
>
> --
> v1 -> v2:
> - Drop configurable module parameter (Sridhar)
>
> v2 -> v3:
> - Drop additional IFF_SLAVE_RENAME_OK flag (Sridhar)
> - Send down and up events around rename (Michael S. Tsirkin)
>
> v3 -> v4:
> - Simplify notification to be sent (Stephen Hemminger)
>
> v4 -> v5:
> - Sync up code with latest net-next (Sridhar)
> - Use proper structure initialization (Stephen, Jiri)
> ---
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> net/core/dev.c | 25 ++++++++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 9823b77..b694184 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1185,7 +1185,21 @@ int dev_change_name(struct net_device *dev, const char *newname)
> BUG_ON(!dev_net(dev));
>
> net = dev_net(dev);
> - if (dev->flags & IFF_UP)
> +
> + /* Allow failover slave to rename even when
> + * it is up and running.
> + *
> + * Failover slaves are special, since userspace
> + * might rename the slave after the interface
> + * has been brought up and running due to
> + * auto-enslavement.
> + *
> + * Failover users don't actually care about slave
> + * name change, as they are only expected to operate
> + * on master interface directly.
> + */
> + if (dev->flags & IFF_UP &&
> + likely(!(dev->priv_flags & IFF_FAILOVER_SLAVE)))
> return -EBUSY;
>
> write_seqcount_begin(&devnet_rename_seq);
> @@ -1232,6 +1246,15 @@ int dev_change_name(struct net_device *dev, const char *newname)
> hlist_add_head_rcu(&dev->name_hlist, dev_name_hash(net, dev->name));
> write_unlock_bh(&dev_base_lock);
>
> + if (unlikely(dev->flags & IFF_UP)) {
> + struct netdev_notifier_change_info change_info = {
> + .info.dev = dev,
> + };
> +
> + call_netdevice_notifiers_info(NETDEV_CHANGE,
> + &change_info.info);
> + }
> +
> ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
> ret = notifier_to_errno(ret);
>
> --
> 1.8.3.1
^ permalink raw reply
* Re: [PATCH net v5] failover: allow name change on IFF_UP slave interfaces
From: Stephen Hemminger @ 2019-04-02 21:53 UTC (permalink / raw)
To: Si-Wei Liu
Cc: jiri, mst, kubakici, sridhar.samudrala, alexander.duyck,
virtualization, liran.alon, netdev, boris.ostrovsky, davem
In-Reply-To: <1554159893-29704-1-git-send-email-si-wei.liu@oracle.com>
On Mon, 1 Apr 2019 19:04:53 -0400
Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> + if (dev->flags & IFF_UP &&
> + likely(!(dev->priv_flags & IFF_FAILOVER_SLAVE)))
Why is property limited to failover slave, it would make sense for netvsc
as well. Why not make it a flag like live address change?
^ 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