Messages in this thread | | | Date | Tue, 08 Oct 2019 15:44:35 -0700 (PDT) | Subject | Re: [PATCH v7 10/21] RISC-V: KVM: Handle MMIO exits for VCPU | From | Palmer Dabbelt <> |
| |
On Mon, 23 Sep 2019 04:12:17 PDT (-0700), pbonzini@redhat.com wrote: > On 04/09/19 18:15, Anup Patel wrote: >> + unsigned long guest_sstatus = >> + vcpu->arch.guest_context.sstatus | SR_MXR; >> + unsigned long guest_hstatus = >> + vcpu->arch.guest_context.hstatus | HSTATUS_SPRV; >> + unsigned long guest_vsstatus, old_stvec, tmp; >> + >> + guest_sstatus = csr_swap(CSR_SSTATUS, guest_sstatus); >> + old_stvec = csr_swap(CSR_STVEC, (ulong)&__kvm_riscv_unpriv_trap); >> + >> + if (read_insn) { >> + guest_vsstatus = csr_read_set(CSR_VSSTATUS, SR_MXR); > > Is this needed? IIUC SSTATUS.MXR encompasses a wider set of permissions: > > The HS-level MXR bit makes any executable page readable. {\tt > vsstatus}.MXR makes readable those pages marked executable at the VS > translation level, but only if readable at the guest-physical > translation level. > > So it should be enough to set SSTATUS.MXR=1 I think. But you also > shouldn't set SSTATUS.MXR=1 in the !read_insn case. > > Also, you can drop the irq save/restore (which is already a save/restore > of SSTATUS) since you already write 0 to SSTATUS.SIE in your csr_swap. > Perhaps add a BUG_ON(guest_sstatus & SR_SIE) before the csr_swap? > >> + asm volatile ("\n" >> + "csrrw %[hstatus], " STR(CSR_HSTATUS) ", %[hstatus]\n" >> + "li %[tilen], 4\n" >> + "li %[tscause], 0\n" >> + "lhu %[val], (%[addr])\n" >> + "andi %[tmp], %[val], 3\n" >> + "addi %[tmp], %[tmp], -3\n" >> + "bne %[tmp], zero, 2f\n" >> + "lhu %[tmp], 2(%[addr])\n" >> + "sll %[tmp], %[tmp], 16\n" >> + "add %[val], %[val], %[tmp]\n" >> + "2: csrw " STR(CSR_HSTATUS) ", %[hstatus]" >> + : [hstatus] "+&r"(guest_hstatus), [val] "=&r" (val), >> + [tmp] "=&r" (tmp), [tilen] "+&r" (tilen), >> + [tscause] "+&r" (tscause) >> + : [addr] "r" (addr)); >> + csr_write(CSR_VSSTATUS, guest_vsstatus); > >> >> +#ifndef CONFIG_RISCV_ISA_C >> + "li %[tilen], 4\n" >> +#else >> + "li %[tilen], 2\n" >> +#endif > > Can you use an assembler directive to force using a non-compressed > format for ld and lw? This would get rid of tilen, which is costing 6 > bytes (if I did the RVC math right) in order to save two. :) > > Paolo > >> + "li %[tscause], 0\n" >> +#ifdef CONFIG_64BIT >> + "ld %[val], (%[addr])\n" >> +#else >> + "lw %[val], (%[addr])\n" >> +#endif To: anup@brainfault.org CC: pbonzini@redhat.com CC: Anup Patel <Anup.Patel@wdc.com> CC: Paul Walmsley <paul.walmsley@sifive.com> CC: rkrcmar@redhat.com CC: daniel.lezcano@linaro.org CC: tglx@linutronix.de CC: graf@amazon.com CC: Atish Patra <Atish.Patra@wdc.com> CC: Alistair Francis <Alistair.Francis@wdc.com> CC: Damien Le Moal <Damien.LeMoal@wdc.com> CC: Christoph Hellwig <hch@infradead.org> CC: kvm@vger.kernel.org CC: linux-riscv@lists.infradead.org CC: linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 10/21] RISC-V: KVM: Handle MMIO exits for VCPU In-Reply-To: <CAAhSdy1-1yxMnjzppmUBxtSOAuwWaPtNZwW+QH1O7LAnEVP8pg@mail.gmail.com>
On Mon, 23 Sep 2019 06:09:43 PDT (-0700), anup@brainfault.org wrote: > On Mon, Sep 23, 2019 at 4:42 PM Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 04/09/19 18:15, Anup Patel wrote: >> > + unsigned long guest_sstatus = >> > + vcpu->arch.guest_context.sstatus | SR_MXR; >> > + unsigned long guest_hstatus = >> > + vcpu->arch.guest_context.hstatus | HSTATUS_SPRV; >> > + unsigned long guest_vsstatus, old_stvec, tmp; >> > + >> > + guest_sstatus = csr_swap(CSR_SSTATUS, guest_sstatus); >> > + old_stvec = csr_swap(CSR_STVEC, (ulong)&__kvm_riscv_unpriv_trap); >> > + >> > + if (read_insn) { >> > + guest_vsstatus = csr_read_set(CSR_VSSTATUS, SR_MXR); >> >> Is this needed? IIUC SSTATUS.MXR encompasses a wider set of permissions: >> >> The HS-level MXR bit makes any executable page readable. {\tt >> vsstatus}.MXR makes readable those pages marked executable at the VS >> translation level, but only if readable at the guest-physical >> translation level. >> >> So it should be enough to set SSTATUS.MXR=1 I think. But you also >> shouldn't set SSTATUS.MXR=1 in the !read_insn case. > > I was being overly cautious here. Initially, I thought SSTATUS.MXR > applies only to Stage2 and VSSTATUS.MXR applies only to Stage1. > > I agree with you. The HS-mode should only need to set SSTATUS.MXR. > >> >> Also, you can drop the irq save/restore (which is already a save/restore >> of SSTATUS) since you already write 0 to SSTATUS.SIE in your csr_swap. >> Perhaps add a BUG_ON(guest_sstatus & SR_SIE) before the csr_swap? > > I had already dropped irq save/restore in v7 series and having BUG_ON() > on guest_sstatus here would be better. > >> >> > + asm volatile ("\n" >> > + "csrrw %[hstatus], " STR(CSR_HSTATUS) ", %[hstatus]\n" >> > + "li %[tilen], 4\n" >> > + "li %[tscause], 0\n" >> > + "lhu %[val], (%[addr])\n" >> > + "andi %[tmp], %[val], 3\n" >> > + "addi %[tmp], %[tmp], -3\n" >> > + "bne %[tmp], zero, 2f\n" >> > + "lhu %[tmp], 2(%[addr])\n" >> > + "sll %[tmp], %[tmp], 16\n" >> > + "add %[val], %[val], %[tmp]\n" >> > + "2: csrw " STR(CSR_HSTATUS) ", %[hstatus]" >> > + : [hstatus] "+&r"(guest_hstatus), [val] "=&r" (val), >> > + [tmp] "=&r" (tmp), [tilen] "+&r" (tilen), >> > + [tscause] "+&r" (tscause) >> > + : [addr] "r" (addr)); >> > + csr_write(CSR_VSSTATUS, guest_vsstatus); >> >> > >> > +#ifndef CONFIG_RISCV_ISA_C >> > + "li %[tilen], 4\n" >> > +#else >> > + "li %[tilen], 2\n" >> > +#endif >> >> Can you use an assembler directive to force using a non-compressed >> format for ld and lw? This would get rid of tilen, which is costing 6 >> bytes (if I did the RVC math right) in order to save two. :) > > I tried looking for it but could not find any assembler directive > to selectively turn-off instruction compression. > >> >> Paolo >> >> > + "li %[tscause], 0\n" >> > +#ifdef CONFIG_64BIT >> > + "ld %[val], (%[addr])\n" >> > +#else >> > + "lw %[val], (%[addr])\n" >> > +#endif > > Regards, > Anup To: pbonzini@redhat.com CC: anup@brainfault.org CC: Anup Patel <Anup.Patel@wdc.com> CC: Paul Walmsley <paul.walmsley@sifive.com> CC: rkrcmar@redhat.com CC: daniel.lezcano@linaro.org CC: tglx@linutronix.de CC: graf@amazon.com CC: Atish Patra <Atish.Patra@wdc.com> CC: Alistair Francis <Alistair.Francis@wdc.com> CC: Damien Le Moal <Damien.LeMoal@wdc.com> CC: Christoph Hellwig <hch@infradead.org> CC: kvm@vger.kernel.org CC: linux-riscv@lists.infradead.org CC: linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 10/21] RISC-V: KVM: Handle MMIO exits for VCPU In-Reply-To: <45fc3ee5-0f68-4e94-cfb3-0727ca52628f@redhat.com>
On Mon, 23 Sep 2019 06:33:14 PDT (-0700), pbonzini@redhat.com wrote: > On 23/09/19 15:09, Anup Patel wrote: >>>> +#ifndef CONFIG_RISCV_ISA_C >>>> + "li %[tilen], 4\n" >>>> +#else >>>> + "li %[tilen], 2\n" >>>> +#endif >>> >>> Can you use an assembler directive to force using a non-compressed >>> format for ld and lw? This would get rid of tilen, which is costing 6 >>> bytes (if I did the RVC math right) in order to save two. :) >> >> I tried looking for it but could not find any assembler directive >> to selectively turn-off instruction compression. > > ".option norvc"? > > Paolo To: anup@brainfault.org CC: pbonzini@redhat.com CC: Anup Patel <Anup.Patel@wdc.com> CC: Paul Walmsley <paul.walmsley@sifive.com> CC: rkrcmar@redhat.com CC: daniel.lezcano@linaro.org CC: tglx@linutronix.de CC: graf@amazon.com CC: Atish Patra <Atish.Patra@wdc.com> CC: Alistair Francis <Alistair.Francis@wdc.com> CC: Damien Le Moal <Damien.LeMoal@wdc.com> CC: Christoph Hellwig <hch@infradead.org> CC: kvm@vger.kernel.org CC: linux-riscv@lists.infradead.org CC: linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 10/21] RISC-V: KVM: Handle MMIO exits for VCPU In-Reply-To: <CAAhSdy29gi2d9c9tumtO68QbB=_+yUYp+ikN3dQ-wa2e-Lesfw@mail.gmail.com>
On Mon, 23 Sep 2019 22:07:43 PDT (-0700), anup@brainfault.org wrote: > On Mon, Sep 23, 2019 at 7:03 PM Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 23/09/19 15:09, Anup Patel wrote: >> >>> +#ifndef CONFIG_RISCV_ISA_C >> >>> + "li %[tilen], 4\n" >> >>> +#else >> >>> + "li %[tilen], 2\n" >> >>> +#endif >> >> >> >> Can you use an assembler directive to force using a non-compressed >> >> format for ld and lw? This would get rid of tilen, which is costing 6 >> >> bytes (if I did the RVC math right) in order to save two. :) >> > >> > I tried looking for it but could not find any assembler directive >> > to selectively turn-off instruction compression. >> >> ".option norvc"? > > Thanks for the hint. I will try ".option norvc"
It should be something like
.option push .option norvc ld ... .option pop
which preserves C support for the rest of the file.
> > Regards, > Anup > >> >> Paolo
| |