Patchwork Not emulated registers on BOOKE_HV (GS-mode)

login
register
mail settings
Submitter Bharat Bhushan
Date May 16, 2012, 7:28 a.m.
Message ID <6A3DF150A5B70D4F9B66A25E3F7C888D03D13A1F@039-SN2MPN1-021.039d.mgd.msft.net>
Download mbox | patch
Permalink /patch/159542/
State New
Headers show

Comments

Bharat Bhushan - May 16, 2012, 7:28 a.m.
Hi Alex,

There is below comment in arch/powerpc/kvm/booke_emulate.c

/*
 * NOTE: some of these registers are not emulated on BOOKE_HV (GS-mode).
 * Their backing store is in real registers, and these functions
 * will return the wrong result if called for them in another context
 * (such as debugging).
 */

"some of these registers are not emulated on BOOKE_HV (GS-mode)" 
 1) Is not that mtspr()/mfspr() for "not emulated" registers should follow EMULATE_FAIL path? So should be ifdef out for BOOKE_HV? Otherwise the emulation code execute.
 2) Or These are not emulated because the GS mode have direct access to these registers, Right? So no trap?


"and these functions will return the wrong result if called for them in another context (such as debugging)."
 1) So do you mean that guest is not supposed to access these registers in normal scenario but the debugger (some command on gdb in guest) can access these register? then does it make sense to treat mtspr() as nop and mfspr returns 0/undefined?

In our local repository Scott Wood removed this comment by ifdef out those registers for BOOKE_HV.
Below is the change (extracted - not the exact patch which does this)
Sethi Varun-B16395 - May 16, 2012, 9:02 a.m.
> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-
> owner@vger.kernel.org] On Behalf Of Bhushan Bharat-R65777
> Sent: Wednesday, May 16, 2012 12:58 PM
> To: Alexander Graf
> Cc: Wood Scott-B07421; Yoder Stuart-B08248; kvm-ppc@vger.kernel.org
> Subject: Not emulated registers on BOOKE_HV (GS-mode)
> 
> Hi Alex,
> 
> There is below comment in arch/powerpc/kvm/booke_emulate.c
> 
> /*
>  * NOTE: some of these registers are not emulated on BOOKE_HV (GS-mode).
>  * Their backing store is in real registers, and these functions
>  * will return the wrong result if called for them in another context
>  * (such as debugging).
>  */
> 
> "some of these registers are not emulated on BOOKE_HV (GS-mode)"
>  1) Is not that mtspr()/mfspr() for "not emulated" registers should
> follow EMULATE_FAIL path? So should be ifdef out for BOOKE_HV? Otherwise
> the emulation code execute.
>  2) Or These are not emulated because the GS mode have direct access to
> these registers, Right? So no trap?
>
For BOOKE_HV mtspr/mfspr would get mapped to hardware maintained guest shadow copies.
For example guest access to dear would get mapped to gdear and wouldn't trap
 
> 
> "and these functions will return the wrong result if called for them in
> another context (such as debugging)."
>  1) So do you mean that guest is not supposed to access these registers
> in normal scenario but the debugger (some command on gdb in guest) can
> access these register? then does it make sense to treat mtspr() as nop
> and mfspr returns 0/undefined?
> 
Actually, in a normal scenario (For BOOKE_HV) guest (MSR[GS] = 1) would be able to directly access these registers
without hypervisor intervention. However I am not sure under what condition would the guest access generate a trap, all
guest accesses would be with MSR[GS]=1.

-Varun

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - May 16, 2012, 9:13 a.m.
On 05/16/2012 11:02 AM, Sethi Varun-B16395 wrote:
>
>> -----Original Message-----
>> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-
>> owner@vger.kernel.org] On Behalf Of Bhushan Bharat-R65777
>> Sent: Wednesday, May 16, 2012 12:58 PM
>> To: Alexander Graf
>> Cc: Wood Scott-B07421; Yoder Stuart-B08248; kvm-ppc@vger.kernel.org
>> Subject: Not emulated registers on BOOKE_HV (GS-mode)
>>
>> Hi Alex,
>>
>> There is below comment in arch/powerpc/kvm/booke_emulate.c
>>
>> /*
>>   * NOTE: some of these registers are not emulated on BOOKE_HV (GS-mode).
>>   * Their backing store is in real registers, and these functions
>>   * will return the wrong result if called for them in another context
>>   * (such as debugging).
>>   */
>>
>> "some of these registers are not emulated on BOOKE_HV (GS-mode)"
>>   1) Is not that mtspr()/mfspr() for "not emulated" registers should
>> follow EMULATE_FAIL path? So should be ifdef out for BOOKE_HV? Otherwise
>> the emulation code execute.
>>   2) Or These are not emulated because the GS mode have direct access to
>> these registers, Right? So no trap?
>>
> For BOOKE_HV mtspr/mfspr would get mapped to hardware maintained guest shadow copies.
> For example guest access to dear would get mapped to gdear and wouldn't trap
>
>> "and these functions will return the wrong result if called for them in
>> another context (such as debugging)."
>>   1) So do you mean that guest is not supposed to access these registers
>> in normal scenario but the debugger (some command on gdb in guest) can
>> access these register? then does it make sense to treat mtspr() as nop
>> and mfspr returns 0/undefined?
>>
> Actually, in a normal scenario (For BOOKE_HV) guest (MSR[GS] = 1) would be able to directly access these registers
> without hypervisor intervention. However I am not sure under what condition would the guest access generate a trap, all
> guest accesses would be with MSR[GS]=1.

