* Re: [PATCH] x86/hyperv: Disable preemption while setting reenlightenment vector
From: Vitaly Kuznetsov @ 2019-06-14 10:08 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Dmitry Safonov, linux-kernel, Prasanna Panchamukhi,
Andy Lutomirski, Borislav Petkov, Cathy Avery, Haiyang Zhang,
H. Peter Anvin, Ingo Molnar, K. Y. Srinivasan,
Michael Kelley (EOSG), Mohammed Gamal, Paolo Bonzini,
Radim Krčmář, Roman Kagan, Sasha Levin,
Stephen Hemminger, Thomas Gleixner, devel, kvm, linux-hyperv, x86
In-Reply-To: <20190614082807.GV3436@hirez.programming.kicks-ass.net>
Peter Zijlstra <peterz@infradead.org> writes:
> @@ -182,7 +182,7 @@ void set_hv_tscchange_cb(void (*cb)(void))
> struct hv_reenlightenment_control re_ctrl = {
> .vector = HYPERV_REENLIGHTENMENT_VECTOR,
> .enabled = 1,
> - .target_vp = hv_vp_index[smp_processor_id()]
> + .target_vp = hv_vp_index[raw_smp_processor_id()]
> };
> struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
>
Yes, this should do, thanks! I'd also suggest to leave a comment like
/*
* This function can get preemted and migrate to a different CPU
* but this doesn't matter. We just need to assign
* reenlightenment notification to some online CPU. In case this
* CPU goes offline, hv_cpu_die() will re-assign it to some
* other online CPU.
*/
--
Vitaly
^ permalink raw reply
* Re: [PATCH] x86/hyperv: Disable preemption while setting reenlightenment vector
From: Dmitry Safonov @ 2019-06-14 11:50 UTC (permalink / raw)
To: Vitaly Kuznetsov, Peter Zijlstra
Cc: linux-kernel, Prasanna Panchamukhi, Andy Lutomirski,
Borislav Petkov, Cathy Avery, Haiyang Zhang, H. Peter Anvin,
Ingo Molnar, K. Y. Srinivasan, Michael Kelley (EOSG),
Mohammed Gamal, Paolo Bonzini, Radim Krčmář,
Roman Kagan, Sasha Levin, Stephen Hemminger, Thomas Gleixner,
devel, kvm, linux-hyperv, x86
In-Reply-To: <877e9o7a4e.fsf@vitty.brq.redhat.com>
On 6/14/19 11:08 AM, Vitaly Kuznetsov wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
>
>> @@ -182,7 +182,7 @@ void set_hv_tscchange_cb(void (*cb)(void))
>> struct hv_reenlightenment_control re_ctrl = {
>> .vector = HYPERV_REENLIGHTENMENT_VECTOR,
>> .enabled = 1,
>> - .target_vp = hv_vp_index[smp_processor_id()]
>> + .target_vp = hv_vp_index[raw_smp_processor_id()]
>> };
>> struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
>>
>
> Yes, this should do, thanks! I'd also suggest to leave a comment like
> /*
> * This function can get preemted and migrate to a different CPU
> * but this doesn't matter. We just need to assign
> * reenlightenment notification to some online CPU. In case this
> * CPU goes offline, hv_cpu_die() will re-assign it to some
> * other online CPU.
> */
What if the cpu goes down just before wrmsrl()?
I mean, hv_cpu_die() will reassign another cpu, but this thread will be
resumed on some other cpu and will write cpu number which is at that
moment already down?
(probably I miss something)
And I presume it's guaranteed that during hv_cpu_die() no other cpu may
go down:
: new_cpu = cpumask_any_but(cpu_online_mask, cpu);
: re_ctrl.target_vp = hv_vp_index[new_cpu];
: wrmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *((u64 *)&re_ctrl));
--
Dima
^ permalink raw reply
* Re: [PATCH] x86/hyperv: Disable preemption while setting reenlightenment vector
From: Peter Zijlstra @ 2019-06-14 12:27 UTC (permalink / raw)
To: Dmitry Safonov
Cc: Vitaly Kuznetsov, linux-kernel, Prasanna Panchamukhi,
Andy Lutomirski, Borislav Petkov, Cathy Avery, Haiyang Zhang,
H. Peter Anvin, Ingo Molnar, K. Y. Srinivasan,
Michael Kelley (EOSG), Mohammed Gamal, Paolo Bonzini,
Radim Krčmář, Roman Kagan, Sasha Levin,
Stephen Hemminger, Thomas Gleixner, devel, kvm, linux-hyperv, x86
In-Reply-To: <cb9e1645-98c2-4341-d6da-4effa4f57fb1@arista.com>
On Fri, Jun 14, 2019 at 12:50:51PM +0100, Dmitry Safonov wrote:
> On 6/14/19 11:08 AM, Vitaly Kuznetsov wrote:
> > Peter Zijlstra <peterz@infradead.org> writes:
> >
> >> @@ -182,7 +182,7 @@ void set_hv_tscchange_cb(void (*cb)(void))
> >> struct hv_reenlightenment_control re_ctrl = {
> >> .vector = HYPERV_REENLIGHTENMENT_VECTOR,
> >> .enabled = 1,
> >> - .target_vp = hv_vp_index[smp_processor_id()]
> >> + .target_vp = hv_vp_index[raw_smp_processor_id()]
> >> };
> >> struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
> >>
> >
> > Yes, this should do, thanks! I'd also suggest to leave a comment like
> > /*
> > * This function can get preemted and migrate to a different CPU
> > * but this doesn't matter. We just need to assign
> > * reenlightenment notification to some online CPU. In case this
> > * CPU goes offline, hv_cpu_die() will re-assign it to some
> > * other online CPU.
> > */
>
> What if the cpu goes down just before wrmsrl()?
> I mean, hv_cpu_die() will reassign another cpu, but this thread will be
> resumed on some other cpu and will write cpu number which is at that
> moment already down?
>
> (probably I miss something)
>
> And I presume it's guaranteed that during hv_cpu_die() no other cpu may
> go down:
> : new_cpu = cpumask_any_but(cpu_online_mask, cpu);
> : re_ctrl.target_vp = hv_vp_index[new_cpu];
> : wrmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *((u64 *)&re_ctrl));
Then cpus_read_lock() is the right interface, not preempt_disable().
I know you probably can't change the HV interface, but I'm thinking its
rather daft you have to specify a CPU at all for this. The HV can just
pick one and send the notification there, who cares.
^ permalink raw reply
* Re: [PATCH] x86/hyperv: Disable preemption while setting reenlightenment vector
From: Dmitry Safonov @ 2019-06-14 14:28 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Vitaly Kuznetsov, linux-kernel, Prasanna Panchamukhi,
Andy Lutomirski, Borislav Petkov, Cathy Avery, Haiyang Zhang,
H. Peter Anvin, Ingo Molnar, K. Y. Srinivasan,
Michael Kelley (EOSG), Mohammed Gamal, Paolo Bonzini,
Radim Krčmář, Roman Kagan, Sasha Levin,
Stephen Hemminger, Thomas Gleixner, devel, kvm, linux-hyperv, x86
In-Reply-To: <20190614122726.GL3436@hirez.programming.kicks-ass.net>
On 6/14/19 1:27 PM, Peter Zijlstra wrote:
> On Fri, Jun 14, 2019 at 12:50:51PM +0100, Dmitry Safonov wrote:
>> On 6/14/19 11:08 AM, Vitaly Kuznetsov wrote:
>>> Peter Zijlstra <peterz@infradead.org> writes:
>>>
>>>> @@ -182,7 +182,7 @@ void set_hv_tscchange_cb(void (*cb)(void))
>>>> struct hv_reenlightenment_control re_ctrl = {
>>>> .vector = HYPERV_REENLIGHTENMENT_VECTOR,
>>>> .enabled = 1,
>>>> - .target_vp = hv_vp_index[smp_processor_id()]
>>>> + .target_vp = hv_vp_index[raw_smp_processor_id()]
>>>> };
>>>> struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
>>>>
>>>
>>> Yes, this should do, thanks! I'd also suggest to leave a comment like
>>> /*
>>> * This function can get preemted and migrate to a different CPU
>>> * but this doesn't matter. We just need to assign
>>> * reenlightenment notification to some online CPU. In case this
>>> * CPU goes offline, hv_cpu_die() will re-assign it to some
>>> * other online CPU.
>>> */
>>
>> What if the cpu goes down just before wrmsrl()?
>> I mean, hv_cpu_die() will reassign another cpu, but this thread will be
>> resumed on some other cpu and will write cpu number which is at that
>> moment already down?
>>
>> (probably I miss something)
>>
>> And I presume it's guaranteed that during hv_cpu_die() no other cpu may
>> go down:
>> : new_cpu = cpumask_any_but(cpu_online_mask, cpu);
>> : re_ctrl.target_vp = hv_vp_index[new_cpu];
>> : wrmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *((u64 *)&re_ctrl));
>
> Then cpus_read_lock() is the right interface, not preempt_disable().
>
> I know you probably can't change the HV interface, but I'm thinking its
> rather daft you have to specify a CPU at all for this. The HV can just
> pick one and send the notification there, who cares.
Heh, I thought cpus_read_lock() is more "internal" api and
preempt_diable() is prefered ;-)
Will send v2 with the suggested comment and cpus_read_lock().
--
Dima
^ permalink raw reply
* [PATCH] ACPI: PM: Export the function acpi_sleep_state_supported()
From: Dexuan Cui @ 2019-06-14 18:19 UTC (permalink / raw)
To: linux-acpi@vger.kernel.org, rjw@rjwysocki.net, lenb@kernel.org,
robert.moore@intel.com, erik.schmauss@intel.com, Michael Kelley
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan, Stephen Hemminger, Haiyang Zhang, Sasha Levin,
olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, vkuznets,
marcelo.cerri@canonical.com, Dexuan Cui
In a Linux VM running on Hyper-V, when ACPI S4 is enabled, the balloon
driver (drivers/hv/hv_balloon.c) needs to ask the host not to do memory
hot-add/remove.
So let's export acpi_sleep_state_supported() for the hv_balloon driver.
This might also be useful to the other drivers in the future.
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
drivers/acpi/sleep.c | 3 ++-
include/acpi/acpi_bus.h | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index a34deccd7317..69755411e008 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -79,7 +79,7 @@ static int acpi_sleep_prepare(u32 acpi_state)
return 0;
}
-static bool acpi_sleep_state_supported(u8 sleep_state)
+bool acpi_sleep_state_supported(u8 sleep_state)
{
acpi_status status;
u8 type_a, type_b;
@@ -89,6 +89,7 @@ static bool acpi_sleep_state_supported(u8 sleep_state)
|| (acpi_gbl_FADT.sleep_control.address
&& acpi_gbl_FADT.sleep_status.address));
}
+EXPORT_SYMBOL_GPL(acpi_sleep_state_supported);
#ifdef CONFIG_ACPI_SLEEP
static u32 acpi_target_sleep_state = ACPI_STATE_S0;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 31b6c87d6240..5b102e7bbf25 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -651,6 +651,8 @@ static inline int acpi_pm_set_bridge_wakeup(struct device *dev, bool enable)
}
#endif
+bool acpi_sleep_state_supported(u8 sleep_state);
+
#ifdef CONFIG_ACPI_SLEEP
u32 acpi_target_system_state(void);
#else
--
2.19.1
^ permalink raw reply related
* [PATCH 1/2] hv_balloon: Use a static page for the balloon_up send buffer
From: Dexuan Cui @ 2019-06-14 18:42 UTC (permalink / raw)
To: linux-hyperv@vger.kernel.org, gregkh@linuxfoundation.org,
Stephen Hemminger, Sasha Levin, Haiyang Zhang, KY Srinivasan,
linux-kernel@vger.kernel.org, Michael Kelley, Tianyu Lan
Cc: olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, vkuznets,
marcelo.cerri@canonical.com, Dexuan Cui
It's unnecessary to dynamically allocate the buffer.
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
drivers/hv/hv_balloon.c | 19 ++++---------------
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index dd475f3bcc8a..13381ea3e3e7 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -504,7 +504,7 @@ enum hv_dm_state {
static __u8 recv_buffer[PAGE_SIZE];
-static __u8 *send_buffer;
+static __u8 balloon_up_send_buffer[PAGE_SIZE];
#define PAGES_IN_2M 512
#define HA_CHUNK (32 * 1024)
@@ -1302,8 +1302,8 @@ static void balloon_up(struct work_struct *dummy)
}
while (!done) {
- bl_resp = (struct dm_balloon_response *)send_buffer;
- memset(send_buffer, 0, PAGE_SIZE);
+ memset(balloon_up_send_buffer, 0, PAGE_SIZE);
+ bl_resp = (struct dm_balloon_response *)balloon_up_send_buffer;
bl_resp->hdr.type = DM_BALLOON_RESPONSE;
bl_resp->hdr.size = sizeof(struct dm_balloon_response);
bl_resp->more_pages = 1;
@@ -1588,19 +1588,11 @@ static int balloon_probe(struct hv_device *dev,
do_hot_add = false;
#endif
- /*
- * First allocate a send buffer.
- */
-
- send_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
- if (!send_buffer)
- return -ENOMEM;
-
ret = vmbus_open(dev->channel, dm_ring_size, dm_ring_size, NULL, 0,
balloon_onchannelcallback, dev);
if (ret)
- goto probe_error0;
+ return ret;
dm_device.dev = dev;
dm_device.state = DM_INITIALIZING;
@@ -1726,8 +1718,6 @@ static int balloon_probe(struct hv_device *dev,
probe_error1:
vmbus_close(dev->channel);
-probe_error0:
- kfree(send_buffer);
return ret;
}
@@ -1746,7 +1736,6 @@ static int balloon_remove(struct hv_device *dev)
vmbus_close(dev->channel);
kthread_stop(dm->thread);
- kfree(send_buffer);
#ifdef CONFIG_MEMORY_HOTPLUG
restore_online_page_callback(&hv_online_page);
unregister_memory_notifier(&hv_memory_nb);
--
2.19.1
^ permalink raw reply related
* [PATCH 2/2] hv_balloon: Reorganize the probe function
From: Dexuan Cui @ 2019-06-14 18:42 UTC (permalink / raw)
To: linux-hyperv@vger.kernel.org, gregkh@linuxfoundation.org,
Stephen Hemminger, Sasha Levin, Haiyang Zhang, KY Srinivasan,
linux-kernel@vger.kernel.org, Michael Kelley, Tianyu Lan
Cc: olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, vkuznets,
marcelo.cerri@canonical.com, Dexuan Cui
In-Reply-To: <1560537692-37400-1-git-send-email-decui@microsoft.com>
Move the code that negotiates with the host to a new function
balloon_connect_vsp() and improve the error handling.
This makes the code more readable and paves the way for the
support of hibernation in future.
Makes no real logic change here.
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
drivers/hv/hv_balloon.c | 124 +++++++++++++++++++++-------------------
1 file changed, 66 insertions(+), 58 deletions(-)
diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 13381ea3e3e7..111ea3599659 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -1574,50 +1574,18 @@ static void balloon_onchannelcallback(void *context)
}
-static int balloon_probe(struct hv_device *dev,
- const struct hv_vmbus_device_id *dev_id)
+static int balloon_connect_vsp(struct hv_device *dev)
{
- int ret;
- unsigned long t;
struct dm_version_request version_req;
struct dm_capabilities cap_msg;
-
-#ifdef CONFIG_MEMORY_HOTPLUG
- do_hot_add = hot_add;
-#else
- do_hot_add = false;
-#endif
+ unsigned long t;
+ int ret;
ret = vmbus_open(dev->channel, dm_ring_size, dm_ring_size, NULL, 0,
- balloon_onchannelcallback, dev);
-
+ balloon_onchannelcallback, dev);
if (ret)
return ret;
- dm_device.dev = dev;
- dm_device.state = DM_INITIALIZING;
- dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN8;
- init_completion(&dm_device.host_event);
- init_completion(&dm_device.config_event);
- INIT_LIST_HEAD(&dm_device.ha_region_list);
- spin_lock_init(&dm_device.ha_lock);
- INIT_WORK(&dm_device.balloon_wrk.wrk, balloon_up);
- INIT_WORK(&dm_device.ha_wrk.wrk, hot_add_req);
- dm_device.host_specified_ha_region = false;
-
- dm_device.thread =
- kthread_run(dm_thread_func, &dm_device, "hv_balloon");
- if (IS_ERR(dm_device.thread)) {
- ret = PTR_ERR(dm_device.thread);
- goto probe_error1;
- }
-
-#ifdef CONFIG_MEMORY_HOTPLUG
- set_online_page_callback(&hv_online_page);
- register_memory_notifier(&hv_memory_nb);
-#endif
-
- hv_set_drvdata(dev, &dm_device);
/*
* Initiate the hand shake with the host and negotiate
* a version that the host can support. We start with the
@@ -1633,16 +1601,15 @@ static int balloon_probe(struct hv_device *dev,
dm_device.version = version_req.version.version;
ret = vmbus_sendpacket(dev->channel, &version_req,
- sizeof(struct dm_version_request),
- (unsigned long)NULL,
- VM_PKT_DATA_INBAND, 0);
+ sizeof(struct dm_version_request),
+ (unsigned long)NULL, VM_PKT_DATA_INBAND, 0);
if (ret)
- goto probe_error2;
+ goto out;
t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
if (t == 0) {
ret = -ETIMEDOUT;
- goto probe_error2;
+ goto out;
}
/*
@@ -1650,8 +1617,8 @@ static int balloon_probe(struct hv_device *dev,
* fail the probe function.
*/
if (dm_device.state == DM_INIT_ERROR) {
- ret = -ETIMEDOUT;
- goto probe_error2;
+ ret = -EPROTO;
+ goto out;
}
pr_info("Using Dynamic Memory protocol version %u.%u\n",
@@ -1684,16 +1651,15 @@ static int balloon_probe(struct hv_device *dev,
cap_msg.max_page_number = -1;
ret = vmbus_sendpacket(dev->channel, &cap_msg,
- sizeof(struct dm_capabilities),
- (unsigned long)NULL,
- VM_PKT_DATA_INBAND, 0);
+ sizeof(struct dm_capabilities),
+ (unsigned long)NULL, VM_PKT_DATA_INBAND, 0);
if (ret)
- goto probe_error2;
+ goto out;
t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
if (t == 0) {
ret = -ETIMEDOUT;
- goto probe_error2;
+ goto out;
}
/*
@@ -1701,23 +1667,65 @@ static int balloon_probe(struct hv_device *dev,
* fail the probe function.
*/
if (dm_device.state == DM_INIT_ERROR) {
- ret = -ETIMEDOUT;
- goto probe_error2;
+ ret = -EPROTO;
+ goto out;
}
+ return 0;
+out:
+ vmbus_close(dev->channel);
+ return ret;
+}
+
+static int balloon_probe(struct hv_device *dev,
+ const struct hv_vmbus_device_id *dev_id)
+{
+ int ret;
+
+#ifdef CONFIG_MEMORY_HOTPLUG
+ do_hot_add = hot_add;
+#else
+ do_hot_add = false;
+#endif
+ dm_device.dev = dev;
+ dm_device.state = DM_INITIALIZING;
+ dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN8;
+ init_completion(&dm_device.host_event);
+ init_completion(&dm_device.config_event);
+ INIT_LIST_HEAD(&dm_device.ha_region_list);
+ spin_lock_init(&dm_device.ha_lock);
+ INIT_WORK(&dm_device.balloon_wrk.wrk, balloon_up);
+ INIT_WORK(&dm_device.ha_wrk.wrk, hot_add_req);
+ dm_device.host_specified_ha_region = false;
+
+#ifdef CONFIG_MEMORY_HOTPLUG
+ set_online_page_callback(&hv_online_page);
+ register_memory_notifier(&hv_memory_nb);
+#endif
+
+ hv_set_drvdata(dev, &dm_device);
+
+ ret = balloon_connect_vsp(dev);
+ if (ret != 0)
+ return ret;
+
dm_device.state = DM_INITIALIZED;
- last_post_time = jiffies;
+
+ dm_device.thread =
+ kthread_run(dm_thread_func, &dm_device, "hv_balloon");
+ if (IS_ERR(dm_device.thread)) {
+ ret = PTR_ERR(dm_device.thread);
+ goto probe_error;
+ }
return 0;
-probe_error2:
+probe_error:
+ vmbus_close(dev->channel);
#ifdef CONFIG_MEMORY_HOTPLUG
+ unregister_memory_notifier(&hv_memory_nb);
restore_online_page_callback(&hv_online_page);
#endif
- kthread_stop(dm_device.thread);
-
-probe_error1:
- vmbus_close(dev->channel);
return ret;
}
@@ -1734,11 +1742,11 @@ static int balloon_remove(struct hv_device *dev)
cancel_work_sync(&dm->balloon_wrk.wrk);
cancel_work_sync(&dm->ha_wrk.wrk);
- vmbus_close(dev->channel);
kthread_stop(dm->thread);
+ vmbus_close(dev->channel);
#ifdef CONFIG_MEMORY_HOTPLUG
- restore_online_page_callback(&hv_online_page);
unregister_memory_notifier(&hv_memory_nb);
+ restore_online_page_callback(&hv_online_page);
#endif
spin_lock_irqsave(&dm_device.ha_lock, flags);
list_for_each_entry_safe(has, tmp, &dm->ha_region_list, list) {
--
2.19.1
^ permalink raw reply related
* RE: [PATCH] ACPI: PM: Export the function acpi_sleep_state_supported()
From: Michael Kelley @ 2019-06-14 20:48 UTC (permalink / raw)
To: Dexuan Cui, linux-acpi@vger.kernel.org, rjw@rjwysocki.net,
lenb@kernel.org, robert.moore@intel.com, erik.schmauss@intel.com
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan, Stephen Hemminger, Haiyang Zhang, Sasha Levin,
olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, vkuznets,
marcelo.cerri@canonical.com
In-Reply-To: <1560536224-35338-1-git-send-email-decui@microsoft.com>
From: Dexuan Cui <decui@microsoft.com> Sent: Friday, June 14, 2019 11:19 AM
>
> In a Linux VM running on Hyper-V, when ACPI S4 is enabled, the balloon
> driver (drivers/hv/hv_balloon.c) needs to ask the host not to do memory
> hot-add/remove.
>
> So let's export acpi_sleep_state_supported() for the hv_balloon driver.
> This might also be useful to the other drivers in the future.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/acpi/sleep.c | 3 ++-
> include/acpi/acpi_bus.h | 2 ++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index a34deccd7317..69755411e008 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -79,7 +79,7 @@ static int acpi_sleep_prepare(u32 acpi_state)
> return 0;
> }
>
> -static bool acpi_sleep_state_supported(u8 sleep_state)
> +bool acpi_sleep_state_supported(u8 sleep_state)
> {
> acpi_status status;
> u8 type_a, type_b;
> @@ -89,6 +89,7 @@ static bool acpi_sleep_state_supported(u8 sleep_state)
> || (acpi_gbl_FADT.sleep_control.address
> && acpi_gbl_FADT.sleep_status.address));
> }
> +EXPORT_SYMBOL_GPL(acpi_sleep_state_supported);
>
> #ifdef CONFIG_ACPI_SLEEP
> static u32 acpi_target_sleep_state = ACPI_STATE_S0;
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 31b6c87d6240..5b102e7bbf25 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -651,6 +651,8 @@ static inline int acpi_pm_set_bridge_wakeup(struct device *dev,
> bool enable)
> }
> #endif
>
> +bool acpi_sleep_state_supported(u8 sleep_state);
> +
> #ifdef CONFIG_ACPI_SLEEP
> u32 acpi_target_system_state(void);
> #else
> --
> 2.19.1
It seems that sleep.c isn't built when on the ARM64 architecture. Using
acpi_sleep_state_supported() directly in hv_balloon.c will be problematic
since hv_balloon.c needs to be architecture independent when the
Hyper-V ARM64 support is added. If that doesn't change, a per-architecture
wrapper will be needed to give hv_balloon.c the correct information. This
may affect whether acpi_sleep_state_supported() needs to be exported vs.
just removing the "static". I'm not sure what the best approach is.
Michael
^ permalink raw reply
* RE: [PATCH 1/2] hv_balloon: Use a static page for the balloon_up send buffer
From: Michael Kelley @ 2019-06-14 20:56 UTC (permalink / raw)
To: Dexuan Cui, linux-hyperv@vger.kernel.org,
gregkh@linuxfoundation.org, Stephen Hemminger, Sasha Levin,
Haiyang Zhang, KY Srinivasan, linux-kernel@vger.kernel.org,
Tianyu Lan
Cc: olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, vkuznets,
marcelo.cerri@canonical.com
In-Reply-To: <1560537692-37400-1-git-send-email-decui@microsoft.com>
From: Dexuan Cui <decui@microsoft.com> Sent: Friday, June 14, 2019 11:42 AM
>
> It's unnecessary to dynamically allocate the buffer.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/hv/hv_balloon.c | 19 ++++---------------
> 1 file changed, 4 insertions(+), 15 deletions(-)
>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* Re: [PATCH] x86/hyperv: Disable preemption while setting reenlightenment vector
From: Vitaly Kuznetsov @ 2019-06-14 21:36 UTC (permalink / raw)
To: Dmitry Safonov, Peter Zijlstra
Cc: linux-kernel, Prasanna Panchamukhi, Andy Lutomirski,
Borislav Petkov, Cathy Avery, Haiyang Zhang, H. Peter Anvin,
Ingo Molnar, K. Y. Srinivasan, Michael Kelley (EOSG),
Mohammed Gamal, Paolo Bonzini, Radim Krčmář,
Roman Kagan, Sasha Levin, Stephen Hemminger, Thomas Gleixner,
devel, kvm, linux-hyperv, x86
In-Reply-To: <cb9e1645-98c2-4341-d6da-4effa4f57fb1@arista.com>
Dmitry Safonov <dima@arista.com> writes:
> On 6/14/19 11:08 AM, Vitaly Kuznetsov wrote:
>> Peter Zijlstra <peterz@infradead.org> writes:
>>
>>> @@ -182,7 +182,7 @@ void set_hv_tscchange_cb(void (*cb)(void))
>>> struct hv_reenlightenment_control re_ctrl = {
>>> .vector = HYPERV_REENLIGHTENMENT_VECTOR,
>>> .enabled = 1,
>>> - .target_vp = hv_vp_index[smp_processor_id()]
>>> + .target_vp = hv_vp_index[raw_smp_processor_id()]
>>> };
>>> struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
>>>
>>
>> Yes, this should do, thanks! I'd also suggest to leave a comment like
>> /*
>> * This function can get preemted and migrate to a different CPU
>> * but this doesn't matter. We just need to assign
>> * reenlightenment notification to some online CPU. In case this
>> * CPU goes offline, hv_cpu_die() will re-assign it to some
>> * other online CPU.
>> */
>
> What if the cpu goes down just before wrmsrl()?
> I mean, hv_cpu_die() will reassign another cpu, but this thread will be
> resumed on some other cpu and will write cpu number which is at that
> moment already down?
>
Right you are, we need to guarantee wrmsr() happens before the CPU gets
a chance to go offline: we don't save the cpu number anywhere, we just
read it with rdmsr() in hv_cpu_die().
>
> And I presume it's guaranteed that during hv_cpu_die() no other cpu may
> go down:
> : new_cpu = cpumask_any_but(cpu_online_mask, cpu);
> : re_ctrl.target_vp = hv_vp_index[new_cpu];
> : wrmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *((u64 *)&re_ctrl));
I *think* I got convinced that CPUs don't go offline simultaneously when
I was writing this.
--
Vitaly
^ permalink raw reply
* Re: [PATCH] x86/hyperv: Disable preemption while setting reenlightenment vector
From: Vitaly Kuznetsov @ 2019-06-14 21:44 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, Prasanna Panchamukhi, Andy Lutomirski,
Borislav Petkov, Cathy Avery, Haiyang Zhang, H. Peter Anvin,
Ingo Molnar, K. Y. Srinivasan, Michael Kelley (EOSG),
Mohammed Gamal, Paolo Bonzini, Radim Krčmář,
Roman Kagan, Sasha Levin, Stephen Hemminger, Thomas Gleixner,
devel, kvm, linux-hyperv, x86, Dmitry Safonov
In-Reply-To: <20190614122726.GL3436@hirez.programming.kicks-ass.net>
Peter Zijlstra <peterz@infradead.org> writes:
>
> I know you probably can't change the HV interface, but I'm thinking its
> rather daft you have to specify a CPU at all for this. The HV can just
> pick one and send the notification there, who cares.
Generally speaking, hypervisor can't know if the CPU is offline (or
e.g. 'isolated') from guest's perspective so I think having an option to
specify affinity for reenlightenment notification is rather a good
thing, not bad.
(Actually, I don't remember if I tried specifying 'HV_ANY' (U32_MAX-1)
here to see what happens. But then I doubt it'll notice the fact that we
offlined some CPU so we may get a totally unexpected IRQ there).
--
Vitaly
^ permalink raw reply
* RE: [PATCH 2/2] hv_balloon: Reorganize the probe function
From: Michael Kelley @ 2019-06-14 21:56 UTC (permalink / raw)
To: Dexuan Cui, linux-hyperv@vger.kernel.org,
gregkh@linuxfoundation.org, Stephen Hemminger, Sasha Levin,
Haiyang Zhang, KY Srinivasan, linux-kernel@vger.kernel.org,
Tianyu Lan
Cc: olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, vkuznets,
marcelo.cerri@canonical.com
In-Reply-To: <1560537692-37400-2-git-send-email-decui@microsoft.com>
From: Dexuan Cui <decui@microsoft.com> Sent: Friday, June 14, 2019 11:43 AM
>
> Move the code that negotiates with the host to a new function
> balloon_connect_vsp() and improve the error handling.
>
> This makes the code more readable and paves the way for the
> support of hibernation in future.
>
> Makes no real logic change here.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/hv/hv_balloon.c | 124 +++++++++++++++++++++-------------------
> 1 file changed, 66 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 13381ea3e3e7..111ea3599659 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -1574,50 +1574,18 @@ static void balloon_onchannelcallback(void *context)
>
> }
>
> -static int balloon_probe(struct hv_device *dev,
> - const struct hv_vmbus_device_id *dev_id)
> +static int balloon_connect_vsp(struct hv_device *dev)
> {
> - int ret;
> - unsigned long t;
> struct dm_version_request version_req;
> struct dm_capabilities cap_msg;
> -
> -#ifdef CONFIG_MEMORY_HOTPLUG
> - do_hot_add = hot_add;
> -#else
> - do_hot_add = false;
> -#endif
> + unsigned long t;
> + int ret;
>
> ret = vmbus_open(dev->channel, dm_ring_size, dm_ring_size, NULL, 0,
> - balloon_onchannelcallback, dev);
> -
> + balloon_onchannelcallback, dev);
> if (ret)
> return ret;
>
> - dm_device.dev = dev;
> - dm_device.state = DM_INITIALIZING;
> - dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN8;
> - init_completion(&dm_device.host_event);
> - init_completion(&dm_device.config_event);
> - INIT_LIST_HEAD(&dm_device.ha_region_list);
> - spin_lock_init(&dm_device.ha_lock);
> - INIT_WORK(&dm_device.balloon_wrk.wrk, balloon_up);
> - INIT_WORK(&dm_device.ha_wrk.wrk, hot_add_req);
> - dm_device.host_specified_ha_region = false;
> -
> - dm_device.thread =
> - kthread_run(dm_thread_func, &dm_device, "hv_balloon");
> - if (IS_ERR(dm_device.thread)) {
> - ret = PTR_ERR(dm_device.thread);
> - goto probe_error1;
> - }
> -
> -#ifdef CONFIG_MEMORY_HOTPLUG
> - set_online_page_callback(&hv_online_page);
> - register_memory_notifier(&hv_memory_nb);
> -#endif
> -
> - hv_set_drvdata(dev, &dm_device);
> /*
> * Initiate the hand shake with the host and negotiate
> * a version that the host can support. We start with the
> @@ -1633,16 +1601,15 @@ static int balloon_probe(struct hv_device *dev,
> dm_device.version = version_req.version.version;
>
> ret = vmbus_sendpacket(dev->channel, &version_req,
> - sizeof(struct dm_version_request),
> - (unsigned long)NULL,
> - VM_PKT_DATA_INBAND, 0);
> + sizeof(struct dm_version_request),
> + (unsigned long)NULL, VM_PKT_DATA_INBAND, 0);
> if (ret)
> - goto probe_error2;
> + goto out;
>
> t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
> if (t == 0) {
> ret = -ETIMEDOUT;
> - goto probe_error2;
> + goto out;
> }
>
> /*
> @@ -1650,8 +1617,8 @@ static int balloon_probe(struct hv_device *dev,
> * fail the probe function.
> */
> if (dm_device.state == DM_INIT_ERROR) {
> - ret = -ETIMEDOUT;
> - goto probe_error2;
> + ret = -EPROTO;
> + goto out;
> }
>
> pr_info("Using Dynamic Memory protocol version %u.%u\n",
> @@ -1684,16 +1651,15 @@ static int balloon_probe(struct hv_device *dev,
> cap_msg.max_page_number = -1;
>
> ret = vmbus_sendpacket(dev->channel, &cap_msg,
> - sizeof(struct dm_capabilities),
> - (unsigned long)NULL,
> - VM_PKT_DATA_INBAND, 0);
> + sizeof(struct dm_capabilities),
> + (unsigned long)NULL, VM_PKT_DATA_INBAND, 0);
> if (ret)
> - goto probe_error2;
> + goto out;
>
> t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
> if (t == 0) {
> ret = -ETIMEDOUT;
> - goto probe_error2;
> + goto out;
> }
>
> /*
> @@ -1701,23 +1667,65 @@ static int balloon_probe(struct hv_device *dev,
> * fail the probe function.
> */
> if (dm_device.state == DM_INIT_ERROR) {
> - ret = -ETIMEDOUT;
> - goto probe_error2;
> + ret = -EPROTO;
> + goto out;
> }
>
> + return 0;
> +out:
> + vmbus_close(dev->channel);
> + return ret;
> +}
> +
> +static int balloon_probe(struct hv_device *dev,
> + const struct hv_vmbus_device_id *dev_id)
> +{
> + int ret;
> +
> +#ifdef CONFIG_MEMORY_HOTPLUG
> + do_hot_add = hot_add;
> +#else
> + do_hot_add = false;
> +#endif
> + dm_device.dev = dev;
> + dm_device.state = DM_INITIALIZING;
> + dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN8;
> + init_completion(&dm_device.host_event);
> + init_completion(&dm_device.config_event);
> + INIT_LIST_HEAD(&dm_device.ha_region_list);
> + spin_lock_init(&dm_device.ha_lock);
> + INIT_WORK(&dm_device.balloon_wrk.wrk, balloon_up);
> + INIT_WORK(&dm_device.ha_wrk.wrk, hot_add_req);
> + dm_device.host_specified_ha_region = false;
> +
> +#ifdef CONFIG_MEMORY_HOTPLUG
> + set_online_page_callback(&hv_online_page);
> + register_memory_notifier(&hv_memory_nb);
> +#endif
> +
> + hv_set_drvdata(dev, &dm_device);
> +
> + ret = balloon_connect_vsp(dev);
> + if (ret != 0)
> + return ret;
> +
> dm_device.state = DM_INITIALIZED;
> - last_post_time = jiffies;
I was curious about the above deletion. But I guess the line
is not needed as the time_after() check in post_status() should
handle an initial value of 0 for last_post_time just fine.
> +
> + dm_device.thread =
> + kthread_run(dm_thread_func, &dm_device, "hv_balloon");
> + if (IS_ERR(dm_device.thread)) {
> + ret = PTR_ERR(dm_device.thread);
> + goto probe_error;
> + }
Just an observation: this thread creation now happens at the end of the
probing process. But that's good, because in the old code, the thread
was started and could run before the protocol version had been
negotiated. So I'll assume your change here is intentional.
>
> return 0;
>
> -probe_error2:
> +probe_error:
> + vmbus_close(dev->channel);
> #ifdef CONFIG_MEMORY_HOTPLUG
> + unregister_memory_notifier(&hv_memory_nb);
Hmmm. Evidently the above cleanup was missing in the
old code.
> restore_online_page_callback(&hv_online_page);
> #endif
> - kthread_stop(dm_device.thread);
> -
> -probe_error1:
> - vmbus_close(dev->channel);
> return ret;
> }
>
> @@ -1734,11 +1742,11 @@ static int balloon_remove(struct hv_device *dev)
> cancel_work_sync(&dm->balloon_wrk.wrk);
> cancel_work_sync(&dm->ha_wrk.wrk);
>
> - vmbus_close(dev->channel);
> kthread_stop(dm->thread);
> + vmbus_close(dev->channel);
Presumably this is an intentional ordering change as well.
The kthread should be stopped before closing the channel.
> #ifdef CONFIG_MEMORY_HOTPLUG
> - restore_online_page_callback(&hv_online_page);
> unregister_memory_notifier(&hv_memory_nb);
> + restore_online_page_callback(&hv_online_page);
And you've changed the ordering of these steps so they are
the inverse of when they are set up. Also a good cleanup ....
> #endif
> spin_lock_irqsave(&dm_device.ha_lock, flags);
> list_for_each_entry_safe(has, tmp, &dm->ha_region_list, list) {
> --
> 2.19.1
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* RE: [PATCH] ACPI: PM: Export the function acpi_sleep_state_supported()
From: Dexuan Cui @ 2019-06-14 22:19 UTC (permalink / raw)
To: Michael Kelley, linux-acpi@vger.kernel.org, rjw@rjwysocki.net,
lenb@kernel.org, robert.moore@intel.com, erik.schmauss@intel.com,
Russell King, Russ Dill, Sebastian Capella, Pavel Machek,
Lorenzo Pieralisi
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan, Stephen Hemminger, Haiyang Zhang, Sasha Levin,
olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, vkuznets,
marcelo.cerri@canonical.com
In-Reply-To: <BL0PR2101MB134895BADA1D8E0FA631D532D7EE0@BL0PR2101MB1348.namprd21.prod.outlook.com>
> -----Original Message-----
> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Friday, June 14, 2019 1:48 PM
> To: Dexuan Cui <decui@microsoft.com>; linux-acpi@vger.kernel.org;
> rjw@rjwysocki.net; lenb@kernel.org; robert.moore@intel.com;
> erik.schmauss@intel.com
> Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; KY Srinivasan
> <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> Haiyang Zhang <haiyangz@microsoft.com>; Sasha Levin
> <Alexander.Levin@microsoft.com>; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; vkuznets <vkuznets@redhat.com>;
> marcelo.cerri@canonical.com
> Subject: RE: [PATCH] ACPI: PM: Export the function
> acpi_sleep_state_supported()
>
> From: Dexuan Cui <decui@microsoft.com> Sent: Friday, June 14, 2019 11:19
> AM
> >
> > In a Linux VM running on Hyper-V, when ACPI S4 is enabled, the balloon
> > driver (drivers/hv/hv_balloon.c) needs to ask the host not to do memory
> > hot-add/remove.
> >
> > So let's export acpi_sleep_state_supported() for the hv_balloon driver.
> > This might also be useful to the other drivers in the future.
> >
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > ---
> > drivers/acpi/sleep.c | 3 ++-
> > include/acpi/acpi_bus.h | 2 ++
> > 2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > index a34deccd7317..69755411e008 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -79,7 +79,7 @@ static int acpi_sleep_prepare(u32 acpi_state)
> > return 0;
> > }
> >
> > -static bool acpi_sleep_state_supported(u8 sleep_state)
> > +bool acpi_sleep_state_supported(u8 sleep_state)
> > {
> > acpi_status status;
> > u8 type_a, type_b;
> > @@ -89,6 +89,7 @@ static bool acpi_sleep_state_supported(u8 sleep_state)
> > || (acpi_gbl_FADT.sleep_control.address
> > && acpi_gbl_FADT.sleep_status.address));
> > }
> > +EXPORT_SYMBOL_GPL(acpi_sleep_state_supported);
> >
> > #ifdef CONFIG_ACPI_SLEEP
> > static u32 acpi_target_sleep_state = ACPI_STATE_S0;
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index 31b6c87d6240..5b102e7bbf25 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -651,6 +651,8 @@ static inline int acpi_pm_set_bridge_wakeup(struct
> device *dev,
> > bool enable)
> > }
> > #endif
> >
> > +bool acpi_sleep_state_supported(u8 sleep_state);
> > +
> > #ifdef CONFIG_ACPI_SLEEP
> > u32 acpi_target_system_state(void);
> > #else
> > --
> > 2.19.1
>
> It seems that sleep.c isn't built when on the ARM64 architecture. Using
> acpi_sleep_state_supported() directly in hv_balloon.c will be problematic
> since hv_balloon.c needs to be architecture independent when the
> Hyper-V ARM64 support is added. If that doesn't change, a per-architecture
> wrapper will be needed to give hv_balloon.c the correct information. This
> may affect whether acpi_sleep_state_supported() needs to be exported vs.
> just removing the "static". I'm not sure what the best approach is.
>
> Michael
+ some ARM experts who worked on arch/arm/kernel/hibernate.c.
drivers/acpi/sleep.c is only built if ACPI_SYSTEM_POWER_STATES_SUPPORT
is defined, but it looks this option is not defined on ARM.
It looks ARM does not support the ACPI S4 state, then how do we know
if an ARM host supports hibernation or not?
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH] ACPI: PM: Export the function acpi_sleep_state_supported()
From: Russell King - ARM Linux admin @ 2019-06-14 22:33 UTC (permalink / raw)
To: Dexuan Cui
Cc: Michael Kelley, linux-acpi@vger.kernel.org, rjw@rjwysocki.net,
lenb@kernel.org, robert.moore@intel.com, erik.schmauss@intel.com,
Russ Dill, Sebastian Capella, Pavel Machek, Lorenzo Pieralisi,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan, Stephen Hemminger, Haiyang Zhang, Sasha Levin,
olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, vkuznets,
marcelo.cerri@canonical.com
In-Reply-To: <PU1P153MB01699020B5BC4287C58F5335BFEE0@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
Hi,
On Fri, Jun 14, 2019 at 10:19:02PM +0000, Dexuan Cui wrote:
> > -----Original Message-----
> > From: Michael Kelley <mikelley@microsoft.com>
> > Sent: Friday, June 14, 2019 1:48 PM
> > To: Dexuan Cui <decui@microsoft.com>; linux-acpi@vger.kernel.org;
> > rjw@rjwysocki.net; lenb@kernel.org; robert.moore@intel.com;
> > erik.schmauss@intel.com
> > Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; KY Srinivasan
> > <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> > Haiyang Zhang <haiyangz@microsoft.com>; Sasha Levin
> > <Alexander.Levin@microsoft.com>; olaf@aepfle.de; apw@canonical.com;
> > jasowang@redhat.com; vkuznets <vkuznets@redhat.com>;
> > marcelo.cerri@canonical.com
> > Subject: RE: [PATCH] ACPI: PM: Export the function
> > acpi_sleep_state_supported()
> >
> > It seems that sleep.c isn't built when on the ARM64 architecture. Using
> > acpi_sleep_state_supported() directly in hv_balloon.c will be problematic
> > since hv_balloon.c needs to be architecture independent when the
> > Hyper-V ARM64 support is added. If that doesn't change, a per-architecture
> > wrapper will be needed to give hv_balloon.c the correct information. This
> > may affect whether acpi_sleep_state_supported() needs to be exported vs.
> > just removing the "static". I'm not sure what the best approach is.
> >
> > Michael
>
> + some ARM experts who worked on arch/arm/kernel/hibernate.c.
>
> drivers/acpi/sleep.c is only built if ACPI_SYSTEM_POWER_STATES_SUPPORT
> is defined, but it looks this option is not defined on ARM.
>
> It looks ARM does not support the ACPI S4 state, then how do we know
> if an ARM host supports hibernation or not?
Don't forget that Linux does not support ACPI on 32-bit ARM, which is
quite different from the situation on 64-bit ARM.
arch/arm/kernel/hibernate.c is only for 32-bit ARM, and is written with
the assumption that there is no interaction required with any firmware
to save state, and later restore state upon resuming.
Or am I missing something?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* RE: [PATCH 2/2] hv_balloon: Reorganize the probe function
From: Dexuan Cui @ 2019-06-14 23:08 UTC (permalink / raw)
To: Michael Kelley, linux-hyperv@vger.kernel.org,
gregkh@linuxfoundation.org, Stephen Hemminger, Sasha Levin,
Haiyang Zhang, KY Srinivasan, linux-kernel@vger.kernel.org,
Tianyu Lan
Cc: olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, vkuznets,
marcelo.cerri@canonical.com
In-Reply-To: <BL0PR2101MB13487B8D2A157AA7FCFD159DD7EE0@BL0PR2101MB1348.namprd21.prod.outlook.com>
> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Friday, June 14, 2019 2:56 PM
> > ...
> > + ret = balloon_connect_vsp(dev);
> > + if (ret != 0)
> > + return ret;
> > +
> > dm_device.state = DM_INITIALIZED;
> > - last_post_time = jiffies;
>
> I was curious about the above deletion. But I guess the line
> is not needed as the time_after() check in post_status() should
> handle an initial value of 0 for last_post_time just fine.
In a 32-bit kernel, sizeof(unsigned long) is 4, and the global 32-bit
varilable "jiffies" can overflow in 49.7 days if HZ is defined as 1000;
so in theory there is a tiny chance time_after() can not work as
expected here (i.e. we're loading hv_balloon driver when the
"jiffies" is just about to overflow, which is highly unlikely in practice);
even if that happens, we do not care, since the consequence is
just that the memory pressure reporting is delayed by 1 second. :-)
> > +
> > + dm_device.thread =
> > + kthread_run(dm_thread_func, &dm_device, "hv_balloon");
> > + if (IS_ERR(dm_device.thread)) {
> > + ret = PTR_ERR(dm_device.thread);
> > + goto probe_error;
> > + }
>
> Just an observation: this thread creation now happens at the end of the
> probing process. But that's good, because in the old code, the thread
> was started and could run before the protocol version had been
> negotiated. So I'll assume your change here is intentional.
Yes, this is intentional.
> >
> > return 0;
> >
> > -probe_error2:
> > +probe_error:
> > + vmbus_close(dev->channel);
> > #ifdef CONFIG_MEMORY_HOTPLUG
> > + unregister_memory_notifier(&hv_memory_nb);
>
> Hmmm. Evidently the above cleanup was missing in the
> old code.
Yes.
> > restore_online_page_callback(&hv_online_page);
> > #endif
> > - kthread_stop(dm_device.thread);
> > -
> > -probe_error1:
> > - vmbus_close(dev->channel);
> > return ret;
> > }
> >
> > @@ -1734,11 +1742,11 @@ static int balloon_remove(struct hv_device
> *dev)
> > cancel_work_sync(&dm->balloon_wrk.wrk);
> > cancel_work_sync(&dm->ha_wrk.wrk);
> >
> > - vmbus_close(dev->channel);
> > kthread_stop(dm->thread);
> > + vmbus_close(dev->channel);
>
> Presumably this is an intentional ordering change as well.
> The kthread should be stopped before closing the channel.
Yes. The old code is buggy: after the vmbus_close(), there is
a small window in which the old code can still try to send
messages to the host via a freed ringbuffer, causing panic.
> > #ifdef CONFIG_MEMORY_HOTPLUG
> > - restore_online_page_callback(&hv_online_page);
> > unregister_memory_notifier(&hv_memory_nb);
> > + restore_online_page_callback(&hv_online_page);
>
> And you've changed the ordering of these steps so they are
> the inverse of when they are set up. Also a good cleanup ....
Yes. The change is not really necessary, but let's just do it
in a better manner.
>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Thaks for the detailed comments!
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH] ACPI: PM: Export the function acpi_sleep_state_supported()
From: Dexuan Cui @ 2019-06-14 23:34 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Michael Kelley, linux-acpi@vger.kernel.org, rjw@rjwysocki.net,
lenb@kernel.org, robert.moore@intel.com, erik.schmauss@intel.com,
Russ Dill, Sebastian Capella, Pavel Machek, Lorenzo Pieralisi,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan, Stephen Hemminger, Haiyang Zhang, Sasha Levin,
olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, vkuznets,
marcelo.cerri@canonical.com
In-Reply-To: <20190614223310.pwwoefu5qdvcuaiy@shell.armlinux.org.uk>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Russell King
> On Fri, Jun 14, 2019 at 10:19:02PM +0000, Dexuan Cui wrote:
> > It looks ARM does not support the ACPI S4 state, then how do we know
> > if an ARM host supports hibernation or not?
>
> Don't forget that Linux does not support ACPI on 32-bit ARM, which is
> quite different from the situation on 64-bit ARM.
>
> arch/arm/kernel/hibernate.c is only for 32-bit ARM, and is written with
> the assumption that there is no interaction required with any firmware
> to save state, and later restore state upon resuming.
>
> Or am I missing something?
Hi Russell,
Thanks for your reply and please excuse me for my ignorance of ARM.
So 32-bit ARM Linux can hibernate even if it doesn't support ACPI, but
I guess not all 32-bit ARM machines support hibernation? If my guess
is correct, is there any standard capability bit or something that can be
used to tell if an ARM machine supports hibernation? I'm purely curious. :-)
Do you imply 64-bit ARM Linux supports ACPI and the ACPI S4 state?
If not, how can we tell if a 64-bit ARM machine supports hibernation or not?
Thanks,
-- Dexuan
^ permalink raw reply
* [PATCH] scsi: storvsc: Add ability to change scsi queue depth
From: Branden Bonaby @ 2019-06-14 23:48 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, sashal, jejb, martin.petersen
Cc: Branden Bonaby, linux-hyperv, linux-scsi, linux-kernel
Adding functionality to allow the SCSI queue depth to be changed,
by utilizing the "scsi_change_queue_depth" function.
Signed-off-by: Branden Bonaby <brandonbonaby94@gmail.com>
---
drivers/scsi/storvsc_drv.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 8472de1007ff..719ca9906fc2 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -387,6 +387,7 @@ enum storvsc_request_type {
static int storvsc_ringbuffer_size = (128 * 1024);
static u32 max_outstanding_req_per_channel;
+static int storvsc_change_queue_depth(struct scsi_device *sdev, int queue_depth);
static int storvsc_vcpus_per_sub_channel = 4;
@@ -1711,6 +1712,7 @@ static struct scsi_host_template scsi_driver = {
.dma_boundary = PAGE_SIZE-1,
.no_write_same = 1,
.track_queue_depth = 1,
+ .change_queue_depth = storvsc_change_queue_depth,
};
enum {
@@ -1917,6 +1919,15 @@ static int storvsc_probe(struct hv_device *device,
return ret;
}
+/* Change a scsi target's queue depth */
+static int storvsc_change_queue_depth(struct scsi_device *sdev, int queue_depth)
+{
+ if (queue_depth > scsi_driver.can_queue){
+ queue_depth = scsi_driver.can_queue;
+ }
+ return scsi_change_queue_depth(sdev, queue_depth);
+}
+
static int storvsc_remove(struct hv_device *dev)
{
struct storvsc_device *stor_device = hv_get_drvdata(dev);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net] hvsock: fix epollout hang from race condition
From: David Miller @ 2019-06-15 2:14 UTC (permalink / raw)
To: sunilmut
Cc: kys, haiyangz, sthemmin, sashal, decui, mikelley, netdev,
linux-hyperv, linux-kernel
In-Reply-To: <MW2PR2101MB11164C6EEAA5C511B395EF3AC0EC0@MW2PR2101MB1116.namprd21.prod.outlook.com>
This adds lots of new warnings:
net/vmw_vsock/hyperv_transport.c: In function ‘hvs_probe’:
net/vmw_vsock/hyperv_transport.c:205:20: warning: ‘vnew’ may be used uninitialized in this function [-Wmaybe-uninitialized]
remote->svm_port = host_ephemeral_port++;
~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
net/vmw_vsock/hyperv_transport.c:332:21: note: ‘vnew’ was declared here
struct vsock_sock *vnew;
^~~~
net/vmw_vsock/hyperv_transport.c:406:22: warning: ‘hvs_new’ may be used uninitialized in this function [-Wmaybe-uninitialized]
hvs_new->vm_srv_id = *if_type;
~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
net/vmw_vsock/hyperv_transport.c:333:23: note: ‘hvs_new’ was declared here
struct hvsock *hvs, *hvs_new;
^~~~~~~
^ permalink raw reply
* Re: [PATCH net] hv_netvsc: Set probe mode to sync
From: David Miller @ 2019-06-15 2:47 UTC (permalink / raw)
To: haiyangz
Cc: sashal, linux-hyperv, netdev, kys, sthemmin, olaf, vkuznets,
linux-kernel
In-Reply-To: <1560459955-37900-1-git-send-email-haiyangz@microsoft.com>
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Thu, 13 Jun 2019 21:06:53 +0000
> For better consistency of synthetic NIC names, we set the probe mode to
> PROBE_FORCE_SYNCHRONOUS. So the names can be aligned with the vmbus
> channel offer sequence.
>
> Fixes: af0a5646cb8d ("use the new async probing feature for the hyperv drivers")
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Applied and queued up for -stable.
^ permalink raw reply
* RE: [PATCH net] hvsock: fix epollout hang from race condition
From: Dexuan Cui @ 2019-06-15 3:22 UTC (permalink / raw)
To: David Miller, Sunil Muthuswamy
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Michael Kelley, netdev@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190614.191456.407433636343988177.davem@davemloft.net>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of David Miller
> Sent: Friday, June 14, 2019 7:15 PM
> To: Sunil Muthuswamy <sunilmut@microsoft.com>
>
> This adds lots of new warnings:
>
> net/vmw_vsock/hyperv_transport.c: In function ‘hvs_probe’:
> net/vmw_vsock/hyperv_transport.c:205:20: warning: ‘vnew’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> remote->svm_port = host_ephemeral_port++;
> ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> net/vmw_vsock/hyperv_transport.c:332:21: note: ‘vnew’ was declared here
> struct vsock_sock *vnew;
> ^~~~
> net/vmw_vsock/hyperv_transport.c:406:22: warning: ‘hvs_new’ may be
> used uninitialized in this function [-Wmaybe-uninitialized]
> hvs_new->vm_srv_id = *if_type;
> ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
> net/vmw_vsock/hyperv_transport.c:333:23: note: ‘hvs_new’ was declared
> here
> struct hvsock *hvs, *hvs_new;
> ^~~~~~~
Hi David,
These warnings are not introduced by this patch from Sunil.
I'm not sure why I didn't notice these warnings before.
Probably my gcc version is not new eought?
Actually these warnings are bogus, as I checked the related functions,
which may confuse the compiler's static analysis.
I'm going to make a patch to initialize the pointers to NULL to suppress
the warnings. My patch will be based on the latest's net.git + this patch
from Sunil.
Thanks,
-- Dexuan
^ permalink raw reply
* [PATCH net] hv_sock: Suppress bogus "may be used uninitialized" warnings
From: Dexuan Cui @ 2019-06-15 5:00 UTC (permalink / raw)
To: netdev@vger.kernel.org, davem@davemloft.net, Michael Kelley
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan, Stephen Hemminger, Haiyang Zhang, Sasha Levin,
olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, vkuznets,
marcelo.cerri@canonical.com, Dexuan Cui
gcc 8.2.0 may report these bogus warnings under some condition:
warning: ‘vnew’ may be used uninitialized in this function
warning: ‘hvs_new’ may be used uninitialized in this function
Actually, the 2 pointers are only initialized and used if the variable
"conn_from_host" is true. The code is not buggy here.
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
net/vmw_vsock/hyperv_transport.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index 8d1ea9eda8a2..cd3f47f54fa7 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -329,8 +329,8 @@ static void hvs_open_connection(struct vmbus_channel *chan)
struct sockaddr_vm addr;
struct sock *sk, *new = NULL;
- struct vsock_sock *vnew;
- struct hvsock *hvs, *hvs_new;
+ struct vsock_sock *vnew = NULL;
+ struct hvsock *hvs, *hvs_new = NULL;
int ret;
if_type = &chan->offermsg.offer.if_type;
--
2.19.1
^ permalink raw reply related
* RE: [PATCH net] hvsock: fix epollout hang from race condition
From: Dexuan Cui @ 2019-06-15 5:03 UTC (permalink / raw)
To: David Miller, Sunil Muthuswamy
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Michael Kelley, netdev@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <PU1P153MB0169BACDA500F94910849770BFE90@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Dexuan Cui
> Sent: Friday, June 14, 2019 8:23 PM
> To: David Miller <davem@davemloft.net>; Sunil Muthuswamy
> <sunilmut@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; sashal@kernel.org; Michael Kelley
> <mikelley@microsoft.com>; netdev@vger.kernel.org;
> linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH net] hvsock: fix epollout hang from race condition
>
> > From: linux-hyperv-owner@vger.kernel.org
> > <linux-hyperv-owner@vger.kernel.org> On Behalf Of David Miller
> > Sent: Friday, June 14, 2019 7:15 PM
> > To: Sunil Muthuswamy <sunilmut@microsoft.com>
> >
> > This adds lots of new warnings:
> >
> > net/vmw_vsock/hyperv_transport.c: In function ‘hvs_probe’:
> > net/vmw_vsock/hyperv_transport.c:205:20: warning: ‘vnew’ may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
> > remote->svm_port = host_ephemeral_port++;
> > ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> > net/vmw_vsock/hyperv_transport.c:332:21: note: ‘vnew’ was declared
> here
> > struct vsock_sock *vnew;
> > ^~~~
> > net/vmw_vsock/hyperv_transport.c:406:22: warning: ‘hvs_new’ may be
> > used uninitialized in this function [-Wmaybe-uninitialized]
> > hvs_new->vm_srv_id = *if_type;
> > ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
> > net/vmw_vsock/hyperv_transport.c:333:23: note: ‘hvs_new’ was declared
> > here
> > struct hvsock *hvs, *hvs_new;
> > ^~~~~~~
>
> Hi David,
> These warnings are not introduced by this patch from Sunil.
Well, technically speaking, the warnings are caused by Suni's patch, but I think it should
be a bug of gcc (I'm using "gcc (Ubuntu 8.2.0-12ubuntu1) 8.2.0"). As you can see, the
line numbers given by gcc, i.e. line 205, line 406, are not related to the warnings.
Actually, the same warnings can repro with the below one-line patch on this commit of
today's net.git:
9a33629ba6b2 ("hv_netvsc: Set probe mode to sync")
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -403,6 +403,7 @@ static void hvs_open_connection(struct vmbus_channel *chan)
set_per_channel_state(chan, conn_from_host ? new : sk);
vmbus_set_chn_rescind_callback(chan, hvs_close_connection);
+ asm ("nop");
if (conn_from_host) {
new->sk_state = TCP_ESTABLISHED;
It looks a simple inline assembly code can confuse gcc. I'm not sure if I should
report a bug for gcc...
I posted a patch to suppress these bogus warnings just now. The Subject is:
[PATCH net] hv_sock: Suppress bogus "may be used uninitialized" warnings
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH] PCI: hv: Fix build error without CONFIG_SYSFS
From: Yuehaibing @ 2019-06-15 6:18 UTC (permalink / raw)
To: Michael Kelley, bhelgaas@google.com, Stephen Hemminger,
sashal@kernel.org, Dexuan Cui, linux-hyperv@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
In-Reply-To: <BYAPR21MB12211EEA95200F437C8E37ECD71A0@BYAPR21MB1221.namprd21.prod.outlook.com>
On 2019/6/2 6:59, Michael Kelley wrote:
> From: YueHaibing <yuehaibing@huawei.com> Sent: Friday, May 31, 2019 8:09 AM
>>
>> while building without CONFIG_SYSFS, fails as below:
>>
>> drivers/pci/controller/pci-hyperv.o: In function 'hv_pci_assign_slots':
>> pci-hyperv.c:(.text+0x40a): undefined reference to 'pci_create_slot'
>> drivers/pci/controller/pci-hyperv.o: In function 'pci_devices_present_work':
>> pci-hyperv.c:(.text+0xc02): undefined reference to 'pci_destroy_slot'
>> drivers/pci/controller/pci-hyperv.o: In function 'hv_pci_remove':
>> pci-hyperv.c:(.text+0xe50): undefined reference to 'pci_destroy_slot'
>> drivers/pci/controller/pci-hyperv.o: In function 'hv_eject_device_work':
>> pci-hyperv.c:(.text+0x11f9): undefined reference to 'pci_destroy_slot'
>>
>> Select SYSFS while PCI_HYPERV is set to fix this.
>>
>
> I'm wondering if is the right way to fix the problem. Conceptually
> is it possible to setup & operate virtual PCI devices like
> pci-hyperv.c does, even if sysfs is not present? Or is it right to
> always required sysfs?
>
> The function pci_dev_assign_slot() in slot.c has a null implementation
> in include/linux/pci.h when CONFIG_SYSFS is not defined, which
> seems to be trying to solve the same problem for that function. And
> if CONFIG_HOTPLUG_PCI is defined but CONFIG_SYSFS is not,
> pci_hp_create_module_link() and pci_hp_remove_module_link()
> look like they would have the same problem. Maybe there should
> be degenerate implementations of pci_create_slot() and
> pci_destroy_slot() for cases when CONFIG_SYSFS is not defined?
>
> But I'll admit I don't know the full story behind how PCI slots
> are represented and used, so maybe I'm off base. I just noticed
> the inconsistency in how other functions in slot.c are handled.
>
> Thoughts?
268a03a42d33 ("PCI: drivers/pci/slot.c should depend on CONFIG_SYSFS")
make slot.o depends CONFIG_SYSFS
commit 268a03a42d3377d5fb41e6e7cbdec4e0b65cab2e
Author: Alex Chiang <achiang@hp.com>
Date: Wed Jun 17 19:03:57 2009 -0600
PCI: drivers/pci/slot.c should depend on CONFIG_SYSFS
There is no way to interact with a physical PCI slot without
sysfs, so encode the dependency and prevent this build error:
drivers/pci/slot.c: In function 'pci_hp_create_module_link':
drivers/pci/slot.c:327: error: 'module_kset' undeclared
This patch _should_ make pci-sysfs.o depend on CONFIG_SYSFS too,
but we cannot (yet) because the PCI core merrily assumes the
existence of sysfs:
drivers/built-in.o: In function `pci_bus_add_device':
drivers/pci/bus.c:89: undefined reference to `pci_create_sysfs_dev_files'
drivers/built-in.o: In function `pci_stop_dev':
drivers/pci/remove.c:24: undefined reference to `pci_remove_sysfs_dev_files'
So do the minimal bit for now and figure out how to untangle it
later.
If No CONFIG_SYSFS, slot.o is not build
>
> Michael
>
>
^ permalink raw reply
* Re: [PATCH] PCI: hv: Fix build error without CONFIG_SYSFS
From: Yuehaibing @ 2019-06-15 6:48 UTC (permalink / raw)
To: Michael Kelley, bhelgaas@google.com, Stephen Hemminger,
sashal@kernel.org, Dexuan Cui, linux-hyperv@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
In-Reply-To: <7d8ca05e-7519-45d8-e694-d31e221696d5@huawei.com>
+cc Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
On 2019/6/15 14:18, Yuehaibing wrote:
>
> On 2019/6/2 6:59, Michael Kelley wrote:
>> From: YueHaibing <yuehaibing@huawei.com> Sent: Friday, May 31, 2019 8:09 AM
>>>
>>> while building without CONFIG_SYSFS, fails as below:
>>>
>>> drivers/pci/controller/pci-hyperv.o: In function 'hv_pci_assign_slots':
>>> pci-hyperv.c:(.text+0x40a): undefined reference to 'pci_create_slot'
>>> drivers/pci/controller/pci-hyperv.o: In function 'pci_devices_present_work':
>>> pci-hyperv.c:(.text+0xc02): undefined reference to 'pci_destroy_slot'
>>> drivers/pci/controller/pci-hyperv.o: In function 'hv_pci_remove':
>>> pci-hyperv.c:(.text+0xe50): undefined reference to 'pci_destroy_slot'
>>> drivers/pci/controller/pci-hyperv.o: In function 'hv_eject_device_work':
>>> pci-hyperv.c:(.text+0x11f9): undefined reference to 'pci_destroy_slot'
>>>
>>> Select SYSFS while PCI_HYPERV is set to fix this.
>>>
>>
>> I'm wondering if is the right way to fix the problem. Conceptually
>> is it possible to setup & operate virtual PCI devices like
>> pci-hyperv.c does, even if sysfs is not present? Or is it right to
>> always required sysfs?
>>
>> The function pci_dev_assign_slot() in slot.c has a null implementation
>> in include/linux/pci.h when CONFIG_SYSFS is not defined, which
>> seems to be trying to solve the same problem for that function. And
>> if CONFIG_HOTPLUG_PCI is defined but CONFIG_SYSFS is not,
>> pci_hp_create_module_link() and pci_hp_remove_module_link()
>> look like they would have the same problem. Maybe there should
>> be degenerate implementations of pci_create_slot() and
>> pci_destroy_slot() for cases when CONFIG_SYSFS is not defined?
>>
>> But I'll admit I don't know the full story behind how PCI slots
>> are represented and used, so maybe I'm off base. I just noticed
>> the inconsistency in how other functions in slot.c are handled.
>>
>> Thoughts?
>
> 268a03a42d33 ("PCI: drivers/pci/slot.c should depend on CONFIG_SYSFS")
>
> make slot.o depends CONFIG_SYSFS
>
> commit 268a03a42d3377d5fb41e6e7cbdec4e0b65cab2e
> Author: Alex Chiang <achiang@hp.com>
> Date: Wed Jun 17 19:03:57 2009 -0600
>
> PCI: drivers/pci/slot.c should depend on CONFIG_SYSFS
>
> There is no way to interact with a physical PCI slot without
> sysfs, so encode the dependency and prevent this build error:
>
> drivers/pci/slot.c: In function 'pci_hp_create_module_link':
> drivers/pci/slot.c:327: error: 'module_kset' undeclared
>
> This patch _should_ make pci-sysfs.o depend on CONFIG_SYSFS too,
> but we cannot (yet) because the PCI core merrily assumes the
> existence of sysfs:
>
> drivers/built-in.o: In function `pci_bus_add_device':
> drivers/pci/bus.c:89: undefined reference to `pci_create_sysfs_dev_files'
> drivers/built-in.o: In function `pci_stop_dev':
> drivers/pci/remove.c:24: undefined reference to `pci_remove_sysfs_dev_files'
>
> So do the minimal bit for now and figure out how to untangle it
> later.
>
> If No CONFIG_SYSFS, slot.o is not build
>
>>
>> Michael
>>
>>
>
>
> .
>
^ permalink raw reply
* RE: [PATCH net] hv_sock: Suppress bogus "may be used uninitialized" warnings
From: Sunil Muthuswamy @ 2019-06-15 7:19 UTC (permalink / raw)
To: Dexuan Cui, netdev@vger.kernel.org, davem@davemloft.net,
Michael Kelley
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan, Stephen Hemminger, Haiyang Zhang, Sasha Levin,
olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, vkuznets,
marcelo.cerri@canonical.com
In-Reply-To: <1560574826-99551-1-git-send-email-decui@microsoft.com>
> -----Original Message-----
> From: linux-hyperv-owner@vger.kernel.org <linux-hyperv-owner@vger.kernel.org> On Behalf Of Dexuan Cui
> Sent: Friday, June 14, 2019 10:01 PM
> To: netdev@vger.kernel.org; davem@davemloft.net; Michael Kelley <mikelley@microsoft.com>
> Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Sasha Levin <Alexander.Levin@microsoft.com>;
> olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com; vkuznets <vkuznets@redhat.com>; marcelo.cerri@canonical.com;
> Dexuan Cui <decui@microsoft.com>
> Subject: [PATCH net] hv_sock: Suppress bogus "may be used uninitialized" warnings
>
> gcc 8.2.0 may report these bogus warnings under some condition:
>
> warning: ‘vnew’ may be used uninitialized in this function
> warning: ‘hvs_new’ may be used uninitialized in this function
>
> Actually, the 2 pointers are only initialized and used if the variable
> "conn_from_host" is true. The code is not buggy here.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> net/vmw_vsock/hyperv_transport.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> index 8d1ea9eda8a2..cd3f47f54fa7 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -329,8 +329,8 @@ static void hvs_open_connection(struct vmbus_channel *chan)
>
> struct sockaddr_vm addr;
> struct sock *sk, *new = NULL;
> - struct vsock_sock *vnew;
> - struct hvsock *hvs, *hvs_new;
> + struct vsock_sock *vnew = NULL;
> + struct hvsock *hvs, *hvs_new = NULL;
> int ret;
>
These are all already fixed under
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=ac383f58f3c98de37fa67452acc5bd677396e9f3
Its just that that commit hasn't merged with the 'net' branch yet.
> if_type = &chan->offermsg.offer.if_type;
> --
> 2.19.1
^ 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