Hi Peter,
Thanks very much for your review, I will check your comments in detail and reply.
On 2017/9/6 1:26, Peter Maydell wrote:
> On 18 August 2017 at 15:23, Dongjiu Geng <gengdongjiu@xxxxxxxxxx> wrote:
>> check if kvm supports guest RAS EXTENSION. if so, set
>> corresponding feature bit for vcpu.
>>
>> Signed-off-by: Dongjiu Geng <gengdongjiu@xxxxxxxxxx>
>> ---
>> linux-headers/linux/kvm.h | 1 +
>> target/arm/cpu.h | 3 +++
>> target/arm/kvm64.c | 8 ++++++++
>> 3 files changed, 12 insertions(+)
>>
>> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
>> index 7971a4f..2aa176e 100644
>> --- a/linux-headers/linux/kvm.h
>> +++ b/linux-headers/linux/kvm.h
>> @@ -929,6 +929,7 @@ struct kvm_ppc_resize_hpt {
>> #define KVM_CAP_PPC_SMT_POSSIBLE 147
>> #define KVM_CAP_HYPERV_SYNIC2 148
>> #define KVM_CAP_HYPERV_VP_INDEX 149
>> +#define KVM_CAP_ARM_RAS_EXTENSION 150
>>
>> #ifdef KVM_CAP_IRQ_ROUTING
>>
>
> Hi. Changes to linux-headers need to be done as a patch of their
> own created using scripts/update-linux-headers.sh run against a
> mainline kernel tree (and with a commit message that quotes the
> kernel commit hash used). This ensures that we have a consistent
> set of headers that don't diverge from the kernel copy.
>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index b39d64a..6b0961b 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -611,6 +611,8 @@ struct ARMCPU {
>>
>> /* CPU has memory protection unit */
>> bool has_mpu;
>> + /* CPU has ras extension unit */
>> + bool has_ras_extension;
>> /* PMSAv7 MPU number of supported regions */
>> uint32_t pmsav7_dregion;
>>
>> @@ -1229,6 +1231,7 @@ enum arm_features {
>> ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
>> ARM_FEATURE_PMU, /* has PMU support */
>> ARM_FEATURE_VBAR, /* has cp15 VBAR */
>> + ARM_FEATURE_RAS_EXTENSION, /*has RAS extension support */
>
> Missing space after '/*' ?
>
>> };
>>
>> static inline int arm_feature(CPUARMState *env, int feature)
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index a16abc8..0781367 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -518,6 +518,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
>> unset_feature(&env->features, ARM_FEATURE_PMU);
>> }
>>
>> + if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_RAS_EXTENSION)) {
>> + cpu->has_ras_extension = true;
>> + set_feature(&env->features, ARM_FEATURE_RAS_EXTENSION);
>> + } else {
>> + cpu->has_ras_extension = false;
>> + unset_feature(&env->features, ARM_FEATURE_RAS_EXTENSION);
>> + }
>> +
>
> Shouldn't we need to also tell the kernel that we actually want
> it to expose RAS to the guest? Compare the PMU code in this
> function, where we set a kvm_init_features bit to do this.
> (This suggests that your ABI for the kernel part of this feature
> may not be correct?)
>
> You should also not be calling set_feature() here -- if the
> CPU features bit doesn't say "this CPU should have the RAS
> extensions" we shouldn't create a CPU with them. Instead
> you should set it in kvm_arm_get_host_cpu_features() (again,
> compare the PMU code).
>
> thanks
> -- PMM
>
> .
>
