* Re: [PATCH v2 2/2] PCI: hv: Retry PCI bus D0 entry when the first attempt failed with invalid device state
From: Lorenzo Pieralisi @ 2020-05-05 15:09 UTC (permalink / raw)
To: Wei Hu
Cc: kys, haiyangz, sthemmin, wei.liu, robh, bhelgaas, linux-hyperv,
linux-pci, linux-kernel, decui, mikelley
In-Reply-To: <20200501053728.24740-1-weh@microsoft.com>
On Fri, May 01, 2020 at 01:37:28PM +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 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 | 40 +++++++++++++++++++++++++++--
> 1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index e6fac0187722..92092a47d3af 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -2739,6 +2739,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
> @@ -2751,8 +2753,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
> @@ -2779,6 +2783,38 @@ 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");
> +
> + /*
> + * Hv_pci_bus_exit() calls hv_send_resource_released()
> + * to free up resources of its child devices.
> + * In the kdump kernel we need to set the
> + * wslot_res_allocated to 255 so it scans all child
> + * devices to release resources allocated in the
> + * normal kernel before panic happened.
> + */
> + hbus->wslot_res_allocated = 255;
I'd rather write a specific function eg hv_pci_bus_shutdown() to
make it explicit. Actually, isn't it something that should *always*
be _enforced_ at host bridge probe time - regardless of the kernel
you are booting on ?
Lorenzo
> +
> + 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",
> @@ -3185,7 +3221,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 {
> @@ -3203,7 +3239,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
* Re: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to release resource properly
From: Lorenzo Pieralisi @ 2020-05-05 15:03 UTC (permalink / raw)
To: Wei Hu
Cc: kys, haiyangz, sthemmin, wei.liu, robh, bhelgaas, linux-hyperv,
linux-pci, linux-kernel, decui, mikelley
In-Reply-To: <20200501053617.24689-1-weh@microsoft.com>
On Fri, May 01, 2020 at 01:36:17PM +0800, Wei Hu wrote:
> Some error cases in hv_pci_probe() were not handled. Fix these error
> paths to release the resourses and clean up the state properly.
This patch does more than that. It adds a variable to store the
number of slots actually allocated - I presume to free only
allocated on slots on the exit path.
Two patches required I am afraid.
> Signed-off-by: Wei Hu <weh@microsoft.com>
> ---
> drivers/pci/controller/pci-hyperv.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index e15022ff63e3..e6fac0187722 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 with resources allocated */
> + int wslot_res_allocated;
I don't understand why you need an int rather than a u32.
Furthermore, I think a bitmap is more appropriate for what this
variable is used for.
> +
> /* hypercall arg, must not cross page boundary */
> struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
>
> @@ -2847,7 +2850,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)
> @@ -2900,6 +2903,8 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
> comp_pkt.completion_status);
> break;
> }
> +
> + hbus->wslot_res_allocated = wslot;
> }
>
> kfree(pkt);
> @@ -2918,10 +2923,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;
> @@ -2936,8 +2941,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;
Do you really need to set it at every loop iteration ?
Moreover, I think a bitmap is better suited for what you are doing,
given that you may skip some loop indexes on !hpdev.
> }
>
> + hbus->wslot_res_allocated = -1;
> +
> return 0;
> }
>
> @@ -3037,6 +3046,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
> @@ -3136,7 +3146,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 +3164,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);
Remove the (void) cast.
Lorenzo
> free_irq_domain:
> irq_domain_remove(hbus->irq_domain);
> free_fwnode:
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH v2 0/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Jon Doron @ 2020-05-05 10:38 UTC (permalink / raw)
To: Roman Kagan, Vitaly Kuznetsov, kvm, linux-hyperv
In-Reply-To: <20200505080158.GA400685@rvkaganb>
On 05/05/2020, Roman Kagan wrote:
>On Mon, May 04, 2020 at 05:55:10PM +0200, Vitaly Kuznetsov wrote:
>> Roman Kagan <rvkagan@yandex-team.ru> writes:
>>
>> > On Sat, Apr 25, 2020 at 09:16:37AM +0300, Jon Doron wrote:
>> >
>> >> If that's indeed the case then probably the only thing needs fixing in my
>> >> scenario is in QEMU where it should not really care for the SCONTROL if it's
>> >> enabled or not.
>> >
>> > Right. However, even this shouldn't be necessary as SeaBIOS from that
>> > branch would enable SCONTROL and leave it that way when passing the
>> > control over to the bootloader, so, unless something explicitly clears
>> > SCONTROL, it should remain set thereafter. I'd rather try going ahead
>> > with that scheme first, because making QEMU ignore SCONTROL appears to
>> > violate the spec.
>>
>> FWIW, I just checked 'genuine' Hyper-V 2016 with
>>
>> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> index fd51bac11b46..c5ea759728d9 100644
>> --- a/arch/x86/hyperv/hv_init.c
>> +++ b/arch/x86/hyperv/hv_init.c
>> @@ -314,10 +314,14 @@ void __init hyperv_init(void)
>> u64 guest_id, required_msrs;
>> union hv_x64_msr_hypercall_contents hypercall_msr;
>> int cpuhp, i;
>> + u64 val;
>>
>> if (x86_hyper_type != X86_HYPER_MS_HYPERV)
>> return;
>>
>> + hv_get_synic_state(val);
>> + printk("Hyper-V: SCONTROL state: %llx\n", val);
>> +
>> /* Absolutely required MSRs */
>> required_msrs = HV_X64_MSR_HYPERCALL_AVAILABLE |
>> HV_X64_MSR_VP_INDEX_AVAILABLE;
>
>Thanks for having done this check!
>
>> and it seems the default state of HV_X64_MSR_SCONTROL is '1', we should
>> probably do the same.
>
>This is the state the OS sees, after the firmware. You'd see the same
>with QEMU/KVM if you used Hyper-V-aware SeaBIOS or OVMF.
>
>> Is there any reason to *not* do this in KVM when
>> KVM_CAP_HYPERV_SYNIC[,2] is enabled?
>
>Yes there is: quoting Hyper-V TLFS v6.0 11.8.1:
>
> At virtual processor creation time and upon processor reset, the value
> of this SCONTROL (SynIC control register) is 0x0000000000000000. Thus,
> message queuing and event flag notifications will be disabled.
>
>And, even if we decide to violate the spec it's better done in
>userspace, loading the initial value and adjusting the synic state at
>vcpu reset.
>
>However leaving it up to the guest (firmware or OS) looks more natural
>to me.
>
>Thanks,
>Roman.
I under where you are coming from in the idea of leaving it to the OS
but I think in this specific case it does not make much sense, after all
HyperV has it's own proprietary BIOS which Windows assumes has setup
some of the MSRs, since we dont have that BIOS we need to "emulate" it's
behaviour.
I also feel like the best approach should be in QEMU in case VMBus
device exists it will also setup the SCONTROL to ENABLED, this way you
are not bound to have a special BIOS in case you have decided to use
HyperV advanced features like VMBus.
Cheers,
-- Jon.
^ permalink raw reply
* Re: [PATCH v2 0/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Roman Kagan @ 2020-05-05 8:01 UTC (permalink / raw)
To: Vitaly Kuznetsov; +Cc: Jon Doron, kvm, linux-hyperv
In-Reply-To: <87a72nelup.fsf@vitty.brq.redhat.com>
On Mon, May 04, 2020 at 05:55:10PM +0200, Vitaly Kuznetsov wrote:
> Roman Kagan <rvkagan@yandex-team.ru> writes:
>
> > On Sat, Apr 25, 2020 at 09:16:37AM +0300, Jon Doron wrote:
> >
> >> If that's indeed the case then probably the only thing needs fixing in my
> >> scenario is in QEMU where it should not really care for the SCONTROL if it's
> >> enabled or not.
> >
> > Right. However, even this shouldn't be necessary as SeaBIOS from that
> > branch would enable SCONTROL and leave it that way when passing the
> > control over to the bootloader, so, unless something explicitly clears
> > SCONTROL, it should remain set thereafter. I'd rather try going ahead
> > with that scheme first, because making QEMU ignore SCONTROL appears to
> > violate the spec.
>
> FWIW, I just checked 'genuine' Hyper-V 2016 with
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index fd51bac11b46..c5ea759728d9 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -314,10 +314,14 @@ void __init hyperv_init(void)
> u64 guest_id, required_msrs;
> union hv_x64_msr_hypercall_contents hypercall_msr;
> int cpuhp, i;
> + u64 val;
>
> if (x86_hyper_type != X86_HYPER_MS_HYPERV)
> return;
>
> + hv_get_synic_state(val);
> + printk("Hyper-V: SCONTROL state: %llx\n", val);
> +
> /* Absolutely required MSRs */
> required_msrs = HV_X64_MSR_HYPERCALL_AVAILABLE |
> HV_X64_MSR_VP_INDEX_AVAILABLE;
Thanks for having done this check!
> and it seems the default state of HV_X64_MSR_SCONTROL is '1', we should
> probably do the same.
This is the state the OS sees, after the firmware. You'd see the same
with QEMU/KVM if you used Hyper-V-aware SeaBIOS or OVMF.
> Is there any reason to *not* do this in KVM when
> KVM_CAP_HYPERV_SYNIC[,2] is enabled?
Yes there is: quoting Hyper-V TLFS v6.0 11.8.1:
At virtual processor creation time and upon processor reset, the value
of this SCONTROL (SynIC control register) is 0x0000000000000000. Thus,
message queuing and event flag notifications will be disabled.
And, even if we decide to violate the spec it's better done in
userspace, loading the initial value and adjusting the synic state at
vcpu reset.
However leaving it up to the guest (firmware or OS) looks more natural
to me.
Thanks,
Roman.
^ permalink raw reply
* RE: [PATCH v2 2/2] PCI: hv: Retry PCI bus D0 entry when the first attempt failed with invalid device state
From: Michael Kelley @ 2020-05-04 23:45 UTC (permalink / raw)
To: Wei Hu, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, lorenzo.pieralisi@arm.com, robh@kernel.org,
bhelgaas@google.com, linux-hyperv@vger.kernel.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
Dexuan Cui
In-Reply-To: <20200501053728.24740-1-weh@microsoft.com>
From: Wei Hu <weh@microsoft.com> Sent: Thursday, April 30, 2020 10:37 PM
>
> 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 | 40 +++++++++++++++++++++++++++--
> 1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index e6fac0187722..92092a47d3af 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -2739,6 +2739,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
> @@ -2751,8 +2753,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
> @@ -2779,6 +2783,38 @@ 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");
> +
> + /*
> + * Hv_pci_bus_exit() calls hv_send_resource_released()
> + * to free up resources of its child devices.
> + * In the kdump kernel we need to set the
> + * wslot_res_allocated to 255 so it scans all child
> + * devices to release resources allocated in the
> + * normal kernel before panic happened.
> + */
> + hbus->wslot_res_allocated = 255;
> +
> + 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",
> @@ -3185,7 +3221,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 {
> @@ -3203,7 +3239,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
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* RE: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to release resource properly
From: Michael Kelley @ 2020-05-04 23:42 UTC (permalink / raw)
To: Wei Hu, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, lorenzo.pieralisi@arm.com, robh@kernel.org,
bhelgaas@google.com, linux-hyperv@vger.kernel.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
Dexuan Cui
In-Reply-To: <20200501053617.24689-1-weh@microsoft.com>
From: Wei Hu <weh@microsoft.com> Sent: Thursday, April 30, 2020 10:36 PM
>
> Some error cases in hv_pci_probe() were not handled. Fix these error
> paths to release the resourses and clean up the state properly.
>
> Signed-off-by: Wei Hu <weh@microsoft.com>
> ---
> drivers/pci/controller/pci-hyperv.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index e15022ff63e3..e6fac0187722 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 with resources allocated */
> + int wslot_res_allocated;
> +
> /* hypercall arg, must not cross page boundary */
> struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
>
> @@ -2847,7 +2850,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)
> @@ -2900,6 +2903,8 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
> comp_pkt.completion_status);
> break;
> }
> +
> + hbus->wslot_res_allocated = wslot;
> }
>
> kfree(pkt);
> @@ -2918,10 +2923,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;
> @@ -2936,8 +2941,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;
> }
>
> @@ -3037,6 +3046,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
> @@ -3136,7 +3146,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 +3164,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
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* Re: [PATCH v2 0/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Vitaly Kuznetsov @ 2020-05-04 15:55 UTC (permalink / raw)
To: Roman Kagan, Jon Doron; +Cc: kvm, linux-hyperv
In-Reply-To: <20200503191900.GA389956@rvkaganb>
Roman Kagan <rvkagan@yandex-team.ru> writes:
> On Sat, Apr 25, 2020 at 09:16:37AM +0300, Jon Doron wrote:
>
>> If that's indeed the case then probably the only thing needs fixing in my
>> scenario is in QEMU where it should not really care for the SCONTROL if it's
>> enabled or not.
>
> Right. However, even this shouldn't be necessary as SeaBIOS from that
> branch would enable SCONTROL and leave it that way when passing the
> control over to the bootloader, so, unless something explicitly clears
> SCONTROL, it should remain set thereafter. I'd rather try going ahead
> with that scheme first, because making QEMU ignore SCONTROL appears to
> violate the spec.
FWIW, I just checked 'genuine' Hyper-V 2016 with
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index fd51bac11b46..c5ea759728d9 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -314,10 +314,14 @@ void __init hyperv_init(void)
u64 guest_id, required_msrs;
union hv_x64_msr_hypercall_contents hypercall_msr;
int cpuhp, i;
+ u64 val;
if (x86_hyper_type != X86_HYPER_MS_HYPERV)
return;
+ hv_get_synic_state(val);
+ printk("Hyper-V: SCONTROL state: %llx\n", val);
+
/* Absolutely required MSRs */
required_msrs = HV_X64_MSR_HYPERCALL_AVAILABLE |
HV_X64_MSR_VP_INDEX_AVAILABLE;
and it seems the default state of HV_X64_MSR_SCONTROL is '1', we should
probably do the same. Is there any reason to *not* do this in KVM when
KVM_CAP_HYPERV_SYNIC[,2] is enabled?
--
Vitaly
^ permalink raw reply related
* Re: [PATCH v2 0/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Roman Kagan @ 2020-05-03 19:19 UTC (permalink / raw)
To: Jon Doron; +Cc: kvm, linux-hyperv, vkuznets
In-Reply-To: <20200425061637.GF1917435@jondnuc>
On Sat, Apr 25, 2020 at 09:16:37AM +0300, Jon Doron wrote:
> On 24/04/2020, Roman Kagan wrote:
> > On Sat, Apr 18, 2020 at 09:41:27AM +0300, Jon Doron wrote:
> > > On 17/04/2020, Roman Kagan wrote:
> > > > On Thu, Apr 16, 2020 at 03:54:30PM +0300, Jon Doron wrote:
> > > > > On 16/04/2020, Roman Kagan wrote:
> > > > > > On Thu, Apr 16, 2020 at 11:38:46AM +0300, Jon Doron wrote:
> > > > > > > According to the TLFS:
> > > > > > > "A write to the end of message (EOM) register by the guest causes the
> > > > > > > hypervisor to scan the internal message buffer queue(s) associated with
> > > > > > > the virtual processor.
> > > > > > >
> > > > > > > If a message buffer queue contains a queued message buffer, the hypervisor
> > > > > > > attempts to deliver the message.
> > > > > > >
> > > > > > > Message delivery succeeds if the SIM page is enabled and the message slot
> > > > > > > corresponding to the SINTx is empty (that is, the message type in the
> > > > > > > header is set to HvMessageTypeNone).
> > > > > > > If a message is successfully delivered, its corresponding internal message
> > > > > > > buffer is dequeued and marked free.
> > > > > > > If the corresponding SINTx is not masked, an edge-triggered interrupt is
> > > > > > > delivered (that is, the corresponding bit in the IRR is set).
> > > > > > >
> > > > > > > This register can be used by guests to poll for messages. It can also be
> > > > > > > used as a way to drain the message queue for a SINTx that has
> > > > > > > been disabled (that is, masked)."
> > > > > >
> > > > > > Doesn't this work already?
> > > > > >
> > > > >
> > > > > Well if you dont have SCONTROL and a GSI associated with the SINT then it
> > > > > does not...
> > > >
> > > > Yes you do need both of these.
> > > >
> > > > > > > So basically this means that we need to exit on EOM so the hypervisor
> > > > > > > will have a chance to send all the pending messages regardless of the
> > > > > > > SCONTROL mechnaisim.
> > > > > >
> > > > > > I might be misinterpreting the spec, but my understanding is that
> > > > > > SCONTROL {en,dis}ables the message queueing completely. What the quoted
> > > > > > part means is that a write to EOM should trigger the message source to
> > > > > > push a new message into the slot, regardless of whether the SINT was
> > > > > > masked or not.
> > > > > >
> > > > > > And this (I think, haven't tested) should already work. The userspace
> > > > > > just keeps using the SINT route as it normally does, posting
> > > > > > notifications to the corresponding irqfd when posting a message, and
> > > > > > waiting on the resamplerfd for the message slot to become free. If the
> > > > > > SINT is masked KVM will skip injecting the interrupt, that's it.
> > > > > >
> > > > > > Roman.
> > > > >
> > > > > That's what I was thinking originally as well, but then i noticed KDNET as a
> > > > > VMBus client (and it basically runs before anything else) is working in this
> > > > > polling mode, where SCONTROL is disabled and it just loops, and if it saw
> > > > > there is a PENDING message flag it will issue an EOM to indicate it has free
> > > > > the slot.
> > > >
> > > > Who sets up the message page then? Doesn't it enabe SCONTROL as well?
> > > >
> > >
> > > KdNet is the one setting the SIMP and it's not setting the SCONTROL, ill
> > > paste output of KVM traces for the relevant MSRs
> > >
> > > > Note that, even if you don't see it being enabled by Windows, it can be
> > > > enabled by the firmware and/or by the bootloader.
> > > >
> > > > Can you perhaps try with the SeaBIOS from
> > > > https://src.openvz.org/projects/UP/repos/seabios branch hv-scsi? It
> > > > enables SCONTROL and leaves it that way.
> > > >
> > > > I'd also suggest tracing kvm_msr events (both reads and writes) for
> > > > SCONTROL and SIMP msrs, to better understand the picture.
> > > >
> > > > So far the change you propose appears too heavy to work around the
> > > > problem of disabled SCONTROL. You seem to be better off just making
> > > > sure it's enabled (either by the firmware or slighly violating the spec
> > > > and initializing to enabled from the start), and sticking to the
> > > > existing infrastructure for posting messages.
> > > >
> > >
> > > I guess there is something I'm missing here but let's say the BIOS would
> > > have set the SCONTROL but the OS is not setting it, who is in charge of
> > > handling the interrupts?
> >
> > SCONTROL doesn't enable the interrupts, it enables SynIC as a whole.
> > The interrupts are enabled via individual SINTx msrs. This SeaBIOS
> > branch does exactly this: it enables the SynIC via SCONTROL, and then
> > specific SynIC functionality via SIMP/SIEFP, but doesn't activate SINTx
> > and works in polling mode.
> >
> > I agree that this global SCONTROL switch seems redundant but it appears
> > to match the spec.
> >
> > > > > (There are a bunch of patches i sent on the QEMU mailing list as well where
> > > > > i CCed you, I will probably revise it a bit but was hoping to get KVM
> > > > > sorted out first).
> > > >
> > > > I'll look through the archive, should be there, thanks.
> > > >
> > > > Roman.
> > >
> > > I tried testing with both the SeaBIOS branch you have suggested and the
> > > EDK2, unfortunately I could not get the EDK2 build to identify my VM drive
> > > to boot from (not sure why)
> > >
> > > Here is an output of KVM trace for the relevant MSRs (SCONTROL and SIMP)
> > >
> > > QEMU Default BIOS
> > > -----------------
> > > qemu-system-x86-613 [000] .... 1121.080722: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000080 data 0x0 host 1
> > > qemu-system-x86-613 [000] .... 1121.080722: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0x0 host 1
> > > qemu-system-x86-613 [000] .N.. 1121.095592: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000080 data 0x0 host 1
> > > qemu-system-x86-613 [000] .N.. 1121.095592: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0x0 host 1
> > > Choose Windows DebugEntry
> > > qemu-system-x86-613 [001] .... 1165.185227: kvm_msr: msr_read 40000083 = 0x0
> > > qemu-system-x86-613 [001] .... 1165.185255: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0xfa1001 host 0
> > > qemu-system-x86-613 [001] .... 1165.185255: kvm_msr: msr_write 40000083 = 0xfa1001
> > > qemu-system-x86-613 [001] .... 1165.193206: kvm_msr: msr_read 40000083 = 0xfa1001
> > > qemu-system-x86-613 [001] .... 1165.193236: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0xfa1000 host 0
> > > qemu-system-x86-613 [001] .... 1165.193237: kvm_msr: msr_write 40000083 = 0xfa1000
> > >
> > >
> > > SeaBIOS hv-scsci
> > > ----------------
> > > qemu-system-x86-656 [001] .... 1313.072714: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000080 data 0x0 host 1
> > > qemu-system-x86-656 [001] .... 1313.072714: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0x0 host 1
> > > qemu-system-x86-656 [001] .... 1313.087752: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000080 data 0x0 host 1
> > > qemu-system-x86-656 [001] .... 1313.087752: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0x0 host 1
> >
> > Initialization (host == 1)
> >
> > > qemu-system-x86-656 [001] .... 1313.156675: kvm_msr: msr_read 40000083 = 0x0
> > > qemu-system-x86-656 [001] .... 1313.156680: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0x7fffe001 host 0
> > > Choose Windows DebugEntry
> >
> > I guess this is a bit misplaced timewise, BIOS is still working here
> >
> > > qemu-system-x86-656 [001] .... 1313.156680: kvm_msr: msr_write 40000083 = 0x7fffe001
> >
> > BIOS sets up message page
> >
> > > qemu-system-x86-656 [001] .... 1313.162111: kvm_msr: msr_read 40000080 = 0x0
> > > qemu-system-x86-656 [001] .... 1313.162118: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000080 data 0x1 host 0
> > > qemu-system-x86-656 [001] .... 1313.162119: kvm_msr: msr_write 40000080 = 0x1
> >
> > BIOS activates SCONTROL
> >
> > > qemu-system-x86-656 [001] .... 1313.246758: kvm_msr: msr_read 40000083 = 0x7fffe001
> > > qemu-system-x86-656 [001] .... 1313.246764: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0x0 host 0
> > > qemu-system-x86-656 [001] .... 1313.246764: kvm_msr: msr_write 40000083 = 0x0
> >
> > BIOS clears message page (it's not needed once the VMBus device was
> > brought up)
> >
> > I guess the choice of Windows DebugEntry appeared somewhere here.
> >
> > > qemu-system-x86-656 [001] .... 1348.904727: kvm_msr: msr_read 40000083 = 0x0
> > > qemu-system-x86-656 [001] .... 1348.904771: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0xfa1001 host 0
> > > qemu-system-x86-656 [001] .... 1348.904772: kvm_msr: msr_write 40000083 = 0xfa1001
> >
> > Bootloader (debug stub?) sets up the message page
> >
> > > qemu-system-x86-656 [001] .... 1348.919170: kvm_msr: msr_read 40000083 = 0xfa1001
> > > qemu-system-x86-656 [001] .... 1348.919183: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0xfa1000 host 0
> > > qemu-system-x86-656 [001] .... 1348.919183: kvm_msr: msr_write 40000083 = 0xfa1000
> >
> > Message page is being disabled again.
> >
> > I guess you only filtered SCONTROL and SIMP, skipping e.g. SVERSION,
> > GUEST_OS_ID, HYPERCALL, etc., which are also part of the exchange here.
> >
>
> Right my bad :( if you want I can re-run the test with the others as well
> (do you need me to?)
No, I just wanted to make sure my assumptions are not completely wrong.
> > > I could not get the EDK2 setup to work though
> > > (https://src.openvz.org/projects/UP/repos/edk2 branch hv-scsi)
> > >
> > > It does not detect my VM hard drive not sure why (this is how i configured
> > > it:
> > > -drive file=./win10.qcow2,format=qcow2,if=none,id=drive_disk0 \
> > > -device virtio-blk-pci,drive=drive_disk0 \
> > >
> > > (Is there something special i need to configure it order for it to work?, I
> > > tried building EDK2 with and without SMM_REQUIRE and SECURE_BOOT_ENABLE)
> >
> > No special configuration I can think of.
> >
> > > But in general it sounds like there is something I dont fully understand
> > > when SCONTROL is enabled, then a GSI is associated with this SintRoute.
> > >
> > > Then when the guest triggers an EOI via the APIC we will trigger the GSI
> > > notification, which will give us another go on trying to copy the message
> > > into it's slot.
> >
> > Right.
> >
> > > So is it the OS that is in charge of setting the EOI?
> >
> > Yes.
> >
> > > If so then it needs to
> > > be aware of SCONTROL being enabled and just having it left set by the BIOS
> > > might not be enough?
> >
> > Yes it needs to be aware of SCONTROL being enabled. However, this
> > awareness may be based on a pure assumption that the previous entity
> > (BIOS or bootloader) did it already.
> >
> > > Also in the TLFS (looking at v6) they mention that message queueing has "3
> > > exit conditions", which will cause the hypervisor to try and attempt to
> > > deliver the additional messages.
> > >
> > > The 3 exit conditions they refer to are:
> > > * Another message buffer is queued.
> > > * The guest indicates the “end of interrupt” by writing to the APIC’s EOI
> > > register.
> > > * The guest indicates the “end of message” by writing to the SynIC’s EOM
> > > register.
> > >
> > > Also notice this additional exit is only if there is a pending message and
> > > not for every EOM.
> >
> > This meaning of "exit" doesn't trivially correspond to what we have in
> > KVM. A write to an msr does cause a vmexit. Then KVM notifies resample
> > eventfds for all SINTs that have them set up, no matter if there's a
> > pending message in the slot. It may be slightly more optimal to only
> > notify those having indicated a pending message, but I don't see the
> > current behavior break anything or violate the spec, so, as EOMs are not
> > used on fast paths, I woudn't bother optimizing.
>
> So based on your answer I got to the following conclusions (correct if they
> are wrong).
>
> First of the one in charge of setting the SCONTROL in the 1st place is the
> BIOS (I dont have a real Hyper-V setup so I cannot really debug it and see,
> not sure which BIOS they have or if we can "rip" it out and run it through
> KVM and see how things look like this way).
>
> If the BIOS has not set the SCONTROL I would expect the OS to have something
> along the lines:
> if (!(get_scontrol() & ENABLED))
> set_scontrol(ENABLED);
>
> So I started looking through the entire Windows system looking what can set
> SCONTROL, I believe I have found the flow to be the following:
>
> VMBus.sys imports winhv.sys (which is an export library) winhv.sys will set
> the SCONTROL prior to VMBus DriverEntry starting here is the complete flow:
> winhv!DllInitialize -> winhv!WinHvpInitialize ->
> winhv!WinHvReportPresentHypervisor -> winhv!WinHvpConnectToSynic ->
> winhv!WinHvpEnableSynic
>
> Eventually WinHvpEnableSynic will simply set SCONTROL (for future reference
> if anyone needs to look into how HyperV register access works in Windows it
> seems like there is an enum representing all the HyperV registers and to
> access it there are helper functions to Get/Set.
> SCONTROL value in the enum is 0x0a0010 .
>
> winhv.sys simply provides very simple API to access the Sints i.e
> (WinHvSetSint / WinHvSetEndOfMessage / WinHvSetSintOnCurrentProcessor /
> WinHvGetSintMessage / etc.)
>
> So basically it seems like the OS does not really care if the BIOS has setup
> the SCONTROL or not, and does so always (if it can) unfortunately in my flow
> (via kdnet) VMBus is not loaded yet and so does winhv.sys so they "fallback"
> into this Polling mode.
>
> So that covers the OS part, after that I have tried looking for relevant
> code in bootmgr and winload (which are Windows boot loader part (like grub)
> and I could not find any code that might setup SCONTROL.
>
> From your experience with this did you see Hyper-V BIOS simply setting the
> SCONTROL? Perhaps if that's the case then the correct fix needs to be in the
> SeaBIOS and the EDK .
All this makes perfect sense to me. Unfortunately it's hard to reason
about the supposed interaction of the components when all you have at
hand are opensource guests on Hyper-V and Windows on an opensource
hypervisor.
E.g. the SeaBIOS code contains a kludge to disable SIMP as soon as vmbus
setup and device enumeration is complete, because otherwise Windows 2016
skips activating the message page, and the hypervisor happily writes to
the stale message page set up by the BIOS.
> I tried to see if Hyper-V supports giving it a BIOS but could not find
> anyway of doing this, so it just might be that Hyper-V assumes the BIOS is
> in charge of setting up SCONTROL for all the boot loader components.
I didn't manage to get Hyper-V load a custom BIOS myself. However I
think you should be able to tell the state the MSRs are left in by the
BIOS on Hyper-V with a custom Linux that would log it prior to
adjusting. I don't have ready access to a Hyper-V machine so can't help
here.
> But in a way it sounds weird because I would expect to see KDNet working
> with the ACPI to trigger the GSI but I could not find any relevant code that
> might do that.
To the best of my knowledge SINTs aren't registered with ACPI.
> As I write this I think I'm starting to get your point just to make sure I
> understand it:
>
> 1. When a new SintRoute is created we associate it with a GSI
Right
> 2. When an EOM is set, we trigger all the GSIs so QEMU will get execution
> time and send all pending messages if it can.
It's in the opposite direction: when EOM is written by the guest, this
triggers the resampler eventfds associated with the GSIs. As a result,
QEMU gets notified and may retry posting respective messages. But yes,
the general idea is right.
> So basically like you said everything "works" from our perspective
> regardless if the system has setup SCONTROL or not, because you trigger the
> interrupt to QEMU regardless of SCONTROL so it can clear the pending
> message.
This is how it works in KVM, indeed.
> If that's indeed the case then probably the only thing needs fixing in my
> scenario is in QEMU where it should not really care for the SCONTROL if it's
> enabled or not.
Right. However, even this shouldn't be necessary as SeaBIOS from that
branch would enable SCONTROL and leave it that way when passing the
control over to the bootloader, so, unless something explicitly clears
SCONTROL, it should remain set thereafter. I'd rather try going ahead
with that scheme first, because making QEMU ignore SCONTROL appears to
violate the spec.
Thanks,
Roman.
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: Dan Williams @ 2020-05-02 18:03 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He, Dave Hansen
In-Reply-To: <467ccba3-80ac-085c-3127-d5618d77d3e0@redhat.com>
On Sat, May 2, 2020 at 2:27 AM David Hildenbrand <david@redhat.com> wrote:
>
> >> Now, let's clarify what I want regarding virtio-mem:
> >>
> >> 1. kexec should not add virtio-mem memory to the initial firmware
> >> memmap. The driver has to be in charge as discussed.
> >> 2. kexec should not place kexec images onto virtio-mem memory. That
> >> would end badly.
> >> 3. kexec should still dump virtio-mem memory via kdump.
> >
> > Ok, but then seems to say to me that dax/kmem is a different type of
> > (driver managed) than virtio-mem and it's confusing to try to apply
> > the same meaning. Why not just call your type for the distinct type it
> > is "System RAM (virtio-mem)" and let any other driver managed memory
> > follow the same "System RAM ($driver)" format if it wants?
>
> I had the same idea but discarded it because it seemed to uglify the
> add_memory() interface (passing yet another parameter only relevant for
> driver managed memory). Maybe we really want a new one, because I like
> that idea:
>
> /*
> * Add special, driver-managed memory to the system as system ram.
> * The resource_name is expected to have the name format "System RAM
> * ($DRIVER)", so user space (esp. kexec-tools)" can special-case it.
> *
> * For this memory, no entries in /sys/firmware/memmap are created,
> * as this memory won't be part of the raw firmware-provided memory map
> * e.g., after a reboot. Also, the created memory resource is flagged
> * with IORESOURCE_MEM_DRIVER_MANAGED, so in-kernel users can special-
> * case this memory (e.g., not place kexec images onto it).
> */
> int add_memory_driver_managed(int nid, u64 start, u64 size,
> const char *resource_name);
>
>
> If we'd ever have to special case it even more in the kernel, we could
> allow to specify further resource flags. While passing the driver name
> instead of the resource_name would be an option, this way we don't have
> to hand craft new resource strings for added memory resources.
>
> Thoughts?
Looks useful to me and simplifies walking /proc/iomem. I personally
like the safety of the string just being the $driver component of the
name, but I won't lose sleep if the interface stays freeform like you
propose.
^ permalink raw reply
* Re: [PATCH v2 0/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Jon Doron @ 2020-05-02 14:47 UTC (permalink / raw)
To: Roman Kagan, kvm, linux-hyperv, vkuznets
In-Reply-To: <20200425061637.GF1917435@jondnuc>
On 25/04/2020, Jon Doron wrote:
>On 24/04/2020, Roman Kagan wrote:
>>On Sat, Apr 18, 2020 at 09:41:27AM +0300, Jon Doron wrote:
>>>On 17/04/2020, Roman Kagan wrote:
>>>> On Thu, Apr 16, 2020 at 03:54:30PM +0300, Jon Doron wrote:
>>>> > On 16/04/2020, Roman Kagan wrote:
>>>> > > On Thu, Apr 16, 2020 at 11:38:46AM +0300, Jon Doron wrote:
>>>> > > > According to the TLFS:
>>>> > > > "A write to the end of message (EOM) register by the guest causes the
>>>> > > > hypervisor to scan the internal message buffer queue(s) associated with
>>>> > > > the virtual processor.
>>>> > > >
>>>> > > > If a message buffer queue contains a queued message buffer, the hypervisor
>>>> > > > attempts to deliver the message.
>>>> > > >
>>>> > > > Message delivery succeeds if the SIM page is enabled and the message slot
>>>> > > > corresponding to the SINTx is empty (that is, the message type in the
>>>> > > > header is set to HvMessageTypeNone).
>>>> > > > If a message is successfully delivered, its corresponding internal message
>>>> > > > buffer is dequeued and marked free.
>>>> > > > If the corresponding SINTx is not masked, an edge-triggered interrupt is
>>>> > > > delivered (that is, the corresponding bit in the IRR is set).
>>>> > > >
>>>> > > > This register can be used by guests to poll for messages. It can also be
>>>> > > > used as a way to drain the message queue for a SINTx that has
>>>> > > > been disabled (that is, masked)."
>>>> > >
>>>> > > Doesn't this work already?
>>>> > >
>>>> >
>>>> > Well if you dont have SCONTROL and a GSI associated with the SINT then it
>>>> > does not...
>>>>
>>>> Yes you do need both of these.
>>>>
>>>> > > > So basically this means that we need to exit on EOM so the hypervisor
>>>> > > > will have a chance to send all the pending messages regardless of the
>>>> > > > SCONTROL mechnaisim.
>>>> > >
>>>> > > I might be misinterpreting the spec, but my understanding is that
>>>> > > SCONTROL {en,dis}ables the message queueing completely. What the quoted
>>>> > > part means is that a write to EOM should trigger the message source to
>>>> > > push a new message into the slot, regardless of whether the SINT was
>>>> > > masked or not.
>>>> > >
>>>> > > And this (I think, haven't tested) should already work. The userspace
>>>> > > just keeps using the SINT route as it normally does, posting
>>>> > > notifications to the corresponding irqfd when posting a message, and
>>>> > > waiting on the resamplerfd for the message slot to become free. If the
>>>> > > SINT is masked KVM will skip injecting the interrupt, that's it.
>>>> > >
>>>> > > Roman.
>>>> >
>>>> > That's what I was thinking originally as well, but then i noticed KDNET as a
>>>> > VMBus client (and it basically runs before anything else) is working in this
>>>> > polling mode, where SCONTROL is disabled and it just loops, and if it saw
>>>> > there is a PENDING message flag it will issue an EOM to indicate it has free
>>>> > the slot.
>>>>
>>>> Who sets up the message page then? Doesn't it enabe SCONTROL as well?
>>>>
>>>
>>>KdNet is the one setting the SIMP and it's not setting the SCONTROL, ill
>>>paste output of KVM traces for the relevant MSRs
>>>
>>>> Note that, even if you don't see it being enabled by Windows, it can be
>>>> enabled by the firmware and/or by the bootloader.
>>>>
>>>> Can you perhaps try with the SeaBIOS from
>>>> https://src.openvz.org/projects/UP/repos/seabios branch hv-scsi? It
>>>> enables SCONTROL and leaves it that way.
>>>>
>>>> I'd also suggest tracing kvm_msr events (both reads and writes) for
>>>> SCONTROL and SIMP msrs, to better understand the picture.
>>>>
>>>> So far the change you propose appears too heavy to work around the
>>>> problem of disabled SCONTROL. You seem to be better off just making
>>>> sure it's enabled (either by the firmware or slighly violating the spec
>>>> and initializing to enabled from the start), and sticking to the
>>>> existing infrastructure for posting messages.
>>>>
>>>
>>>I guess there is something I'm missing here but let's say the BIOS would
>>>have set the SCONTROL but the OS is not setting it, who is in charge of
>>>handling the interrupts?
>>
>>SCONTROL doesn't enable the interrupts, it enables SynIC as a whole.
>>The interrupts are enabled via individual SINTx msrs. This SeaBIOS
>>branch does exactly this: it enables the SynIC via SCONTROL, and then
>>specific SynIC functionality via SIMP/SIEFP, but doesn't activate SINTx
>>and works in polling mode.
>>
>>I agree that this global SCONTROL switch seems redundant but it appears
>>to match the spec.
>>
>>>> > (There are a bunch of patches i sent on the QEMU mailing list as well where
>>>> > i CCed you, I will probably revise it a bit but was hoping to get KVM
>>>> > sorted out first).
>>>>
>>>> I'll look through the archive, should be there, thanks.
>>>>
>>>> Roman.
>>>
>>>I tried testing with both the SeaBIOS branch you have suggested and the
>>>EDK2, unfortunately I could not get the EDK2 build to identify my VM drive
>>>to boot from (not sure why)
>>>
>>>Here is an output of KVM trace for the relevant MSRs (SCONTROL and SIMP)
>>>
>>>QEMU Default BIOS
>>>-----------------
>>> qemu-system-x86-613 [000] .... 1121.080722: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000080 data 0x0 host 1
>>> qemu-system-x86-613 [000] .... 1121.080722: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0x0 host 1
>>> qemu-system-x86-613 [000] .N.. 1121.095592: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000080 data 0x0 host 1
>>> qemu-system-x86-613 [000] .N.. 1121.095592: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0x0 host 1
>>>Choose Windows DebugEntry
>>> qemu-system-x86-613 [001] .... 1165.185227: kvm_msr: msr_read 40000083 = 0x0
>>> qemu-system-x86-613 [001] .... 1165.185255: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0xfa1001 host 0
>>> qemu-system-x86-613 [001] .... 1165.185255: kvm_msr: msr_write 40000083 = 0xfa1001
>>> qemu-system-x86-613 [001] .... 1165.193206: kvm_msr: msr_read 40000083 = 0xfa1001
>>> qemu-system-x86-613 [001] .... 1165.193236: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0xfa1000 host 0
>>> qemu-system-x86-613 [001] .... 1165.193237: kvm_msr: msr_write 40000083 = 0xfa1000
>>>
>>>
>>>SeaBIOS hv-scsci
>>>----------------
>>> qemu-system-x86-656 [001] .... 1313.072714: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000080 data 0x0 host 1
>>> qemu-system-x86-656 [001] .... 1313.072714: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0x0 host 1
>>> qemu-system-x86-656 [001] .... 1313.087752: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000080 data 0x0 host 1
>>> qemu-system-x86-656 [001] .... 1313.087752: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0x0 host 1
>>
>>Initialization (host == 1)
>>
>>> qemu-system-x86-656 [001] .... 1313.156675: kvm_msr: msr_read 40000083 = 0x0
>>> qemu-system-x86-656 [001] .... 1313.156680: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0x7fffe001 host 0
>>>Choose Windows DebugEntry
>>
>>I guess this is a bit misplaced timewise, BIOS is still working here
>>
>>> qemu-system-x86-656 [001] .... 1313.156680: kvm_msr: msr_write 40000083 = 0x7fffe001
>>
>>BIOS sets up message page
>>
>>> qemu-system-x86-656 [001] .... 1313.162111: kvm_msr: msr_read 40000080 = 0x0
>>> qemu-system-x86-656 [001] .... 1313.162118: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000080 data 0x1 host 0
>>> qemu-system-x86-656 [001] .... 1313.162119: kvm_msr: msr_write 40000080 = 0x1
>>
>>BIOS activates SCONTROL
>>
>>> qemu-system-x86-656 [001] .... 1313.246758: kvm_msr: msr_read 40000083 = 0x7fffe001
>>> qemu-system-x86-656 [001] .... 1313.246764: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0x0 host 0
>>> qemu-system-x86-656 [001] .... 1313.246764: kvm_msr: msr_write 40000083 = 0x0
>>
>>BIOS clears message page (it's not needed once the VMBus device was
>>brought up)
>>
>>I guess the choice of Windows DebugEntry appeared somewhere here.
>>
>>> qemu-system-x86-656 [001] .... 1348.904727: kvm_msr: msr_read 40000083 = 0x0
>>> qemu-system-x86-656 [001] .... 1348.904771: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0xfa1001 host 0
>>> qemu-system-x86-656 [001] .... 1348.904772: kvm_msr: msr_write 40000083 = 0xfa1001
>>
>>Bootloader (debug stub?) sets up the message page
>>
>>> qemu-system-x86-656 [001] .... 1348.919170: kvm_msr: msr_read 40000083 = 0xfa1001
>>> qemu-system-x86-656 [001] .... 1348.919183: kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000083 data 0xfa1000 host 0
>>> qemu-system-x86-656 [001] .... 1348.919183: kvm_msr: msr_write 40000083 = 0xfa1000
>>
>>Message page is being disabled again.
>>
>>I guess you only filtered SCONTROL and SIMP, skipping e.g. SVERSION,
>>GUEST_OS_ID, HYPERCALL, etc., which are also part of the exchange here.
>>
>
>Right my bad :( if you want I can re-run the test with the others as
>well (do you need me to?)
>
>>> I could not get the EDK2 setup to work though
>>> (https://src.openvz.org/projects/UP/repos/edk2 branch hv-scsi)
>>>
>>>It does not detect my VM hard drive not sure why (this is how i configured
>>>it:
>>> -drive file=./win10.qcow2,format=qcow2,if=none,id=drive_disk0 \
>>> -device virtio-blk-pci,drive=drive_disk0 \
>>>
>>>(Is there something special i need to configure it order for it to work?, I
>>>tried building EDK2 with and without SMM_REQUIRE and SECURE_BOOT_ENABLE)
>>
>>No special configuration I can think of.
>>
>>>But in general it sounds like there is something I dont fully understand
>>>when SCONTROL is enabled, then a GSI is associated with this SintRoute.
>>>
>>>Then when the guest triggers an EOI via the APIC we will trigger the GSI
>>>notification, which will give us another go on trying to copy the message
>>>into it's slot.
>>
>>Right.
>>
>>>So is it the OS that is in charge of setting the EOI?
>>
>>Yes.
>>
>>>If so then it needs to
>>>be aware of SCONTROL being enabled and just having it left set by the BIOS
>>>might not be enough?
>>
>>Yes it needs to be aware of SCONTROL being enabled. However, this
>>awareness may be based on a pure assumption that the previous entity
>>(BIOS or bootloader) did it already.
>>
>>>Also in the TLFS (looking at v6) they mention that message queueing has "3
>>>exit conditions", which will cause the hypervisor to try and attempt to
>>>deliver the additional messages.
>>>
>>>The 3 exit conditions they refer to are:
>>>* Another message buffer is queued.
>>>* The guest indicates the “end of interrupt” by writing to the APIC’s EOI
>>>register.
>>>* The guest indicates the “end of message” by writing to the SynIC’s EOM
>>>register.
>>>
>>>Also notice this additional exit is only if there is a pending message and
>>>not for every EOM.
>>
>>This meaning of "exit" doesn't trivially correspond to what we have in
>>KVM. A write to an msr does cause a vmexit. Then KVM notifies resample
>>eventfds for all SINTs that have them set up, no matter if there's a
>>pending message in the slot. It may be slightly more optimal to only
>>notify those having indicated a pending message, but I don't see the
>>current behavior break anything or violate the spec, so, as EOMs are not
>>used on fast paths, I woudn't bother optimizing.
>>
>>Roman.
>
>Hi Roman,
>
>So based on your answer I got to the following conclusions (correct if
>they are wrong).
>
>First of the one in charge of setting the SCONTROL in the 1st place is
>the BIOS (I dont have a real Hyper-V setup so I cannot really debug it
>and see, not sure which BIOS they have or if we can "rip" it out and
>run it through KVM and see how things look like this way).
>
>If the BIOS has not set the SCONTROL I would expect the OS to have
>something along the lines:
>if (!(get_scontrol() & ENABLED))
> set_scontrol(ENABLED);
>
>So I started looking through the entire Windows system looking what
>can set SCONTROL, I believe I have found the flow to be the following:
>
>VMBus.sys imports winhv.sys (which is an export library) winhv.sys
>will set the SCONTROL prior to VMBus DriverEntry starting here is the
>complete flow:
>winhv!DllInitialize -> winhv!WinHvpInitialize ->
>winhv!WinHvReportPresentHypervisor -> winhv!WinHvpConnectToSynic ->
>winhv!WinHvpEnableSynic
>
>Eventually WinHvpEnableSynic will simply set SCONTROL (for future
>reference if anyone needs to look into how HyperV register access
>works in Windows it seems like there is an enum representing all the
>HyperV registers and to access it there are helper functions to
>Get/Set.
>SCONTROL value in the enum is 0x0a0010 .
>
>winhv.sys simply provides very simple API to access the Sints i.e
>(WinHvSetSint / WinHvSetEndOfMessage / WinHvSetSintOnCurrentProcessor
>/ WinHvGetSintMessage / etc.)
>
>So basically it seems like the OS does not really care if the BIOS has
>setup the SCONTROL or not, and does so always (if it can)
>unfortunately in my flow (via kdnet) VMBus is not loaded yet and so
>does winhv.sys so they "fallback" into this Polling mode.
>
>So that covers the OS part, after that I have tried looking for
>relevant code in bootmgr and winload (which are Windows boot loader
>part (like grub) and I could not find any code that might setup
>SCONTROL.
>
>From your experience with this did you see Hyper-V BIOS simply setting
>the SCONTROL? Perhaps if that's the case then the correct fix needs to
>be in the SeaBIOS and the EDK .
>
>I tried to see if Hyper-V supports giving it a BIOS but could not find
>anyway of doing this, so it just might be that Hyper-V assumes the
>BIOS is in charge of setting up SCONTROL for all the boot loader
>components.
>
>But in a way it sounds weird because I would expect to see KDNet
>working with the ACPI to trigger the GSI but I could not find any
>relevant code that might do that.
>
>As I write this I think I'm starting to get your point just to make
>sure I understand it:
>
>1. When a new SintRoute is created we associate it with a GSI
>2. When an EOM is set, we trigger all the GSIs so QEMU will get
>execution time and send all pending messages if it can.
>
>So basically like you said everything "works" from our perspective
>regardless if the system has setup SCONTROL or not, because you
>trigger the interrupt to QEMU regardless of SCONTROL so it can clear
>the pending message.
>
>If that's indeed the case then probably the only thing needs fixing in
>my scenario is in QEMU where it should not really care for the
>SCONTROL if it's enabled or not.
>
>Sounds about right?
>
>Thanks,
>-- Jon.
Hi Roman, any chance you can have a quick look at this and see if I
understood you correctly?
Thanks,
-- Jon.
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: David Hildenbrand @ 2020-05-02 9:26 UTC (permalink / raw)
To: Dan Williams
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He, Dave Hansen
In-Reply-To: <CAPcyv4h1nWjszkVJQgeXkUc=-nPv5=Me25BOGFQCpihUyFsD6w@mail.gmail.com>
>> Now, let's clarify what I want regarding virtio-mem:
>>
>> 1. kexec should not add virtio-mem memory to the initial firmware
>> memmap. The driver has to be in charge as discussed.
>> 2. kexec should not place kexec images onto virtio-mem memory. That
>> would end badly.
>> 3. kexec should still dump virtio-mem memory via kdump.
>
> Ok, but then seems to say to me that dax/kmem is a different type of
> (driver managed) than virtio-mem and it's confusing to try to apply
> the same meaning. Why not just call your type for the distinct type it
> is "System RAM (virtio-mem)" and let any other driver managed memory
> follow the same "System RAM ($driver)" format if it wants?
I had the same idea but discarded it because it seemed to uglify the
add_memory() interface (passing yet another parameter only relevant for
driver managed memory). Maybe we really want a new one, because I like
that idea:
/*
* Add special, driver-managed memory to the system as system ram.
* The resource_name is expected to have the name format "System RAM
* ($DRIVER)", so user space (esp. kexec-tools)" can special-case it.
*
* For this memory, no entries in /sys/firmware/memmap are created,
* as this memory won't be part of the raw firmware-provided memory map
* e.g., after a reboot. Also, the created memory resource is flagged
* with IORESOURCE_MEM_DRIVER_MANAGED, so in-kernel users can special-
* case this memory (e.g., not place kexec images onto it).
*/
int add_memory_driver_managed(int nid, u64 start, u64 size,
const char *resource_name);
If we'd ever have to special case it even more in the kernel, we could
allow to specify further resource flags. While passing the driver name
instead of the resource_name would be an option, this way we don't have
to hand craft new resource strings for added memory resources.
Thoughts?
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type
From: David Miller @ 2020-05-01 22:25 UTC (permalink / raw)
To: natechancellor
Cc: kys, haiyangz, sthemmin, wei.liu, linux-hyperv, netdev,
linux-kernel, clang-built-linux, samitolvanen
In-Reply-To: <20200428175455.2109973-1-natechancellor@gmail.com>
From: Nathan Chancellor <natechancellor@gmail.com>
Date: Tue, 28 Apr 2020 10:54:56 -0700
> 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:
...
> 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>
Applied.
^ permalink raw reply
* Re: [PATCH 21/29] mm: remove the pgprot argument to __vmalloc
From: Andrew Morton @ 2020-05-01 22:09 UTC (permalink / raw)
To: John Dorminy
Cc: Wei Liu, Christoph Hellwig, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, x86, David Airlie, Daniel Vetter, Laura Abbott,
Sumit Semwal, Sakari Ailus, Minchan Kim, Nitin Gupta,
Robin Murphy, Christophe Leroy, Peter Zijlstra, linuxppc-dev,
linux-hyperv, dri-devel, linaro-mm-sig, linux-arch, linux-mm,
iommu, linux-arm-kernel, linux-s390, bpf,
Linux Kernel Mailing List, Michael Kelley, Gao Xiang
In-Reply-To: <CAMeeMh_9N0ORhPM8EmkGeeuiDoQY3+QoAPX5QBuK7=gsC5ONng@mail.gmail.com>
On Thu, 30 Apr 2020 22:38:10 -0400 John Dorminy <jdorminy@redhat.com> wrote:
> the change
> description refers to PROT_KERNEL, which is a symbol which does not
> appear to exist; perhaps PAGE_KERNEL was meant?
Yes, thanks, fixed.
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: Dan Williams @ 2020-05-01 21:52 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He
In-Reply-To: <8242c0c5-2df2-fc0c-079a-3be62c113a11@redhat.com>
On Fri, May 1, 2020 at 2:11 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 01.05.20 22:12, Dan Williams wrote:
[..]
> >>> Consider the case of EFI Special Purpose (SP) Memory that is
> >>> marked EFI Conventional Memory with the SP attribute. In that case the
> >>> firmware memory map marked it as conventional RAM, but the kernel
> >>> optionally marks it as System RAM vs Soft Reserved. The 2008 patch
> >>> simply does not consider that case. I'm not sure strict textualism
> >>> works for coding decisions.
> >>
> >> I am no expert on that matter (esp EFI). But looking at the users of
> >> firmware_map_add_early(), the single user is in arch/x86/kernel/e820.c
> >> . So the single source of /sys/firmware/memmap is (besides hotplug) e820.
> >>
> >> "'e820_table_firmware': the original firmware version passed to us by
> >> the bootloader - not modified by the kernel. ... inform the user about
> >> the firmware's notion of memory layout via /sys/firmware/memmap"
> >> (arch/x86/kernel/e820.c)
> >>
> >> How is the EFI Special Purpose (SP) Memory represented in e820?
> >> /sys/firmware/memmap is really simple: just dump in e820. No policies IIUC.
> >
> > e820 now has a Soft Reserved translation for this which means "try to
> > reserve, but treat as System RAM is ok too". It seems generically
> > useful to me that the toggle for determining whether Soft Reserved or
> > System RAM shows up /sys/firmware/memmap is a determination that
> > policy can make. The kernel need not preemptively block it.
>
> So, I think I have to clarify something here. We do have two ways to kexec
>
> 1. kexec_load(): User space (kexec-tools) crafts the memmap (e.g., using
> /sys/firmware/memmap on x86-64) and selects memory where to place the
> kexec images (e.g., using /proc/iomem)
>
> 2. kexec_file_load(): The kernel reuses the (basically) raw firmware
> memmap and selects memory where to place kexec images.
>
> We are talking about changing 1, to behave like 2 in regards to
> dax/kmem. 2. does currently not add any hotplugged memory to the
> fixed-up e820, and it should be fixed regarding hotplugged DIMMs that
> would appear in e820 after a reboot.
>
> Now, all these policy discussions are nice and fun, but I don't really
> see a good reason to (ab)use /sys/firmware/memmap for that (e.g., parent
> properties). If you want to be able to make this configurable, then
> e.g., add a way to configure this in the kernel (for example along with
> kmem) to make 1. and 2. behave the same way. Otherwise, you really only
> can change 1.
That's clearer.
>
>
> Now, let's clarify what I want regarding virtio-mem:
>
> 1. kexec should not add virtio-mem memory to the initial firmware
> memmap. The driver has to be in charge as discussed.
> 2. kexec should not place kexec images onto virtio-mem memory. That
> would end badly.
> 3. kexec should still dump virtio-mem memory via kdump.
Ok, but then seems to say to me that dax/kmem is a different type of
(driver managed) than virtio-mem and it's confusing to try to apply
the same meaning. Why not just call your type for the distinct type it
is "System RAM (virtio-mem)" and let any other driver managed memory
follow the same "System RAM ($driver)" format if it wants?
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: David Hildenbrand @ 2020-05-01 21:10 UTC (permalink / raw)
To: Dan Williams
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He
In-Reply-To: <CAPcyv4iXyOUDZgqhWH1KCObvATL=gP55xEr64rsRfUuJg5B+eQ@mail.gmail.com>
On 01.05.20 22:12, Dan Williams wrote:
> On Fri, May 1, 2020 at 12:18 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 01.05.20 20:43, Dan Williams wrote:
>>> On Fri, May 1, 2020 at 11:14 AM David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 01.05.20 20:03, Dan Williams wrote:
>>>>> On Fri, May 1, 2020 at 10:51 AM David Hildenbrand <david@redhat.com> wrote:
>>>>>>
>>>>>> On 01.05.20 19:45, David Hildenbrand wrote:
>>>>>>> On 01.05.20 19:39, Dan Williams wrote:
>>>>>>>> On Fri, May 1, 2020 at 10:21 AM David Hildenbrand <david@redhat.com> wrote:
>>>>>>>>>
>>>>>>>>> On 01.05.20 18:56, Dan Williams wrote:
>>>>>>>>>> On Fri, May 1, 2020 at 2:34 AM David Hildenbrand <david@redhat.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 01.05.20 00:24, Andrew Morton wrote:
>>>>>>>>>>>> On Thu, 30 Apr 2020 20:43:39 +0200 David Hildenbrand <david@redhat.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Why does the firmware map support hotplug entries?
>>>>>>>>>>>>>
>>>>>>>>>>>>> I assume:
>>>>>>>>>>>>>
>>>>>>>>>>>>> The firmware memmap was added primarily for x86-64 kexec (and still, is
>>>>>>>>>>>>> mostly used on x86-64 only IIRC). There, we had ACPI hotplug. When DIMMs
>>>>>>>>>>>>> get hotplugged on real HW, they get added to e820. Same applies to
>>>>>>>>>>>>> memory added via HyperV balloon (unless memory is unplugged via
>>>>>>>>>>>>> ballooning and you reboot ... the the e820 is changed as well). I assume
>>>>>>>>>>>>> we wanted to be able to reflect that, to make kexec look like a real reboot.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This worked for a while. Then came dax/kmem. Now comes virtio-mem.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> But I assume only Andrew can enlighten us.
>>>>>>>>>>>>>
>>>>>>>>>>>>> @Andrew, any guidance here? Should we really add all memory to the
>>>>>>>>>>>>> firmware memmap, even if this contradicts with the existing
>>>>>>>>>>>>> documentation? (especially, if the actual firmware memmap will *not*
>>>>>>>>>>>>> contain that memory after a reboot)
>>>>>>>>>>>>
>>>>>>>>>>>> For some reason that patch is misattributed - it was authored by
>>>>>>>>>>>> Shaohui Zheng <shaohui.zheng@intel.com>, who hasn't been heard from in
>>>>>>>>>>>> a decade. I looked through the email discussion from that time and I'm
>>>>>>>>>>>> not seeing anything useful. But I wasn't able to locate Dave Hansen's
>>>>>>>>>>>> review comments.
>>>>>>>>>>>
>>>>>>>>>>> Okay, thanks for checking. I think the documentation from 2008 is pretty
>>>>>>>>>>> clear what has to be done here. I will add some of these details to the
>>>>>>>>>>> patch description.
>>>>>>>>>>>
>>>>>>>>>>> Also, now that I know that esp. kexec-tools already don't consider
>>>>>>>>>>> dax/kmem memory properly (memory will not get dumped via kdump) and
>>>>>>>>>>> won't really suffer from a name change in /proc/iomem, I will go back to
>>>>>>>>>>> the MHP_DRIVER_MANAGED approach and
>>>>>>>>>>> 1. Don't create firmware memmap entries
>>>>>>>>>>> 2. Name the resource "System RAM (driver managed)"
>>>>>>>>>>> 3. Flag the resource via something like IORESOURCE_MEM_DRIVER_MANAGED.
>>>>>>>>>>>
>>>>>>>>>>> This way, kernel users and user space can figure out that this memory
>>>>>>>>>>> has different semantics and handle it accordingly - I think that was
>>>>>>>>>>> what Eric was asking for.
>>>>>>>>>>>
>>>>>>>>>>> Of course, open for suggestions.
>>>>>>>>>>
>>>>>>>>>> I'm still more of a fan of this being communicated by "System RAM"
>>>>>>>>>
>>>>>>>>> I was mentioning somewhere in this thread that "System RAM" inside a
>>>>>>>>> hierarchy (like dax/kmem) will already be basically ignored by
>>>>>>>>> kexec-tools. So, placing it inside a hierarchy already makes it look
>>>>>>>>> special already.
>>>>>>>>>
>>>>>>>>> But after all, as we have to change kexec-tools either way, we can
>>>>>>>>> directly go ahead and flag it properly as special (in case there will
>>>>>>>>> ever be other cases where we could no longer distinguish it).
>>>>>>>>>
>>>>>>>>>> being parented especially because that tells you something about how
>>>>>>>>>> the memory is driver-managed and which mechanism might be in play.
>>>>>>>>>
>>>>>>>>> The could be communicated to some degree via the resource hierarchy.
>>>>>>>>>
>>>>>>>>> E.g.,
>>>>>>>>>
>>>>>>>>> [root@localhost ~]# cat /proc/iomem
>>>>>>>>> ...
>>>>>>>>> 140000000-33fffffff : Persistent Memory
>>>>>>>>> 140000000-1481fffff : namespace0.0
>>>>>>>>> 150000000-33fffffff : dax0.0
>>>>>>>>> 150000000-33fffffff : System RAM (driver managed)
>>>>>>>>>
>>>>>>>>> vs.
>>>>>>>>>
>>>>>>>>> :/# cat /proc/iomem
>>>>>>>>> [...]
>>>>>>>>> 140000000-333ffffff : virtio-mem (virtio0)
>>>>>>>>> 140000000-147ffffff : System RAM (driver managed)
>>>>>>>>> 148000000-14fffffff : System RAM (driver managed)
>>>>>>>>> 150000000-157ffffff : System RAM (driver managed)
>>>>>>>>>
>>>>>>>>> Good enough for my taste.
>>>>>>>>>
>>>>>>>>>> What about adding an optional /sys/firmware/memmap/X/parent attribute.
>>>>>>>>>
>>>>>>>>> I really don't want any firmware memmap entries for something that is
>>>>>>>>> not part of the firmware provided memmap. In addition,
>>>>>>>>> /sys/firmware/memmap/ is still a fairly x86_64 specific thing. Only mips
>>>>>>>>> and two arm configs enable it at all.
>>>>>>>>>
>>>>>>>>> So, IMHO, /sys/firmware/memmap/ is definitely not the way to go.
>>>>>>>>
>>>>>>>> I think that's a policy decision and policy decisions do not belong in
>>>>>>>> the kernel. Give the tooling the opportunity to decide whether System
>>>>>>>> RAM stays that way over a kexec. The parenthetical reference otherwise
>>>>>>>> looks out of place to me in the /proc/iomem output. What makes it
>>>>>>>> "driver managed" is how the kernel handles it, not how the kernel
>>>>>>>> names it.
>>>>>>>
>>>>>>> At least, virtio-mem is different. It really *has to be handled* by the
>>>>>>> driver. This is not a policy. It's how it works.
>>>>>
>>>>> ...but that's not necessarily how dax/kmem works.
>>>>>
>>>>
>>>> Yes, and user space could still take that memory and add it to the
>>>> firmware memmap if it really wants to. It knows that it is special. It
>>>> can figure out that it belongs to a dax device using /proc/iomem.
>>>>
>>>>>>>
>>>>>>
>>>>>> Oh, and I don't see why "System RAM (driver managed)" would hinder any
>>>>>> policy in user case to still do what it thinks is the right thing to do
>>>>>> (e.g., for dax).
>>>>>>
>>>>>> "System RAM (driver managed)" would mean: Memory is not part of the raw
>>>>>> firmware memmap. It was detected and added by a driver. Handle with
>>>>>> care, this is special.
>>>>>
>>>>> Oh, no, I was more reacting to your, "don't update
>>>>> /sys/firmware/memmap for the (driver managed) range" choice as being a
>>>>> policy decision. It otherwise feels to me "System RAM (driver
>>>>> managed)" adds confusion for casual users of /proc/iomem and for clued
>>>>> in tools they have the parent association to decide policy.
>>>>
>>>> Not sure if I understand correctly, so bear with me :).
>>>>
>>>> Adding or not adding stuff to /sys/firmware/memmap is not a policy
>>>> decision. If it's not part of the raw firmware-provided memmap, it has
>>>> nothing to do in /sys/firmware/memmap. That's what the documentation
>>>> from 2008 tells us.
>>>
>>> It just occurs to me that there are valid cases for both wanting to
>>> start over with driver managed memory with a kexec and keeping it in
>>> the map.
>>
>> Yes, there might be valid cases. My gut feeling is that in the general
>> case, you want to let the kexec kernel implement a policy/ let the user
>> in the new system decide.
>>
>> But as I said, you can implement in kexec-tools whatever policy you
>> want. It has access to all information.
>
> Right, so why is a new type needed if all the information is there by
> other means?
You mean "System RAM (driver managed)" in /proc/iomem? See below for more.
>
>>> Consider the case of EFI Special Purpose (SP) Memory that is
>>> marked EFI Conventional Memory with the SP attribute. In that case the
>>> firmware memory map marked it as conventional RAM, but the kernel
>>> optionally marks it as System RAM vs Soft Reserved. The 2008 patch
>>> simply does not consider that case. I'm not sure strict textualism
>>> works for coding decisions.
>>
>> I am no expert on that matter (esp EFI). But looking at the users of
>> firmware_map_add_early(), the single user is in arch/x86/kernel/e820.c
>> . So the single source of /sys/firmware/memmap is (besides hotplug) e820.
>>
>> "'e820_table_firmware': the original firmware version passed to us by
>> the bootloader - not modified by the kernel. ... inform the user about
>> the firmware's notion of memory layout via /sys/firmware/memmap"
>> (arch/x86/kernel/e820.c)
>>
>> How is the EFI Special Purpose (SP) Memory represented in e820?
>> /sys/firmware/memmap is really simple: just dump in e820. No policies IIUC.
>
> e820 now has a Soft Reserved translation for this which means "try to
> reserve, but treat as System RAM is ok too". It seems generically
> useful to me that the toggle for determining whether Soft Reserved or
> System RAM shows up /sys/firmware/memmap is a determination that
> policy can make. The kernel need not preemptively block it.
So, I think I have to clarify something here. We do have two ways to kexec
1. kexec_load(): User space (kexec-tools) crafts the memmap (e.g., using
/sys/firmware/memmap on x86-64) and selects memory where to place the
kexec images (e.g., using /proc/iomem)
2. kexec_file_load(): The kernel reuses the (basically) raw firmware
memmap and selects memory where to place kexec images.
We are talking about changing 1, to behave like 2 in regards to
dax/kmem. 2. does currently not add any hotplugged memory to the
fixed-up e820, and it should be fixed regarding hotplugged DIMMs that
would appear in e820 after a reboot.
Now, all these policy discussions are nice and fun, but I don't really
see a good reason to (ab)use /sys/firmware/memmap for that (e.g., parent
properties). If you want to be able to make this configurable, then
e.g., add a way to configure this in the kernel (for example along with
kmem) to make 1. and 2. behave the same way. Otherwise, you really only
can change 1.
Now, let's clarify what I want regarding virtio-mem:
1. kexec should not add virtio-mem memory to the initial firmware
memmap. The driver has to be in charge as discussed.
2. kexec should not place kexec images onto virtio-mem memory. That
would end badly.
3. kexec should still dump virtio-mem memory via kdump.
This has to work when using kexec_load() or kexec_file_load(). This has
to theoretically work on different architectures (especially, without
/sys/firmware/memmap). kexec-tools has to have access to that
information to figure out what to do.
Regarding 1:
- kexec_file_load(): works out of the box currently.
- kexec_load(): Don't create entries in /sys/firmware/memmap (for
reasons discussed)
Regarding 2:
- kexec_file_load(): tag the resources as IORESOURCE_MEM_DRIVER_MANAGED
(inspired by Eric)
- kexec_load(): indicate the memory as "System RAM (driver managed)"
Regarding 3:
- Same as 2. kexec-tools need to be thought to properly consider the
memory during kdump.
Now, you are asking, "why System RAM (driver managed)". I don't think
it's strictly needed right now, but it feels cleaner. E.g., for
virtio-mem the current plan is to have /proc/iomem look like
:/# cat /proc/iomem
[...]
140000000-333ffffff : virtio-mem (virtio0)
140000000-147ffffff : System RAM (driver managed)
148000000-14fffffff : System RAM (driver managed)
150000000-157ffffff : System RAM (driver managed)
One could judge by looking at the hierarchy, that this memory is
special. kexec-tools will skip it currently in either form.
If we all agree here, that we can drop it, then let's drop it,
especially if it would allow dax/kmem to use the same mechanism I am
proposing here for virtio-mem.
Now, it would be fairly simple to add a config option for dax/kmem,
making it configurable in the kernel, whether to add memory via
MHP_DRIVER_MANAGED or just as we do now. It would contradict with the
"raw firmware/prov..." description of /sys/firmware/memmap, but hey,
somebody explicitly configured it, so it can't be wrong.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: Dan Williams @ 2020-05-01 20:12 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He
In-Reply-To: <04242d48-5fa9-6da4-3e4a-991e401eb580@redhat.com>
On Fri, May 1, 2020 at 12:18 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 01.05.20 20:43, Dan Williams wrote:
> > On Fri, May 1, 2020 at 11:14 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 01.05.20 20:03, Dan Williams wrote:
> >>> On Fri, May 1, 2020 at 10:51 AM David Hildenbrand <david@redhat.com> wrote:
> >>>>
> >>>> On 01.05.20 19:45, David Hildenbrand wrote:
> >>>>> On 01.05.20 19:39, Dan Williams wrote:
> >>>>>> On Fri, May 1, 2020 at 10:21 AM David Hildenbrand <david@redhat.com> wrote:
> >>>>>>>
> >>>>>>> On 01.05.20 18:56, Dan Williams wrote:
> >>>>>>>> On Fri, May 1, 2020 at 2:34 AM David Hildenbrand <david@redhat.com> wrote:
> >>>>>>>>>
> >>>>>>>>> On 01.05.20 00:24, Andrew Morton wrote:
> >>>>>>>>>> On Thu, 30 Apr 2020 20:43:39 +0200 David Hildenbrand <david@redhat.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> Why does the firmware map support hotplug entries?
> >>>>>>>>>>>
> >>>>>>>>>>> I assume:
> >>>>>>>>>>>
> >>>>>>>>>>> The firmware memmap was added primarily for x86-64 kexec (and still, is
> >>>>>>>>>>> mostly used on x86-64 only IIRC). There, we had ACPI hotplug. When DIMMs
> >>>>>>>>>>> get hotplugged on real HW, they get added to e820. Same applies to
> >>>>>>>>>>> memory added via HyperV balloon (unless memory is unplugged via
> >>>>>>>>>>> ballooning and you reboot ... the the e820 is changed as well). I assume
> >>>>>>>>>>> we wanted to be able to reflect that, to make kexec look like a real reboot.
> >>>>>>>>>>>
> >>>>>>>>>>> This worked for a while. Then came dax/kmem. Now comes virtio-mem.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> But I assume only Andrew can enlighten us.
> >>>>>>>>>>>
> >>>>>>>>>>> @Andrew, any guidance here? Should we really add all memory to the
> >>>>>>>>>>> firmware memmap, even if this contradicts with the existing
> >>>>>>>>>>> documentation? (especially, if the actual firmware memmap will *not*
> >>>>>>>>>>> contain that memory after a reboot)
> >>>>>>>>>>
> >>>>>>>>>> For some reason that patch is misattributed - it was authored by
> >>>>>>>>>> Shaohui Zheng <shaohui.zheng@intel.com>, who hasn't been heard from in
> >>>>>>>>>> a decade. I looked through the email discussion from that time and I'm
> >>>>>>>>>> not seeing anything useful. But I wasn't able to locate Dave Hansen's
> >>>>>>>>>> review comments.
> >>>>>>>>>
> >>>>>>>>> Okay, thanks for checking. I think the documentation from 2008 is pretty
> >>>>>>>>> clear what has to be done here. I will add some of these details to the
> >>>>>>>>> patch description.
> >>>>>>>>>
> >>>>>>>>> Also, now that I know that esp. kexec-tools already don't consider
> >>>>>>>>> dax/kmem memory properly (memory will not get dumped via kdump) and
> >>>>>>>>> won't really suffer from a name change in /proc/iomem, I will go back to
> >>>>>>>>> the MHP_DRIVER_MANAGED approach and
> >>>>>>>>> 1. Don't create firmware memmap entries
> >>>>>>>>> 2. Name the resource "System RAM (driver managed)"
> >>>>>>>>> 3. Flag the resource via something like IORESOURCE_MEM_DRIVER_MANAGED.
> >>>>>>>>>
> >>>>>>>>> This way, kernel users and user space can figure out that this memory
> >>>>>>>>> has different semantics and handle it accordingly - I think that was
> >>>>>>>>> what Eric was asking for.
> >>>>>>>>>
> >>>>>>>>> Of course, open for suggestions.
> >>>>>>>>
> >>>>>>>> I'm still more of a fan of this being communicated by "System RAM"
> >>>>>>>
> >>>>>>> I was mentioning somewhere in this thread that "System RAM" inside a
> >>>>>>> hierarchy (like dax/kmem) will already be basically ignored by
> >>>>>>> kexec-tools. So, placing it inside a hierarchy already makes it look
> >>>>>>> special already.
> >>>>>>>
> >>>>>>> But after all, as we have to change kexec-tools either way, we can
> >>>>>>> directly go ahead and flag it properly as special (in case there will
> >>>>>>> ever be other cases where we could no longer distinguish it).
> >>>>>>>
> >>>>>>>> being parented especially because that tells you something about how
> >>>>>>>> the memory is driver-managed and which mechanism might be in play.
> >>>>>>>
> >>>>>>> The could be communicated to some degree via the resource hierarchy.
> >>>>>>>
> >>>>>>> E.g.,
> >>>>>>>
> >>>>>>> [root@localhost ~]# cat /proc/iomem
> >>>>>>> ...
> >>>>>>> 140000000-33fffffff : Persistent Memory
> >>>>>>> 140000000-1481fffff : namespace0.0
> >>>>>>> 150000000-33fffffff : dax0.0
> >>>>>>> 150000000-33fffffff : System RAM (driver managed)
> >>>>>>>
> >>>>>>> vs.
> >>>>>>>
> >>>>>>> :/# cat /proc/iomem
> >>>>>>> [...]
> >>>>>>> 140000000-333ffffff : virtio-mem (virtio0)
> >>>>>>> 140000000-147ffffff : System RAM (driver managed)
> >>>>>>> 148000000-14fffffff : System RAM (driver managed)
> >>>>>>> 150000000-157ffffff : System RAM (driver managed)
> >>>>>>>
> >>>>>>> Good enough for my taste.
> >>>>>>>
> >>>>>>>> What about adding an optional /sys/firmware/memmap/X/parent attribute.
> >>>>>>>
> >>>>>>> I really don't want any firmware memmap entries for something that is
> >>>>>>> not part of the firmware provided memmap. In addition,
> >>>>>>> /sys/firmware/memmap/ is still a fairly x86_64 specific thing. Only mips
> >>>>>>> and two arm configs enable it at all.
> >>>>>>>
> >>>>>>> So, IMHO, /sys/firmware/memmap/ is definitely not the way to go.
> >>>>>>
> >>>>>> I think that's a policy decision and policy decisions do not belong in
> >>>>>> the kernel. Give the tooling the opportunity to decide whether System
> >>>>>> RAM stays that way over a kexec. The parenthetical reference otherwise
> >>>>>> looks out of place to me in the /proc/iomem output. What makes it
> >>>>>> "driver managed" is how the kernel handles it, not how the kernel
> >>>>>> names it.
> >>>>>
> >>>>> At least, virtio-mem is different. It really *has to be handled* by the
> >>>>> driver. This is not a policy. It's how it works.
> >>>
> >>> ...but that's not necessarily how dax/kmem works.
> >>>
> >>
> >> Yes, and user space could still take that memory and add it to the
> >> firmware memmap if it really wants to. It knows that it is special. It
> >> can figure out that it belongs to a dax device using /proc/iomem.
> >>
> >>>>>
> >>>>
> >>>> Oh, and I don't see why "System RAM (driver managed)" would hinder any
> >>>> policy in user case to still do what it thinks is the right thing to do
> >>>> (e.g., for dax).
> >>>>
> >>>> "System RAM (driver managed)" would mean: Memory is not part of the raw
> >>>> firmware memmap. It was detected and added by a driver. Handle with
> >>>> care, this is special.
> >>>
> >>> Oh, no, I was more reacting to your, "don't update
> >>> /sys/firmware/memmap for the (driver managed) range" choice as being a
> >>> policy decision. It otherwise feels to me "System RAM (driver
> >>> managed)" adds confusion for casual users of /proc/iomem and for clued
> >>> in tools they have the parent association to decide policy.
> >>
> >> Not sure if I understand correctly, so bear with me :).
> >>
> >> Adding or not adding stuff to /sys/firmware/memmap is not a policy
> >> decision. If it's not part of the raw firmware-provided memmap, it has
> >> nothing to do in /sys/firmware/memmap. That's what the documentation
> >> from 2008 tells us.
> >
> > It just occurs to me that there are valid cases for both wanting to
> > start over with driver managed memory with a kexec and keeping it in
> > the map.
>
> Yes, there might be valid cases. My gut feeling is that in the general
> case, you want to let the kexec kernel implement a policy/ let the user
> in the new system decide.
>
> But as I said, you can implement in kexec-tools whatever policy you
> want. It has access to all information.
Right, so why is a new type needed if all the information is there by
other means?
> > Consider the case of EFI Special Purpose (SP) Memory that is
> > marked EFI Conventional Memory with the SP attribute. In that case the
> > firmware memory map marked it as conventional RAM, but the kernel
> > optionally marks it as System RAM vs Soft Reserved. The 2008 patch
> > simply does not consider that case. I'm not sure strict textualism
> > works for coding decisions.
>
> I am no expert on that matter (esp EFI). But looking at the users of
> firmware_map_add_early(), the single user is in arch/x86/kernel/e820.c
> . So the single source of /sys/firmware/memmap is (besides hotplug) e820.
>
> "'e820_table_firmware': the original firmware version passed to us by
> the bootloader - not modified by the kernel. ... inform the user about
> the firmware's notion of memory layout via /sys/firmware/memmap"
> (arch/x86/kernel/e820.c)
>
> How is the EFI Special Purpose (SP) Memory represented in e820?
> /sys/firmware/memmap is really simple: just dump in e820. No policies IIUC.
e820 now has a Soft Reserved translation for this which means "try to
reserve, but treat as System RAM is ok too". It seems generically
useful to me that the toggle for determining whether Soft Reserved or
System RAM shows up /sys/firmware/memmap is a determination that
policy can make. The kernel need not preemptively block it.
^ permalink raw reply
* Re: [PATCH 00/37]net: manually convert files to ReST format - part 3 (final)
From: David Miller @ 2020-05-01 19:33 UTC (permalink / raw)
To: mchehab+huawei
Cc: linux-doc, linux-kernel, corbet, chessman, netdev, andrew.hendry,
zorik, stranche, irusskikh, jdmason, haiyangz, linux-x25, wei.liu,
linux-hyperv, kvalo, kuba, subashab, dsahern, kys, kou.ishizaki,
jreuter, saeedb, shrijeet, netanel, stas.yakovlev, gtzalik, maxk,
akiyano, linux-wireless, linux-hams, linux-parisc, klassert,
sthemmin
In-Reply-To: <cover.1588344146.git.mchehab+huawei@kernel.org>
From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Date: Fri, 1 May 2020 16:44:22 +0200
> That's the third part (and the final one) of my work to convert the networking
> text files into ReST. it is based on linux-next next-20200430 branch.
>
> The full series (including those ones) are at:
>
> https://git.linuxtv.org/mchehab/experimental.git/log/?h=net-docs
>
> The built output documents, on html format is at:
>
> https://www.infradead.org/~mchehab/kernel_docs/networking/
Series applied, thanks for doing all of this work.
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: David Hildenbrand @ 2020-05-01 19:17 UTC (permalink / raw)
To: Dan Williams
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He
In-Reply-To: <CAPcyv4jjrxQ27rsfmz6wYPgmedevU=KG+wZ0GOm=qiE6tqa+VA@mail.gmail.com>
On 01.05.20 20:43, Dan Williams wrote:
> On Fri, May 1, 2020 at 11:14 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 01.05.20 20:03, Dan Williams wrote:
>>> On Fri, May 1, 2020 at 10:51 AM David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 01.05.20 19:45, David Hildenbrand wrote:
>>>>> On 01.05.20 19:39, Dan Williams wrote:
>>>>>> On Fri, May 1, 2020 at 10:21 AM David Hildenbrand <david@redhat.com> wrote:
>>>>>>>
>>>>>>> On 01.05.20 18:56, Dan Williams wrote:
>>>>>>>> On Fri, May 1, 2020 at 2:34 AM David Hildenbrand <david@redhat.com> wrote:
>>>>>>>>>
>>>>>>>>> On 01.05.20 00:24, Andrew Morton wrote:
>>>>>>>>>> On Thu, 30 Apr 2020 20:43:39 +0200 David Hildenbrand <david@redhat.com> wrote:
>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Why does the firmware map support hotplug entries?
>>>>>>>>>>>
>>>>>>>>>>> I assume:
>>>>>>>>>>>
>>>>>>>>>>> The firmware memmap was added primarily for x86-64 kexec (and still, is
>>>>>>>>>>> mostly used on x86-64 only IIRC). There, we had ACPI hotplug. When DIMMs
>>>>>>>>>>> get hotplugged on real HW, they get added to e820. Same applies to
>>>>>>>>>>> memory added via HyperV balloon (unless memory is unplugged via
>>>>>>>>>>> ballooning and you reboot ... the the e820 is changed as well). I assume
>>>>>>>>>>> we wanted to be able to reflect that, to make kexec look like a real reboot.
>>>>>>>>>>>
>>>>>>>>>>> This worked for a while. Then came dax/kmem. Now comes virtio-mem.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> But I assume only Andrew can enlighten us.
>>>>>>>>>>>
>>>>>>>>>>> @Andrew, any guidance here? Should we really add all memory to the
>>>>>>>>>>> firmware memmap, even if this contradicts with the existing
>>>>>>>>>>> documentation? (especially, if the actual firmware memmap will *not*
>>>>>>>>>>> contain that memory after a reboot)
>>>>>>>>>>
>>>>>>>>>> For some reason that patch is misattributed - it was authored by
>>>>>>>>>> Shaohui Zheng <shaohui.zheng@intel.com>, who hasn't been heard from in
>>>>>>>>>> a decade. I looked through the email discussion from that time and I'm
>>>>>>>>>> not seeing anything useful. But I wasn't able to locate Dave Hansen's
>>>>>>>>>> review comments.
>>>>>>>>>
>>>>>>>>> Okay, thanks for checking. I think the documentation from 2008 is pretty
>>>>>>>>> clear what has to be done here. I will add some of these details to the
>>>>>>>>> patch description.
>>>>>>>>>
>>>>>>>>> Also, now that I know that esp. kexec-tools already don't consider
>>>>>>>>> dax/kmem memory properly (memory will not get dumped via kdump) and
>>>>>>>>> won't really suffer from a name change in /proc/iomem, I will go back to
>>>>>>>>> the MHP_DRIVER_MANAGED approach and
>>>>>>>>> 1. Don't create firmware memmap entries
>>>>>>>>> 2. Name the resource "System RAM (driver managed)"
>>>>>>>>> 3. Flag the resource via something like IORESOURCE_MEM_DRIVER_MANAGED.
>>>>>>>>>
>>>>>>>>> This way, kernel users and user space can figure out that this memory
>>>>>>>>> has different semantics and handle it accordingly - I think that was
>>>>>>>>> what Eric was asking for.
>>>>>>>>>
>>>>>>>>> Of course, open for suggestions.
>>>>>>>>
>>>>>>>> I'm still more of a fan of this being communicated by "System RAM"
>>>>>>>
>>>>>>> I was mentioning somewhere in this thread that "System RAM" inside a
>>>>>>> hierarchy (like dax/kmem) will already be basically ignored by
>>>>>>> kexec-tools. So, placing it inside a hierarchy already makes it look
>>>>>>> special already.
>>>>>>>
>>>>>>> But after all, as we have to change kexec-tools either way, we can
>>>>>>> directly go ahead and flag it properly as special (in case there will
>>>>>>> ever be other cases where we could no longer distinguish it).
>>>>>>>
>>>>>>>> being parented especially because that tells you something about how
>>>>>>>> the memory is driver-managed and which mechanism might be in play.
>>>>>>>
>>>>>>> The could be communicated to some degree via the resource hierarchy.
>>>>>>>
>>>>>>> E.g.,
>>>>>>>
>>>>>>> [root@localhost ~]# cat /proc/iomem
>>>>>>> ...
>>>>>>> 140000000-33fffffff : Persistent Memory
>>>>>>> 140000000-1481fffff : namespace0.0
>>>>>>> 150000000-33fffffff : dax0.0
>>>>>>> 150000000-33fffffff : System RAM (driver managed)
>>>>>>>
>>>>>>> vs.
>>>>>>>
>>>>>>> :/# cat /proc/iomem
>>>>>>> [...]
>>>>>>> 140000000-333ffffff : virtio-mem (virtio0)
>>>>>>> 140000000-147ffffff : System RAM (driver managed)
>>>>>>> 148000000-14fffffff : System RAM (driver managed)
>>>>>>> 150000000-157ffffff : System RAM (driver managed)
>>>>>>>
>>>>>>> Good enough for my taste.
>>>>>>>
>>>>>>>> What about adding an optional /sys/firmware/memmap/X/parent attribute.
>>>>>>>
>>>>>>> I really don't want any firmware memmap entries for something that is
>>>>>>> not part of the firmware provided memmap. In addition,
>>>>>>> /sys/firmware/memmap/ is still a fairly x86_64 specific thing. Only mips
>>>>>>> and two arm configs enable it at all.
>>>>>>>
>>>>>>> So, IMHO, /sys/firmware/memmap/ is definitely not the way to go.
>>>>>>
>>>>>> I think that's a policy decision and policy decisions do not belong in
>>>>>> the kernel. Give the tooling the opportunity to decide whether System
>>>>>> RAM stays that way over a kexec. The parenthetical reference otherwise
>>>>>> looks out of place to me in the /proc/iomem output. What makes it
>>>>>> "driver managed" is how the kernel handles it, not how the kernel
>>>>>> names it.
>>>>>
>>>>> At least, virtio-mem is different. It really *has to be handled* by the
>>>>> driver. This is not a policy. It's how it works.
>>>
>>> ...but that's not necessarily how dax/kmem works.
>>>
>>
>> Yes, and user space could still take that memory and add it to the
>> firmware memmap if it really wants to. It knows that it is special. It
>> can figure out that it belongs to a dax device using /proc/iomem.
>>
>>>>>
>>>>
>>>> Oh, and I don't see why "System RAM (driver managed)" would hinder any
>>>> policy in user case to still do what it thinks is the right thing to do
>>>> (e.g., for dax).
>>>>
>>>> "System RAM (driver managed)" would mean: Memory is not part of the raw
>>>> firmware memmap. It was detected and added by a driver. Handle with
>>>> care, this is special.
>>>
>>> Oh, no, I was more reacting to your, "don't update
>>> /sys/firmware/memmap for the (driver managed) range" choice as being a
>>> policy decision. It otherwise feels to me "System RAM (driver
>>> managed)" adds confusion for casual users of /proc/iomem and for clued
>>> in tools they have the parent association to decide policy.
>>
>> Not sure if I understand correctly, so bear with me :).
>>
>> Adding or not adding stuff to /sys/firmware/memmap is not a policy
>> decision. If it's not part of the raw firmware-provided memmap, it has
>> nothing to do in /sys/firmware/memmap. That's what the documentation
>> from 2008 tells us.
>
> It just occurs to me that there are valid cases for both wanting to
> start over with driver managed memory with a kexec and keeping it in
> the map.
Yes, there might be valid cases. My gut feeling is that in the general
case, you want to let the kexec kernel implement a policy/ let the user
in the new system decide.
But as I said, you can implement in kexec-tools whatever policy you
want. It has access to all information.
> Consider the case of EFI Special Purpose (SP) Memory that is
> marked EFI Conventional Memory with the SP attribute. In that case the
> firmware memory map marked it as conventional RAM, but the kernel
> optionally marks it as System RAM vs Soft Reserved. The 2008 patch
> simply does not consider that case. I'm not sure strict textualism
> works for coding decisions.
I am no expert on that matter (esp EFI). But looking at the users of
firmware_map_add_early(), the single user is in arch/x86/kernel/e820.c
. So the single source of /sys/firmware/memmap is (besides hotplug) e820.
"'e820_table_firmware': the original firmware version passed to us by
the bootloader - not modified by the kernel. ... inform the user about
the firmware's notion of memory layout via /sys/firmware/memmap"
(arch/x86/kernel/e820.c)
How is the EFI Special Purpose (SP) Memory represented in e820?
/sys/firmware/memmap is really simple: just dump in e820. No policies IIUC.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: Dan Williams @ 2020-05-01 18:43 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He
In-Reply-To: <9f3a813e-dc1d-b675-6e69-85beed3057a4@redhat.com>
On Fri, May 1, 2020 at 11:14 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 01.05.20 20:03, Dan Williams wrote:
> > On Fri, May 1, 2020 at 10:51 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 01.05.20 19:45, David Hildenbrand wrote:
> >>> On 01.05.20 19:39, Dan Williams wrote:
> >>>> On Fri, May 1, 2020 at 10:21 AM David Hildenbrand <david@redhat.com> wrote:
> >>>>>
> >>>>> On 01.05.20 18:56, Dan Williams wrote:
> >>>>>> On Fri, May 1, 2020 at 2:34 AM David Hildenbrand <david@redhat.com> wrote:
> >>>>>>>
> >>>>>>> On 01.05.20 00:24, Andrew Morton wrote:
> >>>>>>>> On Thu, 30 Apr 2020 20:43:39 +0200 David Hildenbrand <david@redhat.com> wrote:
> >>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Why does the firmware map support hotplug entries?
> >>>>>>>>>
> >>>>>>>>> I assume:
> >>>>>>>>>
> >>>>>>>>> The firmware memmap was added primarily for x86-64 kexec (and still, is
> >>>>>>>>> mostly used on x86-64 only IIRC). There, we had ACPI hotplug. When DIMMs
> >>>>>>>>> get hotplugged on real HW, they get added to e820. Same applies to
> >>>>>>>>> memory added via HyperV balloon (unless memory is unplugged via
> >>>>>>>>> ballooning and you reboot ... the the e820 is changed as well). I assume
> >>>>>>>>> we wanted to be able to reflect that, to make kexec look like a real reboot.
> >>>>>>>>>
> >>>>>>>>> This worked for a while. Then came dax/kmem. Now comes virtio-mem.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> But I assume only Andrew can enlighten us.
> >>>>>>>>>
> >>>>>>>>> @Andrew, any guidance here? Should we really add all memory to the
> >>>>>>>>> firmware memmap, even if this contradicts with the existing
> >>>>>>>>> documentation? (especially, if the actual firmware memmap will *not*
> >>>>>>>>> contain that memory after a reboot)
> >>>>>>>>
> >>>>>>>> For some reason that patch is misattributed - it was authored by
> >>>>>>>> Shaohui Zheng <shaohui.zheng@intel.com>, who hasn't been heard from in
> >>>>>>>> a decade. I looked through the email discussion from that time and I'm
> >>>>>>>> not seeing anything useful. But I wasn't able to locate Dave Hansen's
> >>>>>>>> review comments.
> >>>>>>>
> >>>>>>> Okay, thanks for checking. I think the documentation from 2008 is pretty
> >>>>>>> clear what has to be done here. I will add some of these details to the
> >>>>>>> patch description.
> >>>>>>>
> >>>>>>> Also, now that I know that esp. kexec-tools already don't consider
> >>>>>>> dax/kmem memory properly (memory will not get dumped via kdump) and
> >>>>>>> won't really suffer from a name change in /proc/iomem, I will go back to
> >>>>>>> the MHP_DRIVER_MANAGED approach and
> >>>>>>> 1. Don't create firmware memmap entries
> >>>>>>> 2. Name the resource "System RAM (driver managed)"
> >>>>>>> 3. Flag the resource via something like IORESOURCE_MEM_DRIVER_MANAGED.
> >>>>>>>
> >>>>>>> This way, kernel users and user space can figure out that this memory
> >>>>>>> has different semantics and handle it accordingly - I think that was
> >>>>>>> what Eric was asking for.
> >>>>>>>
> >>>>>>> Of course, open for suggestions.
> >>>>>>
> >>>>>> I'm still more of a fan of this being communicated by "System RAM"
> >>>>>
> >>>>> I was mentioning somewhere in this thread that "System RAM" inside a
> >>>>> hierarchy (like dax/kmem) will already be basically ignored by
> >>>>> kexec-tools. So, placing it inside a hierarchy already makes it look
> >>>>> special already.
> >>>>>
> >>>>> But after all, as we have to change kexec-tools either way, we can
> >>>>> directly go ahead and flag it properly as special (in case there will
> >>>>> ever be other cases where we could no longer distinguish it).
> >>>>>
> >>>>>> being parented especially because that tells you something about how
> >>>>>> the memory is driver-managed and which mechanism might be in play.
> >>>>>
> >>>>> The could be communicated to some degree via the resource hierarchy.
> >>>>>
> >>>>> E.g.,
> >>>>>
> >>>>> [root@localhost ~]# cat /proc/iomem
> >>>>> ...
> >>>>> 140000000-33fffffff : Persistent Memory
> >>>>> 140000000-1481fffff : namespace0.0
> >>>>> 150000000-33fffffff : dax0.0
> >>>>> 150000000-33fffffff : System RAM (driver managed)
> >>>>>
> >>>>> vs.
> >>>>>
> >>>>> :/# cat /proc/iomem
> >>>>> [...]
> >>>>> 140000000-333ffffff : virtio-mem (virtio0)
> >>>>> 140000000-147ffffff : System RAM (driver managed)
> >>>>> 148000000-14fffffff : System RAM (driver managed)
> >>>>> 150000000-157ffffff : System RAM (driver managed)
> >>>>>
> >>>>> Good enough for my taste.
> >>>>>
> >>>>>> What about adding an optional /sys/firmware/memmap/X/parent attribute.
> >>>>>
> >>>>> I really don't want any firmware memmap entries for something that is
> >>>>> not part of the firmware provided memmap. In addition,
> >>>>> /sys/firmware/memmap/ is still a fairly x86_64 specific thing. Only mips
> >>>>> and two arm configs enable it at all.
> >>>>>
> >>>>> So, IMHO, /sys/firmware/memmap/ is definitely not the way to go.
> >>>>
> >>>> I think that's a policy decision and policy decisions do not belong in
> >>>> the kernel. Give the tooling the opportunity to decide whether System
> >>>> RAM stays that way over a kexec. The parenthetical reference otherwise
> >>>> looks out of place to me in the /proc/iomem output. What makes it
> >>>> "driver managed" is how the kernel handles it, not how the kernel
> >>>> names it.
> >>>
> >>> At least, virtio-mem is different. It really *has to be handled* by the
> >>> driver. This is not a policy. It's how it works.
> >
> > ...but that's not necessarily how dax/kmem works.
> >
>
> Yes, and user space could still take that memory and add it to the
> firmware memmap if it really wants to. It knows that it is special. It
> can figure out that it belongs to a dax device using /proc/iomem.
>
> >>>
> >>
> >> Oh, and I don't see why "System RAM (driver managed)" would hinder any
> >> policy in user case to still do what it thinks is the right thing to do
> >> (e.g., for dax).
> >>
> >> "System RAM (driver managed)" would mean: Memory is not part of the raw
> >> firmware memmap. It was detected and added by a driver. Handle with
> >> care, this is special.
> >
> > Oh, no, I was more reacting to your, "don't update
> > /sys/firmware/memmap for the (driver managed) range" choice as being a
> > policy decision. It otherwise feels to me "System RAM (driver
> > managed)" adds confusion for casual users of /proc/iomem and for clued
> > in tools they have the parent association to decide policy.
>
> Not sure if I understand correctly, so bear with me :).
>
> Adding or not adding stuff to /sys/firmware/memmap is not a policy
> decision. If it's not part of the raw firmware-provided memmap, it has
> nothing to do in /sys/firmware/memmap. That's what the documentation
> from 2008 tells us.
It just occurs to me that there are valid cases for both wanting to
start over with driver managed memory with a kexec and keeping it in
the map. Consider the case of EFI Special Purpose (SP) Memory that is
marked EFI Conventional Memory with the SP attribute. In that case the
firmware memory map marked it as conventional RAM, but the kernel
optionally marks it as System RAM vs Soft Reserved. The 2008 patch
simply does not consider that case. I'm not sure strict textualism
works for coding decisions.
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: David Hildenbrand @ 2020-05-01 18:14 UTC (permalink / raw)
To: Dan Williams
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He
In-Reply-To: <CAPcyv4jGnR_fPtpKBC1rD2KRcT88bTkhqnTMmuwuc+f9Dwrz1g@mail.gmail.com>
On 01.05.20 20:03, Dan Williams wrote:
> On Fri, May 1, 2020 at 10:51 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 01.05.20 19:45, David Hildenbrand wrote:
>>> On 01.05.20 19:39, Dan Williams wrote:
>>>> On Fri, May 1, 2020 at 10:21 AM David Hildenbrand <david@redhat.com> wrote:
>>>>>
>>>>> On 01.05.20 18:56, Dan Williams wrote:
>>>>>> On Fri, May 1, 2020 at 2:34 AM David Hildenbrand <david@redhat.com> wrote:
>>>>>>>
>>>>>>> On 01.05.20 00:24, Andrew Morton wrote:
>>>>>>>> On Thu, 30 Apr 2020 20:43:39 +0200 David Hildenbrand <david@redhat.com> wrote:
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Why does the firmware map support hotplug entries?
>>>>>>>>>
>>>>>>>>> I assume:
>>>>>>>>>
>>>>>>>>> The firmware memmap was added primarily for x86-64 kexec (and still, is
>>>>>>>>> mostly used on x86-64 only IIRC). There, we had ACPI hotplug. When DIMMs
>>>>>>>>> get hotplugged on real HW, they get added to e820. Same applies to
>>>>>>>>> memory added via HyperV balloon (unless memory is unplugged via
>>>>>>>>> ballooning and you reboot ... the the e820 is changed as well). I assume
>>>>>>>>> we wanted to be able to reflect that, to make kexec look like a real reboot.
>>>>>>>>>
>>>>>>>>> This worked for a while. Then came dax/kmem. Now comes virtio-mem.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> But I assume only Andrew can enlighten us.
>>>>>>>>>
>>>>>>>>> @Andrew, any guidance here? Should we really add all memory to the
>>>>>>>>> firmware memmap, even if this contradicts with the existing
>>>>>>>>> documentation? (especially, if the actual firmware memmap will *not*
>>>>>>>>> contain that memory after a reboot)
>>>>>>>>
>>>>>>>> For some reason that patch is misattributed - it was authored by
>>>>>>>> Shaohui Zheng <shaohui.zheng@intel.com>, who hasn't been heard from in
>>>>>>>> a decade. I looked through the email discussion from that time and I'm
>>>>>>>> not seeing anything useful. But I wasn't able to locate Dave Hansen's
>>>>>>>> review comments.
>>>>>>>
>>>>>>> Okay, thanks for checking. I think the documentation from 2008 is pretty
>>>>>>> clear what has to be done here. I will add some of these details to the
>>>>>>> patch description.
>>>>>>>
>>>>>>> Also, now that I know that esp. kexec-tools already don't consider
>>>>>>> dax/kmem memory properly (memory will not get dumped via kdump) and
>>>>>>> won't really suffer from a name change in /proc/iomem, I will go back to
>>>>>>> the MHP_DRIVER_MANAGED approach and
>>>>>>> 1. Don't create firmware memmap entries
>>>>>>> 2. Name the resource "System RAM (driver managed)"
>>>>>>> 3. Flag the resource via something like IORESOURCE_MEM_DRIVER_MANAGED.
>>>>>>>
>>>>>>> This way, kernel users and user space can figure out that this memory
>>>>>>> has different semantics and handle it accordingly - I think that was
>>>>>>> what Eric was asking for.
>>>>>>>
>>>>>>> Of course, open for suggestions.
>>>>>>
>>>>>> I'm still more of a fan of this being communicated by "System RAM"
>>>>>
>>>>> I was mentioning somewhere in this thread that "System RAM" inside a
>>>>> hierarchy (like dax/kmem) will already be basically ignored by
>>>>> kexec-tools. So, placing it inside a hierarchy already makes it look
>>>>> special already.
>>>>>
>>>>> But after all, as we have to change kexec-tools either way, we can
>>>>> directly go ahead and flag it properly as special (in case there will
>>>>> ever be other cases where we could no longer distinguish it).
>>>>>
>>>>>> being parented especially because that tells you something about how
>>>>>> the memory is driver-managed and which mechanism might be in play.
>>>>>
>>>>> The could be communicated to some degree via the resource hierarchy.
>>>>>
>>>>> E.g.,
>>>>>
>>>>> [root@localhost ~]# cat /proc/iomem
>>>>> ...
>>>>> 140000000-33fffffff : Persistent Memory
>>>>> 140000000-1481fffff : namespace0.0
>>>>> 150000000-33fffffff : dax0.0
>>>>> 150000000-33fffffff : System RAM (driver managed)
>>>>>
>>>>> vs.
>>>>>
>>>>> :/# cat /proc/iomem
>>>>> [...]
>>>>> 140000000-333ffffff : virtio-mem (virtio0)
>>>>> 140000000-147ffffff : System RAM (driver managed)
>>>>> 148000000-14fffffff : System RAM (driver managed)
>>>>> 150000000-157ffffff : System RAM (driver managed)
>>>>>
>>>>> Good enough for my taste.
>>>>>
>>>>>> What about adding an optional /sys/firmware/memmap/X/parent attribute.
>>>>>
>>>>> I really don't want any firmware memmap entries for something that is
>>>>> not part of the firmware provided memmap. In addition,
>>>>> /sys/firmware/memmap/ is still a fairly x86_64 specific thing. Only mips
>>>>> and two arm configs enable it at all.
>>>>>
>>>>> So, IMHO, /sys/firmware/memmap/ is definitely not the way to go.
>>>>
>>>> I think that's a policy decision and policy decisions do not belong in
>>>> the kernel. Give the tooling the opportunity to decide whether System
>>>> RAM stays that way over a kexec. The parenthetical reference otherwise
>>>> looks out of place to me in the /proc/iomem output. What makes it
>>>> "driver managed" is how the kernel handles it, not how the kernel
>>>> names it.
>>>
>>> At least, virtio-mem is different. It really *has to be handled* by the
>>> driver. This is not a policy. It's how it works.
>
> ...but that's not necessarily how dax/kmem works.
>
Yes, and user space could still take that memory and add it to the
firmware memmap if it really wants to. It knows that it is special. It
can figure out that it belongs to a dax device using /proc/iomem.
>>>
>>
>> Oh, and I don't see why "System RAM (driver managed)" would hinder any
>> policy in user case to still do what it thinks is the right thing to do
>> (e.g., for dax).
>>
>> "System RAM (driver managed)" would mean: Memory is not part of the raw
>> firmware memmap. It was detected and added by a driver. Handle with
>> care, this is special.
>
> Oh, no, I was more reacting to your, "don't update
> /sys/firmware/memmap for the (driver managed) range" choice as being a
> policy decision. It otherwise feels to me "System RAM (driver
> managed)" adds confusion for casual users of /proc/iomem and for clued
> in tools they have the parent association to decide policy.
Not sure if I understand correctly, so bear with me :).
Adding or not adding stuff to /sys/firmware/memmap is not a policy
decision. If it's not part of the raw firmware-provided memmap, it has
nothing to do in /sys/firmware/memmap. That's what the documentation
from 2008 tells us.
Again, my point is that we don't create /sys/firmware/memmap entries for
dax/kmem and virtio-mem memory - because it's not part of the raw
firmware-provided memmap. I was not suggesting to add something like
"System RAM (driver managed)" there instead, maybe that part was confusing.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: Dan Williams @ 2020-05-01 18:03 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He
In-Reply-To: <0169e822-a6cc-1543-88ed-2a85d95ffb93@redhat.com>
On Fri, May 1, 2020 at 10:51 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 01.05.20 19:45, David Hildenbrand wrote:
> > On 01.05.20 19:39, Dan Williams wrote:
> >> On Fri, May 1, 2020 at 10:21 AM David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>> On 01.05.20 18:56, Dan Williams wrote:
> >>>> On Fri, May 1, 2020 at 2:34 AM David Hildenbrand <david@redhat.com> wrote:
> >>>>>
> >>>>> On 01.05.20 00:24, Andrew Morton wrote:
> >>>>>> On Thu, 30 Apr 2020 20:43:39 +0200 David Hildenbrand <david@redhat.com> wrote:
> >>>>>>
> >>>>>>>>
> >>>>>>>> Why does the firmware map support hotplug entries?
> >>>>>>>
> >>>>>>> I assume:
> >>>>>>>
> >>>>>>> The firmware memmap was added primarily for x86-64 kexec (and still, is
> >>>>>>> mostly used on x86-64 only IIRC). There, we had ACPI hotplug. When DIMMs
> >>>>>>> get hotplugged on real HW, they get added to e820. Same applies to
> >>>>>>> memory added via HyperV balloon (unless memory is unplugged via
> >>>>>>> ballooning and you reboot ... the the e820 is changed as well). I assume
> >>>>>>> we wanted to be able to reflect that, to make kexec look like a real reboot.
> >>>>>>>
> >>>>>>> This worked for a while. Then came dax/kmem. Now comes virtio-mem.
> >>>>>>>
> >>>>>>>
> >>>>>>> But I assume only Andrew can enlighten us.
> >>>>>>>
> >>>>>>> @Andrew, any guidance here? Should we really add all memory to the
> >>>>>>> firmware memmap, even if this contradicts with the existing
> >>>>>>> documentation? (especially, if the actual firmware memmap will *not*
> >>>>>>> contain that memory after a reboot)
> >>>>>>
> >>>>>> For some reason that patch is misattributed - it was authored by
> >>>>>> Shaohui Zheng <shaohui.zheng@intel.com>, who hasn't been heard from in
> >>>>>> a decade. I looked through the email discussion from that time and I'm
> >>>>>> not seeing anything useful. But I wasn't able to locate Dave Hansen's
> >>>>>> review comments.
> >>>>>
> >>>>> Okay, thanks for checking. I think the documentation from 2008 is pretty
> >>>>> clear what has to be done here. I will add some of these details to the
> >>>>> patch description.
> >>>>>
> >>>>> Also, now that I know that esp. kexec-tools already don't consider
> >>>>> dax/kmem memory properly (memory will not get dumped via kdump) and
> >>>>> won't really suffer from a name change in /proc/iomem, I will go back to
> >>>>> the MHP_DRIVER_MANAGED approach and
> >>>>> 1. Don't create firmware memmap entries
> >>>>> 2. Name the resource "System RAM (driver managed)"
> >>>>> 3. Flag the resource via something like IORESOURCE_MEM_DRIVER_MANAGED.
> >>>>>
> >>>>> This way, kernel users and user space can figure out that this memory
> >>>>> has different semantics and handle it accordingly - I think that was
> >>>>> what Eric was asking for.
> >>>>>
> >>>>> Of course, open for suggestions.
> >>>>
> >>>> I'm still more of a fan of this being communicated by "System RAM"
> >>>
> >>> I was mentioning somewhere in this thread that "System RAM" inside a
> >>> hierarchy (like dax/kmem) will already be basically ignored by
> >>> kexec-tools. So, placing it inside a hierarchy already makes it look
> >>> special already.
> >>>
> >>> But after all, as we have to change kexec-tools either way, we can
> >>> directly go ahead and flag it properly as special (in case there will
> >>> ever be other cases where we could no longer distinguish it).
> >>>
> >>>> being parented especially because that tells you something about how
> >>>> the memory is driver-managed and which mechanism might be in play.
> >>>
> >>> The could be communicated to some degree via the resource hierarchy.
> >>>
> >>> E.g.,
> >>>
> >>> [root@localhost ~]# cat /proc/iomem
> >>> ...
> >>> 140000000-33fffffff : Persistent Memory
> >>> 140000000-1481fffff : namespace0.0
> >>> 150000000-33fffffff : dax0.0
> >>> 150000000-33fffffff : System RAM (driver managed)
> >>>
> >>> vs.
> >>>
> >>> :/# cat /proc/iomem
> >>> [...]
> >>> 140000000-333ffffff : virtio-mem (virtio0)
> >>> 140000000-147ffffff : System RAM (driver managed)
> >>> 148000000-14fffffff : System RAM (driver managed)
> >>> 150000000-157ffffff : System RAM (driver managed)
> >>>
> >>> Good enough for my taste.
> >>>
> >>>> What about adding an optional /sys/firmware/memmap/X/parent attribute.
> >>>
> >>> I really don't want any firmware memmap entries for something that is
> >>> not part of the firmware provided memmap. In addition,
> >>> /sys/firmware/memmap/ is still a fairly x86_64 specific thing. Only mips
> >>> and two arm configs enable it at all.
> >>>
> >>> So, IMHO, /sys/firmware/memmap/ is definitely not the way to go.
> >>
> >> I think that's a policy decision and policy decisions do not belong in
> >> the kernel. Give the tooling the opportunity to decide whether System
> >> RAM stays that way over a kexec. The parenthetical reference otherwise
> >> looks out of place to me in the /proc/iomem output. What makes it
> >> "driver managed" is how the kernel handles it, not how the kernel
> >> names it.
> >
> > At least, virtio-mem is different. It really *has to be handled* by the
> > driver. This is not a policy. It's how it works.
...but that's not necessarily how dax/kmem works.
> >
>
> Oh, and I don't see why "System RAM (driver managed)" would hinder any
> policy in user case to still do what it thinks is the right thing to do
> (e.g., for dax).
>
> "System RAM (driver managed)" would mean: Memory is not part of the raw
> firmware memmap. It was detected and added by a driver. Handle with
> care, this is special.
Oh, no, I was more reacting to your, "don't update
/sys/firmware/memmap for the (driver managed) range" choice as being a
policy decision. It otherwise feels to me "System RAM (driver
managed)" adds confusion for casual users of /proc/iomem and for clued
in tools they have the parent association to decide policy.
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: David Hildenbrand @ 2020-05-01 17:51 UTC (permalink / raw)
To: Dan Williams
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He
In-Reply-To: <62dd4ce2-86cc-5b85-734f-ec8766528a1b@redhat.com>
On 01.05.20 19:45, David Hildenbrand wrote:
> On 01.05.20 19:39, Dan Williams wrote:
>> On Fri, May 1, 2020 at 10:21 AM David Hildenbrand <david@redhat.com> wrote:
>>>
>>> On 01.05.20 18:56, Dan Williams wrote:
>>>> On Fri, May 1, 2020 at 2:34 AM David Hildenbrand <david@redhat.com> wrote:
>>>>>
>>>>> On 01.05.20 00:24, Andrew Morton wrote:
>>>>>> On Thu, 30 Apr 2020 20:43:39 +0200 David Hildenbrand <david@redhat.com> wrote:
>>>>>>
>>>>>>>>
>>>>>>>> Why does the firmware map support hotplug entries?
>>>>>>>
>>>>>>> I assume:
>>>>>>>
>>>>>>> The firmware memmap was added primarily for x86-64 kexec (and still, is
>>>>>>> mostly used on x86-64 only IIRC). There, we had ACPI hotplug. When DIMMs
>>>>>>> get hotplugged on real HW, they get added to e820. Same applies to
>>>>>>> memory added via HyperV balloon (unless memory is unplugged via
>>>>>>> ballooning and you reboot ... the the e820 is changed as well). I assume
>>>>>>> we wanted to be able to reflect that, to make kexec look like a real reboot.
>>>>>>>
>>>>>>> This worked for a while. Then came dax/kmem. Now comes virtio-mem.
>>>>>>>
>>>>>>>
>>>>>>> But I assume only Andrew can enlighten us.
>>>>>>>
>>>>>>> @Andrew, any guidance here? Should we really add all memory to the
>>>>>>> firmware memmap, even if this contradicts with the existing
>>>>>>> documentation? (especially, if the actual firmware memmap will *not*
>>>>>>> contain that memory after a reboot)
>>>>>>
>>>>>> For some reason that patch is misattributed - it was authored by
>>>>>> Shaohui Zheng <shaohui.zheng@intel.com>, who hasn't been heard from in
>>>>>> a decade. I looked through the email discussion from that time and I'm
>>>>>> not seeing anything useful. But I wasn't able to locate Dave Hansen's
>>>>>> review comments.
>>>>>
>>>>> Okay, thanks for checking. I think the documentation from 2008 is pretty
>>>>> clear what has to be done here. I will add some of these details to the
>>>>> patch description.
>>>>>
>>>>> Also, now that I know that esp. kexec-tools already don't consider
>>>>> dax/kmem memory properly (memory will not get dumped via kdump) and
>>>>> won't really suffer from a name change in /proc/iomem, I will go back to
>>>>> the MHP_DRIVER_MANAGED approach and
>>>>> 1. Don't create firmware memmap entries
>>>>> 2. Name the resource "System RAM (driver managed)"
>>>>> 3. Flag the resource via something like IORESOURCE_MEM_DRIVER_MANAGED.
>>>>>
>>>>> This way, kernel users and user space can figure out that this memory
>>>>> has different semantics and handle it accordingly - I think that was
>>>>> what Eric was asking for.
>>>>>
>>>>> Of course, open for suggestions.
>>>>
>>>> I'm still more of a fan of this being communicated by "System RAM"
>>>
>>> I was mentioning somewhere in this thread that "System RAM" inside a
>>> hierarchy (like dax/kmem) will already be basically ignored by
>>> kexec-tools. So, placing it inside a hierarchy already makes it look
>>> special already.
>>>
>>> But after all, as we have to change kexec-tools either way, we can
>>> directly go ahead and flag it properly as special (in case there will
>>> ever be other cases where we could no longer distinguish it).
>>>
>>>> being parented especially because that tells you something about how
>>>> the memory is driver-managed and which mechanism might be in play.
>>>
>>> The could be communicated to some degree via the resource hierarchy.
>>>
>>> E.g.,
>>>
>>> [root@localhost ~]# cat /proc/iomem
>>> ...
>>> 140000000-33fffffff : Persistent Memory
>>> 140000000-1481fffff : namespace0.0
>>> 150000000-33fffffff : dax0.0
>>> 150000000-33fffffff : System RAM (driver managed)
>>>
>>> vs.
>>>
>>> :/# cat /proc/iomem
>>> [...]
>>> 140000000-333ffffff : virtio-mem (virtio0)
>>> 140000000-147ffffff : System RAM (driver managed)
>>> 148000000-14fffffff : System RAM (driver managed)
>>> 150000000-157ffffff : System RAM (driver managed)
>>>
>>> Good enough for my taste.
>>>
>>>> What about adding an optional /sys/firmware/memmap/X/parent attribute.
>>>
>>> I really don't want any firmware memmap entries for something that is
>>> not part of the firmware provided memmap. In addition,
>>> /sys/firmware/memmap/ is still a fairly x86_64 specific thing. Only mips
>>> and two arm configs enable it at all.
>>>
>>> So, IMHO, /sys/firmware/memmap/ is definitely not the way to go.
>>
>> I think that's a policy decision and policy decisions do not belong in
>> the kernel. Give the tooling the opportunity to decide whether System
>> RAM stays that way over a kexec. The parenthetical reference otherwise
>> looks out of place to me in the /proc/iomem output. What makes it
>> "driver managed" is how the kernel handles it, not how the kernel
>> names it.
>
> At least, virtio-mem is different. It really *has to be handled* by the
> driver. This is not a policy. It's how it works.
>
Oh, and I don't see why "System RAM (driver managed)" would hinder any
policy in user case to still do what it thinks is the right thing to do
(e.g., for dax).
"System RAM (driver managed)" would mean: Memory is not part of the raw
firmware memmap. It was detected and added by a driver. Handle with
care, this is special.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: David Hildenbrand @ 2020-05-01 17:45 UTC (permalink / raw)
To: Dan Williams
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He
In-Reply-To: <CAPcyv4iOqS0Wbfa2KPfE1axQFGXoRB4mmPRP__Lmqpw6Qpr_ig@mail.gmail.com>
On 01.05.20 19:39, Dan Williams wrote:
> On Fri, May 1, 2020 at 10:21 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 01.05.20 18:56, Dan Williams wrote:
>>> On Fri, May 1, 2020 at 2:34 AM David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 01.05.20 00:24, Andrew Morton wrote:
>>>>> On Thu, 30 Apr 2020 20:43:39 +0200 David Hildenbrand <david@redhat.com> wrote:
>>>>>
>>>>>>>
>>>>>>> Why does the firmware map support hotplug entries?
>>>>>>
>>>>>> I assume:
>>>>>>
>>>>>> The firmware memmap was added primarily for x86-64 kexec (and still, is
>>>>>> mostly used on x86-64 only IIRC). There, we had ACPI hotplug. When DIMMs
>>>>>> get hotplugged on real HW, they get added to e820. Same applies to
>>>>>> memory added via HyperV balloon (unless memory is unplugged via
>>>>>> ballooning and you reboot ... the the e820 is changed as well). I assume
>>>>>> we wanted to be able to reflect that, to make kexec look like a real reboot.
>>>>>>
>>>>>> This worked for a while. Then came dax/kmem. Now comes virtio-mem.
>>>>>>
>>>>>>
>>>>>> But I assume only Andrew can enlighten us.
>>>>>>
>>>>>> @Andrew, any guidance here? Should we really add all memory to the
>>>>>> firmware memmap, even if this contradicts with the existing
>>>>>> documentation? (especially, if the actual firmware memmap will *not*
>>>>>> contain that memory after a reboot)
>>>>>
>>>>> For some reason that patch is misattributed - it was authored by
>>>>> Shaohui Zheng <shaohui.zheng@intel.com>, who hasn't been heard from in
>>>>> a decade. I looked through the email discussion from that time and I'm
>>>>> not seeing anything useful. But I wasn't able to locate Dave Hansen's
>>>>> review comments.
>>>>
>>>> Okay, thanks for checking. I think the documentation from 2008 is pretty
>>>> clear what has to be done here. I will add some of these details to the
>>>> patch description.
>>>>
>>>> Also, now that I know that esp. kexec-tools already don't consider
>>>> dax/kmem memory properly (memory will not get dumped via kdump) and
>>>> won't really suffer from a name change in /proc/iomem, I will go back to
>>>> the MHP_DRIVER_MANAGED approach and
>>>> 1. Don't create firmware memmap entries
>>>> 2. Name the resource "System RAM (driver managed)"
>>>> 3. Flag the resource via something like IORESOURCE_MEM_DRIVER_MANAGED.
>>>>
>>>> This way, kernel users and user space can figure out that this memory
>>>> has different semantics and handle it accordingly - I think that was
>>>> what Eric was asking for.
>>>>
>>>> Of course, open for suggestions.
>>>
>>> I'm still more of a fan of this being communicated by "System RAM"
>>
>> I was mentioning somewhere in this thread that "System RAM" inside a
>> hierarchy (like dax/kmem) will already be basically ignored by
>> kexec-tools. So, placing it inside a hierarchy already makes it look
>> special already.
>>
>> But after all, as we have to change kexec-tools either way, we can
>> directly go ahead and flag it properly as special (in case there will
>> ever be other cases where we could no longer distinguish it).
>>
>>> being parented especially because that tells you something about how
>>> the memory is driver-managed and which mechanism might be in play.
>>
>> The could be communicated to some degree via the resource hierarchy.
>>
>> E.g.,
>>
>> [root@localhost ~]# cat /proc/iomem
>> ...
>> 140000000-33fffffff : Persistent Memory
>> 140000000-1481fffff : namespace0.0
>> 150000000-33fffffff : dax0.0
>> 150000000-33fffffff : System RAM (driver managed)
>>
>> vs.
>>
>> :/# cat /proc/iomem
>> [...]
>> 140000000-333ffffff : virtio-mem (virtio0)
>> 140000000-147ffffff : System RAM (driver managed)
>> 148000000-14fffffff : System RAM (driver managed)
>> 150000000-157ffffff : System RAM (driver managed)
>>
>> Good enough for my taste.
>>
>>> What about adding an optional /sys/firmware/memmap/X/parent attribute.
>>
>> I really don't want any firmware memmap entries for something that is
>> not part of the firmware provided memmap. In addition,
>> /sys/firmware/memmap/ is still a fairly x86_64 specific thing. Only mips
>> and two arm configs enable it at all.
>>
>> So, IMHO, /sys/firmware/memmap/ is definitely not the way to go.
>
> I think that's a policy decision and policy decisions do not belong in
> the kernel. Give the tooling the opportunity to decide whether System
> RAM stays that way over a kexec. The parenthetical reference otherwise
> looks out of place to me in the /proc/iomem output. What makes it
> "driver managed" is how the kernel handles it, not how the kernel
> names it.
At least, virtio-mem is different. It really *has to be handled* by the
driver. This is not a policy. It's how it works.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: Dan Williams @ 2020-05-01 17:39 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He
In-Reply-To: <2d019c11-a478-9d70-abd5-4fd2ebf4bc1d@redhat.com>
On Fri, May 1, 2020 at 10:21 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 01.05.20 18:56, Dan Williams wrote:
> > On Fri, May 1, 2020 at 2:34 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 01.05.20 00:24, Andrew Morton wrote:
> >>> On Thu, 30 Apr 2020 20:43:39 +0200 David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>>>>
> >>>>> Why does the firmware map support hotplug entries?
> >>>>
> >>>> I assume:
> >>>>
> >>>> The firmware memmap was added primarily for x86-64 kexec (and still, is
> >>>> mostly used on x86-64 only IIRC). There, we had ACPI hotplug. When DIMMs
> >>>> get hotplugged on real HW, they get added to e820. Same applies to
> >>>> memory added via HyperV balloon (unless memory is unplugged via
> >>>> ballooning and you reboot ... the the e820 is changed as well). I assume
> >>>> we wanted to be able to reflect that, to make kexec look like a real reboot.
> >>>>
> >>>> This worked for a while. Then came dax/kmem. Now comes virtio-mem.
> >>>>
> >>>>
> >>>> But I assume only Andrew can enlighten us.
> >>>>
> >>>> @Andrew, any guidance here? Should we really add all memory to the
> >>>> firmware memmap, even if this contradicts with the existing
> >>>> documentation? (especially, if the actual firmware memmap will *not*
> >>>> contain that memory after a reboot)
> >>>
> >>> For some reason that patch is misattributed - it was authored by
> >>> Shaohui Zheng <shaohui.zheng@intel.com>, who hasn't been heard from in
> >>> a decade. I looked through the email discussion from that time and I'm
> >>> not seeing anything useful. But I wasn't able to locate Dave Hansen's
> >>> review comments.
> >>
> >> Okay, thanks for checking. I think the documentation from 2008 is pretty
> >> clear what has to be done here. I will add some of these details to the
> >> patch description.
> >>
> >> Also, now that I know that esp. kexec-tools already don't consider
> >> dax/kmem memory properly (memory will not get dumped via kdump) and
> >> won't really suffer from a name change in /proc/iomem, I will go back to
> >> the MHP_DRIVER_MANAGED approach and
> >> 1. Don't create firmware memmap entries
> >> 2. Name the resource "System RAM (driver managed)"
> >> 3. Flag the resource via something like IORESOURCE_MEM_DRIVER_MANAGED.
> >>
> >> This way, kernel users and user space can figure out that this memory
> >> has different semantics and handle it accordingly - I think that was
> >> what Eric was asking for.
> >>
> >> Of course, open for suggestions.
> >
> > I'm still more of a fan of this being communicated by "System RAM"
>
> I was mentioning somewhere in this thread that "System RAM" inside a
> hierarchy (like dax/kmem) will already be basically ignored by
> kexec-tools. So, placing it inside a hierarchy already makes it look
> special already.
>
> But after all, as we have to change kexec-tools either way, we can
> directly go ahead and flag it properly as special (in case there will
> ever be other cases where we could no longer distinguish it).
>
> > being parented especially because that tells you something about how
> > the memory is driver-managed and which mechanism might be in play.
>
> The could be communicated to some degree via the resource hierarchy.
>
> E.g.,
>
> [root@localhost ~]# cat /proc/iomem
> ...
> 140000000-33fffffff : Persistent Memory
> 140000000-1481fffff : namespace0.0
> 150000000-33fffffff : dax0.0
> 150000000-33fffffff : System RAM (driver managed)
>
> vs.
>
> :/# cat /proc/iomem
> [...]
> 140000000-333ffffff : virtio-mem (virtio0)
> 140000000-147ffffff : System RAM (driver managed)
> 148000000-14fffffff : System RAM (driver managed)
> 150000000-157ffffff : System RAM (driver managed)
>
> Good enough for my taste.
>
> > What about adding an optional /sys/firmware/memmap/X/parent attribute.
>
> I really don't want any firmware memmap entries for something that is
> not part of the firmware provided memmap. In addition,
> /sys/firmware/memmap/ is still a fairly x86_64 specific thing. Only mips
> and two arm configs enable it at all.
>
> So, IMHO, /sys/firmware/memmap/ is definitely not the way to go.
I think that's a policy decision and policy decisions do not belong in
the kernel. Give the tooling the opportunity to decide whether System
RAM stays that way over a kexec. The parenthetical reference otherwise
looks out of place to me in the /proc/iomem output. What makes it
"driver managed" is how the kernel handles it, not how the kernel
names it.
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: David Hildenbrand @ 2020-05-01 17:21 UTC (permalink / raw)
To: Dan Williams
Cc: Andrew Morton, Eric W. Biederman, Linux Kernel Mailing List,
Linux MM, virtio-dev, virtualization, linuxppc-dev, Linux ACPI,
linux-nvdimm, linux-hyperv, linux-s390, xen-devel, Michal Hocko,
Michael S . Tsirkin, Michal Hocko, Pankaj Gupta, Wei Yang,
Baoquan He
In-Reply-To: <CAPcyv4j=YKnr1HW4OhAmpzbuKjtfP7FdAn4-V7uA=b-Tcpfu+A@mail.gmail.com>
On 01.05.20 18:56, Dan Williams wrote:
> On Fri, May 1, 2020 at 2:34 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 01.05.20 00:24, Andrew Morton wrote:
>>> On Thu, 30 Apr 2020 20:43:39 +0200 David Hildenbrand <david@redhat.com> wrote:
>>>
>>>>>
>>>>> Why does the firmware map support hotplug entries?
>>>>
>>>> I assume:
>>>>
>>>> The firmware memmap was added primarily for x86-64 kexec (and still, is
>>>> mostly used on x86-64 only IIRC). There, we had ACPI hotplug. When DIMMs
>>>> get hotplugged on real HW, they get added to e820. Same applies to
>>>> memory added via HyperV balloon (unless memory is unplugged via
>>>> ballooning and you reboot ... the the e820 is changed as well). I assume
>>>> we wanted to be able to reflect that, to make kexec look like a real reboot.
>>>>
>>>> This worked for a while. Then came dax/kmem. Now comes virtio-mem.
>>>>
>>>>
>>>> But I assume only Andrew can enlighten us.
>>>>
>>>> @Andrew, any guidance here? Should we really add all memory to the
>>>> firmware memmap, even if this contradicts with the existing
>>>> documentation? (especially, if the actual firmware memmap will *not*
>>>> contain that memory after a reboot)
>>>
>>> For some reason that patch is misattributed - it was authored by
>>> Shaohui Zheng <shaohui.zheng@intel.com>, who hasn't been heard from in
>>> a decade. I looked through the email discussion from that time and I'm
>>> not seeing anything useful. But I wasn't able to locate Dave Hansen's
>>> review comments.
>>
>> Okay, thanks for checking. I think the documentation from 2008 is pretty
>> clear what has to be done here. I will add some of these details to the
>> patch description.
>>
>> Also, now that I know that esp. kexec-tools already don't consider
>> dax/kmem memory properly (memory will not get dumped via kdump) and
>> won't really suffer from a name change in /proc/iomem, I will go back to
>> the MHP_DRIVER_MANAGED approach and
>> 1. Don't create firmware memmap entries
>> 2. Name the resource "System RAM (driver managed)"
>> 3. Flag the resource via something like IORESOURCE_MEM_DRIVER_MANAGED.
>>
>> This way, kernel users and user space can figure out that this memory
>> has different semantics and handle it accordingly - I think that was
>> what Eric was asking for.
>>
>> Of course, open for suggestions.
>
> I'm still more of a fan of this being communicated by "System RAM"
I was mentioning somewhere in this thread that "System RAM" inside a
hierarchy (like dax/kmem) will already be basically ignored by
kexec-tools. So, placing it inside a hierarchy already makes it look
special already.
But after all, as we have to change kexec-tools either way, we can
directly go ahead and flag it properly as special (in case there will
ever be other cases where we could no longer distinguish it).
> being parented especially because that tells you something about how
> the memory is driver-managed and which mechanism might be in play.
The could be communicated to some degree via the resource hierarchy.
E.g.,
[root@localhost ~]# cat /proc/iomem
...
140000000-33fffffff : Persistent Memory
140000000-1481fffff : namespace0.0
150000000-33fffffff : dax0.0
150000000-33fffffff : System RAM (driver managed)
vs.
:/# cat /proc/iomem
[...]
140000000-333ffffff : virtio-mem (virtio0)
140000000-147ffffff : System RAM (driver managed)
148000000-14fffffff : System RAM (driver managed)
150000000-157ffffff : System RAM (driver managed)
Good enough for my taste.
> What about adding an optional /sys/firmware/memmap/X/parent attribute.
I really don't want any firmware memmap entries for something that is
not part of the firmware provided memmap. In addition,
/sys/firmware/memmap/ is still a fairly x86_64 specific thing. Only mips
and two arm configs enable it at all.
So, IMHO, /sys/firmware/memmap/ is definitely not the way to go.
--
Thanks,
David / dhildenb
^ 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