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 <ari...@gmail.com> wrote: >>>>> >>>>>> On 22/05/2020, Igor Mammedow wrote: >>>>>>> On Thu, 21 May 2020 18:02:07 +0200 >>>>>>> Paolo Bonzini <pbonz...@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
