On 05/05/2020 15:50, David Gibson wrote:
> On Tue, May 05, 2020 at 10:56:17AM +1000, Alexey Kardashevskiy wrote:
>>
>>
>> On 04/05/2020 21:30, Greg Kurz wrote:
>>> On Fri, 17 Apr 2020 14:11:05 +1000
>>> Alexey Kardashevskiy <a...@ozlabs.ru> wrote:
>>>
>>>> At the moment the VCPU init sequence includes setting PVR which in case of
>>>> KVM-HV only checks if it matches the hardware PVR mask as PVR cannot be
>>>> virtualized by the hardware. In order to cope with various CPU revisions
>>>> only top 16bit of PVR are checked which works for minor revision updates.
>>>>
>>>> However in every CPU generation starting POWER7 (at least) there were CPUs
>>>> supporting the (almost) same POWER ISA level but having different top
>>>> 16bits of PVR - POWER7+, POWER8E, POWER8NVL; this time we got POWER9+
>>>> with a new PVR family. We would normally add the PVR mask for the new one
>>>> too, the problem with it is that although the physical machines exist,
>>>> P9+ is not going to be released as a product, and this situation is likely
>>>> to repeat in the future.
>>>>
>>>> Instead of adding every new CPU family in QEMU, this adds a new sPAPR
>>>> machine capability to force PVR setting/checking. It is "on" by default
>>>> to preserve the existing behavior. When "off", it is the user's
>>>> responsibility to specify the correct CPU.
>>>>
>>>
>>> I don't quite understand the motivation for this... what does this
>>> buy us ?
>>
>> I answered that part in another mail in this thread, shortly this is to
>> make QEMU work with HV KVM on unknown-to-QEMU CPU family (0x004f0000).
>>
>>
>>>
>>>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>>>> ---
>>>> include/hw/ppc/spapr.h | 5 ++++-
>>>> hw/ppc/spapr.c | 1 +
>>>> hw/ppc/spapr_caps.c | 18 ++++++++++++++++++
>>>> target/ppc/kvm.c | 16 ++++++++++++++--
>>>> 4 files changed, 37 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>> index e579eaf28c05..5ccac4d56871 100644
>>>> --- a/include/hw/ppc/spapr.h
>>>> +++ b/include/hw/ppc/spapr.h
>>>> @@ -81,8 +81,10 @@ typedef enum {
>>>> #define SPAPR_CAP_CCF_ASSIST 0x09
>>>> /* Implements PAPR FWNMI option */
>>>> #define SPAPR_CAP_FWNMI 0x0A
>>>> +/* Implements PAPR PVR option */
>>>> +#define SPAPR_CAP_PVR 0x0B
>>>> /* Num Caps */
>>>> -#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI + 1)
>>>> +#define SPAPR_CAP_NUM (SPAPR_CAP_PVR + 1)
>>>>
>>>> /*
>>>> * Capability Values
>>>> @@ -912,6 +914,7 @@ extern const VMStateDescription
>>>> vmstate_spapr_cap_nested_kvm_hv;
>>>> extern const VMStateDescription vmstate_spapr_cap_large_decr;
>>>> extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>>>> extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>>>> +extern const VMStateDescription vmstate_spapr_cap_pvr;
>>>>
>>>> static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>>>> {
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 841b5ec59b12..ecc74c182b9f 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -4535,6 +4535,7 @@ static void spapr_machine_class_init(ObjectClass
>>>> *oc, void *data)
>>>> smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>>>> smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>>>> smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
>>>> + smc->default_caps.caps[SPAPR_CAP_PVR] = SPAPR_CAP_ON;
>>>> spapr_caps_add_properties(smc, &error_abort);
>>>> smc->irq = &spapr_irq_dual;
>>>> smc->dr_phb_enabled = true;
>>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>>>> index eb54f9422722..398b72b77f9f 100644
>>>> --- a/hw/ppc/spapr_caps.c
>>>> +++ b/hw/ppc/spapr_caps.c
>>>> @@ -525,6 +525,14 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr,
>>>> uint8_t val,
>>>> }
>>>> }
>>>>
>>>> +static void cap_pvr_apply(SpaprMachineState *spapr, uint8_t val, Error
>>>> **errp)
>>>> +{
>>>> + if (val) {
>>>> + return;
>>>> + }
>>>> + warn_report("If you're uing kvm-hv.ko, only \"-cpu host\" is
>>>> supported");
>>>> +}
>>>> +
>>>> SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>>> [SPAPR_CAP_HTM] = {
>>>> .name = "htm",
>>>> @@ -633,6 +641,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] =
>>>> {
>>>> .type = "bool",
>>>> .apply = cap_fwnmi_apply,
>>>> },
>>>> + [SPAPR_CAP_PVR] = {
>>>> + .name = "pvr",
>>>> + .description = "Enforce PVR in KVM",
>>>> + .index = SPAPR_CAP_PVR,
>>>> + .get = spapr_cap_get_bool,
>>>> + .set = spapr_cap_set_bool,
>>>> + .type = "bool",
>>>> + .apply = cap_pvr_apply,
>>>> + },
>>>> };
>>>>
>>>> static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
>>>> @@ -773,6 +790,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv,
>>>> SPAPR_CAP_NESTED_KVM_HV);
>>>> SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>>>> SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>>>> SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
>>>> +SPAPR_CAP_MIG_STATE(pvr, SPAPR_CAP_PVR);
>>>>
>>>> void spapr_caps_init(SpaprMachineState *spapr)
>>>> {
>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>> index 03d0667e8f94..a4adc29b6522 100644
>>>> --- a/target/ppc/kvm.c
>>>> +++ b/target/ppc/kvm.c
>>>> @@ -466,15 +466,27 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>>> PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>> CPUPPCState *cenv = &cpu->env;
>>>> int ret;
>>>> + SpaprMachineState *spapr;
>>>>
>>>
>>> We generally try to avoid adding such explicit dependencies to the
>>> machine code within the target directory... A virtual hypervisor
>>> hook could possibly do the trick but this would require to set
>>> PowerPCCPU::vhyp before kvm_arch_init_vcpu() gets called, eg.
>>> when the vCPU is created in spapr_create_vcpu() rather than
>>> when it gets realized.
>>
>> The only thing kvm_arch_sync_sregs() does is setting PVR, and it does
>> not do even that for Book3E so there is dependency already, mine at
>> least explicit :)
>>
>> I am really not sure what setting PVR buys us, KVM PR will take
>> anything,
>
> PR will take anything, but it changes the behaviour. Basically PR
> will try to match its behaviour to the PVR you specify. It can't
> entirely succeeed in all cases, of course, but that's PR for you.
>
> From an API point of view, the idea is that setting the PVR here
> specifies the model you want the vcpu to appear to be. But, KVM is
> free to say "can't do that".
Except pseries does not care what the CPU appears to be and uses
KVM_REG_PPC_ARCH_COMPAT instead.
So, is this a no? Thanks,
>
>> KVM HV will only take the same family PVR and reject others
>> while it could still run more configurations, let's say -cpu POWER8 on
>> actual POWER9 machine. Dunno. The capability seems a harmless way of
>> relaxing this restriction. Thanks,
>>
>>
>>
>>
>>>
>>>> /* Synchronize sregs with kvm */
>>>> ret = kvm_arch_sync_sregs(cpu);
>>>> if (ret) {
>>>> if (ret == -EINVAL) {
>>>> error_report("Register sync failed... If you're using
>>>> kvm-hv.ko,"
>>>> - " only \"-cpu host\" is possible");
>>>> + " only \"-cpu host\" is supported");
>>>> + }
>>>> + /*
>>>> + * The user chose not to set PVR which makes sense if we are
>>>> running
>>>> + * on a CPU with known ISA level but unknown PVR.
>>>> + */
>>>> + spapr = (SpaprMachineState *)
>>>> + object_dynamic_cast(OBJECT(qdev_get_machine()),
>>>> TYPE_SPAPR_MACHINE);
>>>> +
>>>> + if (spapr && spapr->eff.caps[SPAPR_CAP_PVR] == SPAPR_CAP_OFF) {
>>>> + ret = 0;
>>>> + } else {
>>>> + return ret;
>>>> }
>>>> - return ret;
>>>> }
>>>>
>>>> switch (cenv->mmu_model) {
>>>
>>
>
--
Alexey
