From: Shuo Liu <[email protected]>
ACRN supports partition mode to achieve real-time requirements. In
partition mode, a CPU core can be dedicated to a vCPU of User VM. The
local APIC of the dedicated CPU core can be passthrough to the User VM.
The Service VM controls the assignment of the CPU cores.
Introduce an interface for the Service VM to remove the control of CPU
core from hypervisor perspective so that the CPU core can be a dedicated
CPU core of User VM.
Signed-off-by: Shuo Liu <[email protected]>
Reviewed-by: Zhi Wang <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Cc: Zhi Wang <[email protected]>
Cc: Zhenyu Wang <[email protected]>
Cc: Yu Wang <[email protected]>
Cc: Reinette Chatre <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/virt/acrn/hsm.c | 50 +++++++++++++++++++++++++++++++++++
drivers/virt/acrn/hypercall.h | 14 ++++++++++
2 files changed, 64 insertions(+)
diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
index aaf4e76d27b4..ef5f77a38d1f 100644
--- a/drivers/virt/acrn/hsm.c
+++ b/drivers/virt/acrn/hsm.c
@@ -9,6 +9,7 @@
* Yakui Zhao <[email protected]>
*/
+#include <linux/cpu.h>
#include <linux/io.h>
#include <linux/mm.h>
#include <linux/module.h>
@@ -354,6 +355,47 @@ struct miscdevice acrn_dev = {
.fops = &acrn_fops,
};
+static ssize_t remove_cpu_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ u64 cpu, lapicid;
+ int ret;
+
+ if (kstrtoull(buf, 0, &cpu) < 0)
+ return -EINVAL;
+
+ if (cpu >= num_possible_cpus() || cpu == 0 || !cpu_is_hotpluggable(cpu))
+ return -EINVAL;
+
+ if (cpu_online(cpu))
+ remove_cpu(cpu);
+
+ lapicid = cpu_data(cpu).apicid;
+ dev_dbg(dev, "Try to remove cpu %lld with lapicid %lld\n", cpu, lapicid);
+ ret = hcall_sos_remove_cpu(lapicid);
+ if (ret < 0) {
+ dev_err(dev, "Failed to remove cpu %lld!\n", cpu);
+ goto fail_remove;
+ }
+
+ return count;
+
+fail_remove:
+ add_cpu(cpu);
+ return ret;
+}
+static DEVICE_ATTR_WO(remove_cpu);
+
+static struct attribute *acrn_attrs[] = {
+ &dev_attr_remove_cpu.attr,
+ NULL
+};
+
+static struct attribute_group acrn_attr_group = {
+ .attrs = acrn_attrs,
+};
+
static int __init hsm_init(void)
{
int ret;
@@ -370,13 +412,21 @@ static int __init hsm_init(void)
return ret;
}
+ ret = sysfs_create_group(&acrn_dev.this_device->kobj, &acrn_attr_group);
+ if (ret) {
+ dev_warn(acrn_dev.this_device, "sysfs create failed\n");
+ misc_deregister(&acrn_dev);
+ return ret;
+ }
acrn_ioreq_intr_setup();
+
return 0;
}
static void __exit hsm_exit(void)
{
acrn_ioreq_intr_remove();
+ sysfs_remove_group(&acrn_dev.this_device->kobj, &acrn_attr_group);
misc_deregister(&acrn_dev);
}
module_init(hsm_init);
diff --git a/drivers/virt/acrn/hypercall.h b/drivers/virt/acrn/hypercall.h
index e640632366f0..0cfad05bd1a9 100644
--- a/drivers/virt/acrn/hypercall.h
+++ b/drivers/virt/acrn/hypercall.h
@@ -13,6 +13,9 @@
#define HC_ID 0x80UL
+#define HC_ID_GEN_BASE 0x0UL
+#define HC_SOS_REMOVE_CPU _HC_ID(HC_ID, HC_ID_GEN_BASE + 0x01)
+
#define HC_ID_VM_BASE 0x10UL
#define HC_CREATE_VM _HC_ID(HC_ID, HC_ID_VM_BASE + 0x00)
#define HC_DESTROY_VM _HC_ID(HC_ID, HC_ID_VM_BASE + 0x01)
@@ -42,6 +45,17 @@
#define HC_ID_PM_BASE 0x80UL
#define HC_PM_GET_CPU_STATE _HC_ID(HC_ID, HC_ID_PM_BASE + 0x00)
+/**
+ * hcall_sos_remove_cpu() - Remove a vCPU of Service VM
+ * @cpu: The vCPU to be removed
+ *
+ * Return: 0 on success, <0 on failure
+ */
+static inline long hcall_sos_remove_cpu(u64 cpu)
+{
+ return acrn_hypercall1(HC_SOS_REMOVE_CPU, cpu);
+}
+
/**
* hcall_create_vm() - Create a User VM
* @vminfo: Service VM GPA of info of User VM creation
--
2.28.0
On Tue, Sep 22, 2020 at 07:43:11PM +0800, [email protected] wrote:
> From: Shuo Liu <[email protected]>
>
> ACRN supports partition mode to achieve real-time requirements. In
> partition mode, a CPU core can be dedicated to a vCPU of User VM. The
> local APIC of the dedicated CPU core can be passthrough to the User VM.
> The Service VM controls the assignment of the CPU cores.
>
> Introduce an interface for the Service VM to remove the control of CPU
> core from hypervisor perspective so that the CPU core can be a dedicated
> CPU core of User VM.
>
> Signed-off-by: Shuo Liu <[email protected]>
> Reviewed-by: Zhi Wang <[email protected]>
> Reviewed-by: Reinette Chatre <[email protected]>
> Cc: Zhi Wang <[email protected]>
> Cc: Zhenyu Wang <[email protected]>
> Cc: Yu Wang <[email protected]>
> Cc: Reinette Chatre <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> ---
> drivers/virt/acrn/hsm.c | 50 +++++++++++++++++++++++++++++++++++
> drivers/virt/acrn/hypercall.h | 14 ++++++++++
> 2 files changed, 64 insertions(+)
>
> diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
> index aaf4e76d27b4..ef5f77a38d1f 100644
> --- a/drivers/virt/acrn/hsm.c
> +++ b/drivers/virt/acrn/hsm.c
> @@ -9,6 +9,7 @@
> * Yakui Zhao <[email protected]>
> */
>
> +#include <linux/cpu.h>
> #include <linux/io.h>
> #include <linux/mm.h>
> #include <linux/module.h>
> @@ -354,6 +355,47 @@ struct miscdevice acrn_dev = {
> .fops = &acrn_fops,
> };
>
> +static ssize_t remove_cpu_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + u64 cpu, lapicid;
> + int ret;
> +
> + if (kstrtoull(buf, 0, &cpu) < 0)
> + return -EINVAL;
> +
> + if (cpu >= num_possible_cpus() || cpu == 0 || !cpu_is_hotpluggable(cpu))
> + return -EINVAL;
> +
> + if (cpu_online(cpu))
> + remove_cpu(cpu);
> +
> + lapicid = cpu_data(cpu).apicid;
> + dev_dbg(dev, "Try to remove cpu %lld with lapicid %lld\n", cpu, lapicid);
> + ret = hcall_sos_remove_cpu(lapicid);
> + if (ret < 0) {
> + dev_err(dev, "Failed to remove cpu %lld!\n", cpu);
> + goto fail_remove;
> + }
> +
> + return count;
> +
> +fail_remove:
> + add_cpu(cpu);
> + return ret;
> +}
> +static DEVICE_ATTR_WO(remove_cpu);
> +
> +static struct attribute *acrn_attrs[] = {
> + &dev_attr_remove_cpu.attr,
> + NULL
> +};
> +
> +static struct attribute_group acrn_attr_group = {
> + .attrs = acrn_attrs,
> +};
You create a sysfs attribute without any Documentation/ABI/ update as
well? That's not good.
And why are you trying to emulate CPU hotplug here and not using the
existing CPU hotplug mechanism?
> +
> static int __init hsm_init(void)
> {
> int ret;
> @@ -370,13 +412,21 @@ static int __init hsm_init(void)
> return ret;
> }
>
> + ret = sysfs_create_group(&acrn_dev.this_device->kobj, &acrn_attr_group);
> + if (ret) {
> + dev_warn(acrn_dev.this_device, "sysfs create failed\n");
> + misc_deregister(&acrn_dev);
> + return ret;
> + }
You just raced with userspace and lost. If you want to add attribute
files to a device, use the default attribute group list, and it will be
managed properly for you by the driver core.
Huge hint, if a driver every has to touch a kobject, or call sysfs_*,
then it is probably doing something wrong.
greg k-h
Hi Greg,
On Sun 27.Sep'20 at 12:44:14 +0200, Greg Kroah-Hartman wrote:
>On Tue, Sep 22, 2020 at 07:43:11PM +0800, [email protected] wrote:
>> From: Shuo Liu <[email protected]>
>>
>> ACRN supports partition mode to achieve real-time requirements. In
>> partition mode, a CPU core can be dedicated to a vCPU of User VM. The
>> local APIC of the dedicated CPU core can be passthrough to the User VM.
>> The Service VM controls the assignment of the CPU cores.
>>
>> Introduce an interface for the Service VM to remove the control of CPU
>> core from hypervisor perspective so that the CPU core can be a dedicated
>> CPU core of User VM.
>>
>> Signed-off-by: Shuo Liu <[email protected]>
>> Reviewed-by: Zhi Wang <[email protected]>
>> Reviewed-by: Reinette Chatre <[email protected]>
>> Cc: Zhi Wang <[email protected]>
>> Cc: Zhenyu Wang <[email protected]>
>> Cc: Yu Wang <[email protected]>
>> Cc: Reinette Chatre <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> ---
>> drivers/virt/acrn/hsm.c | 50 +++++++++++++++++++++++++++++++++++
>> drivers/virt/acrn/hypercall.h | 14 ++++++++++
>> 2 files changed, 64 insertions(+)
>>
>> diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
>> index aaf4e76d27b4..ef5f77a38d1f 100644
>> --- a/drivers/virt/acrn/hsm.c
>> +++ b/drivers/virt/acrn/hsm.c
>> @@ -9,6 +9,7 @@
>> * Yakui Zhao <[email protected]>
>> */
>>
>> +#include <linux/cpu.h>
>> #include <linux/io.h>
>> #include <linux/mm.h>
>> #include <linux/module.h>
>> @@ -354,6 +355,47 @@ struct miscdevice acrn_dev = {
>> .fops = &acrn_fops,
>> };
>>
>> +static ssize_t remove_cpu_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + u64 cpu, lapicid;
>> + int ret;
>> +
>> + if (kstrtoull(buf, 0, &cpu) < 0)
>> + return -EINVAL;
>> +
>> + if (cpu >= num_possible_cpus() || cpu == 0 || !cpu_is_hotpluggable(cpu))
>> + return -EINVAL;
>> +
>> + if (cpu_online(cpu))
>> + remove_cpu(cpu);
>> +
>> + lapicid = cpu_data(cpu).apicid;
>> + dev_dbg(dev, "Try to remove cpu %lld with lapicid %lld\n", cpu, lapicid);
>> + ret = hcall_sos_remove_cpu(lapicid);
>> + if (ret < 0) {
>> + dev_err(dev, "Failed to remove cpu %lld!\n", cpu);
>> + goto fail_remove;
>> + }
>> +
>> + return count;
>> +
>> +fail_remove:
>> + add_cpu(cpu);
>> + return ret;
>> +}
>> +static DEVICE_ATTR_WO(remove_cpu);
>> +
>> +static struct attribute *acrn_attrs[] = {
>> + &dev_attr_remove_cpu.attr,
>> + NULL
>> +};
>> +
>> +static struct attribute_group acrn_attr_group = {
>> + .attrs = acrn_attrs,
>> +};
>
>You create a sysfs attribute without any Documentation/ABI/ update as
>well? That's not good.
Sorry, i will add it in the ABI/testing.
>
>And why are you trying to emulate CPU hotplug here and not using the
>existing CPU hotplug mechanism?
The interface introduced here includes:
1) The Service VM virtual CPU hotplug
2) hypercall to the hypervisor to remove one virtual CPU from the
Service VM
The 1) just do the CPU hotplug with kernel API remove_cpu(), and can be
resume back (by CPU online interface) if only 1) is done.
If 2) is done, then the physical CPU will be removed from the Service
VM's CPU pool. The ACRN hypervisor supports passthrough a physical CPU
to a VM. The precondition is that the physical CPU cannot be occupied by
any other VM. This interface intends to do that.
>
>> +
>> static int __init hsm_init(void)
>> {
>> int ret;
>> @@ -370,13 +412,21 @@ static int __init hsm_init(void)
>> return ret;
>> }
>>
>> + ret = sysfs_create_group(&acrn_dev.this_device->kobj, &acrn_attr_group);
>> + if (ret) {
>> + dev_warn(acrn_dev.this_device, "sysfs create failed\n");
>> + misc_deregister(&acrn_dev);
>> + return ret;
>> + }
>
>You just raced with userspace and lost. If you want to add attribute
>files to a device, use the default attribute group list, and it will be
>managed properly for you by the driver core.
>
>Huge hint, if a driver every has to touch a kobject, or call sysfs_*,
>then it is probably doing something wrong.
Do you mean use .groups of struct miscdevice directly ?
If yes, let me follow drivers/char/hw_random/s390-trng.c to do this.
BTW, few driver use the .groups directly. :)
Thanks
shuo
On Mon, Sep 28, 2020 at 12:10:07PM +0800, Shuo A Liu wrote:
> > You just raced with userspace and lost. If you want to add attribute
> > files to a device, use the default attribute group list, and it will be
> > managed properly for you by the driver core.
> >
> > Huge hint, if a driver every has to touch a kobject, or call sysfs_*,
> > then it is probably doing something wrong.
>
> Do you mean use .groups of struct miscdevice directly ?
>
> If yes, let me follow drivers/char/hw_random/s390-trng.c to do this.
> BTW, few driver use the .groups directly. :)
Drivers should almost never be messing with individual sysfs files. And
this ability to use .groups is a "new" one, conversions of existing code
that do not use them is always welcome.
thanks,
greg k-h
On Mon 28.Sep'20 at 7:23:05 +0200, Greg Kroah-Hartman wrote:
>On Mon, Sep 28, 2020 at 12:10:07PM +0800, Shuo A Liu wrote:
>> > You just raced with userspace and lost. If you want to add attribute
>> > files to a device, use the default attribute group list, and it will be
>> > managed properly for you by the driver core.
>> >
>> > Huge hint, if a driver every has to touch a kobject, or call sysfs_*,
>> > then it is probably doing something wrong.
>>
>> Do you mean use .groups of struct miscdevice directly ?
>>
>> If yes, let me follow drivers/char/hw_random/s390-trng.c to do this.
>> BTW, few driver use the .groups directly. :)
>
>Drivers should almost never be messing with individual sysfs files. And
>this ability to use .groups is a "new" one, conversions of existing code
>that do not use them is always welcome.
OK. Thanks for the explanation. I will follow the 'new' way :)
Thanks
shuo