Yup. E500mc would just never reach that code - and even if it did, it 
simply wouldn't do anything useful.

What did you guys need this change for? I'm sure there's some incentive 
behind it, right? The only one I can see right now would be reduced code 
size.


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - May 16, 2012, 10:29 p.m.
On 05/16/2012 04:13 AM, Alexander Graf wrote:
> On 05/16/2012 11:02 AM, Sethi Varun-B16395 wrote:
>>
>>> -----Original Message-----
>>> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-
>>> owner@vger.kernel.org] On Behalf Of Bhushan Bharat-R65777
>>> Sent: Wednesday, May 16, 2012 12:58 PM
>>> To: Alexander Graf
>>> Cc: Wood Scott-B07421; Yoder Stuart-B08248; kvm-ppc@vger.kernel.org
>>> Subject: Not emulated registers on BOOKE_HV (GS-mode)
>>>
>>> Hi Alex,
>>>
>>> There is below comment in arch/powerpc/kvm/booke_emulate.c
>>>
>>> /*
>>>   * NOTE: some of these registers are not emulated on BOOKE_HV
>>> (GS-mode).
>>>   * Their backing store is in real registers, and these functions
>>>   * will return the wrong result if called for them in another context
>>>   * (such as debugging).
>>>   */
>>>
>>> "some of these registers are not emulated on BOOKE_HV (GS-mode)"
>>>   1) Is not that mtspr()/mfspr() for "not emulated" registers should
>>> follow EMULATE_FAIL path? So should be ifdef out for BOOKE_HV? Otherwise
>>> the emulation code execute.
>>>   2) Or These are not emulated because the GS mode have direct access to
>>> these registers, Right? So no trap?
>>>
>> For BOOKE_HV mtspr/mfspr would get mapped to hardware maintained guest
>> shadow copies.
>> For example guest access to dear would get mapped to gdear and
>> wouldn't trap
>>
>>> "and these functions will return the wrong result if called for them in
>>> another context (such as debugging)."
>>>   1) So do you mean that guest is not supposed to access these registers
>>> in normal scenario but the debugger (some command on gdb in guest) can
>>> access these register? then does it make sense to treat mtspr() as nop
>>> and mfspr returns 0/undefined?
>>>
>> Actually, in a normal scenario (For BOOKE_HV) guest (MSR[GS] = 1)
>> would be able to directly access these registers
>> without hypervisor intervention. However I am not sure under what
>> condition would the guest access generate a trap, all
>> guest accesses would be with MSR[GS]=1.
> 
> Yup. E500mc would just never reach that code - and even if it did, it
> simply wouldn't do anything useful.
> 
> What did you guys need this change for? I'm sure there's some incentive
> behind it, right? The only one I can see right now would be reduced code
> size.

This was something Stuart pulled out of my local working tree from a
commit where the entire commit message was "crit ints and misc emulation
stuff to be sorted through". :-P

Looking through previous versions of the history, the ifdefs were how
Varun originally added critical interrupt support.  I probably changed
it to the "NOTE:" version without ifdefs, and the "misc emulation stuff"
patch was just waiting for me to integrate that change properly with
things like crit ints that weren't part of the e500mc patchset I posted.

That said, if this code ever does get used for debug or similar
non-emulation access, it would be nicer to fail than to return garbage
-- but what isn't nice is increasing the points where this stuff is
decided at compile time.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - May 16, 2012, 10:44 p.m.
On 17.05.2012, at 00:29, Scott Wood <scottwood@freescale.com> wrote:

