Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types

[David Hildenbrand]

> Am 21.12.2020 um 20:47 schrieb Eduardo Habkost <ehabkost@redhat.com>:
> 
> +s390 maintainers, a question about feature groups below:
> 
>> On Mon, Dec 21, 2020 at 02:24:18PM +0100, Igor Mammedov wrote:
>> On Fri, 18 Dec 2020 13:07:21 -0500
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>> 
>>> On Fri, Dec 18, 2020 at 06:13:40PM +0100, Igor Mammedov wrote:
>>>> On Wed, 16 Dec 2020 15:52:02 -0500
>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>> 
>>>>> On Thu, Nov 19, 2020 at 11:32:21AM +0100, Vitaly Kuznetsov wrote:  
>>>>>> Enabling Hyper-V emulation for a Windows VM is a tiring experience as it
>>>>>> requires listing all currently supported enlightenments ("hv_*" CPU
>>>>>> features) explicitly. We do have a 'hv_passthrough' mode enabling
>>>>>> everything but it can't be used in production as it prevents migration.
>>>>>> 
>>>>>> Introduce a simple 'hyperv=on' option for all x86 machine types enabling
>>>>>> all currently supported Hyper-V enlightenments. Later, when new
>>>>>> enlightenments get implemented, we will be adding them to newer machine
>>>>>> types only (by disabling them for legacy machine types) thus preserving
>>>>>> migration.
>>>>>> 
>>>>>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>>>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>    
> [...]  
>>>>>> @@ -1194,6 +1208,16 @@ static void x86_machine_class_init(ObjectClass 
>>>>>> *oc, void *data)
>>>>>>     x86mc->save_tsc_khz = true;
>>>>>>     nc->nmi_monitor_handler = x86_nmi;
>>>>>> 
>>>>>> +    /* Hyper-V features enabled with 'hyperv=on' */
>>>>>> +    x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
>>>>>> +        BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
>>>>>> +        BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
>>>>>> +        BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
>>>>>> +        BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
>>>>>> +        BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) 
>>>>>> |
>>>>>> +        BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
>>>>>> +        BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);  
>>>> I'd argue that feature bits do not belong to machine code at all.
>>>> If we have to involve machine at all then it should be a set 
>>>> property/value pairs
>>>> that machine will set on CPU object (I'm not convinced that doing it
>>>> from machine code is good idea though).  
>>> 
>>> The set of default hyperv features needs be defined by the
>>> machine type somehow, we can't avoid that.
>>> 
>>> You are correct that the policy could be implemented using
>>> compat_props, but I don't think we should block a patch just
>>> because we're not using a pure QOM property-based interface to
>>> implement that.
>> I'm fine with 1-4/5 patches but not with this one.
>> With this patch I don't agree with inventing
>> special semantics to property handling when it could
>> be done in a typical and consistent way (especially for
>> the sake of convenience).
>> 
>> 
>>> We need the external interface to be good, though:
>>> 
>>>> 
>>> [...]
>>>>>> static void x86_cpu_hyperv_realize(X86CPU *cpu)
>>>>>> {
>>>>>> +    X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
>>>>>> +    X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
>>>>>> +    uint64_t feat;
>>>>>>     size_t len;
>>>>>> 
>>>>>> +    if (x86ms->hyperv_enabled) {
>>>>>> +        feat = x86mc->default_hyperv_features;
>>>>>> +        /* Enlightened VMCS is only available on Intel/VMX */
>>>>>> +        if (!cpu_has_vmx(&cpu->env)) {
>>>>>> +            feat &= ~BIT(HYPERV_FEAT_EVMCS);
>>>>>> +        }
>>>>>> +
>>>>>> +        cpu->hyperv_features |= feat;  
>>>> that will ignore features user explicitly doesn't want,
>>>> ex:
>>>> -machine hyperv=on -cpu foo,hv-foo=off  
>>> 
>>> Oops, good point.
>>> 
>>> 
>>>> 
>>>> not sure we would like to introduce such invariant,
>>>> in normal qom property handling the latest set property should have effect
>>>> (all other invariants we have in x86 cpu property semantics are comming 
>>>> from legacy handling
>>>> and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs 
>>>> will behave like
>>>> any other QOM object when it come to property handling)
>>>> 
>>>> anyways it's confusing a bit to have cpu flags to come from 2 different 
>>>> places
>>>> 
>>>> -cpu hyperv-use-preset=on,hv-foo=off
>>>> 
>>>> looks less confusing and will heave expected effect
>>>> 
>>>>>> +    }    
>>>>> 
>>>>> I had to dequeue this because it doesn't compile with
>>>>> CONFIG_USER_ONLY:
>>>>> 
>>>>> https://gitlab.com/ehabkost/qemu/-/jobs/916651017
>>>>> 
>>>>> The easiest solution would be to wrap the new code in #ifndef
>>>>> CONFIG_USER_ONLY, but maybe we should try to move all
>>>>> X86Machine-specific code from cpu.c to
>>>>> hw/i386/x86.c:x86_cpu_pre_plug().  
>>>> this looks to me like a preset of feature flags that belongs to CPU,
>>>> and machine code here only as a way to version subset of CPU features.
>>>> 
>>>> Is there a way to implement it without modifying machine?  
>>> 
>>> Maybe there is, but why modifying machine is a problem?
>> 
>> 1. it doesn't let do the job properly (realize time is too late)
>> 2. unnecessarily pushes CPU specific logic to machine code,
>>   it just doesn't belong there.
>>   Sure we can do that here, then some where else and in the end
>>   code becomes unmanageable mess.
>> 
>>> I agree the interface needs to be clear and consistent, though.
>>> Maybe making it a -cpu option would make this clearer and more
>>> consistent.
>>> 
>>>> 
>>>> for example versioned CPUs or maybe something like this:
>>>> 
>>>> for CLI:
>>>> -cpu hyperv-use-preset=on,hv-foo=off  
>>> 
>>> In either case, we must clearly define what should happen if the
>>> preset is (HYPERV_FEAT_X | HYPERV_FEAT_Y), and the command line
>>> has:
>>> 
>>>  -cpu foo,hv-A=on,hv-X=off,hyperv-use-preset=on,hv-B=on,hv-Y=off
>> 
>> current x86 cpu code (it doesn't have typical properties handling
>> for keeping legacy semantics), it will basically reorder all features
>> with 'off' value to the end, so hv-X=off will still have an effect.
>> 
>> However I plan to deprecate those reordering semantics (x86/sparc cpus),
>> to make it consistent with typical property handling
>> (last set value overwrites any previously set one).
>> 
>> That will let us drop custom parsing of -cpu (quite a bit of code) and
>> more importantly make it consistent with -device/device_add cpu-foo.
> 
> Right.
> 
>> 
>> 
>>> or:
>>> 
>>>  -machine hyperv=on -cpu foo,hv-A=on,hv-X=off,hv-B=on,hv-X=off
>>> 
>>> Personally, I don't care what the rules are, as long as: 1) they
>>> are clearly defined and documented; 2) they support the use cases
>>> we need to support.
>> 
>> I'd like to stick with typical property handling rules, and resort to
>> inventing/using other invariant only if there is no other choice.
> 
> What would be the typical handling rules, in this case?  I don't
> remember other cases in x86 where a single property affects
> multiple feature flags.
> 
> We have something similar on s390x, though.  So, a question to
> s390x maintainers:
> 
> If "G" is a feature group including the features X, Y, Z, what is
> the result of:
> 
>   -cpu foo,X=off,G=on,Y=off
> 
> Would X be enabled?  Would Y be enabled?
> 
> I would expect X to be enabled and Y to be disabled, but I'd like
> to confirm.

IIRC, the last one wins. Properties are applied left to right.