From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0EA3CEB64D9 for ; Thu, 15 Jun 2023 04:54:07 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1q9ezP-0001b0-7S; Thu, 15 Jun 2023 00:53:43 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1q9ezM-0001aV-P1; Thu, 15 Jun 2023 00:53:40 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1q9ezK-00011B-Kb; Thu, 15 Jun 2023 00:53:40 -0400 Received: from pps.filterd (m0353725.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 35F4coBO005778; Thu, 15 Jun 2023 04:53:37 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=pp1; bh=j9NzkR7QzB6JCZo2hO29YyH9endTorWnU4AVhWPUMBY=; b=Wmo6U4LfsqHQefP9kA2E41tfEqwmGUQ2fEBXDU2sW7iVlVUzx6I5T+1agb+h08+7Y/JL vOz+sgmKefarqgZEo+h2f6Kcr0+pKQ2IsghMnML2naTairzuZHJbeet5WlOVh+1RRmt+ FTvBON+lOZID13HaBogIsw1Qxv86eMYnNvSTl5b2UgIx8zgDNOzruZsKKRBglWoVjNbw M7orgl37U1Jv4o/GN73yY63rgxfz+GYElFXbYrNLISV3cdef+4U0CiBEG5AoyjfENQ/b xYiwQItY3Rl/2iItNLggm2bwYAEP2o7oIM0tFXH1tHFYYDgrWUDcAkIzyVr9U/IiKlez 0Q== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3r7u8grubf-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 15 Jun 2023 04:53:36 +0000 Received: from m0353725.ppops.net (m0353725.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 35F4d71E007088; Thu, 15 Jun 2023 04:53:36 GMT Received: from ppma04wdc.us.ibm.com (1a.90.2fa9.ip4.static.sl-reverse.com [169.47.144.26]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3r7u8grub9-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 15 Jun 2023 04:53:36 +0000 Received: from pps.filterd (ppma04wdc.us.ibm.com [127.0.0.1]) by ppma04wdc.us.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 35F1BMnA010721; Thu, 15 Jun 2023 04:53:36 GMT Received: from smtprelay01.wdc07v.mail.ibm.com ([9.208.129.119]) by ppma04wdc.us.ibm.com (PPS) with ESMTPS id 3r4gt5s5d9-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 15 Jun 2023 04:53:35 +0000 Received: from smtpav06.wdc07v.mail.ibm.com (smtpav06.wdc07v.mail.ibm.com [10.39.53.233]) by smtprelay01.wdc07v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 35F4rYJQ29098258 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 15 Jun 2023 04:53:35 GMT Received: from smtpav06.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id DE7E958055; Thu, 15 Jun 2023 04:53:34 +0000 (GMT) Received: from smtpav06.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 41E7F5803F; Thu, 15 Jun 2023 04:53:33 +0000 (GMT) Received: from [9.109.242.129] (unknown [9.109.242.129]) by smtpav06.wdc07v.mail.ibm.com (Postfix) with ESMTP; Thu, 15 Jun 2023 04:53:32 +0000 (GMT) Message-ID: <0aa62167-b6f3-bd98-b4c3-35ab599f151f@linux.ibm.com> Date: Thu, 15 Jun 2023 10:23:31 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Subject: Re: [PATCH 3/4] ppc/spapr: load and store l2 state with helper functions Content-Language: en-US To: Nicholas Piggin , qemu-ppc@nongnu.org Cc: qemu-devel@nongnu.org, Daniel Henrique Barboza References: <20230608091344.88685-1-npiggin@gmail.com> <20230608091344.88685-4-npiggin@gmail.com> <90f62b35-aba5-df79-a50d-29f78a79c7b3@linux.ibm.com> From: Harsh Prateek Bora In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: wiQI4dr-7egYZc7LGmzWj1dQuTGSKH3g X-Proofpoint-ORIG-GUID: 4kOtn4en_3QcmKnVbTuumMq0pRAdn06c X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.176.26 definitions=2023-06-15_02,2023-06-14_02,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 bulkscore=0 clxscore=1015 lowpriorityscore=0 phishscore=0 mlxlogscore=999 suspectscore=0 priorityscore=1501 adultscore=0 spamscore=0 impostorscore=0 mlxscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2305260000 definitions=main-2306150037 Received-SPF: pass client-ip=148.163.158.5; envelope-from=harshpb@linux.ibm.com; helo=mx0b-001b2d01.pphosted.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_EF=-0.1, NICE_REPLY_A=-0.098, RCVD_IN_MSPIKE_H5=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org On 6/14/23 17:32, Nicholas Piggin wrote: > On Fri Jun 9, 2023 at 6:00 PM AEST, Harsh Prateek Bora wrote: >> >> >> On 6/8/23 14:43, Nicholas Piggin wrote: >>> Arguably this is just shuffling around register accesses, but one nice >>> thing it does is allow the exit to save away the L2 state then switch >>> the environment to the L1 before copying L2 data back to the L1, which >>> logically flows more naturally and simplifies the error paths. >>> >>> Signed-off-by: Nicholas Piggin >>> --- >>> hw/ppc/spapr_hcall.c | 164 ++++++++++++++++++++++--------------------- >>> 1 file changed, 85 insertions(+), 79 deletions(-) >>> >>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >>> index d5b8d54692..da6440f235 100644 >>> --- a/hw/ppc/spapr_hcall.c >>> +++ b/hw/ppc/spapr_hcall.c >>> @@ -1663,9 +1663,9 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu, >>> target_ulong *args) >>> { >>> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); >>> - CPUState *cs = CPU(cpu); >>> CPUPPCState *env = &cpu->env; >>> SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu); >>> + struct nested_ppc_state l2_state; >>> target_ulong hv_ptr = args[0]; >>> target_ulong regs_ptr = args[1]; >>> target_ulong hdec, now = cpu_ppc_load_tbl(env); >>> @@ -1699,6 +1699,10 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu, >>> return H_PARAMETER; >>> } >>> >>> + if (hv_state.lpid == 0) { >>> + return H_PARAMETER; >>> + } >>> + >>> spapr_cpu->nested_host_state = g_try_new(struct nested_ppc_state, 1); >>> if (!spapr_cpu->nested_host_state) { >>> return H_NO_MEM; >>> @@ -1717,46 +1721,49 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu, >>> return H_P2; >>> } >>> >>> - len = sizeof(env->gpr); >>> + len = sizeof(l2_state.gpr); >>> assert(len == sizeof(regs->gpr)); >>> - memcpy(env->gpr, regs->gpr, len); >>> + memcpy(l2_state.gpr, regs->gpr, len); >>> >>> - env->lr = regs->link; >>> - env->ctr = regs->ctr; >>> - cpu_write_xer(env, regs->xer); >>> - ppc_set_cr(env, regs->ccr); >>> - >>> - env->msr = regs->msr; >>> - env->nip = regs->nip; >>> + l2_state.lr = regs->link; >>> + l2_state.ctr = regs->ctr; >>> + l2_state.xer = regs->xer; >>> + l2_state.cr = regs->ccr; >>> + l2_state.msr = regs->msr; >>> + l2_state.nip = regs->nip; >>> >>> address_space_unmap(CPU(cpu)->as, regs, len, len, false); >>> >>> - env->cfar = hv_state.cfar; >>> - >>> - assert(env->spr[SPR_LPIDR] == 0); >>> - env->spr[SPR_LPIDR] = hv_state.lpid; >>> + l2_state.cfar = hv_state.cfar; >>> + l2_state.lpidr = hv_state.lpid; >>> >>> lpcr_mask = LPCR_DPFD | LPCR_ILE | LPCR_AIL | LPCR_LD | LPCR_MER; >>> lpcr = (env->spr[SPR_LPCR] & ~lpcr_mask) | (hv_state.lpcr & lpcr_mask); >>> lpcr |= LPCR_HR | LPCR_UPRT | LPCR_GTSE | LPCR_HVICE | LPCR_HDICE; >>> lpcr &= ~LPCR_LPES0; >>> - env->spr[SPR_LPCR] = lpcr & pcc->lpcr_mask; >>> + l2_state.lpcr = lpcr & pcc->lpcr_mask; >>> >>> - env->spr[SPR_PCR] = hv_state.pcr; >>> + l2_state.pcr = hv_state.pcr; >>> /* hv_state.amor is not used */ >>> - env->spr[SPR_DPDES] = hv_state.dpdes; >>> - env->spr[SPR_HFSCR] = hv_state.hfscr; >>> - hdec = hv_state.hdec_expiry - now; >>> + l2_state.dpdes = hv_state.dpdes; >>> + l2_state.hfscr = hv_state.hfscr; >>> /* TCG does not implement DAWR*, CIABR, PURR, SPURR, IC, VTB, HEIR SPRs*/ >>> - env->spr[SPR_SRR0] = hv_state.srr0; >>> - env->spr[SPR_SRR1] = hv_state.srr1; >>> - env->spr[SPR_SPRG0] = hv_state.sprg[0]; >>> - env->spr[SPR_SPRG1] = hv_state.sprg[1]; >>> - env->spr[SPR_SPRG2] = hv_state.sprg[2]; >>> - env->spr[SPR_SPRG3] = hv_state.sprg[3]; >>> - env->spr[SPR_BOOKS_PID] = hv_state.pidr; >>> - env->spr[SPR_PPR] = hv_state.ppr; >>> + l2_state.srr0 = hv_state.srr0; >>> + l2_state.srr1 = hv_state.srr1; >>> + l2_state.sprg0 = hv_state.sprg[0]; >>> + l2_state.sprg1 = hv_state.sprg[1]; >>> + l2_state.sprg2 = hv_state.sprg[2]; >>> + l2_state.sprg3 = hv_state.sprg[3]; >>> + l2_state.pidr = hv_state.pidr; >>> + l2_state.ppr = hv_state.ppr; >>> + l2_state.tb_offset = env->tb_env->tb_offset + hv_state.tb_offset; >>> + >>> + /* >>> + * Switch to the nested guest environment and start the "hdec" timer. >>> + */ >>> + nested_load_state(cpu, &l2_state); >>> >>> + hdec = hv_state.hdec_expiry - now; >>> cpu_ppc_hdecr_init(env); >>> cpu_ppc_store_hdecr(env, hdec); >>> >>> @@ -1772,14 +1779,8 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu, >>> * and it's not obviously worth a new data structure to do it. >>> */ >>> >>> - env->tb_env->tb_offset += hv_state.tb_offset; >>> spapr_cpu->in_nested = true; >>> >>> - hreg_compute_hflags(env); >>> - ppc_maybe_interrupt(env); >>> - tlb_flush(cs); >>> - env->reserve_addr = -1; /* Reset the reservation */ >>> - >>> /* >>> * The spapr hcall helper sets env->gpr[3] to the return value, but at >>> * this point the L1 is not returning from the hcall but rather we >>> @@ -1793,49 +1794,69 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp) >>> { >>> CPUPPCState *env = &cpu->env; >>> SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu); >>> - target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */ >>> + struct nested_ppc_state l2_state; >>> target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4]; >>> target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5]; >>> + target_ulong hsrr0, hsrr1, hdar, asdr, hdsisr; >>> struct kvmppc_hv_guest_state *hvstate; >>> struct kvmppc_pt_regs *regs; >>> hwaddr len; >>> >>> assert(spapr_cpu->in_nested); >>> >>> + nested_save_state(&l2_state, cpu); >>> + hsrr0 = env->spr[SPR_HSRR0]; >>> + hsrr1 = env->spr[SPR_HSRR1]; >>> + hdar = env->spr[SPR_HDAR]; >>> + hdsisr = env->spr[SPR_HDSISR]; >>> + asdr = env->spr[SPR_ASDR]; >>> + >>> + /* >>> + * Switch back to the host environment (including for any error). >>> + */ >>> + assert(env->spr[SPR_LPIDR] != 0); >>> + nested_load_state(cpu, spapr_cpu->nested_host_state); >>> + env->gpr[3] = env->excp_vectors[excp]; /* hcall return value */ >>> + >>> cpu_ppc_hdecr_exit(env); >>> >>> + spapr_cpu->in_nested = false; >>> + >>> + g_free(spapr_cpu->nested_host_state); >>> + spapr_cpu->nested_host_state = NULL; >>> + >>> len = sizeof(*hvstate); >>> hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true, >>> MEMTXATTRS_UNSPECIFIED); >>> if (len != sizeof(*hvstate)) { >>> address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true); >>> - r3_return = H_PARAMETER; >>> - goto out_restore_l1; >>> + env->gpr[3] = H_PARAMETER; >>> + return; >>> } >>> >>> - hvstate->cfar = env->cfar; >>> - hvstate->lpcr = env->spr[SPR_LPCR]; >>> - hvstate->pcr = env->spr[SPR_PCR]; >>> - hvstate->dpdes = env->spr[SPR_DPDES]; >>> - hvstate->hfscr = env->spr[SPR_HFSCR]; >>> + hvstate->cfar = l2_state.cfar; >>> + hvstate->lpcr = l2_state.lpcr; >>> + hvstate->pcr = l2_state.pcr; >>> + hvstate->dpdes = l2_state.dpdes; >>> + hvstate->hfscr = l2_state.hfscr; >>> >>> if (excp == POWERPC_EXCP_HDSI) { >>> - hvstate->hdar = env->spr[SPR_HDAR]; >>> - hvstate->hdsisr = env->spr[SPR_HDSISR]; >>> - hvstate->asdr = env->spr[SPR_ASDR]; >>> + hvstate->hdar = hdar; >>> + hvstate->hdsisr = hdsisr; >>> + hvstate->asdr = asdr; >>> } else if (excp == POWERPC_EXCP_HISI) { >>> - hvstate->asdr = env->spr[SPR_ASDR]; >>> + hvstate->asdr = asdr; >>> } >>> >>> /* HEIR should be implemented for HV mode and saved here. */ >>> - hvstate->srr0 = env->spr[SPR_SRR0]; >>> - hvstate->srr1 = env->spr[SPR_SRR1]; >>> - hvstate->sprg[0] = env->spr[SPR_SPRG0]; >>> - hvstate->sprg[1] = env->spr[SPR_SPRG1]; >>> - hvstate->sprg[2] = env->spr[SPR_SPRG2]; >>> - hvstate->sprg[3] = env->spr[SPR_SPRG3]; >>> - hvstate->pidr = env->spr[SPR_BOOKS_PID]; >>> - hvstate->ppr = env->spr[SPR_PPR]; >>> + hvstate->srr0 = l2_state.srr0; >>> + hvstate->srr1 = l2_state.srr1; >>> + hvstate->sprg[0] = l2_state.sprg0; >>> + hvstate->sprg[1] = l2_state.sprg1; >>> + hvstate->sprg[2] = l2_state.sprg2; >>> + hvstate->sprg[3] = l2_state.sprg3; >>> + hvstate->pidr = l2_state.pidr; >>> + hvstate->ppr = l2_state.ppr; >>> >>> /* Is it okay to specify write length larger than actual data written? */ >>> address_space_unmap(CPU(cpu)->as, hvstate, len, len, true); >>> @@ -1845,46 +1866,31 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp) >>> MEMTXATTRS_UNSPECIFIED); >>> if (!regs || len != sizeof(*regs)) { >>> address_space_unmap(CPU(cpu)->as, regs, len, 0, true); >>> - r3_return = H_P2; >>> - goto out_restore_l1; >>> + env->gpr[3] = H_P2; >>> + return; >>> } >>> >>> len = sizeof(env->gpr); >>> assert(len == sizeof(regs->gpr)); >>> - memcpy(regs->gpr, env->gpr, len); >>> + memcpy(regs->gpr, l2_state.gpr, len); >>> >>> - regs->link = env->lr; >>> - regs->ctr = env->ctr; >>> - regs->xer = cpu_read_xer(env); >>> - regs->ccr = ppc_get_cr(env); >>> + regs->link = l2_state.lr; >>> + regs->ctr = l2_state.ctr; >>> + regs->xer = l2_state.xer; >>> + regs->ccr = l2_state.cr; >>> >>> if (excp == POWERPC_EXCP_MCHECK || >>> excp == POWERPC_EXCP_RESET || >>> excp == POWERPC_EXCP_SYSCALL) { >>> - regs->nip = env->spr[SPR_SRR0]; >>> - regs->msr = env->spr[SPR_SRR1] & env->msr_mask; >>> + regs->nip = l2_state.srr0; >>> + regs->msr = l2_state.srr1 & env->msr_mask; >>> } else { >>> - regs->nip = env->spr[SPR_HSRR0]; >>> - regs->msr = env->spr[SPR_HSRR1] & env->msr_mask; >>> + regs->nip = hsrr0; >>> + regs->msr = hsrr1 & env->msr_mask; >>> } >>> >> >> Can the copying of L2 data to L1 be abstraced with an inline helper >> routine as well ? Then the spapr_exit_nested will look more neat: >> - nested_save_state(l2) >> - nested_load_state(host) >> - copy_l2_state_to_l1() <=== something like this? > > I did start to look at that, but I thought having the code to move > the data in and out of the actual the hcall API format in the > hcall functions themselves worked okay. > > If there could be some streamlining or code sharing with the v2 > nested-HV API, I wouldn't mind that but would probably like to see > it done as part of the v2 patches so we can see how it works. > Sure, that can be done along with future patches too. Reviewed-by: Harsh Prateek Bora > Thanks, > Nick >