[PATCH] Drivers: hv: vmbus: Add /sys/bus/vmbus/supported_features

[PATCH] Drivers: hv: vmbus: Add /sys/bus/vmbus/supported_features

[Dexuan Cui]

When a Linux VM runs on Hyper-V, if the host toolstack doesn't support
hibernation for the VM (this happens on old Hyper-V hosts like Windows
Server 2016, or new Hyper-V hosts if the admin or user doesn't declare
the hibernation intent for the VM), the VM is discouraged from trying
hibernation (because the host doesn't guarantee that the VM's virtual
hardware configuration will remain exactly the same across hibernation),
i.e. the VM should not try to set up the swap partition/file for
hibernation, etc.

x86 Hyper-V uses the presence of the virtual ACPI S4 state as the
indication of the host toolstack support for a VM. Currently there is
no easy and reliable way for the userspace to detect the presence of
the state (see https://lkml.org/lkml/2020/12/11/1097).  Add
/sys/bus/vmbus/supported_features for this purpose.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 Documentation/ABI/stable/sysfs-bus-vmbus |  7 +++++++
 drivers/hv/vmbus_drv.c                   | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
index c27b7b89477c..3ba765ae6695 100644
--- a/Documentation/ABI/stable/sysfs-bus-vmbus
+++ b/Documentation/ABI/stable/sysfs-bus-vmbus
@@ -1,3 +1,10 @@
+What:		/sys/bus/vmbus/supported_features
+Date:		Dec 2020
+KernelVersion:	5.11
+Contact:	Dexuan Cui <decui@microsoft.com>
+Description:	Features specific to VMs running on Hyper-V
+Users:		Daemon that sets up swap partition/file for hibernation
+
 What:		/sys/bus/vmbus/devices/<UUID>/id
 Date:		Jul 2009
 KernelVersion:	2.6.31
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index d491fdcee61f..958487a40a18 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -678,6 +678,25 @@ static const struct attribute_group vmbus_dev_group = {
 };
 __ATTRIBUTE_GROUPS(vmbus_dev);
 
+/* Set up bus attribute(s) for /sys/bus/vmbus/supported_features */
+static ssize_t supported_features_show(struct bus_type *bus, char *buf)
+{
+	bool hb = hv_is_hibernation_supported();
+
+	return sprintf(buf, "%s\n", hb ? "hibernation" : "");
+}
+
+static BUS_ATTR_RO(supported_features);
+
+static struct attribute *vmbus_bus_attrs[] = {
+	&bus_attr_supported_features.attr,
+	NULL,
+};
+static const struct attribute_group vmbus_bus_group = {
+	.attrs = vmbus_bus_attrs,
+};
+__ATTRIBUTE_GROUPS(vmbus_bus);
+
 /*
  * vmbus_uevent - add uevent for our device
  *
@@ -1024,6 +1043,7 @@ static struct bus_type  hv_bus = {
 	.uevent =		vmbus_uevent,
 	.dev_groups =		vmbus_dev_groups,
 	.drv_groups =		vmbus_drv_groups,
+	.bus_groups =		vmbus_bus_groups,
 	.pm =			&vmbus_pm,
 };
 
-- 
2.19.1

Re: [PATCH] Drivers: hv: vmbus: Add /sys/bus/vmbus/supported_features

[Wei Liu]

On Tue, Dec 22, 2020 at 04:12:22PM -0800, Dexuan Cui wrote:
> When a Linux VM runs on Hyper-V, if the host toolstack doesn't support
> hibernation for the VM (this happens on old Hyper-V hosts like Windows
> Server 2016, or new Hyper-V hosts if the admin or user doesn't declare
> the hibernation intent for the VM), the VM is discouraged from trying
> hibernation (because the host doesn't guarantee that the VM's virtual
> hardware configuration will remain exactly the same across hibernation),
> i.e. the VM should not try to set up the swap partition/file for
> hibernation, etc.
> 
> x86 Hyper-V uses the presence of the virtual ACPI S4 state as the
> indication of the host toolstack support for a VM. Currently there is
> no easy and reliable way for the userspace to detect the presence of
> the state (see https://lkml.org/lkml/2020/12/11/1097).  Add
> /sys/bus/vmbus/supported_features for this purpose.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  Documentation/ABI/stable/sysfs-bus-vmbus |  7 +++++++
>  drivers/hv/vmbus_drv.c                   | 20 ++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
> index c27b7b89477c..3ba765ae6695 100644
> --- a/Documentation/ABI/stable/sysfs-bus-vmbus
> +++ b/Documentation/ABI/stable/sysfs-bus-vmbus
> @@ -1,3 +1,10 @@
> +What:		/sys/bus/vmbus/supported_features
> +Date:		Dec 2020
> +KernelVersion:	5.11

Too late for 5.11 now.

Given this is a list of strings, do you want to enumerate them in a
Values section or the Description section?

Wei.

RE: [PATCH] Drivers: hv: vmbus: Add /sys/bus/vmbus/supported_features

[Dexuan Cui]

> From: Wei Liu <wei.liu@kernel.org>
> Sent: Tuesday, January 5, 2021 4:58 AM
> > ...
> >  Documentation/ABI/stable/sysfs-bus-vmbus |  7 +++++++
> >  drivers/hv/vmbus_drv.c                   | 20
> ++++++++++++++++++++
> >  2 files changed, 27 insertions(+)
> >
> > diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus
> b/Documentation/ABI/stable/sysfs-bus-vmbus
> > index c27b7b89477c..3ba765ae6695 100644
> > --- a/Documentation/ABI/stable/sysfs-bus-vmbus
> > +++ b/Documentation/ABI/stable/sysfs-bus-vmbus
> > @@ -1,3 +1,10 @@
> > +What:		/sys/bus/vmbus/supported_features
> > +Date:		Dec 2020
> > +KernelVersion:	5.11
> 
> Too late for 5.11 now.

The patch was posted 2 weeks ago, before 5.11-rc1. :-)

If possible, we still want this patch to be in 5.11, because:
1. The patch is small and IMO pretty safe.
2. The patch adds new code and doesn't really change any old code.
3. The patch adds a new sys file, which is needed by some
user space tool to auto-setup the configuration for hibernation.
We'd like to integrate the patch into the Linux distros ASAP and
as we know some distros don't accept a patch if it's not in the
mainline.

So, if the patch itself looks good, IMO it would be better to be
in v5.11 rather than in v5.12, which will need extra time of
2~3 months.

That said, I don't know if there is any hard rule in regard of
the timing here. If there is, then v5.12 is OK to me. :-)
 
> Given this is a list of strings, do you want to enumerate them in a
> Values section or the Description section?
> 
> Wei.

Currently there is only one possible string "hibernation".
Not sure if we would need to add more strings in future, but yes
it's a good idea to list the string in a "Value" section. 

Will post v2 shortly.

Thanks,
-- Dexuan

Re: [PATCH] Drivers: hv: vmbus: Add /sys/bus/vmbus/supported_features

[Wei Liu]

On Tue, Jan 05, 2021 at 11:04:15PM +0000, Dexuan Cui wrote:
> > From: Wei Liu <wei.liu@kernel.org>
> > Sent: Tuesday, January 5, 2021 4:58 AM
> > > ...
> > >  Documentation/ABI/stable/sysfs-bus-vmbus |  7 +++++++
> > >  drivers/hv/vmbus_drv.c                   | 20
> > ++++++++++++++++++++
> > >  2 files changed, 27 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus
> > b/Documentation/ABI/stable/sysfs-bus-vmbus
> > > index c27b7b89477c..3ba765ae6695 100644
> > > --- a/Documentation/ABI/stable/sysfs-bus-vmbus
> > > +++ b/Documentation/ABI/stable/sysfs-bus-vmbus
> > > @@ -1,3 +1,10 @@
> > > +What:		/sys/bus/vmbus/supported_features
> > > +Date:		Dec 2020
> > > +KernelVersion:	5.11
> > 
> > Too late for 5.11 now.
> 
> The patch was posted 2 weeks ago, before 5.11-rc1. :-)
> 
> If possible, we still want this patch to be in 5.11, because:
> 1. The patch is small and IMO pretty safe.
> 2. The patch adds new code and doesn't really change any old code.
> 3. The patch adds a new sys file, which is needed by some
> user space tool to auto-setup the configuration for hibernation.
> We'd like to integrate the patch into the Linux distros ASAP and
> as we know some distros don't accept a patch if it's not in the
> mainline.
> 
> So, if the patch itself looks good, IMO it would be better to be
> in v5.11 rather than in v5.12, which will need extra time of
> 2~3 months.
> 
> That said, I don't know if there is any hard rule in regard of
> the timing here. If there is, then v5.12 is OK to me. :-)
>  

By the time you posted this (Dec 22), 5.11 was already more or less
"frozen". Linus wanted -next patches to be merged before Christmas.

The way I see it this is a new sysfs interface so I think this is
something new, which is for 5.12.

Do you think this should be considered a bug fix?

Wei.

> > Given this is a list of strings, do you want to enumerate them in a
> > Values section or the Description section?
> > 
> > Wei.
> 
> Currently there is only one possible string "hibernation".
> Not sure if we would need to add more strings in future, but yes
> it's a good idea to list the string in a "Value" section. 
> 
> Will post v2 shortly.
> 
> Thanks,
> -- Dexuan

RE: [PATCH] Drivers: hv: vmbus: Add /sys/bus/vmbus/supported_features

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Tuesday, December 22, 2020 4:12 PM
> 
> When a Linux VM runs on Hyper-V, if the host toolstack doesn't support
> hibernation for the VM (this happens on old Hyper-V hosts like Windows
> Server 2016, or new Hyper-V hosts if the admin or user doesn't declare
> the hibernation intent for the VM), the VM is discouraged from trying
> hibernation (because the host doesn't guarantee that the VM's virtual
> hardware configuration will remain exactly the same across hibernation),
> i.e. the VM should not try to set up the swap partition/file for
> hibernation, etc.
> 
> x86 Hyper-V uses the presence of the virtual ACPI S4 state as the
> indication of the host toolstack support for a VM. Currently there is
> no easy and reliable way for the userspace to detect the presence of
> the state (see https://lkml.org/lkml/2020/12/11/1097).  Add
> /sys/bus/vmbus/supported_features for this purpose.

I'm OK with surfacing the hibernation capability via an entry in
/sys/bus/vmbus.  Correct me if I'm wrong, but I think the concept
being surfaced is not "ACPI S4 state" precisely, but slightly more
generally whether hibernation is supported for the VM.  While
those two concepts may be 1:1 for the moment, there might be
future configurations where "hibernation is supported" depends
on other factors as well.

The guidance for things in /sys is that they generally should
be single valued (see Documentation/filesystems/sysfs.rst).  So my
recommendation is to create a "hibernation" entry that has a value
of 0 or 1.  That's the pattern I see in lots of other places in /sys.  If
other Hyper-V or VMbus-related features need to be surfaced in
the future, they would have their own single-valued entry.

Michael

> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  Documentation/ABI/stable/sysfs-bus-vmbus |  7 +++++++
>  drivers/hv/vmbus_drv.c                   | 20 ++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus
> b/Documentation/ABI/stable/sysfs-bus-vmbus
> index c27b7b89477c..3ba765ae6695 100644
> --- a/Documentation/ABI/stable/sysfs-bus-vmbus
> +++ b/Documentation/ABI/stable/sysfs-bus-vmbus
> @@ -1,3 +1,10 @@
> +What:		/sys/bus/vmbus/supported_features
> +Date:		Dec 2020
> +KernelVersion:	5.11
> +Contact:	Dexuan Cui <decui@microsoft.com>
> +Description:	Features specific to VMs running on Hyper-V
> +Users:		Daemon that sets up swap partition/file for hibernation
> +
>  What:		/sys/bus/vmbus/devices/<UUID>/id
>  Date:		Jul 2009
>  KernelVersion:	2.6.31
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index d491fdcee61f..958487a40a18 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -678,6 +678,25 @@ static const struct attribute_group vmbus_dev_group = {
>  };
>  __ATTRIBUTE_GROUPS(vmbus_dev);
> 
> +/* Set up bus attribute(s) for /sys/bus/vmbus/supported_features */
> +static ssize_t supported_features_show(struct bus_type *bus, char *buf)
> +{
> +	bool hb = hv_is_hibernation_supported();
> +
> +	return sprintf(buf, "%s\n", hb ? "hibernation" : "");
> +}
> +
> +static BUS_ATTR_RO(supported_features);
> +
> +static struct attribute *vmbus_bus_attrs[] = {
> +	&bus_attr_supported_features.attr,
> +	NULL,
> +};
> +static const struct attribute_group vmbus_bus_group = {
> +	.attrs = vmbus_bus_attrs,
> +};
> +__ATTRIBUTE_GROUPS(vmbus_bus);
> +
>  /*
>   * vmbus_uevent - add uevent for our device
>   *
> @@ -1024,6 +1043,7 @@ static struct bus_type  hv_bus = {
>  	.uevent =		vmbus_uevent,
>  	.dev_groups =		vmbus_dev_groups,
>  	.drv_groups =		vmbus_drv_groups,
> +	.bus_groups =		vmbus_bus_groups,
>  	.pm =			&vmbus_pm,
>  };
> 
> --
> 2.19.1

RE: [PATCH] Drivers: hv: vmbus: Add /sys/bus/vmbus/supported_features

[Dexuan Cui]

> From: Wei Liu <wei.liu@kernel.org>
> Sent: Wednesday, January 6, 2021 8:24 AM
> To: Dexuan Cui <decui@microsoft.com>
> >
> > That said, I don't know if there is any hard rule in regard of
> > the timing here. If there is, then v5.12 is OK to me. :-)
> >
> 
> By the time you posted this (Dec 22), 5.11 was already more or less
> "frozen". Linus wanted -next patches to be merged before Christmas.

Got it. Thanks for the explanation!

> The way I see it this is a new sysfs interface so I think this is
> something new, which is for 5.12.

Ok. 

> Do you think this should be considered a bug fix?
> 
> Wei.

Then let's not consider it for v5.11. For now I think the userspace
tool/daemon can check 'dmesg' for the existence of ACPI S4 state
as a workaround. This is not ideal, but it should work reasonably
well, assuming the tool/daemon runs early enough, so the kernel
msg buffer is not yet filled up and overwritten. :-)

Thanks,
-- Dexuan

RE: [PATCH] Drivers: hv: vmbus: Add /sys/bus/vmbus/supported_features

[Dexuan Cui]

> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Wednesday, January 6, 2021 9:38 AM
> From: Dexuan Cui <decui@microsoft.com>
> Sent: Tuesday, December 22, 2020 4:12 PM
> >
> > When a Linux VM runs on Hyper-V, if the host toolstack doesn't support
> > hibernation for the VM (this happens on old Hyper-V hosts like Windows
> > Server 2016, or new Hyper-V hosts if the admin or user doesn't declare
> > the hibernation intent for the VM), the VM is discouraged from trying
> > hibernation (because the host doesn't guarantee that the VM's virtual
> > hardware configuration will remain exactly the same across hibernation),
> > i.e. the VM should not try to set up the swap partition/file for
> > hibernation, etc.
> >
> > x86 Hyper-V uses the presence of the virtual ACPI S4 state as the
> > indication of the host toolstack support for a VM. Currently there is
> > no easy and reliable way for the userspace to detect the presence of
> > the state (see ...).  Add
> > /sys/bus/vmbus/supported_features for this purpose.
>
> I'm OK with surfacing the hibernation capability via an entry in
> /sys/bus/vmbus.  Correct me if I'm wrong, but I think the concept
> being surfaced is not "ACPI S4 state" precisely, but slightly more
> generally whether hibernation is supported for the VM.  While
> those two concepts may be 1:1 for the moment, there might be
> future configurations where "hibernation is supported" depends
> on other factors as well.

For x86, I believe the virtual ACPI S4 state exists only when the
admin/user declares the intent of "enable hibernation for the VM" via
some PowwerShell/WMI command. On Azure, if a VM size is not suitable
for hibernation (e.g. an existing VM has an ephemeral local disk),
the toolstack on the host should not enable the ACPI S4 state for the
VM. That's why we implemented hv_is_hibernation_supported() for x86 by
checking the ACPI S4 state, and we have used the function
hv_is_hibernation_supported() in hv_utils and hv_balloon for quite a
while.

For ARM, IIRC there is no concept of ACPI S4 state, so currently
hv_is_hibernation_supported() is actually not implemented. Not sure
why hv_utils and hv_balloon can build successfully... :-) Probably
Boqun can help to take a look.

>
> The guidance for things in /sys is that they generally should
> be single valued (see Documentation/filesystems/sysfs.rst).  So my
> recommendation is to create a "hibernation" entry that has a value
> of 0 or 1.
>
> Michael

Got it. Then let's use /sys/bus/vmbus/hibernation.

Will post v3.

Thanks,
-- Dexuan

Re: [PATCH] Drivers: hv: vmbus: Add /sys/bus/vmbus/supported_features

[Boqun Feng]

On Wed, Jan 06, 2021 at 08:49:32PM +0000, Dexuan Cui wrote:
> > From: Michael Kelley <mikelley@microsoft.com>
> > Sent: Wednesday, January 6, 2021 9:38 AM
> > From: Dexuan Cui <decui@microsoft.com>
> > Sent: Tuesday, December 22, 2020 4:12 PM
> > >
> > > When a Linux VM runs on Hyper-V, if the host toolstack doesn't support
> > > hibernation for the VM (this happens on old Hyper-V hosts like Windows
> > > Server 2016, or new Hyper-V hosts if the admin or user doesn't declare
> > > the hibernation intent for the VM), the VM is discouraged from trying
> > > hibernation (because the host doesn't guarantee that the VM's virtual
> > > hardware configuration will remain exactly the same across hibernation),
> > > i.e. the VM should not try to set up the swap partition/file for
> > > hibernation, etc.
> > >
> > > x86 Hyper-V uses the presence of the virtual ACPI S4 state as the
> > > indication of the host toolstack support for a VM. Currently there is
> > > no easy and reliable way for the userspace to detect the presence of
> > > the state (see ...).  Add
> > > /sys/bus/vmbus/supported_features for this purpose.
> >
> > I'm OK with surfacing the hibernation capability via an entry in
> > /sys/bus/vmbus.  Correct me if I'm wrong, but I think the concept
> > being surfaced is not "ACPI S4 state" precisely, but slightly more
> > generally whether hibernation is supported for the VM.  While
> > those two concepts may be 1:1 for the moment, there might be
> > future configurations where "hibernation is supported" depends
> > on other factors as well.
> 
> For x86, I believe the virtual ACPI S4 state exists only when the
> admin/user declares the intent of "enable hibernation for the VM" via
> some PowwerShell/WMI command. On Azure, if a VM size is not suitable
> for hibernation (e.g. an existing VM has an ephemeral local disk),
> the toolstack on the host should not enable the ACPI S4 state for the
> VM. That's why we implemented hv_is_hibernation_supported() for x86 by
> checking the ACPI S4 state, and we have used the function
> hv_is_hibernation_supported() in hv_utils and hv_balloon for quite a
> while.
> 
> For ARM, IIRC there is no concept of ACPI S4 state, so currently
> hv_is_hibernation_supported() is actually not implemented. Not sure

Because the core support for ARM64 Hyper-V is not merged yet. In
Michael's core patchset, hv_is_hibernation_supported() is implemented as
always returning false, and there is more work (other than Michael's
core pachset) to make hiberation work on ARM64 Hyper-V guest.

Regards,
Boqun

> why hv_utils and hv_balloon can build successfully... :-) Probably
> Boqun can help to take a look.
> 
> >
> > The guidance for things in /sys is that they generally should
> > be single valued (see Documentation/filesystems/sysfs.rst).  So my
> > recommendation is to create a "hibernation" entry that has a value
> > of 0 or 1.
> >
> > Michael
> 
> Got it. Then let's use /sys/bus/vmbus/hibernation.
> 
> Will post v3.
> 
> Thanks,
> -- Dexuan
>