> On 05/16/2012 04:13 AM, Alexander Graf wrote:
>> On 05/16/2012 11:02 AM, Sethi Varun-B16395 wrote:
>>> 
>>>> -----Original Message-----
>>>> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-
>>>> owner@vger.kernel.org] On Behalf Of Bhushan Bharat-R65777
>>>> Sent: Wednesday, May 16, 2012 12:58 PM
>>>> To: Alexander Graf
>>>> Cc: Wood Scott-B07421; Yoder Stuart-B08248; kvm-ppc@vger.kernel.org
>>>> Subject: Not emulated registers on BOOKE_HV (GS-mode)
>>>> 
>>>> Hi Alex,
>>>> 
>>>> There is below comment in arch/powerpc/kvm/booke_emulate.c
>>>> 
>>>> /*
>>>>  * NOTE: some of these registers are not emulated on BOOKE_HV
>>>> (GS-mode).
>>>>  * Their backing store is in real registers, and these functions
>>>>  * will return the wrong result if called for them in another context
>>>>  * (such as debugging).
>>>>  */
>>>> 
>>>> "some of these registers are not emulated on BOOKE_HV (GS-mode)"
>>>>  1) Is not that mtspr()/mfspr() for "not emulated" registers should
>>>> follow EMULATE_FAIL path? So should be ifdef out for BOOKE_HV? Otherwise
>>>> the emulation code execute.
>>>>  2) Or These are not emulated because the GS mode have direct access to
>>>> these registers, Right? So no trap?
>>>> 
>>> For BOOKE_HV mtspr/mfspr would get mapped to hardware maintained guest
>>> shadow copies.
>>> For example guest access to dear would get mapped to gdear and
>>> wouldn't trap
>>> 
>>>> "and these functions will return the wrong result if called for them in
>>>> another context (such as debugging)."
>>>>  1) So do you mean that guest is not supposed to access these registers
>>>> in normal scenario but the debugger (some command on gdb in guest) can
>>>> access these register? then does it make sense to treat mtspr() as nop
>>>> and mfspr returns 0/undefined?
>>>> 
>>> Actually, in a normal scenario (For BOOKE_HV) guest (MSR[GS] = 1)
>>> would be able to directly access these registers
>>> without hypervisor intervention. However I am not sure under what
>>> condition would the guest access generate a trap, all
>>> guest accesses would be with MSR[GS]=1.
>> 
>> Yup. E500mc would just never reach that code - and even if it did, it
>> simply wouldn't do anything useful.
>> 
>> What did you guys need this change for? I'm sure there's some incentive
>> behind it, right? The only one I can see right now would be reduced code
>> size.
> 
> This was something Stuart pulled out of my local working tree from a
> commit where the entire commit message was "crit ints and misc emulation
> stuff to be sorted through". :-P
> 
> Looking through previous versions of the history, the ifdefs were how
> Varun originally added critical interrupt support.  I probably changed
> it to the "NOTE:" version without ifdefs, and the "misc emulation stuff"
> patch was just waiting for me to integrate that change properly with
> things like crit ints that weren't part of the e500mc patchset I posted.
> 
> That said, if this code ever does get used for debug or similar
> non-emulation access, it would be nicer to fail than to return garbage
> -- but what isn't nice is increasing the points where this stuff is
> decided at compile time.

Yeah. I have a brach on my tree caed emul-rework. If you like, feel free to check it out. It's what I envisioned a cleaner API to the whole instruction emulation.

Unfortunately, it turned out to be significantly slower (measurable) with more memory overhead (allocated and text) than what we have today. So I figured I won't go with it for now...

Long-term, we should try to come up with better solutions to the whole thing though. I just haven't found the ideal one yet ;).

Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c
index 83c3796..6d78906 100644
--- a/arch/powerpc/kvm/booke_emulate.c
+++ b/arch/powerpc/kvm/booke_emulate.c
@@ -46,18 +46,21 @@  int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
        switch (get_op(inst)) {
        case 19:
                switch (get_xop(inst)) {
+#ifndef CONFIG_KVM_BOOKE_HV
                case OP_19_XOP_RFI:
                        kvmppc_emul_rfi(vcpu);
                        kvmppc_set_exit_type(vcpu, EMULATED_RFI_EXITS);
                        *advance = 0;
                        break;

+#endif
                default:
                        emulated = EMULATE_FAIL;
                        break;
                }
                break;

+#ifndef CONFIG_KVM_BOOKE_HV
        case 31:
                switch (get_xop(inst)) {

@@ -89,6 +92,7 @@  int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,

                break;

+#endif
        default:
                emulated = EMULATE_FAIL;
        }
@@ -96,23 +100,19 @@  int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
        return emulated;
 }

-/*
- * NOTE: some of these registers are not emulated on BOOKE_HV (GS-mode).
- * Their backing store is in real registers, and these functions
- * will return the wrong result if called for them in another context
- * (such as debugging).
- */
 int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
 {
        int emulated = EMULATE_DONE;

        switch (sprn) {
+#ifndef CONFIG_KVM_BOOKE_HV
        case SPRN_DEAR:
                vcpu->arch.shared->dar = spr_val;
                break;
        case SPRN_ESR:
                vcpu->arch.shared->esr = spr_val;
                break;
+#endif
        case SPRN_DBCR0:
                vcpu->arch.dbcr0 = spr_val;
                break;
@@ -223,6 +223,7 @@  int kvmppc_booke_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val)
        int emulated = EMULATE_DONE;

        switch (sprn) {
+#ifndef CONFIG_KVM_BOOKE_HV
        case SPRN_IVPR:
                *spr_val = vcpu->arch.ivpr;
                break;
@@ -232,6 +233,7 @@  int kvmppc_booke_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val)
        case SPRN_ESR:
                *spr_val = vcpu->arch.shared->esr;
                break;
+#endif
        case SPRN_DBCR0:
                *spr_val = vcpu->arch.dbcr0;
                break;