lkml.org 
[lkml]   [2020]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 17/17] virt: acrn: Introduce an interface for Service VM to control vCPU
    On Tue, Sep 22, 2020 at 07:43:11PM +0800, shuo.a.liu@intel.com wrote:
    > From: Shuo Liu <shuo.a.liu@intel.com>
    >
    > 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 <shuo.a.liu@intel.com>
    > Reviewed-by: Zhi Wang <zhi.a.wang@intel.com>
    > Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
    > Cc: Zhi Wang <zhi.a.wang@intel.com>
    > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
    > Cc: Yu Wang <yu1.wang@intel.com>
    > Cc: Reinette Chatre <reinette.chatre@intel.com>
    > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    > ---
    > 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 <yakui.zhao@intel.com>
    > */
    >
    > +#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

    \
     
     \ /
      Last update: 2020-09-27 12:44    [W:4.921 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site