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

[Jon Doron <arilou@gmail.com>]

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.

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?

>> 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?

>> 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.
>
>Thanks,
>Maciej

Thanks,
-- Jon.