diff for duplicates of <1373390018.8183.194@snotra> diff --git a/a/1.txt b/N1/1.txt index 30327d4..4e34351 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,97 +1,97 @@ On 07/08/2013 08:39:05 AM, Alexander Graf wrote: -> +>=20 > On 28.06.2013, at 11:20, Mihai Caraman wrote: -> -> > lwepx faults needs to be handled by KVM and this implies additional +>=20 +> > lwepx faults needs to be handled by KVM and this implies additional =20 > code -> > in DO_KVM macro to identify the source of the exception originated +> > in DO_KVM macro to identify the source of the exception originated =20 > from > > host context. This requires to check the Exception Syndrome Register -> > (ESR[EPID]) and External PID Load Context Register (EPLC[EGS]) for +> > (ESR[EPID]) and External PID Load Context Register (EPLC[EGS]) for =20 > DTB_MISS, > > DSI and LRAT exceptions which is too intrusive for the host. > > -> > Get rid of lwepx and acquire last instuction in +> > Get rid of lwepx and acquire last instuction in =20 > kvmppc_handle_exit() by -> > searching for the physical address and kmap it. This fixes an +> > searching for the physical address and kmap it. This fixes an =20 > infinite loop -> +>=20 > What's the difference in speed for this? -> -> Also, could we call lwepx later in host code, when +>=20 +> Also, could we call lwepx later in host code, when =20 > kvmppc_get_last_inst() gets invoked? -Any use of lwepx is problematic unless we want to add overhead to the +Any use of lwepx is problematic unless we want to add overhead to the =20 main Linux TLB miss handler. > > + return; > > + } > > + -> > + mas3 = mfspr(SPRN_MAS3); -> > + pr = vcpu->arch.shared->msr & MSR_PR; -> > + if ((pr && (!(mas3 & MAS3_UX))) || ((!pr) && (!(mas3 & +> > + mas3 =3D mfspr(SPRN_MAS3); +> > + pr =3D vcpu->arch.shared->msr & MSR_PR; +> > + if ((pr && (!(mas3 & MAS3_UX))) || ((!pr) && (!(mas3 & =20 > MAS3_SX)))) { > > + /* -> > + * Another thread may rewrite the TLB entry in +> > + * Another thread may rewrite the TLB entry in =20 > parallel, don't -> > + * execute from the address if the execute permission +> > + * execute from the address if the execute permission =20 > is not set -> +>=20 > Isn't this racy? -Yes, that's the point. We want to access permissions atomically with -the address. If the guest races here, the unpredictable behavior is -its own fault, but we don't want to make it worse by assuming that the +Yes, that's the point. We want to access permissions atomically with =20 +the address. If the guest races here, the unpredictable behavior is =20 +its own fault, but we don't want to make it worse by assuming that the =20 new TLB entry is executable just because the old TLB entry was. -There's still a potential problem if the instruction at the new TLB -entry is valid but not something that KVM emulates (because it wouldn't -have trapped). Given that the guest is already engaging in -unpredictable behavior, though, and that it's no longer a security -issue (it'll just cause the guest to exit), I don't think we need to +There's still a potential problem if the instruction at the new TLB =20 +entry is valid but not something that KVM emulates (because it wouldn't =20 +have trapped). Given that the guest is already engaging in =20 +unpredictable behavior, though, and that it's no longer a security =20 +issue (it'll just cause the guest to exit), I don't think we need to =20 worry too much about it. > > + */ -> > + vcpu->arch.fault_esr = 0; -> > + *exit_nr = BOOKE_INTERRUPT_INST_STORAGE; +> > + vcpu->arch.fault_esr =3D 0; +> > + *exit_nr =3D BOOKE_INTERRUPT_INST_STORAGE; > > + return; > > + } > > + > > + /* Get page size */ -> > + if (MAS0_GET_TLBSEL(mfspr(SPRN_MAS0)) = 0) -> > + psize_shift = PAGE_SHIFT; +> > + if (MAS0_GET_TLBSEL(mfspr(SPRN_MAS0)) =3D=3D 0) +> > + psize_shift =3D PAGE_SHIFT; > > + else -> > + psize_shift = MAS1_GET_TSIZE(mas1) + 10; +> > + psize_shift =3D MAS1_GET_TSIZE(mas1) + 10; > > + -> > + mas7_mas3 = (((u64) mfspr(SPRN_MAS7)) << 32) | +> > + mas7_mas3 =3D (((u64) mfspr(SPRN_MAS7)) << 32) | > > + mfspr(SPRN_MAS3); -> -> You're non-atomically reading MAS3/MAS7 after you've checked for -> permissions on MAS3. I'm surprised there's no handler that allows +>=20 +> You're non-atomically reading MAS3/MAS7 after you've checked for =20 +> permissions on MAS3. I'm surprised there's no handler that allows =20 > MAS3/7 access through the new, combined SPR for 64bit systems. -There is, but then we'd need to special-case 64-bit systems. Why does -atomicity matter here? The MAS registers were filled in when we did -the tlbsx. They are thread-local. They don't magically change just -because the other thread rewrites the TLB entry that was used to fill +There is, but then we'd need to special-case 64-bit systems. Why does =20 +atomicity matter here? The MAS registers were filled in when we did =20 +the tlbsx. They are thread-local. They don't magically change just =20 +because the other thread rewrites the TLB entry that was used to fill =20 them. -> > + addr = (mas7_mas3 & (~0ULL << psize_shift)) | +> > + addr =3D (mas7_mas3 & (~0ULL << psize_shift)) | > > + (geaddr & ((1ULL << psize_shift) - 1ULL)); > > + > > + /* Map a page and get guest's instruction */ -> > + page = pfn_to_page(addr >> PAGE_SHIFT); -> -> So it seems to me like you're jumping through a lot of hoops to make -> sure this works for LRAT and non-LRAT at the same time. Can't we just +> > + page =3D pfn_to_page(addr >> PAGE_SHIFT); +>=20 +> So it seems to me like you're jumping through a lot of hoops to make =20 +> sure this works for LRAT and non-LRAT at the same time. Can't we just =20 > treat them as the different things they are? -> -> What if we have different MMU backends for LRAT and non-LRAT? The -> non-LRAT case could then try lwepx, if that fails, fall back to read -> the shadow TLB. For the LRAT case, we'd do lwepx, if that fails fall +>=20 +> What if we have different MMU backends for LRAT and non-LRAT? The =20 +> non-LRAT case could then try lwepx, if that fails, fall back to read =20 +> the shadow TLB. For the LRAT case, we'd do lwepx, if that fails fall =20 > back to this logic. -This isn't about LRAT; it's about hardware threads. It also fixes the +This isn't about LRAT; it's about hardware threads. It also fixes the =20 handling of execute-only pages on current chips. --Scott +-Scott= diff --git a/a/content_digest b/N1/content_digest index a3f9a84..35b828d 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,7 +1,7 @@ "ref\03079C0F5-E13B-4572-8C9D-BCE558AF64D8@suse.de\0" "From\0Scott Wood <scottwood@freescale.com>\0" "Subject\0Re: [PATCH 2/2] KVM: PPC: Book3E: Get vcpu's last instruction for emulation\0" - "Date\0Tue, 09 Jul 2013 17:13:38 +0000\0" + "Date\0Tue, 9 Jul 2013 12:13:38 -0500\0" "To\0Alexander Graf <agraf@suse.de>\0" "Cc\0Mihai Caraman <mihai.caraman@freescale.com>" linuxppc-dev@lists.ozlabs.org @@ -10,101 +10,101 @@ "\00:1\0" "b\0" "On 07/08/2013 08:39:05 AM, Alexander Graf wrote:\n" - "> \n" + ">=20\n" "> On 28.06.2013, at 11:20, Mihai Caraman wrote:\n" - "> \n" - "> > lwepx faults needs to be handled by KVM and this implies additional \n" + ">=20\n" + "> > lwepx faults needs to be handled by KVM and this implies additional =20\n" "> code\n" - "> > in DO_KVM macro to identify the source of the exception originated \n" + "> > in DO_KVM macro to identify the source of the exception originated =20\n" "> from\n" "> > host context. This requires to check the Exception Syndrome Register\n" - "> > (ESR[EPID]) and External PID Load Context Register (EPLC[EGS]) for \n" + "> > (ESR[EPID]) and External PID Load Context Register (EPLC[EGS]) for =20\n" "> DTB_MISS,\n" "> > DSI and LRAT exceptions which is too intrusive for the host.\n" "> >\n" - "> > Get rid of lwepx and acquire last instuction in \n" + "> > Get rid of lwepx and acquire last instuction in =20\n" "> kvmppc_handle_exit() by\n" - "> > searching for the physical address and kmap it. This fixes an \n" + "> > searching for the physical address and kmap it. This fixes an =20\n" "> infinite loop\n" - "> \n" + ">=20\n" "> What's the difference in speed for this?\n" - "> \n" - "> Also, could we call lwepx later in host code, when \n" + ">=20\n" + "> Also, could we call lwepx later in host code, when =20\n" "> kvmppc_get_last_inst() gets invoked?\n" "\n" - "Any use of lwepx is problematic unless we want to add overhead to the \n" + "Any use of lwepx is problematic unless we want to add overhead to the =20\n" "main Linux TLB miss handler.\n" "\n" "> > +\t\treturn;\n" "> > +\t}\n" "> > +\n" - "> > +\tmas3 = mfspr(SPRN_MAS3);\n" - "> > +\tpr = vcpu->arch.shared->msr & MSR_PR;\n" - "> > +\tif ((pr && (!(mas3 & MAS3_UX))) || ((!pr) && (!(mas3 & \n" + "> > +\tmas3 =3D mfspr(SPRN_MAS3);\n" + "> > +\tpr =3D vcpu->arch.shared->msr & MSR_PR;\n" + "> > +\tif ((pr && (!(mas3 & MAS3_UX))) || ((!pr) && (!(mas3 & =20\n" "> MAS3_SX)))) {\n" "> > +\t \t/*\n" - "> > +\t\t * Another thread may rewrite the TLB entry in \n" + "> > +\t\t * Another thread may rewrite the TLB entry in =20\n" "> parallel, don't\n" - "> > +\t\t * execute from the address if the execute permission \n" + "> > +\t\t * execute from the address if the execute permission =20\n" "> is not set\n" - "> \n" + ">=20\n" "> Isn't this racy?\n" "\n" - "Yes, that's the point. We want to access permissions atomically with \n" - "the address. If the guest races here, the unpredictable behavior is \n" - "its own fault, but we don't want to make it worse by assuming that the \n" + "Yes, that's the point. We want to access permissions atomically with =20\n" + "the address. If the guest races here, the unpredictable behavior is =20\n" + "its own fault, but we don't want to make it worse by assuming that the =20\n" "new TLB entry is executable just because the old TLB entry was.\n" "\n" - "There's still a potential problem if the instruction at the new TLB \n" - "entry is valid but not something that KVM emulates (because it wouldn't \n" - "have trapped). Given that the guest is already engaging in \n" - "unpredictable behavior, though, and that it's no longer a security \n" - "issue (it'll just cause the guest to exit), I don't think we need to \n" + "There's still a potential problem if the instruction at the new TLB =20\n" + "entry is valid but not something that KVM emulates (because it wouldn't =20\n" + "have trapped). Given that the guest is already engaging in =20\n" + "unpredictable behavior, though, and that it's no longer a security =20\n" + "issue (it'll just cause the guest to exit), I don't think we need to =20\n" "worry too much about it.\n" "\n" "> > +\t\t */\n" - "> > +\t\tvcpu->arch.fault_esr = 0;\n" - "> > +\t\t*exit_nr = BOOKE_INTERRUPT_INST_STORAGE;\n" + "> > +\t\tvcpu->arch.fault_esr =3D 0;\n" + "> > +\t\t*exit_nr =3D BOOKE_INTERRUPT_INST_STORAGE;\n" "> > +\t\treturn;\n" "> > +\t}\n" "> > +\n" "> > +\t/* Get page size */\n" - "> > +\tif (MAS0_GET_TLBSEL(mfspr(SPRN_MAS0)) = 0)\n" - "> > +\t\tpsize_shift = PAGE_SHIFT;\n" + "> > +\tif (MAS0_GET_TLBSEL(mfspr(SPRN_MAS0)) =3D=3D 0)\n" + "> > +\t\tpsize_shift =3D PAGE_SHIFT;\n" "> > +\telse\n" - "> > +\t\tpsize_shift = MAS1_GET_TSIZE(mas1) + 10;\n" + "> > +\t\tpsize_shift =3D MAS1_GET_TSIZE(mas1) + 10;\n" "> > +\n" - "> > +\tmas7_mas3 = (((u64) mfspr(SPRN_MAS7)) << 32) |\n" + "> > +\tmas7_mas3 =3D (((u64) mfspr(SPRN_MAS7)) << 32) |\n" "> > +\t\t mfspr(SPRN_MAS3);\n" - "> \n" - "> You're non-atomically reading MAS3/MAS7 after you've checked for \n" - "> permissions on MAS3. I'm surprised there's no handler that allows \n" + ">=20\n" + "> You're non-atomically reading MAS3/MAS7 after you've checked for =20\n" + "> permissions on MAS3. I'm surprised there's no handler that allows =20\n" "> MAS3/7 access through the new, combined SPR for 64bit systems.\n" "\n" - "There is, but then we'd need to special-case 64-bit systems. Why does \n" - "atomicity matter here? The MAS registers were filled in when we did \n" - "the tlbsx. They are thread-local. They don't magically change just \n" - "because the other thread rewrites the TLB entry that was used to fill \n" + "There is, but then we'd need to special-case 64-bit systems. Why does =20\n" + "atomicity matter here? The MAS registers were filled in when we did =20\n" + "the tlbsx. They are thread-local. They don't magically change just =20\n" + "because the other thread rewrites the TLB entry that was used to fill =20\n" "them.\n" "\n" - "> > +\taddr = (mas7_mas3 & (~0ULL << psize_shift)) |\n" + "> > +\taddr =3D (mas7_mas3 & (~0ULL << psize_shift)) |\n" "> > +\t (geaddr & ((1ULL << psize_shift) - 1ULL));\n" "> > +\n" "> > +\t/* Map a page and get guest's instruction */\n" - "> > +\tpage = pfn_to_page(addr >> PAGE_SHIFT);\n" - "> \n" - "> So it seems to me like you're jumping through a lot of hoops to make \n" - "> sure this works for LRAT and non-LRAT at the same time. Can't we just \n" + "> > +\tpage =3D pfn_to_page(addr >> PAGE_SHIFT);\n" + ">=20\n" + "> So it seems to me like you're jumping through a lot of hoops to make =20\n" + "> sure this works for LRAT and non-LRAT at the same time. Can't we just =20\n" "> treat them as the different things they are?\n" - "> \n" - "> What if we have different MMU backends for LRAT and non-LRAT? The \n" - "> non-LRAT case could then try lwepx, if that fails, fall back to read \n" - "> the shadow TLB. For the LRAT case, we'd do lwepx, if that fails fall \n" + ">=20\n" + "> What if we have different MMU backends for LRAT and non-LRAT? The =20\n" + "> non-LRAT case could then try lwepx, if that fails, fall back to read =20\n" + "> the shadow TLB. For the LRAT case, we'd do lwepx, if that fails fall =20\n" "> back to this logic.\n" "\n" - "This isn't about LRAT; it's about hardware threads. It also fixes the \n" + "This isn't about LRAT; it's about hardware threads. It also fixes the =20\n" "handling of execute-only pages on current chips.\n" "\n" - -Scott + -Scott= -fee817174643ec23b6fab88acb955ac2f740687647ced3deb4c2cb21e3f682a4 +63d65fa3313ed0d0b5e249a845e15dd918b737292dd2e1e4d59497866979973e
diff --git a/a/1.txt b/N2/1.txt index 30327d4..c1c294c 100644 --- a/a/1.txt +++ b/N2/1.txt @@ -58,7 +58,7 @@ worry too much about it. > > + } > > + > > + /* Get page size */ -> > + if (MAS0_GET_TLBSEL(mfspr(SPRN_MAS0)) = 0) +> > + if (MAS0_GET_TLBSEL(mfspr(SPRN_MAS0)) == 0) > > + psize_shift = PAGE_SHIFT; > > + else > > + psize_shift = MAS1_GET_TSIZE(mas1) + 10; diff --git a/a/content_digest b/N2/content_digest index a3f9a84..c7aa2ff 100644 --- a/a/content_digest +++ b/N2/content_digest @@ -1,7 +1,7 @@ "ref\03079C0F5-E13B-4572-8C9D-BCE558AF64D8@suse.de\0" "From\0Scott Wood <scottwood@freescale.com>\0" "Subject\0Re: [PATCH 2/2] KVM: PPC: Book3E: Get vcpu's last instruction for emulation\0" - "Date\0Tue, 09 Jul 2013 17:13:38 +0000\0" + "Date\0Tue, 9 Jul 2013 12:13:38 -0500\0" "To\0Alexander Graf <agraf@suse.de>\0" "Cc\0Mihai Caraman <mihai.caraman@freescale.com>" linuxppc-dev@lists.ozlabs.org @@ -69,7 +69,7 @@ "> > +\t}\n" "> > +\n" "> > +\t/* Get page size */\n" - "> > +\tif (MAS0_GET_TLBSEL(mfspr(SPRN_MAS0)) = 0)\n" + "> > +\tif (MAS0_GET_TLBSEL(mfspr(SPRN_MAS0)) == 0)\n" "> > +\t\tpsize_shift = PAGE_SHIFT;\n" "> > +\telse\n" "> > +\t\tpsize_shift = MAS1_GET_TSIZE(mas1) + 10;\n" @@ -107,4 +107,4 @@ "\n" -Scott -fee817174643ec23b6fab88acb955ac2f740687647ced3deb4c2cb21e3f682a4 +bb2468d25c935741365fbca5350ed28fba7e015902178e4b60b45bedd549fead
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.