Re: [RFC v12 51/65] target/arm: cpu-sve: split TCG and KVM functionality

[Claudio Fontana]

On 3/28/21 8:21 PM, Richard Henderson wrote:
> On 3/26/21 1:36 PM, Claudio Fontana wrote:
>> +++ b/target/arm/kvm/kvm-sve.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * QEMU AArch64 CPU SVE KVM interface
>> + *
>> + * Copyright 2021 SUSE LLC
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef KVM_SVE_H
>> +#define KVM_SVE_H
>> +
>> +/* note: SVE is an AARCH64-only option, only include this for 
>> TARGET_AARCH64 */
> 
> A pointless comment given that kvm itself is aarch64-only.
> 
>> -void cpu_sve_finalize_features(ARMCPU *cpu, Error **errp)
>> +bool cpu_sve_finalize_features(ARMCPU *cpu, Error **errp)
> 
> The commit message does not mention the interface change here.


Ok, would need the two-steps here too.


> 
> I'm not sure about the rest of this patch.  It saves a miniscule amount of 
> code 
> in a kvm-only build, but I don't know that it clarifies things at all.


I would say that it does, if only by making the code flow easier to follow.


> 
> As yet, the other arm hw accelerators do not support SVE, but I assume that's 
> only a matter of time.  The ARM Neoverse cpus support SVE, and will require 
> Xen 
> to have an answer soon.  (Apple will do whatever Apple does, given that it 
> fabs 
> its own ARM cpus, but I expect they won't delay SVE forever.)
> 
> It's not clear to me what bits of the kvm code here is really kvm specific, 
> or 
> if we'll have to move it back.
> 
> I'm tempted to leave it alone for now.


But fair enough, if nobody speaks up and finds them useful other than me, I 
would drop this from the series.


> 
> 
> r~
>