| Message ID | 20180530124706.25284-3-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 30, 2018 at 01:47:02PM +0100, Marc Zyngier wrote: > Set/Way handling is one of the ugliest corners of KVM. We shouldn't > have to handle that, but better safe than sorry. > > Thankfully, FWB fixes this for us by not requiering any maintenance > whatsoever, which means we don't have to emulate S/W CMOs, and don't > have to track VM ops either. > > We still have to trap S/W though, if only to prevent the guest from > doing something bad. S/W ops *also* do I-cache maintenance, so we'd still need to emulate that. Though it looks like we're missing that today... > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/kvm/sys_regs.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 6e3b969391fd..9a740f159245 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -195,7 +195,13 @@ static bool access_dcsw(struct kvm_vcpu *vcpu, > if (!p->is_write) > return read_from_write_only(vcpu, p, r); > > - kvm_set_way_flush(vcpu); > + /* > + * Only track S/W ops if we don't have FWB. It still indicates > + * that the guest is a bit broken... > + */ > + if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) > + kvm_set_way_flush(vcpu); > + Assuming we implement I-cache maintenance, we can have something like: if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) kvm_set_way_flush_dcache(vcpu); kvm_set_way_flush_icache(vcpu); Thanks, Mark. > return true; > } > > -- > 2.17.1 >
On 31/05/18 12:51, Mark Rutland wrote: > On Wed, May 30, 2018 at 01:47:02PM +0100, Marc Zyngier wrote: >> Set/Way handling is one of the ugliest corners of KVM. We shouldn't >> have to handle that, but better safe than sorry. >> >> Thankfully, FWB fixes this for us by not requiering any maintenance >> whatsoever, which means we don't have to emulate S/W CMOs, and don't >> have to track VM ops either. >> >> We still have to trap S/W though, if only to prevent the guest from >> doing something bad. > > S/W ops *also* do I-cache maintenance, so we'd still need to emulate > that. Though it looks like we're missing that today... This doesn't look right: CSSELR_EL1 does indeed have an InD bit, but that's only for the purpose of reading CSSIDR_EL1. DC CSW and co directly take a level *without* the InD bit, and seem to be limited to "data and unified cache". Am I missing something? Thanks, M.
On Thu, May 31, 2018 at 02:00:11PM +0100, Marc Zyngier wrote: > On 31/05/18 12:51, Mark Rutland wrote: > > On Wed, May 30, 2018 at 01:47:02PM +0100, Marc Zyngier wrote: > >> Set/Way handling is one of the ugliest corners of KVM. We shouldn't > >> have to handle that, but better safe than sorry. > >> > >> Thankfully, FWB fixes this for us by not requiering any maintenance > >> whatsoever, which means we don't have to emulate S/W CMOs, and don't > >> have to track VM ops either. > >> > >> We still have to trap S/W though, if only to prevent the guest from > >> doing something bad. > > > > S/W ops *also* do I-cache maintenance, so we'd still need to emulate > > that. Though it looks like we're missing that today... > > This doesn't look right: CSSELR_EL1 does indeed have an InD bit, but > that's only for the purpose of reading CSSIDR_EL1. DC CSW and co > directly take a level *without* the InD bit, and seem to be limited to > "data and unified cache". > > Am I missing something? No; I was mistaken. Sorry for the noise! Mark.
On Wed, May 30, 2018 at 01:47:02PM +0100, Marc Zyngier wrote: > Set/Way handling is one of the ugliest corners of KVM. We shouldn't > have to handle that, but better safe than sorry. > > Thankfully, FWB fixes this for us by not requiering any maintenance > whatsoever, which means we don't have to emulate S/W CMOs, and don't > have to track VM ops either. I tiny bit of rationale here would have been nice. As I understand it, if we're presenting the guest with a fully coherent system, there should never be a need to invalidate anything, because the guest will always see the most recent value no matter how it sings and dances, right? > > We still have to trap S/W though, if only to prevent the guest from > doing something bad. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/kvm/sys_regs.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 6e3b969391fd..9a740f159245 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -195,7 +195,13 @@ static bool access_dcsw(struct kvm_vcpu *vcpu, > if (!p->is_write) > return read_from_write_only(vcpu, p, r); > > - kvm_set_way_flush(vcpu); > + /* > + * Only track S/W ops if we don't have FWB. It still indicates > + * that the guest is a bit broken... > + */ Is it strictly true that the guest is broken if it does any form of S/W ops? Does the guest actually know that it's running on a fully coherent system, or is the argument that no software, ever, should do S/W, even for reboot etc.? I think this should have slightly more info, or that part of the comment should just be dropped, to avoid misleading future readers who don't have the full picture. > + if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) > + kvm_set_way_flush(vcpu); > + > return true; > } > > -- > 2.17.1 > Besides the usual nits on commentary: Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
On Sat, 09 Jun 2018 10:26:40 +0100, Christoffer Dall wrote: > > On Wed, May 30, 2018 at 01:47:02PM +0100, Marc Zyngier wrote: > > Set/Way handling is one of the ugliest corners of KVM. We shouldn't > > have to handle that, but better safe than sorry. > > > > Thankfully, FWB fixes this for us by not requiering any maintenance > > whatsoever, which means we don't have to emulate S/W CMOs, and don't > > have to track VM ops either. > > I tiny bit of rationale here would have been nice. As I understand it, > if we're presenting the guest with a fully coherent system, there should > never be a need to invalidate anything, because the guest will always > see the most recent value no matter how it sings and dances, right? The guest may not even know about the "fully coherent system". It may continue to issue its CMOs as before, not realising that they are not required. > > > > We still have to trap S/W though, if only to prevent the guest from > > doing something bad. > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > --- > > arch/arm64/kvm/sys_regs.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index 6e3b969391fd..9a740f159245 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -195,7 +195,13 @@ static bool access_dcsw(struct kvm_vcpu *vcpu, > > if (!p->is_write) > > return read_from_write_only(vcpu, p, r); > > > > - kvm_set_way_flush(vcpu); > > + /* > > + * Only track S/W ops if we don't have FWB. It still indicates > > + * that the guest is a bit broken... > > + */ > > Is it strictly true that the guest is broken if it does any form of S/W > ops? Does the guest actually know that it's running on a fully coherent > system, or is the argument that no software, ever, should do S/W, even > for reboot etc.? S/W should really only be used in power-management scenario. I really cannot think of a single valid (or even safe) reason to issue a S/W operation outside of PM, when you're guaranteed that there is only a single CPU up and running. A guest OS cannot enforce this requirement, so that's really always broken. > I think this should have slightly more info, or that part of the comment > should just be dropped, to avoid misleading future readers who don't > have the full picture. Happy to add more details when I respin this series. > > > + if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) > > + kvm_set_way_flush(vcpu); > > + > > return true; > > } > > > > -- > > 2.17.1 > > > > Besides the usual nits on commentary: > > Reviewed-by: Christoffer Dall <christoffer.dall@arm.com> Thanks, M.
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 6e3b969391fd..9a740f159245 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -195,7 +195,13 @@ static bool access_dcsw(struct kvm_vcpu *vcpu, if (!p->is_write) return read_from_write_only(vcpu, p, r); - kvm_set_way_flush(vcpu); + /* + * Only track S/W ops if we don't have FWB. It still indicates + * that the guest is a bit broken... + */ + if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) + kvm_set_way_flush(vcpu); + return true; }
Set/Way handling is one of the ugliest corners of KVM. We shouldn't have to handle that, but better safe than sorry. Thankfully, FWB fixes this for us by not requiering any maintenance whatsoever, which means we don't have to emulate S/W CMOs, and don't have to track VM ops either. We still have to trap S/W though, if only to prevent the guest from doing something bad. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/kvm/sys_regs.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)