| Message ID | 71b0ae9d-baea-1f65-3c01-5d87f4f4746f@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Alex, On 30/09/16 20:31, Alexander Graf wrote: > > > On 30.09.16 17:43, Christoffer Dall wrote: >> On Fri, Sep 30, 2016 at 05:38:11PM +0200, Alexander Graf wrote: >>> >>> >>> On 30.09.16 16:54, Alexander Graf wrote: >>>> >>>> >>>> On 27.09.16 21:08, Christoffer Dall wrote: >>>>> Hi Alex, >>>>> >>>>> Marc and I have been looking at this during Linaro connect and have >>>>> slightly reworked your patch into this small series. >>>>> >>>>> It would be good if you could have a look at it and test it out. >>>>> >>>>> I've tested it with your QEMU, and it works for UP, but secondary CPUs >>>>> fail to come up, and it looks like the kernel never gets an IPI for >>>>> those CPUs from userspace. Any chance you're willing to take a look at >>>>> that? >>>> >>>> I still need to see whether I can come up with a prettier solution, but >>>> for now this works: >>>> >>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c >>> >>> Eh, no, not in i386 code :). But the problem seems to be a missing >>> mpstate sync. >>> >> Yeah, that looked really dodgy. Have you tested it? :) > > This time around tested with the correct command line parameters I hope > :). I'll send a pretty patch later. > > diff --git a/target-arm/kvm.c b/target-arm/kvm.c > index b4c8fe2..b549f00 100644 > --- a/target-arm/kvm.c > +++ b/target-arm/kvm.c > @@ -173,6 +173,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s) > */ > kvm_async_interrupts_allowed = true; > > + /* > + * PSCI wakes up secondary cores, so we always need to > + * have vCPUs waiting in kernel space > + */ > + kvm_halt_in_kernel_allowed = true; > + > cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE); > > type_register_static(&host_arm_cpu_type_info); What the status of userspace for this thing? Are QEMU patches being posted and reviewed? Thanks, M.
> Am 28.10.2016 um 16:38 schrieb Marc Zyngier <marc.zyngier@arm.com>: > > Alex, > >> On 30/09/16 20:31, Alexander Graf wrote: >> >> >>> On 30.09.16 17:43, Christoffer Dall wrote: >>>> On Fri, Sep 30, 2016 at 05:38:11PM +0200, Alexander Graf wrote: >>>> >>>> >>>>> On 30.09.16 16:54, Alexander Graf wrote: >>>>> >>>>> >>>>>> On 27.09.16 21:08, Christoffer Dall wrote: >>>>>> Hi Alex, >>>>>> >>>>>> Marc and I have been looking at this during Linaro connect and have >>>>>> slightly reworked your patch into this small series. >>>>>> >>>>>> It would be good if you could have a look at it and test it out. >>>>>> >>>>>> I've tested it with your QEMU, and it works for UP, but secondary CPUs >>>>>> fail to come up, and it looks like the kernel never gets an IPI for >>>>>> those CPUs from userspace. Any chance you're willing to take a look at >>>>>> that? >>>>> >>>>> I still need to see whether I can come up with a prettier solution, but >>>>> for now this works: >>>>> >>>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c >>>> >>>> Eh, no, not in i386 code :). But the problem seems to be a missing >>>> mpstate sync. >>>> >>> Yeah, that looked really dodgy. Have you tested it? :) >> >> This time around tested with the correct command line parameters I hope >> :). I'll send a pretty patch later. >> >> diff --git a/target-arm/kvm.c b/target-arm/kvm.c >> index b4c8fe2..b549f00 100644 >> --- a/target-arm/kvm.c >> +++ b/target-arm/kvm.c >> @@ -173,6 +173,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s) >> */ >> kvm_async_interrupts_allowed = true; >> >> + /* >> + * PSCI wakes up secondary cores, so we always need to >> + * have vCPUs waiting in kernel space >> + */ >> + kvm_halt_in_kernel_allowed = true; >> + >> cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE); >> >> type_register_static(&host_arm_cpu_type_info); > > What the status of userspace for this thing? Are QEMU patches being > posted and reviewed? I didn't see a notification that the patches were merged. Are they in Linus' tree yet? Then I can post enablement to qemu-devel. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/10/16 16:52, Alexander Graf wrote: > > >> Am 28.10.2016 um 16:38 schrieb Marc Zyngier <marc.zyngier@arm.com>: >> >> Alex, >> >>> On 30/09/16 20:31, Alexander Graf wrote: >>> >>> >>>> On 30.09.16 17:43, Christoffer Dall wrote: >>>>> On Fri, Sep 30, 2016 at 05:38:11PM +0200, Alexander Graf wrote: >>>>> >>>>> >>>>>> On 30.09.16 16:54, Alexander Graf wrote: >>>>>> >>>>>> >>>>>>> On 27.09.16 21:08, Christoffer Dall wrote: >>>>>>> Hi Alex, >>>>>>> >>>>>>> Marc and I have been looking at this during Linaro connect and have >>>>>>> slightly reworked your patch into this small series. >>>>>>> >>>>>>> It would be good if you could have a look at it and test it out. >>>>>>> >>>>>>> I've tested it with your QEMU, and it works for UP, but secondary CPUs >>>>>>> fail to come up, and it looks like the kernel never gets an IPI for >>>>>>> those CPUs from userspace. Any chance you're willing to take a look at >>>>>>> that? >>>>>> >>>>>> I still need to see whether I can come up with a prettier solution, but >>>>>> for now this works: >>>>>> >>>>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c >>>>> >>>>> Eh, no, not in i386 code :). But the problem seems to be a missing >>>>> mpstate sync. >>>>> >>>> Yeah, that looked really dodgy. Have you tested it? :) >>> >>> This time around tested with the correct command line parameters I hope >>> :). I'll send a pretty patch later. >>> >>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c >>> index b4c8fe2..b549f00 100644 >>> --- a/target-arm/kvm.c >>> +++ b/target-arm/kvm.c >>> @@ -173,6 +173,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s) >>> */ >>> kvm_async_interrupts_allowed = true; >>> >>> + /* >>> + * PSCI wakes up secondary cores, so we always need to >>> + * have vCPUs waiting in kernel space >>> + */ >>> + kvm_halt_in_kernel_allowed = true; >>> + >>> cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE); >>> >>> type_register_static(&host_arm_cpu_type_info); >> >> What the status of userspace for this thing? Are QEMU patches being >> posted and reviewed? > > I didn't see a notification that the patches were merged. Are they in > Linus' tree yet? Then I can post enablement to qemu-devel. I think you got it backward. I have no intention of merging them until I see a vague consensus on the userspace API, and a set of patches ready to be merged in QEMU. Thanks, M.
> Am 28.10.2016 um 17:57 schrieb Marc Zyngier <marc.zyngier@arm.com>: > >> On 28/10/16 16:52, Alexander Graf wrote: >> >> >>> Am 28.10.2016 um 16:38 schrieb Marc Zyngier <marc.zyngier@arm.com>: >>> >>> Alex, >>> >>>> On 30/09/16 20:31, Alexander Graf wrote: >>>> >>>> >>>>>> On 30.09.16 17:43, Christoffer Dall wrote: >>>>>> On Fri, Sep 30, 2016 at 05:38:11PM +0200, Alexander Graf wrote: >>>>>> >>>>>> >>>>>>> On 30.09.16 16:54, Alexander Graf wrote: >>>>>>> >>>>>>> >>>>>>>> On 27.09.16 21:08, Christoffer Dall wrote: >>>>>>>> Hi Alex, >>>>>>>> >>>>>>>> Marc and I have been looking at this during Linaro connect and have >>>>>>>> slightly reworked your patch into this small series. >>>>>>>> >>>>>>>> It would be good if you could have a look at it and test it out. >>>>>>>> >>>>>>>> I've tested it with your QEMU, and it works for UP, but secondary CPUs >>>>>>>> fail to come up, and it looks like the kernel never gets an IPI for >>>>>>>> those CPUs from userspace. Any chance you're willing to take a look at >>>>>>>> that? >>>>>>> >>>>>>> I still need to see whether I can come up with a prettier solution, but >>>>>>> for now this works: >>>>>>> >>>>>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c >>>>>> >>>>>> Eh, no, not in i386 code :). But the problem seems to be a missing >>>>>> mpstate sync. >>>>>> >>>>> Yeah, that looked really dodgy. Have you tested it? :) >>>> >>>> This time around tested with the correct command line parameters I hope >>>> :). I'll send a pretty patch later. >>>> >>>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c >>>> index b4c8fe2..b549f00 100644 >>>> --- a/target-arm/kvm.c >>>> +++ b/target-arm/kvm.c >>>> @@ -173,6 +173,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s) >>>> */ >>>> kvm_async_interrupts_allowed = true; >>>> >>>> + /* >>>> + * PSCI wakes up secondary cores, so we always need to >>>> + * have vCPUs waiting in kernel space >>>> + */ >>>> + kvm_halt_in_kernel_allowed = true; >>>> + >>>> cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE); >>>> >>>> type_register_static(&host_arm_cpu_type_info); >>> >>> What the status of userspace for this thing? Are QEMU patches being >>> posted and reviewed? >> >> I didn't see a notification that the patches were merged. Are they in >> Linus' tree yet? Then I can post enablement to qemu-devel. > > I think you got it backward. I have no intention of merging them until I > see a vague consensus on the userspace API, and a set of patches ready > to be merged in QEMU. That's not how kvm apis are made. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > What the status of userspace for this thing? Are QEMU patches being > > > > posted and reviewed? > > > > > > I didn't see a notification that the patches were merged. Are they in > > > Linus' tree yet? Then I can post enablement to qemu-devel. > > > > I think you got it backward. I have no intention of merging them until I > > see a vague consensus on the userspace API, and a set of patches ready > > to be merged in QEMU. > > That's not how kvm apis are made. Actually I think it's always been like this, depending on what Marc meant for "ready to be merged in QEMU". It doesn't make sense to merge KVM APIs without having userspace patches at least posted as RFC to qemu-devel, and without having at least a positive response from the QEMU architecture maintainer. ARM does require a bit more care because there's no overlap between kernel and userspace maintainers, so perhaps that's the source of the confusion? Now, of course merging the patches in QEMU may take a month or two depending on the timing (because you have to wait for the patches to be merged into Linus's tree and for the KVM headers to be updated in QEMU---which is not going to happen during freeze of course). So of course the KVM patch thus can be committed even if QEMU is in freeze, as long as the QEMU architecture maintainer gives an overall green light. In fact a couple weeks from now would be the perfect time to post the patches to QEMU. Then you can get a review from Peter once he's back from vacation (beginning of December), the KVM patches can easily make it for the 4.10 merge window (end of December), and the userspace patches will hopefully be merged in QEMU 2.9 in January 2017. Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 29 Oct 2016, at 15:19, Paolo Bonzini <pbonzini@redhat.com> wrote: > >>>>> What the status of userspace for this thing? Are QEMU patches being >>>>> posted and reviewed? >>>> >>>> I didn't see a notification that the patches were merged. Are they in >>>> Linus' tree yet? Then I can post enablement to qemu-devel. >>> >>> I think you got it backward. I have no intention of merging them until I >>> see a vague consensus on the userspace API, and a set of patches ready >>> to be merged in QEMU. >> >> That's not how kvm apis are made. > > Actually I think it's always been like this, depending on what Marc meant for > "ready to be merged in QEMU". It doesn't make sense to merge KVM APIs without > having userspace patches at least posted as RFC to qemu-devel, and without > having at least a positive response from the QEMU architecture maintainer. I halfway agree. I do agree that there needs to be a reference implementation that proves the API to make sense. That bit is referenced in the cover letter of the patch set. Peter as the QEMU architecture maintainer has been part of the review process and involved from the beginning. In fact, the current approach was his idea. Do we need to fly the loop over qemu-devel? I doubt it, but if it makes you happy I can post an RFC there too. > ARM does require a bit more care because there's no overlap between kernel > and userspace maintainers, so perhaps that's the source of the confusion? > > Now, of course merging the patches in QEMU may take a month or two depending > on the timing (because you have to wait for the patches to be merged into > Linus's tree and for the KVM headers to be updated in QEMU---which is not > going to happen during freeze of course). So of course the KVM patch thus > can be committed even if QEMU is in freeze, as long as the QEMU architecture > maintainer gives an overall green light. Right. My plan was to make sure that people agree on the KVM interface. Then directly send non-RFC patches to qemu-devel, which can only happen when the KVM patches are merged. I didn’t see any reason why that approach would be controversial, since everyone who really cared was involved. But again, I don’t care strongly enough to make a fuss. If people are happier with RFC patches on the ML rather than a github link to RFC patches, I’ll send them. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/target-arm/kvm.c b/target-arm/kvm.c index b4c8fe2..b549f00 100644 --- a/target-arm/kvm.c +++ b/target-arm/kvm.c @@ -173,6 +173,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s) */ kvm_async_interrupts_allowed = true; + /* + * PSCI wakes up secondary cores, so we always need to + * have vCPUs waiting in kernel space + */ + kvm_halt_in_kernel_allowed = true; + cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE); type_register_static(&host_arm_cpu_type_info);