Re: [PATCH v4 5/6] i386: Hyper-V VMBus ACPI DSDT entry

[Maciej S. Szmigiero]

On 15.06.2020 04:40, Jon Doron wrote:
> On 14/06/2020, Maciej S. Szmigiero wrote:
>> Hi Jon,
>>
>> On 14.06.2020 16:11, Jon Doron wrote:
>>> On 28/05/2020, Jon Doron wrote:
>>>> On 28/05/2020, Igor Mammedov wrote:
>>>>> On Thu, 28 May 2020 08:26:42 +0300
>>>>> Jon Doron <arilou@gmail.com> wrote:
>>>>>
>>>>>> On 22/05/2020, Igor Mammedow wrote:
>>>>>>> On Thu, 21 May 2020 18:02:07 +0200
>>>>>>> Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>>>
>>>>>>>> On 13/05/20 17:34, Igor Mammedov wrote:
>>>>>>>> > I'd rather avoid using random IRQ numbers (considering we are
>>>>>>>> > dealing with black-box here). So if it's really necessary to have
>>>>>>>> > IRQ described here, I'd suggest to implement them in device model
>>>>>>>> > so they would be reserved and QEMU would error out in a sane way if
>>>>>>>> > IRQ conflict is detected.
>>>>>>>>
>>>>>>>> We don't generally detect ISA IRQ conflicts though, do we?
>>>>>>>
>>>>>>> that I don't know that's why I'm not suggesting how to do it.
>>>>>>> The point is hard-coding in AML random IRQs is not right thing to do,
>>>>>>> (especially with the lack of 'any' spec), as minimum AML should pull
>>>>>>> it from device model and that probably should be configurable and set
>>>>>>> by board.
>>>>>>>
>>>>>>> Other thing is:
>>>>>>> I haven't looked at VMBus device model in detail, but DSDT part aren't
>>>>>>> matching device though (device model is not ISA device hence AML part
>>>>>>> shouldn't be on in ISA scope), where to put it is open question.
>>>>>>> There were other issues with AML code, I've commented on, so I was
>>>>>>> waiting on respin with comments addressed.
>>>>>>> I don't think that this patch is good enough for merging.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> But it seems like the current patch does match what's Microsoft HyperV
>>>>>> is publishing in it's APCI tables.
>>>>>>
>>>>>> I dont think it's correct for us to "fix" Microsoft emulation even if
>>>>>> it's wrong, since that's what Windows probably expects to see...
>>>>>>
>>>>>> I tried looking where Microsoft uses the ACPI tables to identify the
>>>>>> VMBus but without much luck in order to understand how flexible a change
>>>>>> would be for the OS to still detect the VMBus device, but in general
>>>>>> I think "correcting" something that is emulated 1:1 because there is no
>>>>>> spec is the right way.
>>>>>
>>>>> I'd agree, if removing nonsense would break VMBus detection (does it?).
>>>>> if something is that doesn't make sense but has to stay because it is need
>>>>> to make windows happy, that's fine , just add annotate is with comment,
>>>>> so it won't confuse anyone why that code exists there later on.
>>>>>
>>>>> I suggest to:
>>>>> 1. try dropping _PS* & _STA as it doesn't actually does anything and _PS3 
>>>>> is plain wrong
>>>>> 2. drop one IRQ, newer hyper-v seems to be doing fine with only one
>>>>> 3. it's not ISA device, I'd suggest to move into _SB scope
>>>>> 4. I don't know much about IRQs but
>>>>>      git grep DEFINE_PROP_ | grep -i iqr
>>>>>   yields nothing so I'm not sure if it's acceptable. Typically it's board 
>>>>> that assigns
>>>>>   IRQ and not device, for Sysbus devices (see: 
>>>>> sysbus_init_irq/sysbus_connect_irq).
>>>>>   So I'd leave it upto Paolo or someone else to decide/comment on.
>>>>>
>>>>
>>>> Sounds like a plan, I'll try to come up with the test results
>>>> (at least for Windows 10 guest which is  what I have setup) and update
>>>> this thread with the results.
>>>>
>>>> -- Jon.
>>>>
>>>>>>
>>>>>>>>
>>>>>>>> Paolo
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>> Hi guys,
>>>
>>> Sorry for the delay...
>>>
>>> So first ill clarify what was the test, the test was to see the device
>>> "Microsoft Hyper-V Virtual Machine Bus" in Windows Device Manager under
>>> "System devices" with a state of "working properly".
>>>
>>> It seems like it's ok to drop all the _PS* and _STA.
>>>
>>> It seems to be functioning with single IRQ as well, it is worth noting that 
>>> even when i dropped the entire _CRS (so no IRQs resources are required, the 
>>> device was still showing that it's functioning, but I suspect this might 
>>> affect the child devices like hv-net and hv-scsi).
>>
>> I guess you tested a single Windows version, correct?
>> It may be that requirements differ between Windows versions, just as you
>> say below about the required enlightenments.
>>
> 
> Yes I have tested only a single version, but from the looks of it there is 
> only a single IRQ in the HyperV APCI table that I have looked at, and I 
> believe the one you have pasted as well in the past, so that change sounds 
> reasonable to me.

I meant also other changes, like dropping of some ACPI methods.

> As for the enlightenments dont you prefer to have all those as mandatory for 
> VMBus in order not to run into the issue I have encountered?

Well, if somebody wants to run only older guests they will be content with
just a limited set of enlightenments, right?

But I don't know what's the QEMU project policy is here.

>>> With that said I did run into a small issue I set-up Win10 1903 (aka 19H1) 
>>> and it seems like VMBus now requires to have the following features enabled:
>>> HV_VP_RUNTIME_AVAILABLE
>>> HV_TIME_REF_COUNT_AVAILABLE
>>> HV_SYNIC_AVAILABLE
>>> HV_SYNTIMERS_AVAILABLE
>>> HV_APIC_ACCESS_AVAILABLE
>>> HV_HYPERCALL_AVAILABLE
>>> HV_VP_INDEX_AVAILABLE
>>>
>>> So notice that previously only SYNIC and VPINDEX was needed, now you need 
>>> the whole thing so you need to run qemu with something like
>>> -cpu 
>>> host,hv-relaxed,hv_spinlocks=0x1fff,hv_time,hv-vapic,hv-vpindex,hv-synic,hv-runtime,hv-stimer
>>>
>>> The validation was done in winhv!WinHvpCheckPartitionPrivileges .
>>>
>>> Paolo I noticed you have done a PULL request, would you like to wait on it 
>>> and we will submit a version with a single IRQ (selectable by user 
>>> property) and go with Igor's suggestion dropping _PS* and _STA (though like 
>>> I said before I prefer to mimic the original HyperV with it's bugs, but 
>>> I'll leave this decision to you).
>>
>> The code is already in the upstream QEMU tree, it's a known-working code,
>> so I think it is better to simply work incrementally on further improving
>> the current version rather than backing it out and merging it again later.
>>
>> This way it will (hopefully) get some wider testing sooner.
>>
>> Not to mention that it is less likely for some other QEMU change to
>> accidentally break it.
>>
> 
> That's great I agree incrementally is the right way here, should we open 
> another Thread to discuss all these changes or do you prefer we will keep 
> going on this one?

If that's going to be an incremental change it is probably worth its
own email thread.
Otherwise the existing one will grow without bounds.

>>> Also today VMBus only verifies SYNIC is enabled I'm not sure how but I 
>>> wonder if we want to some how exports from the CPU which other HV features 
>>> are enabled so we can verify all the required ones are set, would 
>>> appreciate if you have any suggestions here.
>>>
> 
> Thanks,
> -- Jon.

Thanks,
Maciej