All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
To: Alexander Graf <agraf@suse.de>
Cc: Paul Mackerras <paulus@samba.org>,
	"kvm-ppc@vger.kernel.org" <kvm-ppc@vger.kernel.org>,
	"kvm@vger.kernel.org list" <kvm@vger.kernel.org>
Subject: Re: [PATCH] target-ppc: Update slb array with correct index values.
Date: Thu, 22 Aug 2013 13:32:00 +0000	[thread overview]
Message-ID: <87k3jd277j.fsf@linux.vnet.ibm.com> (raw)
In-Reply-To: <14BF15D9-CF86-43F8-9327-7B2BB42FFEFF@suse.de>

Alexander Graf <agraf@suse.de> writes:

> Am 21.08.2013 um 16:59 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 
>> 
>> ....
>> 
>>>> 
>>>> On HV KVM yes, that would be the end of the list, but PR KVM could
>>>> give you entry 0 containing esid=0 and vsid=0 followed by valid
>>>> entries.  Perhaps the best approach is to ignore any entries with
>>>> SLB_ESID_V clear.
>>> 
>>> That means we don't clear entries we don't receive from the kernel because they're V=0 but which were V=1 before. Which with the current code is probably already broken.
>>> 
>>> So yes, clear all cached entries first (to make sure we have no stale
>>> ones), then loop through all and only add entries with V=1 should fix
>>> everything for PR as well as HV.
>> 
>> This is more or less what the patch is doing. The kernel already
>> does memset of all the slb entries. The only difference is we don't
>> depend on the slb index in the return value. Instead we just use the
>> array index as the slb index. Do we really need to make sure the slb
>> index remain same ?
>
> Yes, otherwise get/set change SLB numbering which the guest doesn't
> expect.

how about 

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 30a870e..313f866 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1033,9 +1033,21 @@ int kvm_arch_get_registers(CPUState *cs)
 
         /* Sync SLB */
 #ifdef TARGET_PPC64
+        /*
+         * KVM_GET_SREGS doesn't retun slb entry with slot information
+         * same as index. So don't depend on the slot information in
+         * the returned value.
+         * Zero out the SLB array invalidating all the entries
+         */
+        memset(env->slb, 0, 64 * sizeof(ppc_slb_t));
         for (i = 0; i < 64; i++) {
-            ppc_store_slb(env, sregs.u.s.ppc64.slb[i].slbe,
-                               sregs.u.s.ppc64.slb[i].slbv);
+            target_ulong rb = sregs.u.s.ppc64.slb[i].slbe;
+            target_ulong rs = sregs.u.s.ppc64.slb[i].slbv;
+            /*
+             * Only restore valid entries
+             */
+            if (rb & SLB_ESID_V)
+                ppc_store_slb(env, rb, rs);
         }
 #endif


WARNING: multiple messages have this Message-ID (diff)
From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
To: Alexander Graf <agraf@suse.de>
Cc: Paul Mackerras <paulus@samba.org>,
	"kvm-ppc\@vger.kernel.org" <kvm-ppc@vger.kernel.org>,
	"kvm\@vger.kernel.org list" <kvm@vger.kernel.org>
Subject: Re: [PATCH] target-ppc: Update slb array with correct index values.
Date: Thu, 22 Aug 2013 18:50:00 +0530	[thread overview]
Message-ID: <87k3jd277j.fsf@linux.vnet.ibm.com> (raw)
In-Reply-To: <14BF15D9-CF86-43F8-9327-7B2BB42FFEFF@suse.de>

Alexander Graf <agraf@suse.de> writes:

> Am 21.08.2013 um 16:59 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 
>> 
>> ....
>> 
>>>> 
>>>> On HV KVM yes, that would be the end of the list, but PR KVM could
>>>> give you entry 0 containing esid==0 and vsid==0 followed by valid
>>>> entries.  Perhaps the best approach is to ignore any entries with
>>>> SLB_ESID_V clear.
>>> 
>>> That means we don't clear entries we don't receive from the kernel because they're V=0 but which were V=1 before. Which with the current code is probably already broken.
>>> 
>>> So yes, clear all cached entries first (to make sure we have no stale
>>> ones), then loop through all and only add entries with V=1 should fix
>>> everything for PR as well as HV.
>> 
>> This is more or less what the patch is doing. The kernel already
>> does memset of all the slb entries. The only difference is we don't
>> depend on the slb index in the return value. Instead we just use the
>> array index as the slb index. Do we really need to make sure the slb
>> index remain same ?
>
> Yes, otherwise get/set change SLB numbering which the guest doesn't
> expect.

how about 

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 30a870e..313f866 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1033,9 +1033,21 @@ int kvm_arch_get_registers(CPUState *cs)
 
         /* Sync SLB */
 #ifdef TARGET_PPC64
+        /*
+         * KVM_GET_SREGS doesn't retun slb entry with slot information
+         * same as index. So don't depend on the slot information in
+         * the returned value.
+         * Zero out the SLB array invalidating all the entries
+         */
+        memset(env->slb, 0, 64 * sizeof(ppc_slb_t));
         for (i = 0; i < 64; i++) {
-            ppc_store_slb(env, sregs.u.s.ppc64.slb[i].slbe,
-                               sregs.u.s.ppc64.slb[i].slbv);
+            target_ulong rb = sregs.u.s.ppc64.slb[i].slbe;
+            target_ulong rs = sregs.u.s.ppc64.slb[i].slbv;
+            /*
+             * Only restore valid entries
+             */
+            if (rb & SLB_ESID_V)
+                ppc_store_slb(env, rb, rs);
         }
 #endif


  reply	other threads:[~2013-08-22 13:32 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-11 18:28 [PATCH] target-ppc: Update slb array with correct index values Aneesh Kumar K.V
2013-08-19  7:37 ` Aneesh Kumar K.V
2013-08-19  8:21   ` Alexander Graf
2013-08-19  8:21     ` Alexander Graf
2013-08-20 13:57     ` Aneesh Kumar K.V
2013-08-20 13:57       ` Aneesh Kumar K.V
2013-08-21  7:02       ` Alexander Graf
2013-08-21  7:02         ` Alexander Graf
2013-08-21  9:07         ` Aneesh Kumar K.V
2013-08-21  9:19           ` Aneesh Kumar K.V
2013-08-21  5:11     ` Paul Mackerras
2013-08-21  5:11       ` Paul Mackerras
2013-08-21  7:37       ` Alexander Graf
2013-08-21  7:37         ` Alexander Graf
2013-08-21  7:42         ` Alexander Graf
2013-08-21  7:42           ` Alexander Graf
2013-08-21  9:25         ` Paul Mackerras
2013-08-21  9:25           ` Paul Mackerras
2013-08-21 10:03           ` Alexander Graf
2013-08-21 10:03             ` Alexander Graf
2013-08-21 15:59             ` Aneesh Kumar K.V
2013-08-21 15:59               ` Aneesh Kumar K.V
2013-08-21 23:32               ` Alexander Graf
2013-08-21 23:32                 ` Alexander Graf
2013-08-22 13:20                 ` Aneesh Kumar K.V [this message]
2013-08-22 13:32                   ` Aneesh Kumar K.V
2013-08-22 16:36                   ` Alexander Graf
2013-08-22 16:36                     ` Alexander Graf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87k3jd277j.fsf@linux.vnet.ibm.com \
    --to=aneesh.kumar@linux.vnet.ibm.com \
    --cc=agraf@suse.de \
    --cc=kvm-ppc@vger.kernel.org \
    --cc=kvm@vger.kernel.org \
    --cc=paulus@samba.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.