* Re: [GIT PULL] Hyper-V commits for 5.7-rc4
From: pr-tracker-bot @ 2020-04-27 20:40 UTC (permalink / raw)
To: Wei Liu
Cc: Linus Torvalds, Wei Liu, Linux on Hyper-V List, Linux Kernel List,
Michael Kelley, kys, sthemmin, haiyangz
In-Reply-To: <20200427111945.6qdvgimt3nt3ja57@liuwe-devbox-debian-v2.j3c5onc20sse1dnehy4noqpfcg.zx.internal.cloudapp.net>
The pull request you sent on Mon, 27 Apr 2020 11:19:45 +0000:
> ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git tags/hyperv-fixes-signed
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/869997be0e3d4b994aa04bfdf3e534b9072e6a17
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH] PCI: pci-hyperv: Retry PCI bus D0 entry when the first attempt failed with invalid device state 0xC0000184.
From: Bjorn Helgaas @ 2020-04-27 22:51 UTC (permalink / raw)
To: Wei Hu
Cc: kys, haiyangz, sthemmin, wei.liu, lorenzo.pieralisi, robh,
linux-hyperv, linux-pci, linux-kernel, decui, mikelley
In-Reply-To: <20200426132430.1756-1-weh@microsoft.com>
Please pay attention to the changelog history and make yours match:
$ git log --oneline drivers/pci/controller/pci-hyperv.c
1cf106d93245 ("PCI: hv: Introduce hv_msi_entry")
61bfd920abbf ("PCI: hv: Move retarget related structures into tlfs header")
b00f80fcfaa0 ("PCI: hv: Move hypercall related definitions into tlfs header")
067fb6c97e7e ("PCI: hv: Replace zero-length array with flexible-array member")
999dd956d838 ("PCI: hv: Add support for protocol 1.3 and support PCI_BUS_RELATIONS2")
f9ad0f361cf3 ("PCI: hv: Decouple the func definition in hv_dr_state from VSP message")
42c3d41832ef ("PCI: hv: Add missing kfree(hbus) in hv_pci_probe()'s error handling path")
e658a4fea8ef ("PCI: hv: Remove unnecessary type casting from kzalloc")
No period at end of subject.
On Sun, Apr 26, 2020 at 09:24:30PM +0800, Wei Hu wrote:
> In the case of kdump, the PCI device was not cleanly shut down
> before the kdump kernel starts. This causes the initial
> attempt of entering D0 state in the kdump kernel to fail with
> invalid device state 0xC0000184 returned from Hyper-V host.
> When this happens, explicitly call PCI bus exit and retry to
> enter the D0 state.
>
> Also fix the PCI probe failure path to release the PCI device
> resource properly.
This sounds like two separate fixes that should be in separate
patches?
> Signed-off-by: Wei Hu <weh@microsoft.com>
> ---
> drivers/pci/controller/pci-hyperv.c | 34 ++++++++++++++++++++++++++++-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index e15022ff63e3..eb4781fa058d 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -2736,6 +2736,10 @@ static void hv_free_config_window(struct hv_pcibus_device *hbus)
> vmbus_free_mmio(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
> }
>
> +#define STATUS_INVALID_DEVICE_STATE 0xC0000184
> +
> +static int hv_pci_bus_exit(struct hv_device *hdev, bool hibernating);
> +
> /**
> * hv_pci_enter_d0() - Bring the "bus" into the D0 power state
> * @hdev: VMBus's tracking struct for this root PCI bus
> @@ -2748,8 +2752,10 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
> struct pci_bus_d0_entry *d0_entry;
> struct hv_pci_compl comp_pkt;
> struct pci_packet *pkt;
> + bool retry = true;
> int ret;
>
> +enter_d0_retry:
> /*
> * Tell the host that the bus is ready to use, and moved into the
> * powered-on state. This includes telling the host which region
> @@ -2780,6 +2786,30 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
> dev_err(&hdev->device,
> "PCI Pass-through VSP failed D0 Entry with status %x\n",
> comp_pkt.completion_status);
> +
> + /*
> + * In certain case (Kdump) the pci device of interest was
> + * not cleanly shut down and resource is still held on host
> + * side, the host could return STATUS_INVALID_DEVICE_STATE.
> + * We need to explicitly request host to release the resource
> + * and try to enter D0 again.
> + */
> + if (comp_pkt.completion_status == STATUS_INVALID_DEVICE_STATE &&
> + retry) {
> + ret = hv_pci_bus_exit(hdev, true);
> +
> + retry = false;
> +
> + if (ret == 0) {
> + kfree(pkt);
> + goto enter_d0_retry;
> + } else {
> + dev_err(&hdev->device,
> + "PCI bus D0 exit failed with ret %d\n",
> + ret);
> + }
> + }
> +
> ret = -EPROTO;
> goto exit;
> }
> @@ -3136,7 +3166,7 @@ static int hv_pci_probe(struct hv_device *hdev,
>
> ret = hv_pci_allocate_bridge_windows(hbus);
> if (ret)
> - goto free_irq_domain;
> + goto exit_d0;
>
> ret = hv_send_resources_allocated(hdev);
> if (ret)
> @@ -3154,6 +3184,8 @@ static int hv_pci_probe(struct hv_device *hdev,
>
> free_windows:
> hv_pci_free_bridge_windows(hbus);
> +exit_d0:
> + (void) hv_pci_bus_exit(hdev, true);
> free_irq_domain:
> irq_domain_remove(hbus->irq_domain);
> free_fwnode:
> --
> 2.20.1
>
^ permalink raw reply
* [PATCH] hv_netvsc: Fix netvsc_start_xmit's return type
From: Nathan Chancellor @ 2020-04-28 3:30 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
Cc: linux-hyperv, netdev, linux-kernel, clang-built-linux,
Sami Tolvanen, Nathan Chancellor
netvsc_start_xmit is used as a callback function for the ndo_start_xmit
function pointer. ndo_start_xmit's return type is netdev_tx_t but
netvsc_start_xmit's return type is int.
This causes a failure with Control Flow Integrity (CFI), which requires
function pointer prototypes and callback function definitions to match
exactly. When CFI is in enforcing, the kernel panics. When booting a
CFI kernel with WSL 2, the VM is immediately terminated because of this:
$ wsl.exe -d ubuntu
The Windows Subsystem for Linux instance has terminated.
Avoid this by using the right return type for netvsc_start_xmit.
Fixes: fceaf24a943d8 ("Staging: hv: add the Hyper-V virtual network driver")
Link: https://github.com/ClangBuiltLinux/linux/issues/1009
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
Do note that netvsc_xmit still returns int because netvsc_xmit has a
potential return from netvsc_vf_xmit, which does not return netdev_tx_t
because of the call to dev_queue_xmit.
I am not sure if that is an oversight that was introduced by
commit 0c195567a8f6e ("netvsc: transparent VF management") or if
everything works properly as it is now.
My patch is purely concerned with making the definition match the
prototype so it should be NFC aside from avoiding the CFI panic.
drivers/net/hyperv/netvsc_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index d8e86bdbfba1e..ebcfbae056900 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -707,7 +707,8 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
goto drop;
}
-static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
+ struct net_device *ndev)
{
return netvsc_xmit(skb, ndev, false);
}
base-commit: 51184ae37e0518fd90cb437a2fbc953ae558cd0d
--
2.26.2
^ permalink raw reply related
* [PATCH v2] PCI: hv: Retry PCI bus D0 entry when the first attempt failed with invalid device state
From: Wei Hu @ 2020-04-28 4:21 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, wei.liu, lorenzo.pieralisi, robh,
bhelgaas, linux-hyperv, linux-pci, linux-kernel, decui, mikelley
Cc: Wei Hu
In the case of kdump, the PCI device was not cleanly shut down
before the kdump kernel starts. This causes the initial
attempt of entering D0 state in the kdump kernel to fail with
invalid device state returned from Hyper-V host.
When this happens, explicitly call PCI bus exit and retry to
enter the D0 state.
Signed-off-by: Wei Hu <weh@microsoft.com>
---
v2: Incorporate review comments from Michael Kelley, Dexuan Cui and
Bjorn Helgaas
drivers/pci/controller/pci-hyperv.c | 30 +++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index e15022ff63e3..0a42c228b231 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -2736,6 +2736,8 @@ static void hv_free_config_window(struct hv_pcibus_device *hbus)
vmbus_free_mmio(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
}
+static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs);
+
/**
* hv_pci_enter_d0() - Bring the "bus" into the D0 power state
* @hdev: VMBus's tracking struct for this root PCI bus
@@ -2748,8 +2750,10 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
struct pci_bus_d0_entry *d0_entry;
struct hv_pci_compl comp_pkt;
struct pci_packet *pkt;
+ bool retry = true;
int ret;
+enter_d0_retry:
/*
* Tell the host that the bus is ready to use, and moved into the
* powered-on state. This includes telling the host which region
@@ -2776,6 +2780,28 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
if (ret)
goto exit;
+ /*
+ * In certain case (Kdump) the pci device of interest was
+ * not cleanly shut down and resource is still held on host
+ * side, the host could return invalid device status.
+ * We need to explicitly request host to release the resource
+ * and try to enter D0 again.
+ */
+ if (comp_pkt.completion_status < 0 && retry) {
+ retry = false;
+
+ dev_err(&hdev->device, "Retrying D0 Entry\n");
+
+ ret = hv_pci_bus_exit(hdev, true);
+
+ if (ret == 0) {
+ kfree(pkt);
+ goto enter_d0_retry;
+ }
+ dev_err(&hdev->device,
+ "Retrying D0 failed with ret %d\n", ret);
+ }
+
if (comp_pkt.completion_status < 0) {
dev_err(&hdev->device,
"PCI Pass-through VSP failed D0 Entry with status %x\n",
@@ -3173,7 +3199,7 @@ static int hv_pci_probe(struct hv_device *hdev,
return ret;
}
-static int hv_pci_bus_exit(struct hv_device *hdev, bool hibernating)
+static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs)
{
struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
struct {
@@ -3191,7 +3217,7 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool hibernating)
if (hdev->channel->rescind)
return 0;
- if (!hibernating) {
+ if (!keep_devs) {
/* Delete any children which might still exist. */
dr = kzalloc(sizeof(*dr), GFP_KERNEL);
if (dr && hv_pci_start_relations_work(hbus, dr))
--
2.20.1
^ permalink raw reply related
* RE: [PATCH] PCI: pci-hyperv: Retry PCI bus D0 entry when the first attempt failed with invalid device state 0xC0000184.
From: Wei Hu @ 2020-04-28 6:41 UTC (permalink / raw)
To: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, linux-hyperv@vger.kernel.org, Dexuan Cui
In-Reply-To: <MW2PR2101MB105241B940784CF73A8A9BB0D7AF0@MW2PR2101MB1052.namprd21.prod.outlook.com>
> -----Original Message-----
> From: Michael Kelley <mikelley@microsoft.com>
> > > The above is good. But there's another error case that isn't
> > > handled correctly. If create_root_hv_pci_bus() fails,
> > > hv_send_resources_released() should be called.
> > >
> > > Fixing these two error cases should probably go in a separate patch:
> > > One patch for the retry problem in kdump, and a separate patch for
> > > these error cases.
> >
> > [Wei Hu]
> > hv_send_resources_released() is called in the added hv_pci_bus_exit().
> > If hv_send_resources_allocated() fails, is it correct to call
> hv_send_resources_released()?
> > Allocation can fail in the middle. So I am not sure if calling
> > hv_send_resources_released() won't cause any side effect.
> >
>
> Ah yes, you are right. But that brings up a separate problem.
> If hv_pci_allocate_bridge_windows() or hv_send_resources_allocated() fails,
> then the error path will call hv_pci_bus_exit(), which will call
> hv_send_resources_released(), even if hv_send_resources_allocated() was
> never called or didn't fully succeed. As you noted,
> hv_send_resources_allocated() does multiple steps, some of which might have
> succeeded, and some of which didn't. The mismatch might cause problems.
> That means fixing this error handling is going to be a bit more complex. Each
> operation needs
> to be individually undone, and only if it previously succeeded. Could you
> follow up
> with the Hyper-V people to see if there's a problem with doing the
> RESOURCES_RELEASED message on a slot where RESOURCES_ASSIGNED was
> not done or wasn't successful?
> If doing a spurious RESOURCES_RELEASED is harmless, that will make the error
> cleanup easier.
>
I think we can clean this up by doing like following, without the need of consulting
Hyper-V team. The kdump retry also works with this by setting the wslot_res_allocated
to 255 before calling hv_pci_bus_exit() to retry. Let me know what you think.
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 0a42c228b231..06f31f5777f9 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -480,6 +480,9 @@ struct hv_pcibus_device {
struct workqueue_struct *wq;
+ /* Highest slot of child device allocated resources allocated*/
+ int wslot_res_allocated;
+
/* hypercall arg, must not cross page boundary */
struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
@@ -2873,7 +2876,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
struct hv_pci_dev *hpdev;
struct pci_packet *pkt;
size_t size_res;
- u32 wslot;
+ int wslot;
int ret;
size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2)
@@ -2926,6 +2929,8 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
comp_pkt.completion_status);
break;
}
+
+ hbus->wslot_res_allocated = wslot;
}
kfree(pkt);
@@ -2944,10 +2949,10 @@ static int hv_send_resources_released(struct hv_device *hdev)
struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
struct pci_child_message pkt;
struct hv_pci_dev *hpdev;
- u32 wslot;
+ int wslot;
int ret;
- for (wslot = 0; wslot < 256; wslot++) {
+ for (wslot = hbus->wslot_res_allocated; wslot >= 0; wslot--) {
hpdev = get_pcichild_wslot(hbus, wslot);
if (!hpdev)
continue;
@@ -2962,8 +2967,12 @@ static int hv_send_resources_released(struct hv_device *hdev)
VM_PKT_DATA_INBAND, 0);
if (ret)
return ret;
+
+ hbus->wslot_res_allocated = wslot - 1;
}
+ hbus->wslot_res_allocated = -1;
+
return 0;
}
@@ -3063,6 +3072,7 @@ static int hv_pci_probe(struct hv_device *hdev,
if (!hbus)
return -ENOMEM;
hbus->state = hv_pcibus_init;
+ hbus->wslot_res_allocated = -1;
/*
* The PCI bus "domain" is what is called "segment" in ACPI and other
@@ -3162,7 +3172,7 @@ static int hv_pci_probe(struct hv_device *hdev,
ret = hv_pci_allocate_bridge_windows(hbus);
if (ret)
- goto free_irq_domain;
+ goto exit_d0;
ret = hv_send_resources_allocated(hdev);
if (ret)
@@ -3180,6 +3190,8 @@ static int hv_pci_probe(struct hv_device *hdev,
free_windows:
hv_pci_free_bridge_windows(hbus);
+exit_d0:
+ (void) hv_pci_bus_exit(hdev, true);
free_irq_domain:
irq_domain_remove(hbus->irq_domain);
free_fwnode:
^ permalink raw reply related
* Re: [PATCH net-next 0/3] vsock: support network namespace
From: Jason Wang @ 2020-04-28 8:13 UTC (permalink / raw)
To: Stefano Garzarella, davem, Stefan Hajnoczi, Michael S. Tsirkin
Cc: linux-kernel, Jorgen Hansen, kvm, virtualization, linux-hyperv,
Dexuan Cui, netdev
In-Reply-To: <20200427142518.uwssa6dtasrp3bfc@steredhat>
On 2020/4/27 下午10:25, Stefano Garzarella wrote:
> Hi David, Michael, Stefan,
> I'm restarting to work on this topic since Kata guys are interested to
> have that, especially on the guest side.
>
> While working on the v2 I had few doubts, and I'd like to have your
> suggestions:
>
> 1. netns assigned to the device inside the guest
>
> Currently I assigned this device to 'init_net'. Maybe it is better
> if we allow the user to decide which netns assign to the device
> or to disable this new feature to have the same behavior as before
> (host reachable from any netns).
> I think we can handle this in the vsock core and not in the single
> transports.
>
> The simplest way that I found, is to add a new
> IOCTL_VM_SOCKETS_ASSIGN_G2H_NETNS to /dev/vsock to enable the feature
> and assign the device to the same netns of the process that do the
> ioctl(), but I'm not sure it is clean enough.
>
> Maybe it is better to add new rtnetlink messages, but I'm not sure if
> it is feasible since we don't have a netdev device.
>
> What do you suggest?
As we've discussed, it should be a netdev probably in either guest or
host side. And it would be much simpler if we want do implement
namespace then. No new API is needed.
Thanks
>
>
> 2. netns assigned in the host
>
> As Michael suggested, I added a new /dev/vhost-vsock-netns to allow
> userspace application to use this new feature, leaving to
> /dev/vhost-vsock the previous behavior (guest reachable from any
> netns).
>
> I like this approach, but I had these doubts:
>
> - I need to allocate a new minor for that device (e.g.
> VHOST_VSOCK_NETNS_MINOR) or is there an alternative way that I can
> use?
>
> - It is vhost-vsock specific, should we provide something handled in
> the vsock core, maybe centralizing the CID allocation and adding a
> new IOCTL or rtnetlink message like for the guest side?
> (maybe it could be a second step, and for now we can continue with
> the new device)
>
>
> Thanks for the help,
> Stefano
>
>
> On Thu, Jan 16, 2020 at 06:24:25PM +0100, Stefano Garzarella wrote:
>> RFC -> v1:
>> * added 'netns' module param to vsock.ko to enable the
>> network namespace support (disabled by default)
>> * added 'vsock_net_eq()' to check the "net" assigned to a socket
>> only when 'netns' support is enabled
>>
>> RFC: https://patchwork.ozlabs.org/cover/1202235/
>>
>> Now that we have multi-transport upstream, I started to take a look to
>> support network namespace in vsock.
>>
>> As we partially discussed in the multi-transport proposal [1], it could
>> be nice to support network namespace in vsock to reach the following
>> goals:
>> - isolate host applications from guest applications using the same ports
>> with CID_ANY
>> - assign the same CID of VMs running in different network namespaces
>> - partition VMs between VMMs or at finer granularity
>>
>> This new feature is disabled by default, because it changes vsock's
>> behavior with network namespaces and could break existing applications.
>> It can be enabled with the new 'netns' module parameter of vsock.ko.
>>
>> This implementation provides the following behavior:
>> - packets received from the host (received by G2H transports) are
>> assigned to the default netns (init_net)
>> - packets received from the guest (received by H2G - vhost-vsock) are
>> assigned to the netns of the process that opens /dev/vhost-vsock
>> (usually the VMM, qemu in my tests, opens the /dev/vhost-vsock)
>> - for vmci I need some suggestions, because I don't know how to do
>> and test the same in the vmci driver, for now vmci uses the
>> init_net
>> - loopback packets are exchanged only in the same netns
>>
>> I tested the series in this way:
>> l0_host$ qemu-system-x86_64 -m 4G -M accel=kvm -smp 4 \
>> -drive file=/tmp/vsockvm0.img,if=virtio --nographic \
>> -device vhost-vsock-pci,guest-cid=3
>>
>> l1_vm$ echo 1 > /sys/module/vsock/parameters/netns
>>
>> l1_vm$ ip netns add ns1
>> l1_vm$ ip netns add ns2
>> # same CID on different netns
>> l1_vm$ ip netns exec ns1 qemu-system-x86_64 -m 1G -M accel=kvm -smp 2 \
>> -drive file=/tmp/vsockvm1.img,if=virtio --nographic \
>> -device vhost-vsock-pci,guest-cid=4
>> l1_vm$ ip netns exec ns2 qemu-system-x86_64 -m 1G -M accel=kvm -smp 2 \
>> -drive file=/tmp/vsockvm2.img,if=virtio --nographic \
>> -device vhost-vsock-pci,guest-cid=4
>>
>> # all iperf3 listen on CID_ANY and port 5201, but in different netns
>> l1_vm$ ./iperf3 --vsock -s # connection from l0 or guests started
>> # on default netns (init_net)
>> l1_vm$ ip netns exec ns1 ./iperf3 --vsock -s
>> l1_vm$ ip netns exec ns1 ./iperf3 --vsock -s
>>
>> l0_host$ ./iperf3 --vsock -c 3
>> l2_vm1$ ./iperf3 --vsock -c 2
>> l2_vm2$ ./iperf3 --vsock -c 2
>>
>> [1] https://www.spinics.net/lists/netdev/msg575792.html
>>
>> Stefano Garzarella (3):
>> vsock: add network namespace support
>> vsock/virtio_transport_common: handle netns of received packets
>> vhost/vsock: use netns of process that opens the vhost-vsock device
>>
>> drivers/vhost/vsock.c | 29 ++++++++++++-----
>> include/linux/virtio_vsock.h | 2 ++
>> include/net/af_vsock.h | 7 +++--
>> net/vmw_vsock/af_vsock.c | 41 +++++++++++++++++++------
>> net/vmw_vsock/hyperv_transport.c | 5 +--
>> net/vmw_vsock/virtio_transport.c | 2 ++
>> net/vmw_vsock/virtio_transport_common.c | 12 ++++++--
>> net/vmw_vsock/vmci_transport.c | 5 +--
>> 8 files changed, 78 insertions(+), 25 deletions(-)
>>
>> --
>> 2.24.1
>>
^ permalink raw reply
* Re: [PATCH] hv_netvsc: Fix netvsc_start_xmit's return type
From: Wei Liu @ 2020-04-28 10:08 UTC (permalink / raw)
To: Nathan Chancellor
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, netdev, linux-kernel, clang-built-linux,
Sami Tolvanen
In-Reply-To: <20200428033042.44561-1-natechancellor@gmail.com>
On Mon, Apr 27, 2020 at 08:30:43PM -0700, Nathan Chancellor wrote:
> netvsc_start_xmit is used as a callback function for the ndo_start_xmit
> function pointer. ndo_start_xmit's return type is netdev_tx_t but
> netvsc_start_xmit's return type is int.
>
> This causes a failure with Control Flow Integrity (CFI), which requires
> function pointer prototypes and callback function definitions to match
> exactly. When CFI is in enforcing, the kernel panics. When booting a
> CFI kernel with WSL 2, the VM is immediately terminated because of this:
>
> $ wsl.exe -d ubuntu
> The Windows Subsystem for Linux instance has terminated.
>
> Avoid this by using the right return type for netvsc_start_xmit.
>
> Fixes: fceaf24a943d8 ("Staging: hv: add the Hyper-V virtual network driver")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1009
Please consider pulling in the panic log from #1009 to the commit
message. It is much better than the one line message above.
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> Do note that netvsc_xmit still returns int because netvsc_xmit has a
> potential return from netvsc_vf_xmit, which does not return netdev_tx_t
> because of the call to dev_queue_xmit.
>
> I am not sure if that is an oversight that was introduced by
> commit 0c195567a8f6e ("netvsc: transparent VF management") or if
> everything works properly as it is now.
>
> My patch is purely concerned with making the definition match the
> prototype so it should be NFC aside from avoiding the CFI panic.
>
> drivers/net/hyperv/netvsc_drv.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index d8e86bdbfba1e..ebcfbae056900 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -707,7 +707,8 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
> goto drop;
> }
>
> -static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
> + struct net_device *ndev)
> {
> return netvsc_xmit(skb, ndev, false);
> }
>
> base-commit: 51184ae37e0518fd90cb437a2fbc953ae558cd0d
> --
> 2.26.2
>
^ permalink raw reply
* Re: [PATCH net-next 0/3] vsock: support network namespace
From: Stefano Garzarella @ 2020-04-28 16:00 UTC (permalink / raw)
To: Jason Wang
Cc: davem, Stefan Hajnoczi, Michael S. Tsirkin, linux-kernel,
Jorgen Hansen, kvm, virtualization, linux-hyperv, Dexuan Cui,
netdev
In-Reply-To: <224cdc10-1532-7ddc-f113-676d43d8f322@redhat.com>
On Tue, Apr 28, 2020 at 04:13:22PM +0800, Jason Wang wrote:
>
> On 2020/4/27 下午10:25, Stefano Garzarella wrote:
> > Hi David, Michael, Stefan,
> > I'm restarting to work on this topic since Kata guys are interested to
> > have that, especially on the guest side.
> >
> > While working on the v2 I had few doubts, and I'd like to have your
> > suggestions:
> >
> > 1. netns assigned to the device inside the guest
> >
> > Currently I assigned this device to 'init_net'. Maybe it is better
> > if we allow the user to decide which netns assign to the device
> > or to disable this new feature to have the same behavior as before
> > (host reachable from any netns).
> > I think we can handle this in the vsock core and not in the single
> > transports.
> >
> > The simplest way that I found, is to add a new
> > IOCTL_VM_SOCKETS_ASSIGN_G2H_NETNS to /dev/vsock to enable the feature
> > and assign the device to the same netns of the process that do the
> > ioctl(), but I'm not sure it is clean enough.
> >
> > Maybe it is better to add new rtnetlink messages, but I'm not sure if
> > it is feasible since we don't have a netdev device.
> >
> > What do you suggest?
>
>
> As we've discussed, it should be a netdev probably in either guest or host
> side. And it would be much simpler if we want do implement namespace then.
> No new API is needed.
>
Thanks Jason!
It would be cool, but I don't have much experience on netdev.
Do you see any particular obstacles?
I'll take a look to understand how to do it, surely in the guest would
be very useful to have the vsock device as a netdev and maybe also in the host.
Stefano
^ permalink raw reply
* [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
From: Nathan Chancellor @ 2020-04-28 17:54 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
Cc: linux-hyperv, netdev, linux-kernel, clang-built-linux,
Sami Tolvanen, Nathan Chancellor
In-Reply-To: <20200428100828.aslw3pn5nhwtlsnt@liuwe-devbox-debian-v2.j3c5onc20sse1dnehy4noqpfcg.zx.internal.cloudapp.net>
netvsc_start_xmit is used as a callback function for the ndo_start_xmit
function pointer. ndo_start_xmit's return type is netdev_tx_t but
netvsc_start_xmit's return type is int.
This causes a failure with Control Flow Integrity (CFI), which requires
function pointer prototypes and callback function definitions to match
exactly. When CFI is in enforcing, the kernel panics. When booting a
CFI kernel with WSL 2, the VM is immediately terminated because of this.
The splat when CONFIG_CFI_PERMISSIVE is used:
[ 5.916765] CFI failure (target: netvsc_start_xmit+0x0/0x10):
[ 5.916771] WARNING: CPU: 8 PID: 0 at kernel/cfi.c:29 __cfi_check_fail+0x2e/0x40
[ 5.916772] Modules linked in:
[ 5.916774] CPU: 8 PID: 0 Comm: swapper/8 Not tainted 5.7.0-rc3-next-20200424-microsoft-cbl-00001-ged4eb37d2c69-dirty #1
[ 5.916776] RIP: 0010:__cfi_check_fail+0x2e/0x40
[ 5.916777] Code: 48 c7 c7 70 98 63 a9 48 c7 c6 11 db 47 a9 e8 69 55 59 00 85 c0 75 02 5b c3 48 c7 c7 73 c6 43 a9 48 89 de 31 c0 e8 12 2d f0 ff <0f> 0b 5b c3 00 00 cc cc 00 00 cc cc 00 00 cc cc 00 00 85 f6 74 25
[ 5.916778] RSP: 0018:ffffa803c0260b78 EFLAGS: 00010246
[ 5.916779] RAX: 712a1af25779e900 RBX: ffffffffa8cf7950 RCX: ffffffffa962cf08
[ 5.916779] RDX: ffffffffa9c36b60 RSI: 0000000000000082 RDI: ffffffffa9c36b5c
[ 5.916780] RBP: ffff8ffc4779c2c0 R08: 0000000000000001 R09: ffffffffa9c3c300
[ 5.916781] R10: 0000000000000151 R11: ffffffffa9c36b60 R12: ffff8ffe39084000
[ 5.916782] R13: ffffffffa8cf7950 R14: ffffffffa8d12cb0 R15: ffff8ffe39320140
[ 5.916784] FS: 0000000000000000(0000) GS:ffff8ffe3bc00000(0000) knlGS:0000000000000000
[ 5.916785] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 5.916786] CR2: 00007ffef5749408 CR3: 00000002f4f5e000 CR4: 0000000000340ea0
[ 5.916787] Call Trace:
[ 5.916788] <IRQ>
[ 5.916790] __cfi_check+0x3ab58/0x450e0
[ 5.916793] ? dev_hard_start_xmit+0x11f/0x160
[ 5.916795] ? sch_direct_xmit+0xf2/0x230
[ 5.916796] ? __dev_queue_xmit.llvm.11471227737707190958+0x69d/0x8e0
[ 5.916797] ? neigh_resolve_output+0xdf/0x220
[ 5.916799] ? neigh_connected_output.cfi_jt+0x8/0x8
[ 5.916801] ? ip6_finish_output2+0x398/0x4c0
[ 5.916803] ? nf_nat_ipv6_out+0x10/0xa0
[ 5.916804] ? nf_hook_slow+0x84/0x100
[ 5.916807] ? ip6_input_finish+0x8/0x8
[ 5.916807] ? ip6_output+0x6f/0x110
[ 5.916808] ? __ip6_local_out.cfi_jt+0x8/0x8
[ 5.916810] ? mld_sendpack+0x28e/0x330
[ 5.916811] ? ip_rt_bug+0x8/0x8
[ 5.916813] ? mld_ifc_timer_expire+0x2db/0x400
[ 5.916814] ? neigh_proxy_process+0x8/0x8
[ 5.916816] ? call_timer_fn+0x3d/0xd0
[ 5.916817] ? __run_timers+0x2a9/0x300
[ 5.916819] ? rcu_core_si+0x8/0x8
[ 5.916820] ? run_timer_softirq+0x14/0x30
[ 5.916821] ? __do_softirq+0x154/0x262
[ 5.916822] ? native_x2apic_icr_write+0x8/0x8
[ 5.916824] ? irq_exit+0xba/0xc0
[ 5.916825] ? hv_stimer0_vector_handler+0x99/0xe0
[ 5.916826] ? hv_stimer0_callback_vector+0xf/0x20
[ 5.916826] </IRQ>
[ 5.916828] ? hv_stimer_global_cleanup.cfi_jt+0x8/0x8
[ 5.916829] ? raw_setsockopt+0x8/0x8
[ 5.916830] ? default_idle+0xe/0x10
[ 5.916832] ? do_idle.llvm.10446269078108580492+0xb7/0x130
[ 5.916833] ? raw_setsockopt+0x8/0x8
[ 5.916833] ? cpu_startup_entry+0x15/0x20
[ 5.916835] ? cpu_hotplug_enable.cfi_jt+0x8/0x8
[ 5.916836] ? start_secondary+0x188/0x190
[ 5.916837] ? secondary_startup_64+0xa5/0xb0
[ 5.916838] ---[ end trace f2683fa869597ba5 ]---
Avoid this by using the right return type for netvsc_start_xmit.
Fixes: fceaf24a943d8 ("Staging: hv: add the Hyper-V virtual network driver")
Link: https://github.com/ClangBuiltLinux/linux/issues/1009
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
v1 -> v2:
* Move splat into commit message rather than issue.
Comment from previous version:
Do note that netvsc_xmit still returns int because netvsc_xmit has a
potential return from netvsc_vf_xmit, which does not return netdev_tx_t
because of the call to dev_queue_xmit.
I am not sure if that is an oversight that was introduced by
commit 0c195567a8f6e ("netvsc: transparent VF management") or if
everything works properly as it is now.
My patch is purely concerned with making the definition match the
prototype so it should be NFC aside from avoiding the CFI panic.
drivers/net/hyperv/netvsc_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index d8e86bdbfba1e..ebcfbae056900 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -707,7 +707,8 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
goto drop;
}
-static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
+ struct net_device *ndev)
{
return netvsc_xmit(skb, ndev, false);
}
base-commit: 51184ae37e0518fd90cb437a2fbc953ae558cd0d
--
2.26.2
^ permalink raw reply related
* Re: [PATCH net-next 0/3] vsock: support network namespace
From: Jason Wang @ 2020-04-29 9:21 UTC (permalink / raw)
To: Stefano Garzarella
Cc: davem, Stefan Hajnoczi, Michael S. Tsirkin, linux-kernel,
Jorgen Hansen, kvm, virtualization, linux-hyperv, Dexuan Cui,
netdev
In-Reply-To: <20200428160052.o3ihui4262xogyg4@steredhat>
On 2020/4/29 上午12:00, Stefano Garzarella wrote:
> On Tue, Apr 28, 2020 at 04:13:22PM +0800, Jason Wang wrote:
>> On 2020/4/27 下午10:25, Stefano Garzarella wrote:
>>> Hi David, Michael, Stefan,
>>> I'm restarting to work on this topic since Kata guys are interested to
>>> have that, especially on the guest side.
>>>
>>> While working on the v2 I had few doubts, and I'd like to have your
>>> suggestions:
>>>
>>> 1. netns assigned to the device inside the guest
>>>
>>> Currently I assigned this device to 'init_net'. Maybe it is better
>>> if we allow the user to decide which netns assign to the device
>>> or to disable this new feature to have the same behavior as before
>>> (host reachable from any netns).
>>> I think we can handle this in the vsock core and not in the single
>>> transports.
>>>
>>> The simplest way that I found, is to add a new
>>> IOCTL_VM_SOCKETS_ASSIGN_G2H_NETNS to /dev/vsock to enable the feature
>>> and assign the device to the same netns of the process that do the
>>> ioctl(), but I'm not sure it is clean enough.
>>>
>>> Maybe it is better to add new rtnetlink messages, but I'm not sure if
>>> it is feasible since we don't have a netdev device.
>>>
>>> What do you suggest?
>> As we've discussed, it should be a netdev probably in either guest or host
>> side. And it would be much simpler if we want do implement namespace then.
>> No new API is needed.
>>
> Thanks Jason!
>
> It would be cool, but I don't have much experience on netdev.
> Do you see any particular obstacles?
I don't see but if there's we can try to find a solution or ask for
netdev experts for that. I do hear from somebody that is interested in
having netdev in the past.
>
> I'll take a look to understand how to do it, surely in the guest would
> be very useful to have the vsock device as a netdev and maybe also in the host.
Yes, it's worth to have a try then we will have a unified management
interface and we will benefit from it in the future.
Starting form guest is good idea which should be less complicated than host.
Thanks
>
> Stefano
>
^ permalink raw reply
* Re: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
From: Wei Liu @ 2020-04-29 10:10 UTC (permalink / raw)
To: Nathan Chancellor, davem
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, netdev, linux-kernel, clang-built-linux,
Sami Tolvanen
In-Reply-To: <20200428175455.2109973-1-natechancellor@gmail.com>
David
Do you want this to go through net tree? I can submit it via hyperv tree
if that's preferred.
Wei.
On Tue, Apr 28, 2020 at 10:54:56AM -0700, Nathan Chancellor wrote:
> netvsc_start_xmit is used as a callback function for the ndo_start_xmit
> function pointer. ndo_start_xmit's return type is netdev_tx_t but
> netvsc_start_xmit's return type is int.
>
> This causes a failure with Control Flow Integrity (CFI), which requires
> function pointer prototypes and callback function definitions to match
> exactly. When CFI is in enforcing, the kernel panics. When booting a
> CFI kernel with WSL 2, the VM is immediately terminated because of this.
>
> The splat when CONFIG_CFI_PERMISSIVE is used:
>
> [ 5.916765] CFI failure (target: netvsc_start_xmit+0x0/0x10):
> [ 5.916771] WARNING: CPU: 8 PID: 0 at kernel/cfi.c:29 __cfi_check_fail+0x2e/0x40
> [ 5.916772] Modules linked in:
> [ 5.916774] CPU: 8 PID: 0 Comm: swapper/8 Not tainted 5.7.0-rc3-next-20200424-microsoft-cbl-00001-ged4eb37d2c69-dirty #1
> [ 5.916776] RIP: 0010:__cfi_check_fail+0x2e/0x40
> [ 5.916777] Code: 48 c7 c7 70 98 63 a9 48 c7 c6 11 db 47 a9 e8 69 55 59 00 85 c0 75 02 5b c3 48 c7 c7 73 c6 43 a9 48 89 de 31 c0 e8 12 2d f0 ff <0f> 0b 5b c3 00 00 cc cc 00 00 cc cc 00 00 cc cc 00 00 85 f6 74 25
> [ 5.916778] RSP: 0018:ffffa803c0260b78 EFLAGS: 00010246
> [ 5.916779] RAX: 712a1af25779e900 RBX: ffffffffa8cf7950 RCX: ffffffffa962cf08
> [ 5.916779] RDX: ffffffffa9c36b60 RSI: 0000000000000082 RDI: ffffffffa9c36b5c
> [ 5.916780] RBP: ffff8ffc4779c2c0 R08: 0000000000000001 R09: ffffffffa9c3c300
> [ 5.916781] R10: 0000000000000151 R11: ffffffffa9c36b60 R12: ffff8ffe39084000
> [ 5.916782] R13: ffffffffa8cf7950 R14: ffffffffa8d12cb0 R15: ffff8ffe39320140
> [ 5.916784] FS: 0000000000000000(0000) GS:ffff8ffe3bc00000(0000) knlGS:0000000000000000
> [ 5.916785] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 5.916786] CR2: 00007ffef5749408 CR3: 00000002f4f5e000 CR4: 0000000000340ea0
> [ 5.916787] Call Trace:
> [ 5.916788] <IRQ>
> [ 5.916790] __cfi_check+0x3ab58/0x450e0
> [ 5.916793] ? dev_hard_start_xmit+0x11f/0x160
> [ 5.916795] ? sch_direct_xmit+0xf2/0x230
> [ 5.916796] ? __dev_queue_xmit.llvm.11471227737707190958+0x69d/0x8e0
> [ 5.916797] ? neigh_resolve_output+0xdf/0x220
> [ 5.916799] ? neigh_connected_output.cfi_jt+0x8/0x8
> [ 5.916801] ? ip6_finish_output2+0x398/0x4c0
> [ 5.916803] ? nf_nat_ipv6_out+0x10/0xa0
> [ 5.916804] ? nf_hook_slow+0x84/0x100
> [ 5.916807] ? ip6_input_finish+0x8/0x8
> [ 5.916807] ? ip6_output+0x6f/0x110
> [ 5.916808] ? __ip6_local_out.cfi_jt+0x8/0x8
> [ 5.916810] ? mld_sendpack+0x28e/0x330
> [ 5.916811] ? ip_rt_bug+0x8/0x8
> [ 5.916813] ? mld_ifc_timer_expire+0x2db/0x400
> [ 5.916814] ? neigh_proxy_process+0x8/0x8
> [ 5.916816] ? call_timer_fn+0x3d/0xd0
> [ 5.916817] ? __run_timers+0x2a9/0x300
> [ 5.916819] ? rcu_core_si+0x8/0x8
> [ 5.916820] ? run_timer_softirq+0x14/0x30
> [ 5.916821] ? __do_softirq+0x154/0x262
> [ 5.916822] ? native_x2apic_icr_write+0x8/0x8
> [ 5.916824] ? irq_exit+0xba/0xc0
> [ 5.916825] ? hv_stimer0_vector_handler+0x99/0xe0
> [ 5.916826] ? hv_stimer0_callback_vector+0xf/0x20
> [ 5.916826] </IRQ>
> [ 5.916828] ? hv_stimer_global_cleanup.cfi_jt+0x8/0x8
> [ 5.916829] ? raw_setsockopt+0x8/0x8
> [ 5.916830] ? default_idle+0xe/0x10
> [ 5.916832] ? do_idle.llvm.10446269078108580492+0xb7/0x130
> [ 5.916833] ? raw_setsockopt+0x8/0x8
> [ 5.916833] ? cpu_startup_entry+0x15/0x20
> [ 5.916835] ? cpu_hotplug_enable.cfi_jt+0x8/0x8
> [ 5.916836] ? start_secondary+0x188/0x190
> [ 5.916837] ? secondary_startup_64+0xa5/0xb0
> [ 5.916838] ---[ end trace f2683fa869597ba5 ]---
>
> Avoid this by using the right return type for netvsc_start_xmit.
>
> Fixes: fceaf24a943d8 ("Staging: hv: add the Hyper-V virtual network driver")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1009
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> v1 -> v2:
>
> * Move splat into commit message rather than issue.
>
> Comment from previous version:
>
> Do note that netvsc_xmit still returns int because netvsc_xmit has a
> potential return from netvsc_vf_xmit, which does not return netdev_tx_t
> because of the call to dev_queue_xmit.
>
> I am not sure if that is an oversight that was introduced by
> commit 0c195567a8f6e ("netvsc: transparent VF management") or if
> everything works properly as it is now.
>
> My patch is purely concerned with making the definition match the
> prototype so it should be NFC aside from avoiding the CFI panic.
>
> drivers/net/hyperv/netvsc_drv.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index d8e86bdbfba1e..ebcfbae056900 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -707,7 +707,8 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
> goto drop;
> }
>
> -static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
> + struct net_device *ndev)
> {
> return netvsc_xmit(skb, ndev, false);
> }
>
> base-commit: 51184ae37e0518fd90cb437a2fbc953ae558cd0d
> --
> 2.26.2
>
^ permalink raw reply
* [PATCH v1 0/3] mm/memory_hotplug: Make virtio-mem play nicely with kexec-tools
From: David Hildenbrand @ 2020-04-29 16:08 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mm, virtio-dev, virtualization, linuxppc-dev, linux-acpi,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Andrew Morton, Michael S . Tsirkin, David Hildenbrand, Baoquan He,
Benjamin Herrenschmidt, Boris Ostrovsky, Christian Borntraeger,
Dan Williams, Dave Jiang, Eric Biederman, Greg Kroah-Hartman,
Haiyang Zhang, Heiko Carstens, Jason Wang, Juergen Gross,
K. Y. Srinivasan, Len Brown, Leonardo Bras, Michael Ellerman,
Michal Hocko, Nathan Lynch, Oscar Salvador, Pankaj Gupta,
Paul Mackerras, Pingfan Liu, Rafael J. Wysocki,
Stefano Stabellini, Stephen Hemminger, Thomas Gleixner,
Vasily Gorbik, Vishal Verma, Wei Liu, Wei Yang
This series is based on [1]:
[PATCH v2 00/10] virtio-mem: paravirtualized memory
That will hopefull get picked up soon, rebased to -next.
The following patches were reverted from -next [2]:
[PATCH 0/3] kexec/memory_hotplug: Prevent removal and accidental use
As discussed in that thread, they should be reverted from -next already.
In theory, if people agree, we could take the first two patches via the
-mm tree now and the last (virtio-mem) patch via MST's tree once picking up
virtio-mem. No strong feelings.
Memory added by virtio-mem is special and might contain logical holes,
especially after memory unplug, but also when adding memory in
sub-section size. While memory in these holes can usually be read, that
memory should not be touched. virtio-mem managed device memory is never
exposed via any firmware memmap (esp., e820). The device driver will
request to plug memory from the hypervisor and add it to Linux.
On a cold start, all memory is unplugged, and the guest driver will first
request to plug memory from the hypervisor, to then add it to Linux. After
a reboot, all memory will get unplugged (except in rare, special cases). In
case the device driver comes up and detects that some memory is still
plugged after a reboot, it will manually request to unplug all memory from
the hypervisor first - to then request to plug memory from the hypervisor
and add to Linux. This is essentially a defragmentation step, where all
logical holes are removed.
As the device driver is responsible for detecting, adding and managing that
memory, also kexec should treat it like that. It is special. We need a way
to teach kexec-tools to not add that memory to the fixed-up firmware
memmap, to not place kexec images onto this memory, but still allow kdump
to dump it. Add a flag to tell memory hotplug code to
not create /sys/firmware/memmap entries and to indicate it via
"System RAM (driver managed)" in /proc/iomem.
Before this series, kexec_file_load() already did the right thing (for
virtio-mem) by not adding that memory to the fixed-up firmware memmap and
letting the device driver handle it. With this series, also kexec_load() -
which relies on user space to provide a fixed up firmware memmap - does
the right thing with virtio-mem memory.
When the virtio-mem device driver(s) come up, they will request to unplug
all memory from the hypervisor first (esp. defragment), to then request to
plug consecutive memory ranges from the hypervisor, and add them to Linux
- just like on a reboot where we still have memory plugged.
[1] https://lore.kernel.org/r/20200311171422.10484-1-david@redhat.com/
[2] https://lore.kernel.org/r/20200326180730.4754-1-james.morse@arm.com
David Hildenbrand (3):
mm/memory_hotplug: Prepare passing flags to add_memory() and friends
mm/memory_hotplug: Introduce MHP_DRIVER_MANAGED
virtio-mem: Add memory with MHP_DRIVER_MANAGED
arch/powerpc/platforms/powernv/memtrace.c | 2 +-
.../platforms/pseries/hotplug-memory.c | 2 +-
drivers/acpi/acpi_memhotplug.c | 2 +-
drivers/base/memory.c | 2 +-
drivers/dax/kmem.c | 2 +-
drivers/hv/hv_balloon.c | 2 +-
drivers/s390/char/sclp_cmd.c | 2 +-
drivers/virtio/virtio_mem.c | 3 +-
drivers/xen/balloon.c | 2 +-
include/linux/memory_hotplug.h | 15 +++++++--
mm/memory_hotplug.c | 31 +++++++++++++------
11 files changed, 44 insertions(+), 21 deletions(-)
--
2.25.3
^ permalink raw reply
* [PATCH v1 1/3] mm/memory_hotplug: Prepare passing flags to add_memory() and friends
From: David Hildenbrand @ 2020-04-29 16:08 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mm, virtio-dev, virtualization, linuxppc-dev, linux-acpi,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Andrew Morton, Michael S . Tsirkin, David Hildenbrand,
Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Dan Williams,
Vishal Verma, Dave Jiang, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Jason Wang, Boris Ostrovsky, Juergen Gross,
Stefano Stabellini, Thomas Gleixner, Pingfan Liu, Leonardo Bras,
Nathan Lynch, Oscar Salvador, Michal Hocko, Baoquan He, Wei Yang,
Pankaj Gupta, Eric Biederman
In-Reply-To: <20200429160803.109056-1-david@redhat.com>
We soon want to pass flags - prepare for that.
This patch is based on a similar patch by Oscar Salvador:
https://lkml.kernel.org/r/20190625075227.15193-3-osalvador@suse.de
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Pingfan Liu <kernelfans@gmail.com>
Cc: Leonardo Bras <leobras.c@gmail.com>
Cc: Nathan Lynch <nathanl@linux.ibm.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Cc: linux-nvdimm@lists.01.org
Cc: linux-hyperv@vger.kernel.org
Cc: linux-s390@vger.kernel.org
Cc: virtualization@lists.linux-foundation.org
Cc: xen-devel@lists.xenproject.org
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/powerpc/platforms/powernv/memtrace.c | 2 +-
arch/powerpc/platforms/pseries/hotplug-memory.c | 2 +-
drivers/acpi/acpi_memhotplug.c | 2 +-
drivers/base/memory.c | 2 +-
drivers/dax/kmem.c | 2 +-
drivers/hv/hv_balloon.c | 2 +-
drivers/s390/char/sclp_cmd.c | 2 +-
drivers/virtio/virtio_mem.c | 2 +-
drivers/xen/balloon.c | 2 +-
include/linux/memory_hotplug.h | 7 ++++---
mm/memory_hotplug.c | 11 ++++++-----
11 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
index 13b369d2cc45..a7475d18c671 100644
--- a/arch/powerpc/platforms/powernv/memtrace.c
+++ b/arch/powerpc/platforms/powernv/memtrace.c
@@ -224,7 +224,7 @@ static int memtrace_online(void)
ent->mem = 0;
}
- if (add_memory(ent->nid, ent->start, ent->size)) {
+ if (add_memory(ent->nid, ent->start, ent->size, 0)) {
pr_err("Failed to add trace memory to node %d\n",
ent->nid);
ret += 1;
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 5ace2f9a277e..ae44eba46ca0 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -646,7 +646,7 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
block_sz = memory_block_size_bytes();
/* Add the memory */
- rc = __add_memory(lmb->nid, lmb->base_addr, block_sz);
+ rc = __add_memory(lmb->nid, lmb->base_addr, block_sz, 0);
if (rc) {
invalidate_lmb_associativity_index(lmb);
return rc;
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index e294f44a7850..d91b3584d4b2 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -207,7 +207,7 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
if (node < 0)
node = memory_add_physaddr_to_nid(info->start_addr);
- result = __add_memory(node, info->start_addr, info->length);
+ result = __add_memory(node, info->start_addr, info->length, 0);
/*
* If the memory block has been used by the kernel, add_memory()
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 2b09b68b9f78..c0ef7d9e310a 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -432,7 +432,7 @@ static ssize_t probe_store(struct device *dev, struct device_attribute *attr,
nid = memory_add_physaddr_to_nid(phys_addr);
ret = __add_memory(nid, phys_addr,
- MIN_MEMORY_BLOCK_SIZE * sections_per_block);
+ MIN_MEMORY_BLOCK_SIZE * sections_per_block, 0);
if (ret)
goto out;
diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
index 3d0a7e702c94..e159184e0ba0 100644
--- a/drivers/dax/kmem.c
+++ b/drivers/dax/kmem.c
@@ -65,7 +65,7 @@ int dev_dax_kmem_probe(struct device *dev)
new_res->flags = IORESOURCE_SYSTEM_RAM;
new_res->name = dev_name(dev);
- rc = add_memory(numa_node, new_res->start, resource_size(new_res));
+ rc = add_memory(numa_node, new_res->start, resource_size(new_res), 0);
if (rc) {
release_resource(new_res);
kfree(new_res);
diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 32e3bc0aa665..0194bed1a573 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -726,7 +726,7 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
nid = memory_add_physaddr_to_nid(PFN_PHYS(start_pfn));
ret = add_memory(nid, PFN_PHYS((start_pfn)),
- (HA_CHUNK << PAGE_SHIFT));
+ (HA_CHUNK << PAGE_SHIFT), 0);
if (ret) {
pr_err("hot_add memory failed error is %d\n", ret);
diff --git a/drivers/s390/char/sclp_cmd.c b/drivers/s390/char/sclp_cmd.c
index a864b21af602..a6a908244c74 100644
--- a/drivers/s390/char/sclp_cmd.c
+++ b/drivers/s390/char/sclp_cmd.c
@@ -406,7 +406,7 @@ static void __init add_memory_merged(u16 rn)
if (!size)
goto skip_add;
for (addr = start; addr < start + size; addr += block_size)
- add_memory(0, addr, block_size);
+ add_memory(0, addr, block_size, 0);
skip_add:
first_rn = rn;
num = 1;
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 96e687531933..3101cbf9e59d 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -421,7 +421,7 @@ static int virtio_mem_mb_add(struct virtio_mem *vm, unsigned long mb_id)
nid = memory_add_physaddr_to_nid(addr);
dev_dbg(&vm->vdev->dev, "adding memory block: %lu\n", mb_id);
- return add_memory(nid, addr, memory_block_size_bytes());
+ return add_memory(nid, addr, memory_block_size_bytes(), 0);
}
/*
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 0c142bcab79d..6ec0373fa624 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -347,7 +347,7 @@ static enum bp_state reserve_additional_memory(void)
mutex_unlock(&balloon_mutex);
/* add_memory_resource() requires the device_hotplug lock */
lock_device_hotplug();
- rc = add_memory_resource(nid, resource);
+ rc = add_memory_resource(nid, resource, 0);
unlock_device_hotplug();
mutex_lock(&balloon_mutex);
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index d641828e5596..bf0e3edb8688 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -340,9 +340,10 @@ extern void set_zone_contiguous(struct zone *zone);
extern void clear_zone_contiguous(struct zone *zone);
extern void __ref free_area_init_core_hotplug(int nid);
-extern int __add_memory(int nid, u64 start, u64 size);
-extern int add_memory(int nid, u64 start, u64 size);
-extern int add_memory_resource(int nid, struct resource *resource);
+extern int __add_memory(int nid, u64 start, u64 size, unsigned long flags);
+extern int add_memory(int nid, u64 start, u64 size, unsigned long flags);
+extern int add_memory_resource(int nid, struct resource *resource,
+ unsigned long flags);
extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
unsigned long nr_pages, struct vmem_altmap *altmap);
extern void remove_pfn_range_from_zone(struct zone *zone,
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 4ac6395ee2fc..ebdf6541d074 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1000,7 +1000,8 @@ static int online_memory_block(struct memory_block *mem, void *arg)
*
* we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG
*/
-int __ref add_memory_resource(int nid, struct resource *res)
+int __ref add_memory_resource(int nid, struct resource *res,
+ unsigned long flags)
{
struct mhp_params params = { .pgprot = PAGE_KERNEL };
u64 start, size;
@@ -1078,7 +1079,7 @@ int __ref add_memory_resource(int nid, struct resource *res)
}
/* requires device_hotplug_lock, see add_memory_resource() */
-int __ref __add_memory(int nid, u64 start, u64 size)
+int __ref __add_memory(int nid, u64 start, u64 size, unsigned long flags)
{
struct resource *res;
int ret;
@@ -1087,18 +1088,18 @@ int __ref __add_memory(int nid, u64 start, u64 size)
if (IS_ERR(res))
return PTR_ERR(res);
- ret = add_memory_resource(nid, res);
+ ret = add_memory_resource(nid, res, flags);
if (ret < 0)
release_memory_resource(res);
return ret;
}
-int add_memory(int nid, u64 start, u64 size)
+int add_memory(int nid, u64 start, u64 size, unsigned long flags)
{
int rc;
lock_device_hotplug();
- rc = __add_memory(nid, start, size);
+ rc = __add_memory(nid, start, size, flags);
unlock_device_hotplug();
return rc;
--
2.25.3
^ permalink raw reply related
* [PATCH v1 3/3] virtio-mem: Add memory with MHP_DRIVER_MANAGED
From: David Hildenbrand @ 2020-04-29 16:08 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mm, virtio-dev, virtualization, linuxppc-dev, linux-acpi,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Andrew Morton, Michael S . Tsirkin, David Hildenbrand, Jason Wang,
Michal Hocko, Eric Biederman
In-Reply-To: <20200429160803.109056-1-david@redhat.com>
We don't want /sys/firmware/memmap entries and we want to indicate
our memory as "System RAM (driver managed)" in /proc/iomem. This is
especially relevant for kexec-tools, which have to be updated to
support dumping virtio-mem memory after this patch. Expected behavior in
kexec-tools:
- Don't use this memory when creating a fixed-up firmware memmap. Works
now out of the box on x86-64.
- Don't use this memory for placing kexec segments. Works now out of the
box on x86-64.
- Consider "System RAM (driver managed)" when creating the elfcorehdr
for kdump. This memory has to be dumped. Needs update of kexec-tools.
With this patch on x86-64:
/proc/iomem:
00000000-00000fff : Reserved
00001000-0009fbff : System RAM
[...]
fffc0000-ffffffff : Reserved
100000000-13fffffff : System RAM
140000000-147ffffff : System RAM (driver managed)
340000000-347ffffff : System RAM (driver managed)
348000000-34fffffff : System RAM (driver managed)
[..]
3280000000-32ffffffff : PCI Bus 0000:00
/sys/firmware/memmap:
0000000000000000-000000000009fc00 (System RAM)
000000000009fc00-00000000000a0000 (Reserved)
00000000000f0000-0000000000100000 (Reserved)
0000000000100000-00000000bffe0000 (System RAM)
00000000bffe0000-00000000c0000000 (Reserved)
00000000feffc000-00000000ff000000 (Reserved)
00000000fffc0000-0000000100000000 (Reserved)
0000000100000000-0000000140000000 (System RAM)
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
drivers/virtio/virtio_mem.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 3101cbf9e59d..6f658d1aeac4 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -421,7 +421,8 @@ static int virtio_mem_mb_add(struct virtio_mem *vm, unsigned long mb_id)
nid = memory_add_physaddr_to_nid(addr);
dev_dbg(&vm->vdev->dev, "adding memory block: %lu\n", mb_id);
- return add_memory(nid, addr, memory_block_size_bytes(), 0);
+ return add_memory(nid, addr, memory_block_size_bytes(),
+ MHP_DRIVER_MANAGED);
}
/*
--
2.25.3
^ permalink raw reply related
* [PATCH v1 2/3] mm/memory_hotplug: Introduce MHP_DRIVER_MANAGED
From: David Hildenbrand @ 2020-04-29 16:08 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mm, virtio-dev, virtualization, linuxppc-dev, linux-acpi,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Andrew Morton, Michael S . Tsirkin, David Hildenbrand,
Michal Hocko, Pankaj Gupta, Wei Yang, Baoquan He, Eric Biederman
In-Reply-To: <20200429160803.109056-1-david@redhat.com>
Some paravirtualized devices that add memory via add_memory() and
friends (esp. virtio-mem) don't want to create entries in
/sys/firmware/memmap/ - primarily to hinder kexec from adding this
memory to the boot memmap of the kexec kernel.
In fact, such memory is never exposed via the firmware (e.g., e820), but
only via the device, so exposing this memory via /sys/firmware/memmap/ is
wrong:
"kexec needs the raw firmware-provided memory map to setup the
parameter segment of the kernel that should be booted with
kexec. Also, the raw memory map is useful for debugging. For
that reason, /sys/firmware/memmap is an interface that provides
the raw memory map to userspace." [1]
We want to let user space know that memory which is always detected,
added, and managed via a (device) driver - like memory managed by
virtio-mem - is special. It cannot be used for placing kexec segments
and the (device) driver is responsible for re-adding memory that
(eventually shrunk/grown/defragmented) memory after a reboot/kexec. It
should e.g., not be added to a fixed up firmware memmap. However, it should
be dumped by kdump.
Also, such memory could behave differently than an ordinary DIMM - e.g.,
memory managed by virtio-mem can have holes inside added memory resource,
which should not be touched, especially for writing.
Let's expose that memory as "System RAM (driver managed)" e.g., via
/pro/iomem.
We don't have to worry about firmware_map_remove() on the removal path.
If there is no entry, it will simply return with -EINVAL.
[1] https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-memmap
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
include/linux/memory_hotplug.h | 8 ++++++++
mm/memory_hotplug.c | 20 ++++++++++++++++----
2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index bf0e3edb8688..cc538584b39e 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -68,6 +68,14 @@ struct mhp_params {
pgprot_t pgprot;
};
+/* Flags used for add_memory() and friends. */
+
+/*
+ * Don't create entries in /sys/firmware/memmap/ and expose memory as
+ * "System RAM (driver managed)" in e.g., /proc/iomem
+ */
+#define MHP_DRIVER_MANAGED 1
+
/*
* Zone resizing functions
*
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index ebdf6541d074..cfa0721280aa 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -98,11 +98,11 @@ void mem_hotplug_done(void)
u64 max_mem_size = U64_MAX;
/* add this memory to iomem resource */
-static struct resource *register_memory_resource(u64 start, u64 size)
+static struct resource *register_memory_resource(u64 start, u64 size,
+ const char *resource_name)
{
struct resource *res;
unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
- char *resource_name = "System RAM";
/*
* Make sure value parsed from 'mem=' only restricts memory adding
@@ -1058,7 +1058,8 @@ int __ref add_memory_resource(int nid, struct resource *res,
BUG_ON(ret);
/* create new memmap entry */
- firmware_map_add_hotplug(start, start + size, "System RAM");
+ if (!(flags & MHP_DRIVER_MANAGED))
+ firmware_map_add_hotplug(start, start + size, "System RAM");
/* device_online() will take the lock when calling online_pages() */
mem_hotplug_done();
@@ -1081,10 +1082,21 @@ int __ref add_memory_resource(int nid, struct resource *res,
/* requires device_hotplug_lock, see add_memory_resource() */
int __ref __add_memory(int nid, u64 start, u64 size, unsigned long flags)
{
+ const char *resource_name = "System RAM";
struct resource *res;
int ret;
- res = register_memory_resource(start, size);
+ /*
+ * Indicate that memory managed by a driver is special. It's always
+ * detected and added via a driver, should not be given to the kexec
+ * kernel for booting when manually crafting the firmware memmap, and
+ * no kexec segments should be placed on it. However, kdump should
+ * dump this memory.
+ */
+ if (flags & MHP_DRIVER_MANAGED)
+ resource_name = "System RAM (driver managed)";
+
+ res = register_memory_resource(start, size, resource_name);
if (IS_ERR(res))
return PTR_ERR(res);
--
2.25.3
^ permalink raw reply related
* Re: [PATCH v1 1/3] mm/memory_hotplug: Prepare passing flags to add_memory() and friends
From: Wei Liu @ 2020-04-29 16:41 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, virtio-dev, virtualization, linuxppc-dev,
linux-acpi, linux-nvdimm, linux-hyperv, linux-s390, xen-devel,
Michal Hocko, Andrew Morton, Michael S . Tsirkin,
Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
Rafael J. Wysocki, Len Brown, Greg Kroah-Hartman, Dan Williams,
Vishal Verma, Dave Jiang, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Jason Wang, Boris Ostrovsky, Juergen Gross,
Stefano Stabellini, Thomas Gleixner, Pingfan Liu, Leonardo Bras,
Nathan Lynch, Oscar Salvador, Michal Hocko, Baoquan He, Wei Yang,
Pankaj Gupta, Eric Biederman
In-Reply-To: <20200429160803.109056-2-david@redhat.com>
On Wed, Apr 29, 2020 at 06:08:01PM +0200, David Hildenbrand wrote:
> We soon want to pass flags - prepare for that.
>
> This patch is based on a similar patch by Oscar Salvador:
>
> https://lkml.kernel.org/r/20190625075227.15193-3-osalvador@suse.de
>
[...]
> ---
> drivers/hv/hv_balloon.c | 2 +-
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 32e3bc0aa665..0194bed1a573 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -726,7 +726,7 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
>
> nid = memory_add_physaddr_to_nid(PFN_PHYS(start_pfn));
> ret = add_memory(nid, PFN_PHYS((start_pfn)),
> - (HA_CHUNK << PAGE_SHIFT));
> + (HA_CHUNK << PAGE_SHIFT), 0);
>
> if (ret) {
> pr_err("hot_add memory failed error is %d\n", ret);
Acked-by: Wei Liu <wei.liu@kernel.org>
^ permalink raw reply
* Re: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
From: David Miller @ 2020-04-29 18:11 UTC (permalink / raw)
To: wei.liu
Cc: natechancellor, kys, haiyangz, sthemmin, linux-hyperv, netdev,
linux-kernel, clang-built-linux, samitolvanen
In-Reply-To: <20200429101055.rdrpchkypbkwxscj@debian>
From: Wei Liu <wei.liu@kernel.org>
Date: Wed, 29 Apr 2020 11:10:55 +0100
> Do you want this to go through net tree? I can submit it via hyperv tree
> if that's preferred.
I'll be taking this, thanks.
^ permalink raw reply
* RE: [PATCH] PCI: pci-hyperv: Retry PCI bus D0 entry when the first attempt failed with invalid device state 0xC0000184.
From: Michael Kelley @ 2020-04-29 23:47 UTC (permalink / raw)
To: Wei Hu, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, linux-hyperv@vger.kernel.org, Dexuan Cui
In-Reply-To: <SG2P153MB0213A7F57F401678A744084EBBAC0@SG2P153MB0213.APCP153.PROD.OUTLOOK.COM>
From: Wei Hu <weh@microsoft.com> Sent: Monday, April 27, 2020 11:42 PM
> > From: Michael Kelley <mikelley@microsoft.com>
> > > > The above is good. But there's another error case that isn't
> > > > handled correctly. If create_root_hv_pci_bus() fails,
> > > > hv_send_resources_released() should be called.
> > > >
> > > > Fixing these two error cases should probably go in a separate patch:
> > > > One patch for the retry problem in kdump, and a separate patch for
> > > > these error cases.
> > >
> > > [Wei Hu]
> > > hv_send_resources_released() is called in the added hv_pci_bus_exit().
> > > If hv_send_resources_allocated() fails, is it correct to call
> > hv_send_resources_released()?
> > > Allocation can fail in the middle. So I am not sure if calling
> > > hv_send_resources_released() won't cause any side effect.
> > >
> >
> > Ah yes, you are right. But that brings up a separate problem.
> > If hv_pci_allocate_bridge_windows() or hv_send_resources_allocated() fails,
> > then the error path will call hv_pci_bus_exit(), which will call
> > hv_send_resources_released(), even if hv_send_resources_allocated() was
> > never called or didn't fully succeed. As you noted,
> > hv_send_resources_allocated() does multiple steps, some of which might have
> > succeeded, and some of which didn't. The mismatch might cause problems.
> > That means fixing this error handling is going to be a bit more complex. Each
> > operation needs
> > to be individually undone, and only if it previously succeeded. Could you
> > follow up
> > with the Hyper-V people to see if there's a problem with doing the
> > RESOURCES_RELEASED message on a slot where RESOURCES_ASSIGNED was
> > not done or wasn't successful?
> > If doing a spurious RESOURCES_RELEASED is harmless, that will make the error
> > cleanup easier.
> >
>
> I think we can clean this up by doing like following, without the need of consulting
> Hyper-V team. The kdump retry also works with this by setting the wslot_res_allocated
> to 255 before calling hv_pci_bus_exit() to retry. Let me know what you think.
I like the overall approach. I've reviewed your code below, and I think it works.
For the kdump situation, the assumption is that if we get the failure in enter_d0(),
then the PCI device must have been successfully set up in the main kernel. The
occupied slots found by the kdump kernel must the same as the occupied slots
that were found (and setup) by the main kernel. Therefore it is OK to iterate
through all 256 slots in hv_send_resources_released() on the retry path. Make
sure to add a comment with that reasoning. :-)
Michael
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 0a42c228b231..06f31f5777f9 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -480,6 +480,9 @@ struct hv_pcibus_device {
>
> struct workqueue_struct *wq;
>
> + /* Highest slot of child device allocated resources allocated*/
> + int wslot_res_allocated;
> +
> /* hypercall arg, must not cross page boundary */
> struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
>
> @@ -2873,7 +2876,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
> struct hv_pci_dev *hpdev;
> struct pci_packet *pkt;
> size_t size_res;
> - u32 wslot;
> + int wslot;
> int ret;
>
> size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2)
> @@ -2926,6 +2929,8 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
> comp_pkt.completion_status);
> break;
> }
> +
> + hbus->wslot_res_allocated = wslot;
> }
>
> kfree(pkt);
> @@ -2944,10 +2949,10 @@ static int hv_send_resources_released(struct hv_device *hdev)
> struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> struct pci_child_message pkt;
> struct hv_pci_dev *hpdev;
> - u32 wslot;
> + int wslot;
> int ret;
>
> - for (wslot = 0; wslot < 256; wslot++) {
> + for (wslot = hbus->wslot_res_allocated; wslot >= 0; wslot--) {
> hpdev = get_pcichild_wslot(hbus, wslot);
> if (!hpdev)
> continue;
> @@ -2962,8 +2967,12 @@ static int hv_send_resources_released(struct hv_device *hdev)
> VM_PKT_DATA_INBAND, 0);
> if (ret)
> return ret;
> +
> + hbus->wslot_res_allocated = wslot - 1;
> }
>
> + hbus->wslot_res_allocated = -1;
> +
> return 0;
> }
>
> @@ -3063,6 +3072,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> if (!hbus)
> return -ENOMEM;
> hbus->state = hv_pcibus_init;
> + hbus->wslot_res_allocated = -1;
>
> /*
> * The PCI bus "domain" is what is called "segment" in ACPI and other
> @@ -3162,7 +3172,7 @@ static int hv_pci_probe(struct hv_device *hdev,
>
> ret = hv_pci_allocate_bridge_windows(hbus);
> if (ret)
> - goto free_irq_domain;
> + goto exit_d0;
>
> ret = hv_send_resources_allocated(hdev);
> if (ret)
> @@ -3180,6 +3190,8 @@ static int hv_pci_probe(struct hv_device *hdev,
>
> free_windows:
> hv_pci_free_bridge_windows(hbus);
> +exit_d0:
> + (void) hv_pci_bus_exit(hdev, true);
> free_irq_domain:
> irq_domain_remove(hbus->irq_domain);
> free_fwnode:
^ permalink raw reply
* RE: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
From: Michael Kelley @ 2020-04-30 0:06 UTC (permalink / raw)
To: Nathan Chancellor, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu
Cc: linux-hyperv@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com,
Sami Tolvanen
In-Reply-To: <20200428175455.2109973-1-natechancellor@gmail.com>
From: Nathan Chancellor <natechancellor@gmail.com> Sent: Tuesday, April 28, 2020 10:55 AM
>
> Do note that netvsc_xmit still returns int because netvsc_xmit has a
> potential return from netvsc_vf_xmit, which does not return netdev_tx_t
> because of the call to dev_queue_xmit.
>
> I am not sure if that is an oversight that was introduced by
> commit 0c195567a8f6e ("netvsc: transparent VF management") or if
> everything works properly as it is now.
>
> My patch is purely concerned with making the definition match the
> prototype so it should be NFC aside from avoiding the CFI panic.
>
While it probably works correctly now, I'm not too keen on just
changing the return type for netvsc_start_xmit() and assuming the
'int' that is returned from netvsc_xmit() will be correctly mapped to
the netdev_tx_t enum type. While that mapping probably happens
correctly at the moment, this really should have a more holistic fix.
Nathan -- are you willing to look at doing the more holistic fix? Or
should we see about asking Haiyang Zhang to do it?
Michael
^ permalink raw reply
* Re: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
From: Nathan Chancellor @ 2020-04-30 6:01 UTC (permalink / raw)
To: Michael Kelley
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com,
Sami Tolvanen
In-Reply-To: <MW2PR2101MB10522D4D5EBAB469FE5B4D8BD7AA0@MW2PR2101MB1052.namprd21.prod.outlook.com>
Hi Michael,
On Thu, Apr 30, 2020 at 12:06:09AM +0000, Michael Kelley wrote:
> From: Nathan Chancellor <natechancellor@gmail.com> Sent: Tuesday, April 28, 2020 10:55 AM
> >
> > Do note that netvsc_xmit still returns int because netvsc_xmit has a
> > potential return from netvsc_vf_xmit, which does not return netdev_tx_t
> > because of the call to dev_queue_xmit.
> >
> > I am not sure if that is an oversight that was introduced by
> > commit 0c195567a8f6e ("netvsc: transparent VF management") or if
> > everything works properly as it is now.
> >
> > My patch is purely concerned with making the definition match the
> > prototype so it should be NFC aside from avoiding the CFI panic.
> >
>
> While it probably works correctly now, I'm not too keen on just
> changing the return type for netvsc_start_xmit() and assuming the
> 'int' that is returned from netvsc_xmit() will be correctly mapped to
> the netdev_tx_t enum type. While that mapping probably happens
> correctly at the moment, this really should have a more holistic fix.
While it might work correctly, I am not sure that the mapping is
correct, hence that comment.
netdev_tx_t is an enum with two acceptable types, 0x00 and 0x10. Up
until commit 0c195567a8f6e ("netvsc: transparent VF management"),
netvsc_xmit was guaranteed to return something of type netdev_tx_t.
However, after said commit, we could return anything from
netvsc_vf_xmit, which in turn calls dev_queue_xmit then
__dev_queue_xmit which will return either an error code (-ENOMEM or
-ENETDOWN) or something from __dev_xmit_skb, which appears to be
NET_XMIT_SUCCESS, NET_XMIT_DROP, or NET_XMIT_CN.
It does not look like netvsc_xmit or netvsc_vf_xmit try to convert those
returns to netdev_tx_t in some way; netvsc_vf_xmit just passes the
return value up to netvsc_xmit, which is the part that I am unsure
about...
> Nathan -- are you willing to look at doing the more holistic fix? Or
> should we see about asking Haiyang Zhang to do it?
I would be fine trying to look at a more holistic fix but I know
basically nothing about this subsystem. I am unsure if something like
this would be acceptable or if something else needs to happen.
Otherwise, I'd be fine with you guys taking a look and just giving me
reported-by credit.
Cheers,
Nathan
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index d8e86bdbfba1e..a39480cfb8fa7 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -520,7 +520,8 @@ static int netvsc_vf_xmit(struct net_device *net, struct net_device *vf_netdev,
return rc;
}
-static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
+static netdev_tx_t netvsc_xmit(struct sk_buff *skb, struct net_device *net,
+ bool xdp_tx)
{
struct net_device_context *net_device_ctx = netdev_priv(net);
struct hv_netvsc_packet *packet = NULL;
@@ -537,8 +538,11 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
*/
vf_netdev = rcu_dereference_bh(net_device_ctx->vf_netdev);
if (vf_netdev && netif_running(vf_netdev) &&
- !netpoll_tx_running(net))
- return netvsc_vf_xmit(net, vf_netdev, skb);
+ !netpoll_tx_running(net)) {
+ if (!netvsc_vf_xmit(net, vf_netdev, skb))
+ return NETDEV_TX_OK;
+ goto drop;
+ }
/* We will atmost need two pages to describe the rndis
* header. We can only transmit MAX_PAGE_BUFFER_COUNT number
@@ -707,7 +711,8 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
goto drop;
}
-static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
+ struct net_device *ndev)
{
return netvsc_xmit(skb, ndev, false);
}
^ permalink raw reply related
* Re: [PATCH v1 2/3] mm/memory_hotplug: Introduce MHP_DRIVER_MANAGED
From: David Hildenbrand @ 2020-04-30 7:19 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mm, virtio-dev, virtualization, linuxppc-dev, linux-acpi,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Andrew Morton, Michael S . Tsirkin, Michal Hocko, Pankaj Gupta,
Wei Yang, Baoquan He, Eric Biederman, Dan Williams,
Pavel Tatashin, Dave Hansen
In-Reply-To: <20200429160803.109056-3-david@redhat.com>
On 29.04.20 18:08, David Hildenbrand wrote:
> Some paravirtualized devices that add memory via add_memory() and
> friends (esp. virtio-mem) don't want to create entries in
> /sys/firmware/memmap/ - primarily to hinder kexec from adding this
> memory to the boot memmap of the kexec kernel.
>
> In fact, such memory is never exposed via the firmware (e.g., e820), but
> only via the device, so exposing this memory via /sys/firmware/memmap/ is
> wrong:
> "kexec needs the raw firmware-provided memory map to setup the
> parameter segment of the kernel that should be booted with
> kexec. Also, the raw memory map is useful for debugging. For
> that reason, /sys/firmware/memmap is an interface that provides
> the raw memory map to userspace." [1]
>
> We want to let user space know that memory which is always detected,
> added, and managed via a (device) driver - like memory managed by
> virtio-mem - is special. It cannot be used for placing kexec segments
> and the (device) driver is responsible for re-adding memory that
> (eventually shrunk/grown/defragmented) memory after a reboot/kexec. It
> should e.g., not be added to a fixed up firmware memmap. However, it should
> be dumped by kdump.
>
> Also, such memory could behave differently than an ordinary DIMM - e.g.,
> memory managed by virtio-mem can have holes inside added memory resource,
> which should not be touched, especially for writing.
>
> Let's expose that memory as "System RAM (driver managed)" e.g., via
> /pro/iomem.
>
> We don't have to worry about firmware_map_remove() on the removal path.
> If there is no entry, it will simply return with -EINVAL.
>
> [1] https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-memmap
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> include/linux/memory_hotplug.h | 8 ++++++++
> mm/memory_hotplug.c | 20 ++++++++++++++++----
> 2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index bf0e3edb8688..cc538584b39e 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -68,6 +68,14 @@ struct mhp_params {
> pgprot_t pgprot;
> };
>
> +/* Flags used for add_memory() and friends. */
> +
> +/*
> + * Don't create entries in /sys/firmware/memmap/ and expose memory as
> + * "System RAM (driver managed)" in e.g., /proc/iomem
> + */
> +#define MHP_DRIVER_MANAGED 1
> +
> /*
> * Zone resizing functions
> *
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index ebdf6541d074..cfa0721280aa 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -98,11 +98,11 @@ void mem_hotplug_done(void)
> u64 max_mem_size = U64_MAX;
>
> /* add this memory to iomem resource */
> -static struct resource *register_memory_resource(u64 start, u64 size)
> +static struct resource *register_memory_resource(u64 start, u64 size,
> + const char *resource_name)
> {
> struct resource *res;
> unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
> - char *resource_name = "System RAM";
>
> /*
> * Make sure value parsed from 'mem=' only restricts memory adding
> @@ -1058,7 +1058,8 @@ int __ref add_memory_resource(int nid, struct resource *res,
> BUG_ON(ret);
>
> /* create new memmap entry */
> - firmware_map_add_hotplug(start, start + size, "System RAM");
> + if (!(flags & MHP_DRIVER_MANAGED))
> + firmware_map_add_hotplug(start, start + size, "System RAM");
>
> /* device_online() will take the lock when calling online_pages() */
> mem_hotplug_done();
> @@ -1081,10 +1082,21 @@ int __ref add_memory_resource(int nid, struct resource *res,
> /* requires device_hotplug_lock, see add_memory_resource() */
> int __ref __add_memory(int nid, u64 start, u64 size, unsigned long flags)
> {
> + const char *resource_name = "System RAM";
> struct resource *res;
> int ret;
>
> - res = register_memory_resource(start, size);
> + /*
> + * Indicate that memory managed by a driver is special. It's always
> + * detected and added via a driver, should not be given to the kexec
> + * kernel for booting when manually crafting the firmware memmap, and
> + * no kexec segments should be placed on it. However, kdump should
> + * dump this memory.
> + */
> + if (flags & MHP_DRIVER_MANAGED)
> + resource_name = "System RAM (driver managed)";
> +
> + res = register_memory_resource(start, size, resource_name);
> if (IS_ERR(res))
> return PTR_ERR(res);
>
>
BTW, I was wondering if this is actually also something that
drivers/dax/kmem.c wants to use for adding memory.
Just because we decided to use some DAX memory in the current kernel as
system ram, doesn't mean we should make that decision for the kexec
kernel (e.g., using it as initial memory, placing kexec binaries onto
it, etc.). This is also not what we would observe during a real reboot.
I can see that the "System RAM" resource will show up as child resource
under the device e.g., in /proc/iomem.
However, entries in /sys/firmware/memmap/ are created as "System RAM".
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v1 2/3] mm/memory_hotplug: Introduce MHP_DRIVER_MANAGED
From: Dan Williams @ 2020-04-30 8:11 UTC (permalink / raw)
To: David Hildenbrand
Cc: Linux Kernel Mailing List, Linux MM, virtio-dev, virtualization,
linuxppc-dev, Linux ACPI, linux-nvdimm, linux-hyperv, linux-s390,
xen-devel, Michal Hocko, Andrew Morton, Michael S . Tsirkin,
Michal Hocko, Pankaj Gupta, Wei Yang, Baoquan He, Eric Biederman,
Pavel Tatashin, Dave Hansen
In-Reply-To: <a7305cd8-8b2f-1d8f-7654-ecf777c46df6@redhat.com>
On Thu, Apr 30, 2020 at 12:20 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 29.04.20 18:08, David Hildenbrand wrote:
> > Some paravirtualized devices that add memory via add_memory() and
> > friends (esp. virtio-mem) don't want to create entries in
> > /sys/firmware/memmap/ - primarily to hinder kexec from adding this
> > memory to the boot memmap of the kexec kernel.
> >
> > In fact, such memory is never exposed via the firmware (e.g., e820), but
> > only via the device, so exposing this memory via /sys/firmware/memmap/ is
> > wrong:
> > "kexec needs the raw firmware-provided memory map to setup the
> > parameter segment of the kernel that should be booted with
> > kexec. Also, the raw memory map is useful for debugging. For
> > that reason, /sys/firmware/memmap is an interface that provides
> > the raw memory map to userspace." [1]
> >
> > We want to let user space know that memory which is always detected,
> > added, and managed via a (device) driver - like memory managed by
> > virtio-mem - is special. It cannot be used for placing kexec segments
> > and the (device) driver is responsible for re-adding memory that
> > (eventually shrunk/grown/defragmented) memory after a reboot/kexec. It
> > should e.g., not be added to a fixed up firmware memmap. However, it should
> > be dumped by kdump.
> >
> > Also, such memory could behave differently than an ordinary DIMM - e.g.,
> > memory managed by virtio-mem can have holes inside added memory resource,
> > which should not be touched, especially for writing.
> >
> > Let's expose that memory as "System RAM (driver managed)" e.g., via
> > /pro/iomem.
> >
> > We don't have to worry about firmware_map_remove() on the removal path.
> > If there is no entry, it will simply return with -EINVAL.
> >
> > [1] https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-memmap
> >
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> > Cc: Wei Yang <richard.weiyang@gmail.com>
> > Cc: Baoquan He <bhe@redhat.com>
> > Cc: Eric Biederman <ebiederm@xmission.com>
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> > include/linux/memory_hotplug.h | 8 ++++++++
> > mm/memory_hotplug.c | 20 ++++++++++++++++----
> > 2 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> > index bf0e3edb8688..cc538584b39e 100644
> > --- a/include/linux/memory_hotplug.h
> > +++ b/include/linux/memory_hotplug.h
> > @@ -68,6 +68,14 @@ struct mhp_params {
> > pgprot_t pgprot;
> > };
> >
> > +/* Flags used for add_memory() and friends. */
> > +
> > +/*
> > + * Don't create entries in /sys/firmware/memmap/ and expose memory as
> > + * "System RAM (driver managed)" in e.g., /proc/iomem
> > + */
> > +#define MHP_DRIVER_MANAGED 1
> > +
> > /*
> > * Zone resizing functions
> > *
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index ebdf6541d074..cfa0721280aa 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -98,11 +98,11 @@ void mem_hotplug_done(void)
> > u64 max_mem_size = U64_MAX;
> >
> > /* add this memory to iomem resource */
> > -static struct resource *register_memory_resource(u64 start, u64 size)
> > +static struct resource *register_memory_resource(u64 start, u64 size,
> > + const char *resource_name)
> > {
> > struct resource *res;
> > unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
> > - char *resource_name = "System RAM";
> >
> > /*
> > * Make sure value parsed from 'mem=' only restricts memory adding
> > @@ -1058,7 +1058,8 @@ int __ref add_memory_resource(int nid, struct resource *res,
> > BUG_ON(ret);
> >
> > /* create new memmap entry */
> > - firmware_map_add_hotplug(start, start + size, "System RAM");
> > + if (!(flags & MHP_DRIVER_MANAGED))
> > + firmware_map_add_hotplug(start, start + size, "System RAM");
> >
> > /* device_online() will take the lock when calling online_pages() */
> > mem_hotplug_done();
> > @@ -1081,10 +1082,21 @@ int __ref add_memory_resource(int nid, struct resource *res,
> > /* requires device_hotplug_lock, see add_memory_resource() */
> > int __ref __add_memory(int nid, u64 start, u64 size, unsigned long flags)
> > {
> > + const char *resource_name = "System RAM";
> > struct resource *res;
> > int ret;
> >
> > - res = register_memory_resource(start, size);
> > + /*
> > + * Indicate that memory managed by a driver is special. It's always
> > + * detected and added via a driver, should not be given to the kexec
> > + * kernel for booting when manually crafting the firmware memmap, and
> > + * no kexec segments should be placed on it. However, kdump should
> > + * dump this memory.
> > + */
> > + if (flags & MHP_DRIVER_MANAGED)
> > + resource_name = "System RAM (driver managed)";
> > +
> > + res = register_memory_resource(start, size, resource_name);
> > if (IS_ERR(res))
> > return PTR_ERR(res);
> >
> >
>
> BTW, I was wondering if this is actually also something that
> drivers/dax/kmem.c wants to use for adding memory.
>
> Just because we decided to use some DAX memory in the current kernel as
> system ram, doesn't mean we should make that decision for the kexec
> kernel (e.g., using it as initial memory, placing kexec binaries onto
> it, etc.). This is also not what we would observe during a real reboot.
Agree.
> I can see that the "System RAM" resource will show up as child resource
> under the device e.g., in /proc/iomem.
>
> However, entries in /sys/firmware/memmap/ are created as "System RAM".
True. Do you think this rename should just be limited to what type
/sys/firmware/memmap/ emits? I have the concern, but no proof
currently, that there are /proc/iomem walkers that explicitly look for
"System RAM", but might be thrown off by "System RAM (driver
managed)". I was not aware of /sys/firmware/memmap until about 5
minutes ago.
^ permalink raw reply
* Re: [PATCH v1 2/3] mm/memory_hotplug: Introduce MHP_DRIVER_MANAGED
From: David Hildenbrand @ 2020-04-30 8:20 UTC (permalink / raw)
To: Dan Williams
Cc: Linux Kernel Mailing List, Linux MM, virtio-dev, virtualization,
linuxppc-dev, Linux ACPI, linux-nvdimm, linux-hyperv, linux-s390,
xen-devel, Michal Hocko, Andrew Morton, Michael S . Tsirkin,
Michal Hocko, Pankaj Gupta, Wei Yang, Baoquan He, Eric Biederman,
Pavel Tatashin, Dave Hansen
In-Reply-To: <CAPcyv4i04+QLxiOyz04_eef2DFetEFKBUmi2A4xxw9abQD8hNQ@mail.gmail.com>
On 30.04.20 10:11, Dan Williams wrote:
> On Thu, Apr 30, 2020 at 12:20 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 29.04.20 18:08, David Hildenbrand wrote:
>>> Some paravirtualized devices that add memory via add_memory() and
>>> friends (esp. virtio-mem) don't want to create entries in
>>> /sys/firmware/memmap/ - primarily to hinder kexec from adding this
>>> memory to the boot memmap of the kexec kernel.
>>>
>>> In fact, such memory is never exposed via the firmware (e.g., e820), but
>>> only via the device, so exposing this memory via /sys/firmware/memmap/ is
>>> wrong:
>>> "kexec needs the raw firmware-provided memory map to setup the
>>> parameter segment of the kernel that should be booted with
>>> kexec. Also, the raw memory map is useful for debugging. For
>>> that reason, /sys/firmware/memmap is an interface that provides
>>> the raw memory map to userspace." [1]
>>>
>>> We want to let user space know that memory which is always detected,
>>> added, and managed via a (device) driver - like memory managed by
>>> virtio-mem - is special. It cannot be used for placing kexec segments
>>> and the (device) driver is responsible for re-adding memory that
>>> (eventually shrunk/grown/defragmented) memory after a reboot/kexec. It
>>> should e.g., not be added to a fixed up firmware memmap. However, it should
>>> be dumped by kdump.
>>>
>>> Also, such memory could behave differently than an ordinary DIMM - e.g.,
>>> memory managed by virtio-mem can have holes inside added memory resource,
>>> which should not be touched, especially for writing.
>>>
>>> Let's expose that memory as "System RAM (driver managed)" e.g., via
>>> /pro/iomem.
>>>
>>> We don't have to worry about firmware_map_remove() on the removal path.
>>> If there is no entry, it will simply return with -EINVAL.
>>>
>>> [1] https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-memmap
>>>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
>>> Cc: Wei Yang <richard.weiyang@gmail.com>
>>> Cc: Baoquan He <bhe@redhat.com>
>>> Cc: Eric Biederman <ebiederm@xmission.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> include/linux/memory_hotplug.h | 8 ++++++++
>>> mm/memory_hotplug.c | 20 ++++++++++++++++----
>>> 2 files changed, 24 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>>> index bf0e3edb8688..cc538584b39e 100644
>>> --- a/include/linux/memory_hotplug.h
>>> +++ b/include/linux/memory_hotplug.h
>>> @@ -68,6 +68,14 @@ struct mhp_params {
>>> pgprot_t pgprot;
>>> };
>>>
>>> +/* Flags used for add_memory() and friends. */
>>> +
>>> +/*
>>> + * Don't create entries in /sys/firmware/memmap/ and expose memory as
>>> + * "System RAM (driver managed)" in e.g., /proc/iomem
>>> + */
>>> +#define MHP_DRIVER_MANAGED 1
>>> +
>>> /*
>>> * Zone resizing functions
>>> *
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index ebdf6541d074..cfa0721280aa 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -98,11 +98,11 @@ void mem_hotplug_done(void)
>>> u64 max_mem_size = U64_MAX;
>>>
>>> /* add this memory to iomem resource */
>>> -static struct resource *register_memory_resource(u64 start, u64 size)
>>> +static struct resource *register_memory_resource(u64 start, u64 size,
>>> + const char *resource_name)
>>> {
>>> struct resource *res;
>>> unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
>>> - char *resource_name = "System RAM";
>>>
>>> /*
>>> * Make sure value parsed from 'mem=' only restricts memory adding
>>> @@ -1058,7 +1058,8 @@ int __ref add_memory_resource(int nid, struct resource *res,
>>> BUG_ON(ret);
>>>
>>> /* create new memmap entry */
>>> - firmware_map_add_hotplug(start, start + size, "System RAM");
>>> + if (!(flags & MHP_DRIVER_MANAGED))
>>> + firmware_map_add_hotplug(start, start + size, "System RAM");
>>>
>>> /* device_online() will take the lock when calling online_pages() */
>>> mem_hotplug_done();
>>> @@ -1081,10 +1082,21 @@ int __ref add_memory_resource(int nid, struct resource *res,
>>> /* requires device_hotplug_lock, see add_memory_resource() */
>>> int __ref __add_memory(int nid, u64 start, u64 size, unsigned long flags)
>>> {
>>> + const char *resource_name = "System RAM";
>>> struct resource *res;
>>> int ret;
>>>
>>> - res = register_memory_resource(start, size);
>>> + /*
>>> + * Indicate that memory managed by a driver is special. It's always
>>> + * detected and added via a driver, should not be given to the kexec
>>> + * kernel for booting when manually crafting the firmware memmap, and
>>> + * no kexec segments should be placed on it. However, kdump should
>>> + * dump this memory.
>>> + */
>>> + if (flags & MHP_DRIVER_MANAGED)
>>> + resource_name = "System RAM (driver managed)";
>>> +
>>> + res = register_memory_resource(start, size, resource_name);
>>> if (IS_ERR(res))
>>> return PTR_ERR(res);
>>>
>>>
>>
>> BTW, I was wondering if this is actually also something that
>> drivers/dax/kmem.c wants to use for adding memory.
>>
>> Just because we decided to use some DAX memory in the current kernel as
>> system ram, doesn't mean we should make that decision for the kexec
>> kernel (e.g., using it as initial memory, placing kexec binaries onto
>> it, etc.). This is also not what we would observe during a real reboot.
>
> Agree.
>
>> I can see that the "System RAM" resource will show up as child resource
>> under the device e.g., in /proc/iomem.
>>
>> However, entries in /sys/firmware/memmap/ are created as "System RAM".
>
> True. Do you think this rename should just be limited to what type
> /sys/firmware/memmap/ emits? I have the concern, but no proof
We could split this patch into
MHP_NO_FIRMWARE_MEMMAP (create firmware memmap entries)
and
MHP_DRIVER_MANAGED (name of the resource)
See below, the latter might not be needed.
> currently, that there are /proc/iomem walkers that explicitly look for
> "System RAM", but might be thrown off by "System RAM (driver
> managed)". I was not aware of /sys/firmware/memmap until about 5
> minutes ago.
The only two users of /proc/iomem I am aware of are kexec-tools and some
s390x tools.
kexec-tools on x86-64 uses /sys/firmware/memmap to craft the initial
memmap, but uses /proc/iomem to
a) Find places for kexec images
b) Detect memory regions to dump via kdump
I am not yet sure if we really need the "System RAM (driver managed)"
part. If we can teach kexec-tools to
a) Don't place kexec images on "System RAM" that has a parent resource
(most likely requires kexec-tools changes)
b) Consider for kdump "System RAM" that has a parent resource
we might be able to avoid renaming that. (I assume that's already done)
E.g., regarding virtio-mem (patch #3) I am currently also looking into
creating a parent resource instead, like dax/kmem to avoid the rename:
:/# cat /proc/iomem
00000000-00000fff : Reserved
[...]
100000000-13fffffff : System RAM
140000000-33fffffff : virtio0
140000000-147ffffff : System RAM
148000000-14fffffff : System RAM
150000000-157ffffff : System RAM
340000000-303fffffff : virtio1
340000000-347ffffff : System RAM
3280000000-32ffffffff : PCI Bus 0000:00
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v1 2/3] mm/memory_hotplug: Introduce MHP_DRIVER_MANAGED
From: Dan Williams @ 2020-04-30 8:34 UTC (permalink / raw)
To: David Hildenbrand
Cc: Linux Kernel Mailing List, Linux MM, virtio-dev, virtualization,
linuxppc-dev, Linux ACPI, linux-nvdimm, linux-hyperv, linux-s390,
xen-devel, Michal Hocko, Andrew Morton, Michael S . Tsirkin,
Michal Hocko, Pankaj Gupta, Wei Yang, Baoquan He, Eric Biederman,
Pavel Tatashin, Dave Hansen
In-Reply-To: <e32522cd-31bb-e129-47a6-9ec13b570506@redhat.com>
On Thu, Apr 30, 2020 at 1:21 AM David Hildenbrand <david@redhat.com> wrote:
> >> Just because we decided to use some DAX memory in the current kernel as
> >> system ram, doesn't mean we should make that decision for the kexec
> >> kernel (e.g., using it as initial memory, placing kexec binaries onto
> >> it, etc.). This is also not what we would observe during a real reboot.
> >
> > Agree.
> >
> >> I can see that the "System RAM" resource will show up as child resource
> >> under the device e.g., in /proc/iomem.
> >>
> >> However, entries in /sys/firmware/memmap/ are created as "System RAM".
> >
> > True. Do you think this rename should just be limited to what type
> > /sys/firmware/memmap/ emits? I have the concern, but no proof
>
> We could split this patch into
>
> MHP_NO_FIRMWARE_MEMMAP (create firmware memmap entries)
>
> and
>
> MHP_DRIVER_MANAGED (name of the resource)
>
> See below, the latter might not be needed.
>
> > currently, that there are /proc/iomem walkers that explicitly look for
> > "System RAM", but might be thrown off by "System RAM (driver
> > managed)". I was not aware of /sys/firmware/memmap until about 5
> > minutes ago.
>
> The only two users of /proc/iomem I am aware of are kexec-tools and some
> s390x tools.
>
> kexec-tools on x86-64 uses /sys/firmware/memmap to craft the initial
> memmap, but uses /proc/iomem to
> a) Find places for kexec images
> b) Detect memory regions to dump via kdump
>
> I am not yet sure if we really need the "System RAM (driver managed)"
> part. If we can teach kexec-tools to
> a) Don't place kexec images on "System RAM" that has a parent resource
> (most likely requires kexec-tools changes)
> b) Consider for kdump "System RAM" that has a parent resource
> we might be able to avoid renaming that. (I assume that's already done)
>
> E.g., regarding virtio-mem (patch #3) I am currently also looking into
> creating a parent resource instead, like dax/kmem to avoid the rename:
>
> :/# cat /proc/iomem
> 00000000-00000fff : Reserved
> [...]
> 100000000-13fffffff : System RAM
> 140000000-33fffffff : virtio0
> 140000000-147ffffff : System RAM
> 148000000-14fffffff : System RAM
> 150000000-157ffffff : System RAM
> 340000000-303fffffff : virtio1
> 340000000-347ffffff : System RAM
> 3280000000-32ffffffff : PCI Bus 0000:00
Looks good to me if it flies with kexec-tools.
^ permalink raw reply
* [PATCH v2 0/3] mm/memory_hotplug: Allow to not create firmware memmap entries
From: David Hildenbrand @ 2020-04-30 10:29 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mm, virtio-dev, virtualization, linuxppc-dev, linux-acpi,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Andrew Morton, Michael S . Tsirkin, David Hildenbrand, Baoquan He,
Benjamin Herrenschmidt, Boris Ostrovsky, Christian Borntraeger,
Dan Williams, Dave Hansen, Dave Jiang, Eric Biederman,
Greg Kroah-Hartman, Haiyang Zhang, Heiko Carstens, Jason Wang,
Juergen Gross, K. Y. Srinivasan, Len Brown, Leonardo Bras,
Michael Ellerman, Michal Hocko, Nathan Lynch, Oscar Salvador,
Pankaj Gupta, Paul Mackerras, Pavel Tatashin, Pingfan Liu,
Rafael J. Wysocki, Stefano Stabellini, Stephen Hemminger,
Thomas Gleixner, Vasily Gorbik, Vishal Verma, Wei Liu, Wei Yang
This is the follow up of [1]:
[PATCH v1 0/3] mm/memory_hotplug: Make virtio-mem play nicely with
kexec-tools
I realized that this is not only helpful for virtio-mem, but also for
dax/kmem - it's a fix for that use case (see patch #3) of persistent
memory.
Also, while testing, I discovered that kexec-tools will *not* add dax/kmem
memory (anything not directly under the root when parsing /proc/iomem) to
the elfcorehdr, so this memory will never get included in a dump. This
probably has to be fixed in kexec-tools - virtio-mem will require this as
well.
v1 -> v2:
- Don't change the resource name
- Rename the flag to MHP_NO_FIRMWARE_MEMMAP to reflect what it is doing
- Rephrase subjects/descriptions
- Use the flag for dax/kmem
I'll have to rebase virtio-mem on these changes, there will be a resend.
[1] https://lkml.kernel.org/r/20200429160803.109056-1-david@redhat.com
David Hildenbrand (3):
mm/memory_hotplug: Prepare passing flags to add_memory() and friends
mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
device-dax: Add system ram (add_memory()) with MHP_NO_FIRMWARE_MEMMAP
arch/powerpc/platforms/powernv/memtrace.c | 2 +-
arch/powerpc/platforms/pseries/hotplug-memory.c | 2 +-
drivers/acpi/acpi_memhotplug.c | 2 +-
drivers/base/memory.c | 2 +-
drivers/dax/kmem.c | 3 ++-
drivers/hv/hv_balloon.c | 2 +-
drivers/s390/char/sclp_cmd.c | 2 +-
drivers/xen/balloon.c | 2 +-
include/linux/memory_hotplug.h | 15 ++++++++++++---
mm/memory_hotplug.c | 14 ++++++++------
10 files changed, 29 insertions(+), 17 deletions(-)
--
2.25.3
^ 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