David Vrabel
2016-Apr-05 09:45 UTC
[Xen-devel] [PATCH v4 4/6] xen: add xen_pin_vcpu() to support calling functions on a dedicated pcpu
On 05/04/16 06:10, Juergen Gross wrote:> Some hardware models (e.g. Dell Studio 1555 laptops) require calls to > the firmware to be issued on cpu 0 only. As Dom0 might have to use > these calls, add xen_pin_vcpu() to achieve this functionality. > > In case either the domain doesn't have the privilege to make the > related hypercall or the hypervisor isn't supporting it, issue a > warning once and disable further pinning attempts.[...]> --- a/arch/x86/xen/enlighten.c > +++ b/arch/x86/xen/enlighten.c > @@ -1885,6 +1885,45 @@ static void xen_set_cpu_features(struct cpuinfo_x86 *c) > } > } > > +static void xen_pin_vcpu(int cpu) > +{ > + static bool disable_pinning; > + struct sched_pin_override pin_override; > + int ret; > + > + if (disable_pinning) > + return; > + > + pin_override.pcpu = cpu; > + ret = HYPERVISOR_sched_op(SCHEDOP_pin_override, &pin_override);/* Ignore errors when removing override. */> + if (cpu < 0) > + return; > + > + switch (ret) { > + case -ENOSYS: > + pr_warn("The kernel tried to call a function on physical cpu %d, but Xen isn't\n" > + "supporting this. In case of problems you might consider vcpu pinning.\n", > + cpu); > + disable_pinning = true; > + break; > + case -EPERM: > + WARN(1, "Trying to pin vcpu without having privilege to do so\n"); > + disable_pinning = true; > + break; > + case -EINVAL: > + case -EBUSY: > + pr_warn("The kernel tried to call a function on physical cpu %d, but this cpu\n" > + "seems not to be available. Please check your Xen cpu configuration.\n", > + cpu); > + break; > + case 0: > + break; > + default: > + WARN(1, "rc %d while trying to pin vcpu\n", ret); > + disable_pinning = true; > + }These messages are a bit wordy for my taste and since they don't say what function failed or what driver is affected they're not as useful as they could be. I'd probably turn these all into: if (cpu >= 0 && ret < 0) { pr_warn("Failed to pin VCPU %d to physical CPU %d: %d", smp_processor_id(), cpu, ret); disable_pinning = true; } And look at getting the user of this API to print a more useful error. "i8k: unable to call SMM BIOS on physical CPU %d: %d" Or whatever. David
Juergen Gross
2016-Apr-05 10:01 UTC
[Xen-devel] [PATCH v4 4/6] xen: add xen_pin_vcpu() to support calling functions on a dedicated pcpu
On 05/04/16 11:45, David Vrabel wrote:> On 05/04/16 06:10, Juergen Gross wrote: >> Some hardware models (e.g. Dell Studio 1555 laptops) require calls to >> the firmware to be issued on cpu 0 only. As Dom0 might have to use >> these calls, add xen_pin_vcpu() to achieve this functionality. >> >> In case either the domain doesn't have the privilege to make the >> related hypercall or the hypervisor isn't supporting it, issue a >> warning once and disable further pinning attempts. > [...] >> --- a/arch/x86/xen/enlighten.c >> +++ b/arch/x86/xen/enlighten.c >> @@ -1885,6 +1885,45 @@ static void xen_set_cpu_features(struct cpuinfo_x86 *c) >> } >> } >> >> +static void xen_pin_vcpu(int cpu) >> +{ >> + static bool disable_pinning; >> + struct sched_pin_override pin_override; >> + int ret; >> + >> + if (disable_pinning) >> + return; >> + >> + pin_override.pcpu = cpu; >> + ret = HYPERVISOR_sched_op(SCHEDOP_pin_override, &pin_override); > > /* Ignore errors when removing override. */Okay.>> + if (cpu < 0) >> + return; >> + >> + switch (ret) { >> + case -ENOSYS: >> + pr_warn("The kernel tried to call a function on physical cpu %d, but Xen isn't\n" >> + "supporting this. In case of problems you might consider vcpu pinning.\n", >> + cpu); >> + disable_pinning = true; >> + break; >> + case -EPERM: >> + WARN(1, "Trying to pin vcpu without having privilege to do so\n"); >> + disable_pinning = true; >> + break; >> + case -EINVAL: >> + case -EBUSY: >> + pr_warn("The kernel tried to call a function on physical cpu %d, but this cpu\n" >> + "seems not to be available. Please check your Xen cpu configuration.\n", >> + cpu); >> + break; >> + case 0: >> + break; >> + default: >> + WARN(1, "rc %d while trying to pin vcpu\n", ret); >> + disable_pinning = true; >> + } > > These messages are a bit wordy for my taste and since they don't say > what function failed or what driver is affected they're not as useful asDid you notice I used WARN() for the cases where a usage error is to be suspected? This will print a stack backtrace helping to identify the driver. I can work on the message text, of course.> they could be. I'd probably turn these all into: > > if (cpu >= 0 && ret < 0) { > pr_warn("Failed to pin VCPU %d to physical CPU %d: %d", > smp_processor_id(), cpu, ret); > disable_pinning = true; > }No, I don't think this is a good idea. In the EINVAL or EBUSY case a simple Xen admin command might be enough to make the next call succeed. I don't want to disable pinning in this case.> And look at getting the user of this API to print a more useful error. > > "i8k: unable to call SMM BIOS on physical CPU %d: %d"TBH: I think this should be done by another patch. This is something the maintainers of the callers' code should decide. Juergen
David Vrabel
2016-Apr-05 10:03 UTC
[Xen-devel] [PATCH v4 4/6] xen: add xen_pin_vcpu() to support calling functions on a dedicated pcpu
On 05/04/16 11:01, Juergen Gross wrote:> > No, I don't think this is a good idea. In the EINVAL or EBUSY case a > simple Xen admin command might be enough to make the next call succeed. > I don't want to disable pinning in this case.Ok. Acked-by: David Vrabel <david.vrabel at citrix.com> David
Seemingly Similar Threads
- [PATCH v4 4/6] xen: add xen_pin_vcpu() to support calling functions on a dedicated pcpu
- [Xen-devel] [PATCH v4 4/6] xen: add xen_pin_vcpu() to support calling functions on a dedicated pcpu
- [Xen-devel] [PATCH v4 4/6] xen: add xen_pin_vcpu() to support calling functions on a dedicated pcpu
- [PATCH v4 0/6] Support calling functions on dedicated physical cpu
- [PATCH v4 0/6] Support calling functions on dedicated physical cpu