Re: [PATCH v2] staging: unisys: remove duplicate header
On Tue, 2014-12-02 at 16:20 +0530, Sudip Mukherjee wrote:
> these header files were included multiple times
>
> Signed-off-by: Sudip Mukherjee
Looks good to me, thanks for the help. :)
Signed-off-by: Benjamin Romer
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH v2] staging: unisys: remove duplicate header
On Tue, 2014-12-02 at 16:20 +0530, Sudip Mukherjee wrote:
these header files were included multiple times
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
Looks good to me, thanks for the help. :)
Signed-off-by: Benjamin Romer benjamin.ro...@unisys.com
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Tue, 2014-09-09 at 16:11 +0530, Sudip Mukherjee wrote:
> fixed sparse warning : context imbalance in 'resume_device'
> unexpected unlock
> this patch will generate warning from checkpatch for
> lines over 80 character , but since those are user-visible strings
> so it was not modified.
>
> Signed-off-by: Sudip Mukherjee
I tested your changes, and this patch works fine. Thanks!
-- Ben
Acked-by: Benjamin Romer
Tested-by: Benjamin Romer
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Tue, 2014-09-09 at 16:11 +0530, Sudip Mukherjee wrote:
fixed sparse warning : context imbalance in 'resume_device'
unexpected unlock
this patch will generate warning from checkpatch for
lines over 80 character , but since those are user-visible strings
so it was not modified.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
I tested your changes, and this patch works fine. Thanks!
-- Ben
Acked-by: Benjamin Romer benjamin.ro...@unisys.com
Tested-by: Benjamin Romer benjamin.ro...@unisys.com
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Mon, 2014-09-08 at 10:24 -0700, Greg Kroah-Hartman wrote: > If you test it, do a tested-by. > > If you sign off on it (i.e. it flows through you to me), then a > signed-off is correct. > > Hope this helps, > > greg k-h It does. :) We should add the Tested-by then, too. Acked-by: Benjamin Romer Tested-by: Benjamin Romer -- Ben
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Mon, 2014-09-08 at 10:06 -0700, Greg Kroah-Hartman wrote:
> Traditionally, you would respond with a:
> Acked-by: Developer Name
> so I can add it to the patch.
>
> Care to do that here?
>
> thanks,
>
> greg k-h
Of course. :)
Acked-by: Benjamin Romer
Should I do this instead of Tested-by and Signed-off-by, in the future?
-- Ben
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Mon, 2014-09-08 at 22:08 +0530, Sudip Mukherjee wrote: > Hi Ben, > sorry to disturb you again. i got confused , which one is perfect one > combined patch or > separate patches? > > thanks > sudip Two patches, as you preferred. -- Ben
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Mon, 2014-09-08 at 21:57 +0530, Sudip Mukherjee wrote: > Hi Ben, > thanks. the same file is having two more similar warnings. if you want i can > resend a patch fixing all the three warnings , or i can send two separate > patches. > I personally will prefer two separate patches , as that will be > easier for you to test and review. > > thanks > sudip That would be perfect, thanks. :) -- Ben
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Fri, 2014-09-05 at 14:52 +0530, Sudip Mukherjee wrote: > fixed sparse warning : context imbalance in 'pause_device' > unexpected unlock > this patch will generate warning from checkpatch for > lines over 80 character , but since those are user-visible strings > so it was not modified. > > Signed-off-by: Sudip Mukherjee > --- > > hi , can you please review the patch and see if the approach is correct. > The functiion is still doing the same what it was doing , only the logic > is changed. if the approach is ok, then i can send a patch to fix the > other two similar warning in the file. Hi Sudip, I was able to successfully build and test your patch. The changes look good to me too, so I think we should take this patch. :) Thanks! -- Ben Signed-off-by: Benjamin Romer Tested-by: Benjamin Romer
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Fri, 2014-09-05 at 14:52 +0530, Sudip Mukherjee wrote: fixed sparse warning : context imbalance in 'pause_device' unexpected unlock this patch will generate warning from checkpatch for lines over 80 character , but since those are user-visible strings so it was not modified. Signed-off-by: Sudip Mukherjee su...@vectorindia.org --- hi , can you please review the patch and see if the approach is correct. The functiion is still doing the same what it was doing , only the logic is changed. if the approach is ok, then i can send a patch to fix the other two similar warning in the file. Hi Sudip, I was able to successfully build and test your patch. The changes look good to me too, so I think we should take this patch. :) Thanks! -- Ben Signed-off-by: Benjamin Romer benjamin.ro...@unisys.com Tested-by: Benjamin Romer benjamin.ro...@unisys.com
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Mon, 2014-09-08 at 21:57 +0530, Sudip Mukherjee wrote: Hi Ben, thanks. the same file is having two more similar warnings. if you want i can resend a patch fixing all the three warnings , or i can send two separate patches. I personally will prefer two separate patches , as that will be easier for you to test and review. thanks sudip That would be perfect, thanks. :) -- Ben
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Mon, 2014-09-08 at 22:08 +0530, Sudip Mukherjee wrote: Hi Ben, sorry to disturb you again. i got confused , which one is perfect one combined patch or separate patches? thanks sudip Two patches, as you preferred. -- Ben
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Mon, 2014-09-08 at 10:06 -0700, Greg Kroah-Hartman wrote:
Traditionally, you would respond with a:
Acked-by: Developer Name email@address
so I can add it to the patch.
Care to do that here?
thanks,
greg k-h
Of course. :)
Acked-by: Benjamin Romer benjamin.ro...@unisys.com
Should I do this instead of Tested-by and Signed-off-by, in the future?
-- Ben
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Mon, 2014-09-08 at 10:24 -0700, Greg Kroah-Hartman wrote: If you test it, do a tested-by. If you sign off on it (i.e. it flows through you to me), then a signed-off is correct. Hope this helps, greg k-h It does. :) We should add the Tested-by then, too. Acked-by: Benjamin Romer benjamin.ro...@unisys.com Tested-by: Benjamin Romer benjamin.ro...@unisys.com -- Ben
Re: [PATCH 2/7] staging: unisys: move uislib/platform proc entry to debugfs
On Tue, 2014-05-20 at 10:09 +0900, Greg KH wrote: > On Mon, May 19, 2014 at 10:57:08PM +0300, Dan Carpenter wrote: > > On Mon, May 19, 2014 at 09:42:22AM -0500, Romer, Benjamin M wrote: > > > On Sun, 2014-05-18 at 09:49 -0700, Greg KH wrote: > > > > Also, why are these entries moving to debugfs at all? Why are they > > > > needed? Who will use them? Are tools relying on them to be there? > > > > > > The tuning entries are sometimes used to help adjust the behavior of our > > > IO service partitions for better performance. > > > > That sounds like it really belongs in sysfs instead of debugfs. > > Exactly. debugfs files are for "debugging". Consider them files that > your driver can work properly if no one ever touches them. > That is what I was trying to say - these are used when someone is changing the behavior of IO service partitions, not the guest partition where the driver is running. No typical user will need or want to change these settings - only someone working on the IO *server side* performance will need access to them. All of our drivers communicate with another partition to perform IO on shared devices. By turning off features, or changing the rate of messaging in the channels, it makes it easier for someone working on the IO service partition to tune the performance there. The guest itself isn't being tuned. > "tuning" files imply something that has to be touched by users. > Ideally, you would never need such a thing as no one wants to have to > write things to files to make the kernel work better. That is indeed what we want - a user should not need to touch these settings. Someone manipulating these particular settings would be tweaking performance on the server end, like I was trying to say. I believe that most of our proc entries are really debug-time tweaks of that sort, and not something a typical user would ever want to touch. > But if you really > need it, they should be sysfs files, with the needed documentation. > > thanks, > > greg k-h Sorry about the confusion! -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140
Re: [PATCH 2/7] staging: unisys: move uislib/platform proc entry to debugfs
On Tue, 2014-05-20 at 10:09 +0900, Greg KH wrote: On Mon, May 19, 2014 at 10:57:08PM +0300, Dan Carpenter wrote: On Mon, May 19, 2014 at 09:42:22AM -0500, Romer, Benjamin M wrote: On Sun, 2014-05-18 at 09:49 -0700, Greg KH wrote: Also, why are these entries moving to debugfs at all? Why are they needed? Who will use them? Are tools relying on them to be there? The tuning entries are sometimes used to help adjust the behavior of our IO service partitions for better performance. That sounds like it really belongs in sysfs instead of debugfs. Exactly. debugfs files are for debugging. Consider them files that your driver can work properly if no one ever touches them. That is what I was trying to say - these are used when someone is changing the behavior of IO service partitions, not the guest partition where the driver is running. No typical user will need or want to change these settings - only someone working on the IO *server side* performance will need access to them. All of our drivers communicate with another partition to perform IO on shared devices. By turning off features, or changing the rate of messaging in the channels, it makes it easier for someone working on the IO service partition to tune the performance there. The guest itself isn't being tuned. tuning files imply something that has to be touched by users. Ideally, you would never need such a thing as no one wants to have to write things to files to make the kernel work better. That is indeed what we want - a user should not need to touch these settings. Someone manipulating these particular settings would be tweaking performance on the server end, like I was trying to say. I believe that most of our proc entries are really debug-time tweaks of that sort, and not something a typical user would ever want to touch. But if you really need it, they should be sysfs files, with the needed documentation. thanks, greg k-h Sorry about the confusion! -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140
Re: [PATCH 2/7] staging: unisys: move uislib/platform proc entry to debugfs
On Sun, 2014-05-18 at 09:49 -0700, Greg KH wrote:
> There's no need to keep this dentry around, you can just remove all the
> debugfs files in your directory recursively when you exit. That will
> save you code and logic overall.
Ah, thanks! I wasn't aware that I could do that. That's an easy fix. :)
> I'll leave this for now, you can clean that up in a future patch.
Thanks! I'll do that. :)
> Also, why are these entries moving to debugfs at all? Why are they
> needed? Who will use them? Are tools relying on them to be there?
The tuning entries are sometimes used to help adjust the behavior of our
IO service partitions for better performance. I'm not sure about the
platform entry, but I'll find out if anyone still uses it. If not, I'll
send another patch to remove it entirely.
> thanks,
>
> greg k-h
--
Ben Romer | Software Engineer |
Virtual Systems Development
Unisys Corporation | 2476
Swedesford Rd | Malvern, PA 19355
| 610-648-7140
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH 2/7] staging: unisys: move uislib/platform proc entry to debugfs
On Sun, 2014-05-18 at 09:49 -0700, Greg KH wrote:
There's no need to keep this dentry around, you can just remove all the
debugfs files in your directory recursively when you exit. That will
save you code and logic overall.
Ah, thanks! I wasn't aware that I could do that. That's an easy fix. :)
I'll leave this for now, you can clean that up in a future patch.
Thanks! I'll do that. :)
Also, why are these entries moving to debugfs at all? Why are they
needed? Who will use them? Are tools relying on them to be there?
The tuning entries are sometimes used to help adjust the behavior of our
IO service partitions for better performance. I'm not sure about the
platform entry, but I'll find out if anyone still uses it. If not, I'll
send another patch to remove it entirely.
thanks,
greg k-h
--
Ben Romer | Software Engineer |
Virtual Systems Development
Unisys Corporation | 2476
Swedesford Rd | Malvern, PA 19355
| 610-648-7140
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [visorchipset] invalid opcode: 0000 [#1] PREEMPT SMP
On Sat, 2014-04-12 at 01:35 +0800, Jet Chen wrote: > Hi Ben, > > I re-tested this case with/without option -enable-kvm. > > qemu-system-x86_64 -cpu Haswell,+smep,+smap invalid op > qemu-system-x86_64 -cpu kvm64 invalid op > qemu-system-x86_64 -cpu Haswell,+smep,+smap -enable-kvm everything OK > qemu-system-x86_64 -cpu kvm64 -enable-kvm everything OK > > I think this is probably a bug in QEMU. > Sorry for misleading you. I am not experienced in QEMU usage. I don't realize > I need try this case with different options Until read Peter's reply. > > As Peter said, QEMU probably should *not* set the hypervisor bit. But based > on my testing, I think KVM works properly in this case. > > Thanks, > Jet Great, thanks! Sorry for the trouble. :) -- Ben
Re: [visorchipset] invalid opcode: 0000 [#1] PREEMPT SMP
On Fri, 2014-04-11 at 10:40 -0700, H. Peter Anvin wrote: > On 04/11/2014 10:35 AM, Jet Chen wrote: > > > > As Peter said, QEMU probably should *not* set the hypervisor bit. But based > > on my testing, I think KVM works properly in this case. > > > > Either way, unless there is a CPUID interface exposed in CPUID levels > 0x4000+, then relying on the hypervisor bit to do VMCALL is wrong in > the extreme. > > -hpa > > I'll pass your feedback on to the people who wrote the bad code. Sorry for the trouble. :) -- Ben
Re: [visorchipset] invalid opcode: 0000 [#1] PREEMPT SMP
On Thu, 2014-04-10 at 19:28 -0700, H. Peter Anvin wrote: > On 04/10/2014 06:19 AM, Romer, Benjamin M wrote: > > > > I'm confused by the intended behavior of KVM.. Is the intention of the > > -cpu switch to fully emulate a particular CPU? If that's the case, the > > Intel documentation says bit 31 should always be 0, so the value > > returned by the cpuid instruction isn't correct. If the intention is to > > present a VM with a specific CPU architecture, the CPU ought to behave > > as described in Intel's virtualization documentation and just vmexit > > instead of faulting with invalid op, IMHO. > > > > I've already said the check in the code was insufficient, and I'm trying > > to fix that part now. :) > > > > I'm still confused where KVM comes into the picture. Are you actually > using KVM (and thus talking about nested virtualization) or are you > using Qemu in JIT mode and running another hypervisor underneath? The test that Fengguang used to find the problem was running the linux kernel directly using KVM. When the kernel was run with "-cpu Haswell, +smep,+smap" set, the vmcall failed with invalid op, but when the kernel is run with "-cpu qemu64", the vmcall causes a vmexit, as it should. My point is, the vmcall was made because the hypervisor bit was set. If this bit had been turned off, as it would be on a real processor, the vmcall wouldn't have happened. > The hypervisor bit is a complete red herring. If the guest CPU is > running in VT-x mode, then VMCALL should VMEXIT inside the guest > (invoking the guest root VT-x), The CPU is running in VT-X. That was my point, the kernel is running in the KVM guest, and KVM is setting the CPU feature bits such that bit 31 is enabled. I don't think it's a red herring because the kernel uses this bit elsewhere - it is reported as X86_FEATURE_HYPERVISOR in the CPU features, and can be checked with the cpu_has_hypervisor macro (which was not used by the original author of the code in the driver, but should have been). VMWare and KVM support in the kernel also check for this bit before checking their hypervisor leaves for an ID. If it's not properly set it affects more than just the s-Par drivers. > but the fact still remains that you > should never, ever, invoke VMCALL unless you know what hypervisor you > have underneath. From the standpoint of the s-Par drivers, yes, I agree (as I already said). However, VMCALL is not a privileged instruction, so anyone could use it from user space and go right past the OS straight to the hypervisor. IMHO, making it *lethal* to the guest is a bad idea, since any user could hard-stop the guest with a couple of lines of C. -- Ben
Re: [visorchipset] invalid opcode: 0000 [#1] PREEMPT SMP
On Thu, 2014-04-10 at 19:28 -0700, H. Peter Anvin wrote: On 04/10/2014 06:19 AM, Romer, Benjamin M wrote: I'm confused by the intended behavior of KVM.. Is the intention of the -cpu switch to fully emulate a particular CPU? If that's the case, the Intel documentation says bit 31 should always be 0, so the value returned by the cpuid instruction isn't correct. If the intention is to present a VM with a specific CPU architecture, the CPU ought to behave as described in Intel's virtualization documentation and just vmexit instead of faulting with invalid op, IMHO. I've already said the check in the code was insufficient, and I'm trying to fix that part now. :) I'm still confused where KVM comes into the picture. Are you actually using KVM (and thus talking about nested virtualization) or are you using Qemu in JIT mode and running another hypervisor underneath? The test that Fengguang used to find the problem was running the linux kernel directly using KVM. When the kernel was run with -cpu Haswell, +smep,+smap set, the vmcall failed with invalid op, but when the kernel is run with -cpu qemu64, the vmcall causes a vmexit, as it should. My point is, the vmcall was made because the hypervisor bit was set. If this bit had been turned off, as it would be on a real processor, the vmcall wouldn't have happened. The hypervisor bit is a complete red herring. If the guest CPU is running in VT-x mode, then VMCALL should VMEXIT inside the guest (invoking the guest root VT-x), The CPU is running in VT-X. That was my point, the kernel is running in the KVM guest, and KVM is setting the CPU feature bits such that bit 31 is enabled. I don't think it's a red herring because the kernel uses this bit elsewhere - it is reported as X86_FEATURE_HYPERVISOR in the CPU features, and can be checked with the cpu_has_hypervisor macro (which was not used by the original author of the code in the driver, but should have been). VMWare and KVM support in the kernel also check for this bit before checking their hypervisor leaves for an ID. If it's not properly set it affects more than just the s-Par drivers. but the fact still remains that you should never, ever, invoke VMCALL unless you know what hypervisor you have underneath. From the standpoint of the s-Par drivers, yes, I agree (as I already said). However, VMCALL is not a privileged instruction, so anyone could use it from user space and go right past the OS straight to the hypervisor. IMHO, making it *lethal* to the guest is a bad idea, since any user could hard-stop the guest with a couple of lines of C. -- Ben
Re: [visorchipset] invalid opcode: 0000 [#1] PREEMPT SMP
On Fri, 2014-04-11 at 10:40 -0700, H. Peter Anvin wrote: On 04/11/2014 10:35 AM, Jet Chen wrote: As Peter said, QEMU probably should *not* set the hypervisor bit. But based on my testing, I think KVM works properly in this case. Either way, unless there is a CPUID interface exposed in CPUID levels 0x4000+, then relying on the hypervisor bit to do VMCALL is wrong in the extreme. -hpa I'll pass your feedback on to the people who wrote the bad code. Sorry for the trouble. :) -- Ben
Re: [visorchipset] invalid opcode: 0000 [#1] PREEMPT SMP
On Sat, 2014-04-12 at 01:35 +0800, Jet Chen wrote: Hi Ben, I re-tested this case with/without option -enable-kvm. qemu-system-x86_64 -cpu Haswell,+smep,+smap invalid op qemu-system-x86_64 -cpu kvm64 invalid op qemu-system-x86_64 -cpu Haswell,+smep,+smap -enable-kvm everything OK qemu-system-x86_64 -cpu kvm64 -enable-kvm everything OK I think this is probably a bug in QEMU. Sorry for misleading you. I am not experienced in QEMU usage. I don't realize I need try this case with different options Until read Peter's reply. As Peter said, QEMU probably should *not* set the hypervisor bit. But based on my testing, I think KVM works properly in this case. Thanks, Jet Great, thanks! Sorry for the trouble. :) -- Ben
[PATCH] staging: unisys: fix copyright notices
This patch replaces the inconsistent copyright symbols in each source file with (C). Signed-off-by: Benjamin Romer --- drivers/staging/unisys/channels/channel.c | 2 +- drivers/staging/unisys/channels/chanstub.c | 2 +- drivers/staging/unisys/channels/chanstub.h | 2 +- drivers/staging/unisys/common-spar/include/channels/channel.h | 2 +- drivers/staging/unisys/common-spar/include/channels/channel_guid.h | 2 +- drivers/staging/unisys/common-spar/include/channels/controlframework.h | 2 +- drivers/staging/unisys/common-spar/include/channels/controlvmchannel.h | 2 +- drivers/staging/unisys/common-spar/include/channels/diagchannel.h | 2 +- drivers/staging/unisys/common-spar/include/channels/iochannel.h | 2 +- drivers/staging/unisys/common-spar/include/channels/vbuschannel.h | 2 +- drivers/staging/unisys/common-spar/include/controlvmcompletionstatus.h | 2 +- .../staging/unisys/common-spar/include/diagnostics/appos_subsystems.h | 2 +- drivers/staging/unisys/common-spar/include/iovmcall_gnuc.h | 2 +- drivers/staging/unisys/common-spar/include/vbusdeviceinfo.h | 2 +- drivers/staging/unisys/common-spar/include/version.h| 2 +- drivers/staging/unisys/common-spar/include/vmcallinterface.h| 2 +- drivers/staging/unisys/include/commontypes.h| 2 +- drivers/staging/unisys/include/guestlinuxdebug.h| 2 +- drivers/staging/unisys/include/guidutils.h | 2 +- drivers/staging/unisys/include/periodic_work.h | 2 +- drivers/staging/unisys/include/procobjecttree.h | 2 +- drivers/staging/unisys/include/sparstop.h | 2 +- drivers/staging/unisys/include/timskmod.h | 2 +- drivers/staging/unisys/include/timskmodutils.h | 2 +- drivers/staging/unisys/include/uisqueue.h | 2 +- drivers/staging/unisys/include/uisthread.h | 2 +- drivers/staging/unisys/include/uisutils.h | 2 +- drivers/staging/unisys/include/uniklog.h| 2 +- drivers/staging/unisys/uislib/uislib.c | 2 +- drivers/staging/unisys/uislib/uisqueue.c| 2 +- drivers/staging/unisys/uislib/uisthread.c | 2 +- drivers/staging/unisys/uislib/uisutils.c| 2 +- drivers/staging/unisys/virthba/virthba.c| 2 +- drivers/staging/unisys/virthba/virthba.h| 2 +- drivers/staging/unisys/virtpci/virtpci.c| 2 +- drivers/staging/unisys/virtpci/virtpci.h| 2 +- drivers/staging/unisys/visorchannel/globals.h | 2 +- drivers/staging/unisys/visorchannel/visorchannel.h | 2 +- drivers/staging/unisys/visorchannel/visorchannel_funcs.c| 2 +- drivers/staging/unisys/visorchannel/visorchannel_main.c | 2 +- drivers/staging/unisys/visorchipset/controlvm.h | 2 +- drivers/staging/unisys/visorchipset/controlvm_direct.c | 2 +- drivers/staging/unisys/visorchipset/file.c | 2 +- drivers/staging/unisys/visorchipset/file.h | 2 +- drivers/staging/unisys/visorchipset/globals.h | 2 +- drivers/staging/unisys/visorchipset/parser.c| 2 +- drivers/staging/unisys/visorchipset/parser.h| 2 +- drivers/staging/unisys/visorchipset/testing.h | 2 +- drivers/staging/unisys/visorchipset/visorchipset.h | 2 +- drivers/staging/unisys/visorchipset/visorchipset_main.c | 2 +- drivers/staging/unisys/visorchipset/visorchipset_umode.h| 2 +- drivers/staging/unisys/visorutil/charqueue.c| 2 +- drivers/staging/unisys/visorutil/charqueue.h| 2 +- drivers/staging/unisys/visorutil/easyproc.c | 2 +- drivers/staging/unisys/visorutil/easyproc.h | 2 +- drivers/staging/unisys/visorutil/memregion.h| 2 +- drivers/staging/unisys/visorutil/memregion_direct.c | 2 +- drivers/staging/unisys/visorutil/periodic_work.c| 2 +- drivers/staging/unisys/visorutil/procobjecttree.c | 2 +- drivers/staging/unisys/visorutil/visorkmodutils.c | 2 +- 60 files changed, 60 insertions(+), 60 deletions(-) diff --git
Re: [PATCH] unisys: staging: Check for s-Par firmware before initializing s-Par modules
On Wed, 2014-04-09 at 12:25 -0700, Greg Kroah-Hartman wrote: > Patches need to do only one thing, so can you please split this up in to > multiple patches, each one only doing one thing. Sorry about that! I'll send another patch that fixes all of the copyright statements at once then. :) > You forgot to add a "Reported-by:" line here, and possibly a > "Tested-by:" if someone tested it and reported that it solved the > problem. Proper attribution is very important. Will do! Thanks for all the feedback. I'll rework my patch. :) -- Ben
Re: [visorchipset] invalid opcode: 0000 [#1] PREEMPT SMP
On Wed, 2014-04-09 at 16:10 -0700, H. Peter Anvin wrote: > On 04/09/2014 04:01 PM, Fengguang Wu wrote: > > CC the KVM people: it looks like a KVM problem that can be triggered by > > > > qemu-system-x86_64 -cpu Haswell,+smep,+smap > > I'm really confused. First of all, is this a KVM problem or is it a > Qemu JIT problem? > > Either seems really wonky. It is questionable at best whether or not > Qemu in JIT mode should set the hypervisor bit IMO. However, even so, > you *better* not call VMCALL *just* because the hypervisor bit is set. > > The reason for it is that you have absolutely no idea what VMCALL is > going to do on any one hypervisor... different hypervisors even use > completely different conventions for VMCALL, and some might not accept > VMCALL at all and might just terminate your guest with extreme prejudice. > > So what is actually going on here? > > -hpa > I'm confused by the intended behavior of KVM.. Is the intention of the -cpu switch to fully emulate a particular CPU? If that's the case, the Intel documentation says bit 31 should always be 0, so the value returned by the cpuid instruction isn't correct. If the intention is to present a VM with a specific CPU architecture, the CPU ought to behave as described in Intel's virtualization documentation and just vmexit instead of faulting with invalid op, IMHO. I've already said the check in the code was insufficient, and I'm trying to fix that part now. :)
Re: [visorchipset] invalid opcode: 0000 [#1] PREEMPT SMP
On Wed, 2014-04-09 at 16:10 -0700, H. Peter Anvin wrote: On 04/09/2014 04:01 PM, Fengguang Wu wrote: CC the KVM people: it looks like a KVM problem that can be triggered by qemu-system-x86_64 -cpu Haswell,+smep,+smap I'm really confused. First of all, is this a KVM problem or is it a Qemu JIT problem? Either seems really wonky. It is questionable at best whether or not Qemu in JIT mode should set the hypervisor bit IMO. However, even so, you *better* not call VMCALL *just* because the hypervisor bit is set. The reason for it is that you have absolutely no idea what VMCALL is going to do on any one hypervisor... different hypervisors even use completely different conventions for VMCALL, and some might not accept VMCALL at all and might just terminate your guest with extreme prejudice. So what is actually going on here? -hpa I'm confused by the intended behavior of KVM.. Is the intention of the -cpu switch to fully emulate a particular CPU? If that's the case, the Intel documentation says bit 31 should always be 0, so the value returned by the cpuid instruction isn't correct. If the intention is to present a VM with a specific CPU architecture, the CPU ought to behave as described in Intel's virtualization documentation and just vmexit instead of faulting with invalid op, IMHO. I've already said the check in the code was insufficient, and I'm trying to fix that part now. :)
Re: [PATCH] unisys: staging: Check for s-Par firmware before initializing s-Par modules
On Wed, 2014-04-09 at 12:25 -0700, Greg Kroah-Hartman wrote: Patches need to do only one thing, so can you please split this up in to multiple patches, each one only doing one thing. Sorry about that! I'll send another patch that fixes all of the copyright statements at once then. :) You forgot to add a Reported-by: line here, and possibly a Tested-by: if someone tested it and reported that it solved the problem. Proper attribution is very important. Will do! Thanks for all the feedback. I'll rework my patch. :) -- Ben
[PATCH] staging: unisys: fix copyright notices
This patch replaces the inconsistent copyright symbols in each source file with (C). Signed-off-by: Benjamin Romer benjamin.ro...@unisys.com --- drivers/staging/unisys/channels/channel.c | 2 +- drivers/staging/unisys/channels/chanstub.c | 2 +- drivers/staging/unisys/channels/chanstub.h | 2 +- drivers/staging/unisys/common-spar/include/channels/channel.h | 2 +- drivers/staging/unisys/common-spar/include/channels/channel_guid.h | 2 +- drivers/staging/unisys/common-spar/include/channels/controlframework.h | 2 +- drivers/staging/unisys/common-spar/include/channels/controlvmchannel.h | 2 +- drivers/staging/unisys/common-spar/include/channels/diagchannel.h | 2 +- drivers/staging/unisys/common-spar/include/channels/iochannel.h | 2 +- drivers/staging/unisys/common-spar/include/channels/vbuschannel.h | 2 +- drivers/staging/unisys/common-spar/include/controlvmcompletionstatus.h | 2 +- .../staging/unisys/common-spar/include/diagnostics/appos_subsystems.h | 2 +- drivers/staging/unisys/common-spar/include/iovmcall_gnuc.h | 2 +- drivers/staging/unisys/common-spar/include/vbusdeviceinfo.h | 2 +- drivers/staging/unisys/common-spar/include/version.h| 2 +- drivers/staging/unisys/common-spar/include/vmcallinterface.h| 2 +- drivers/staging/unisys/include/commontypes.h| 2 +- drivers/staging/unisys/include/guestlinuxdebug.h| 2 +- drivers/staging/unisys/include/guidutils.h | 2 +- drivers/staging/unisys/include/periodic_work.h | 2 +- drivers/staging/unisys/include/procobjecttree.h | 2 +- drivers/staging/unisys/include/sparstop.h | 2 +- drivers/staging/unisys/include/timskmod.h | 2 +- drivers/staging/unisys/include/timskmodutils.h | 2 +- drivers/staging/unisys/include/uisqueue.h | 2 +- drivers/staging/unisys/include/uisthread.h | 2 +- drivers/staging/unisys/include/uisutils.h | 2 +- drivers/staging/unisys/include/uniklog.h| 2 +- drivers/staging/unisys/uislib/uislib.c | 2 +- drivers/staging/unisys/uislib/uisqueue.c| 2 +- drivers/staging/unisys/uislib/uisthread.c | 2 +- drivers/staging/unisys/uislib/uisutils.c| 2 +- drivers/staging/unisys/virthba/virthba.c| 2 +- drivers/staging/unisys/virthba/virthba.h| 2 +- drivers/staging/unisys/virtpci/virtpci.c| 2 +- drivers/staging/unisys/virtpci/virtpci.h| 2 +- drivers/staging/unisys/visorchannel/globals.h | 2 +- drivers/staging/unisys/visorchannel/visorchannel.h | 2 +- drivers/staging/unisys/visorchannel/visorchannel_funcs.c| 2 +- drivers/staging/unisys/visorchannel/visorchannel_main.c | 2 +- drivers/staging/unisys/visorchipset/controlvm.h | 2 +- drivers/staging/unisys/visorchipset/controlvm_direct.c | 2 +- drivers/staging/unisys/visorchipset/file.c | 2 +- drivers/staging/unisys/visorchipset/file.h | 2 +- drivers/staging/unisys/visorchipset/globals.h | 2 +- drivers/staging/unisys/visorchipset/parser.c| 2 +- drivers/staging/unisys/visorchipset/parser.h| 2 +- drivers/staging/unisys/visorchipset/testing.h | 2 +- drivers/staging/unisys/visorchipset/visorchipset.h | 2 +- drivers/staging/unisys/visorchipset/visorchipset_main.c | 2 +- drivers/staging/unisys/visorchipset/visorchipset_umode.h| 2 +- drivers/staging/unisys/visorutil/charqueue.c| 2 +- drivers/staging/unisys/visorutil/charqueue.h| 2 +- drivers/staging/unisys/visorutil/easyproc.c | 2 +- drivers/staging/unisys/visorutil/easyproc.h | 2 +- drivers/staging/unisys/visorutil/memregion.h| 2 +- drivers/staging/unisys/visorutil/memregion_direct.c | 2 +- drivers/staging/unisys/visorutil/periodic_work.c| 2 +- drivers/staging/unisys/visorutil/procobjecttree.c | 2 +- drivers/staging/unisys/visorutil/visorkmodutils.c | 2 +- 60 files changed, 60 insertions(+), 60 deletions(-)
[PATCH] unisys: staging: Check for s-Par firmware before initializing s-Par modules
This patch adds a function, is_spar_system(), to check that s-Par
firmware is present, and then uses this function at the beginning of
each module to verify that the modules are being run on an s-Par system
before beginning initialization. If the firmware is not detected, the
module will return a failure code.
Checking for s-Par firmware uses the cpuid instruction to verify that
the processor is running with virtualization turned on, and then uses a
second cpuid instruction to check that the hypervisor ID is
"UnisysSpar64".
Additionally, some minor clean-up was done on copyright tags and
unnecessary messages were removed from the visorchipset_main() function.
This patch was tested with KVM to ensure that the modules do not load
when s-Par firmware is not present, and tested with s-Par 4.0.12 to
verify that the modules load correctly when the firmware is present.
Signed-off-by: Benjamin Romer
---
diff --git a/drivers/staging/unisys/include/timskmodutils.h
b/drivers/staging/unisys/include/timskmodutils.h
index 2d81d46..cc439d3 100644
--- a/drivers/staging/unisys/include/timskmodutils.h
+++ b/drivers/staging/unisys/include/timskmodutils.h
@@ -1,6 +1,6 @@
/* timskmodutils.h
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -71,5 +71,6 @@
u64 somethings, char *buf, size_t bufsize);
struct seq_file *visor_seq_file_new_buffer(void *buf, size_t buf_size);
void visor_seq_file_done_buffer(struct seq_file *m);
+int is_spar_system( void );
#endif
diff --git a/drivers/staging/unisys/uislib/uislib.c
b/drivers/staging/unisys/uislib/uislib.c
index 8ea9c46..aa60ccb 100644
--- a/drivers/staging/unisys/uislib/uislib.c
+++ b/drivers/staging/unisys/uislib/uislib.c
@@ -1,6 +1,6 @@
/* uislib.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -2276,6 +2276,11 @@
static int __init
uislib_mod_init(void)
{
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( "s-Par not detected.\n" );
+ return -EPERM;
+ }
LOGINF("MONITORAPIS");
diff --git a/drivers/staging/unisys/virthba/virthba.c
b/drivers/staging/unisys/virthba/virthba.c
index 817b11d..862509d 100644
--- a/drivers/staging/unisys/virthba/virthba.c
+++ b/drivers/staging/unisys/virthba/virthba.c
@@ -1,6 +1,6 @@
/* virthba.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -1691,6 +1691,12 @@
int error;
int i;
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( "s-Par not detected.\n" );
+ return -EPERM;
+ }
+
LOGINF("Entering virthba_mod_init...\n");
POSTCODE_LINUX_2(VHBA_CREATE_ENTRY_PC, POSTCODE_SEVERITY_INFO);
diff --git a/drivers/staging/unisys/virtpci/virtpci.c
b/drivers/staging/unisys/virtpci/virtpci.c
index 8e34650..0d06f7e 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -1,6 +1,6 @@
/* virtpci.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -21,6 +21,7 @@
#ifdef CONFIG_MODVERSIONS
#include
#endif
+#include "timskmodutils.h"
#include "uniklog.h"
#include "diagnostics/appos_subsystems.h"
#include "uisutils.h"
@@ -1686,6 +1687,11 @@
{
int ret;
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( "s-Par not detected.\n" );
+ return -EPERM;
+ }
LOGINF("Module build: Date:%s Time:%s...\n", __DATE__, __TIME__);
diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c
b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index 8252ca1..aa35aa2 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -1,6 +1,6 @@
/* visorchipset_main.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -2681,18 +2681,13 @@
struct proc_dir_entry *toolaction_file;
struct proc_dir_entry *bootToTool_file;
- LOGINF("chipset driver version %s loaded", VERSION);
- /* process module options */
- POSTCODE_LINUX_2(DRIVER_ENTRY_PC, POSTCODE_SEVERITY_INFO);
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk(
[PATCH] unisys: staging: Check for s-Par firmware before initializing s-Par modules
This patch adds a function, is_spar_system(), to check that s-Par
firmware is present, and then uses this function at the beginning of
each module to verify that the modules are being run on an s-Par system
before beginning initialization. If the firmware is not detected, the
module will return a failure code.
Checking for s-Par firmware uses the cpuid instruction to verify that
the processor is running with virtualization turned on, and then uses a
second cpuid instruction to check that the hypervisor ID is
UnisysSpar64.
Additionally, some minor clean-up was done on copyright tags and
unnecessary messages were removed from the visorchipset_main() function.
This patch was tested with KVM to ensure that the modules do not load
when s-Par firmware is not present, and tested with s-Par 4.0.12 to
verify that the modules load correctly when the firmware is present.
Signed-off-by: Benjamin Romer benjamin.ro...@unisys.com
---
diff --git a/drivers/staging/unisys/include/timskmodutils.h
b/drivers/staging/unisys/include/timskmodutils.h
index 2d81d46..cc439d3 100644
--- a/drivers/staging/unisys/include/timskmodutils.h
+++ b/drivers/staging/unisys/include/timskmodutils.h
@@ -1,6 +1,6 @@
/* timskmodutils.h
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -71,5 +71,6 @@
u64 somethings, char *buf, size_t bufsize);
struct seq_file *visor_seq_file_new_buffer(void *buf, size_t buf_size);
void visor_seq_file_done_buffer(struct seq_file *m);
+int is_spar_system( void );
#endif
diff --git a/drivers/staging/unisys/uislib/uislib.c
b/drivers/staging/unisys/uislib/uislib.c
index 8ea9c46..aa60ccb 100644
--- a/drivers/staging/unisys/uislib/uislib.c
+++ b/drivers/staging/unisys/uislib/uislib.c
@@ -1,6 +1,6 @@
/* uislib.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -2276,6 +2276,11 @@
static int __init
uislib_mod_init(void)
{
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( s-Par not detected.\n );
+ return -EPERM;
+ }
LOGINF(MONITORAPIS);
diff --git a/drivers/staging/unisys/virthba/virthba.c
b/drivers/staging/unisys/virthba/virthba.c
index 817b11d..862509d 100644
--- a/drivers/staging/unisys/virthba/virthba.c
+++ b/drivers/staging/unisys/virthba/virthba.c
@@ -1,6 +1,6 @@
/* virthba.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -1691,6 +1691,12 @@
int error;
int i;
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( s-Par not detected.\n );
+ return -EPERM;
+ }
+
LOGINF(Entering virthba_mod_init...\n);
POSTCODE_LINUX_2(VHBA_CREATE_ENTRY_PC, POSTCODE_SEVERITY_INFO);
diff --git a/drivers/staging/unisys/virtpci/virtpci.c
b/drivers/staging/unisys/virtpci/virtpci.c
index 8e34650..0d06f7e 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -1,6 +1,6 @@
/* virtpci.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -21,6 +21,7 @@
#ifdef CONFIG_MODVERSIONS
#include config/modversions.h
#endif
+#include timskmodutils.h
#include uniklog.h
#include diagnostics/appos_subsystems.h
#include uisutils.h
@@ -1686,6 +1687,11 @@
{
int ret;
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( s-Par not detected.\n );
+ return -EPERM;
+ }
LOGINF(Module build: Date:%s Time:%s...\n, __DATE__, __TIME__);
diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c
b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index 8252ca1..aa35aa2 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -1,6 +1,6 @@
/* visorchipset_main.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -2681,18 +2681,13 @@
struct proc_dir_entry *toolaction_file;
struct proc_dir_entry *bootToTool_file;
- LOGINF(chipset driver version %s loaded, VERSION);
- /* process module options */
- POSTCODE_LINUX_2(DRIVER_ENTRY_PC, POSTCODE_SEVERITY_INFO);
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+
Re: [visorchipset] invalid opcode: 0000 [#1] PREEMPT SMP
On Tue, 2014-04-08 at 10:53 +0800, Fengguang Wu wrote: > Hi Benjamin, > > > Fengguang, > > > > I ran your script against freshly-checked-out source from staging-next, and > > was not able to reproduce the error with it. My boot log is attached. I > > noticed that your log did not have "Hypervisor detected: KVM" in the trace. > > The KVM options in your script also differ substantially from the ones > > shown at the end of your trace... > > > When I reran your script with the "-cpu Haswell,+smep,+smap" option I was > > able to get the same result as you. IMHO KVM should not be setting this bit > > if it's emulating bare metal. > > Sorry.. We tried to provide a simplified reproduce script and in your > case, it has a significant mismatch with the real KVM options. We'll > fix it, thanks for pointing it out! > > Thanks, > Fengguang That will be helpful, and as I mentioned, I can reproduce your results, but I'm still not sure why a virtualized processor is giving an invalid opcode fault on a vmcall. The Intel documentation is pretty specific about this - IF not in VMX operation THEN #UD; ELSIF in VMX non-root operation THEN VM exit. Either KVM should be saying "I'm a real processor and not a virtual CPU, really!" - in which case, the hypervisor bit should be off and vmcalls should cause an invalid opcode fault, or, KVM should be saying "I'm a vritualized processor!" and setting the hypervisor bit, and doing a vmexit on vmcall instead. This seems like a KVM bug to me. -- Ben
Re: [visorchipset] invalid opcode: 0000 [#1] PREEMPT SMP
On Tue, 2014-04-08 at 10:53 +0800, Fengguang Wu wrote: Hi Benjamin, Fengguang, I ran your script against freshly-checked-out source from staging-next, and was not able to reproduce the error with it. My boot log is attached. I noticed that your log did not have Hypervisor detected: KVM in the trace. The KVM options in your script also differ substantially from the ones shown at the end of your trace... When I reran your script with the -cpu Haswell,+smep,+smap option I was able to get the same result as you. IMHO KVM should not be setting this bit if it's emulating bare metal. Sorry.. We tried to provide a simplified reproduce script and in your case, it has a significant mismatch with the real KVM options. We'll fix it, thanks for pointing it out! Thanks, Fengguang That will be helpful, and as I mentioned, I can reproduce your results, but I'm still not sure why a virtualized processor is giving an invalid opcode fault on a vmcall. The Intel documentation is pretty specific about this - IF not in VMX operation THEN #UD; ELSIF in VMX non-root operation THEN VM exit. Either KVM should be saying I'm a real processor and not a virtual CPU, really! - in which case, the hypervisor bit should be off and vmcalls should cause an invalid opcode fault, or, KVM should be saying I'm a vritualized processor! and setting the hypervisor bit, and doing a vmexit on vmcall instead. This seems like a KVM bug to me. -- Ben
