> Am 21.12.2020 um 20:47 schrieb Eduardo Habkost <ehabk...@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 <ehabk...@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 <ehabk...@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 <vkuzn...@redhat.com>
>>>>>> Reviewed-by: Eduardo Habkost <ehabk...@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.
