Re: [PATCH] arm64/kvm: correct the error report in kvm_handle_guest_abort

[Marc Zyngier <maz@kernel.org>]

On 2021-01-18 13:04, Jianyong Wu wrote:
> Hi Marc,
> 
>> -----Original Message-----
>> From: kvmarm-bounces@lists.cs.columbia.edu <kvmarm-
>> bounces@lists.cs.columbia.edu> On Behalf Of Jianyong Wu
>> Sent: Saturday, January 16, 2021 4:47 PM
>> To: Marc Zyngier <maz@kernel.org>
>> Cc: Justin He <Justin.He@arm.com>; nd <nd@arm.com>; will@kernel.org;
>> kvmarm@lists.cs.columbia.edu; linux-arm-kernel@lists.infradead.org
>> Subject: RE: [PATCH] arm64/kvm: correct the error report in
>> kvm_handle_guest_abort
>> 
>> Hi Marc,
>> 
>> > -----Original Message-----
>> > From: Marc Zyngier <maz@kernel.org>
>> > Sent: Friday, January 15, 2021 7:21 PM
>> > To: Jianyong Wu <Jianyong.Wu@arm.com>
>> > Cc: James Morse <James.Morse@arm.com>; will@kernel.org; Suzuki
>> Poulose
>> > <Suzuki.Poulose@arm.com>; linux-arm-kernel@lists.infradead.org;
>> > kvmarm@lists.cs.columbia.edu; Steve Capper <Steve.Capper@arm.com>;
>> > Justin He <Justin.He@arm.com>; nd <nd@arm.com>
>> > Subject: Re: [PATCH] arm64/kvm: correct the error report in
>> > kvm_handle_guest_abort
>> >
>> > On 2021-01-15 09:30, Jianyong Wu wrote:
>> > > Currently, error report when cache maintenance at read-only memory
>> > > range, like rom, is not clear enough and even not correct. As the
>> > > specific error is definitely known by kvm, it is obliged to give it
>> > > out.
>> > >
>> > > Fox example, in a qemu/kvm VM, if the guest do dc at the pflash
>> > > range from
>> > > 0 to 128M, error is reported by kvm as "Data abort outside memslots
>> > > with no valid syndrome info" which is not quite correct.
>> > >
>> > > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
>> > > ---
>> > >  arch/arm64/kvm/mmu.c | 12 +++++++++---
>> > >  1 file changed, 9 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index
>> > > 7d2257cc5438..de66b7e38a5b 100644
>> > > --- a/arch/arm64/kvm/mmu.c
>> > > +++ b/arch/arm64/kvm/mmu.c
>> > > @@ -1022,9 +1022,15 @@ int kvm_handle_guest_abort(struct kvm_vcpu
>> > > *vcpu)
>> > >  		 * So let's assume that the guest is just being
>> > >  		 * cautious, and skip the instruction.
>> > >  		 */
>> > > -		if (kvm_is_error_hva(hva) && kvm_vcpu_dabt_is_cm(vcpu))
>> > {
>> > > -			kvm_incr_pc(vcpu);
>> > > -			ret = 1;
>> > > +		if (kvm_vcpu_dabt_is_cm(vcpu)) {
>> > > +			if (kvm_is_error_hva(hva)) {
>> > > +				kvm_incr_pc(vcpu);
>> > > +				ret = 1;
>> > > +				goto out_unlock;
>> > > +			}
>> > > +
>> > > +			kvm_err("Do cache maintenance in the read-only
>> > memory range\n");
>> >
>> > We don't scream on the console for guests bugs.
>> Ok
>> 
>> >
>> > > +			ret = -EFAULT;
>> > >  			goto out_unlock;
>> > >  		}
>> >
>> > And what is userspace going to do with that? To be honest, I'd rather
>> > not report it in any case:
>> >
>> > - either it isn't mapped, and there is no cache to clean/invalidate
>> > - or it is mapped read-only:
>> >    - if it is a "DC IVAC", the guest should get the fault as per
>> >      the ARM ARM. But I don't think we can identify the particular CMO
>> >      at this stage, so actually performing an invalidation is the least
>> >      bad thing to do.
>> >
>> > How about this (untested)?
> 
> I have tested for this. It works that DC ops can pass on memory range
> for rom. But there is performance issue. It takes too long a time that
> do DC on rom range compared with on  normal memory range. Here is some
> data:
> Ops                  memory type                                size
>                   time
> dc civac         rom memory                                128M
>        6700ms;
> dc civac       writable normal memory             128M                
> 300ms;
> 
> It's a single thread test and may be worse on multi thread. I'm not
> sure we can bear it. WDYT?

The problem is that the guest is invalidating one cache-line at
a time, but we invalidate 128M at a time in your example.

I would say that I really don't care how slow it is. We cannot know
which address the guest is trying to invalidate (as your commit
message shows, there is no syndrome information available).

So it seems our only choices are:
- don't do any invalidation, which is likely to break the guest
- invalidate everything, always

Given that, I'd rather have a slow guest. Also, it very much looks
like no existing SW does this, so I cannot say I care much.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel