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

[Jon Doron <arilou@gmail.com>]

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).

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).

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.

Cheers,
-- Jon.