| Message ID | 20220107185512.25321-6-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMX support for KVM | expand |
On Fri, Jan 07, 2022 at 01:54:56PM -0500, Paolo Bonzini wrote: > From: Jing Liu <jing2.liu@intel.com> > > vCPU threads are different from native tasks regarding to the initial XFD > value. While all native tasks follow a fixed value (init_fpstate::xfd) > established by the FPU core at boot, vCPU threads need to obey the reset > value (i.e. ZERO) defined by the specification, to meet the expectation of > the guest. > > Let the caller supply an argument and adjust the host and guest related > invocations accordingly. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> If Jing is author, then tglx's SOB should come after Jing's to mean, tglx handled it further. As it is now, it looks wrong. Ditto for patches 10, 11, 12, 13. Also, I wonder if all those Signed-off-by's do mean "handled" or Co-developed-by but I haven't tracked that particular pile so... > Signed-off-by: Jing Liu <jing2.liu@intel.com> > Signed-off-by: Yang Zhong <yang.zhong@intel.com> > Message-Id: <20220105123532.12586-6-yang.zhong@intel.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> From: Borislav Petkov <bp@alien8.de> > Sent: Saturday, January 8, 2022 3:44 AM > > On Fri, Jan 07, 2022 at 01:54:56PM -0500, Paolo Bonzini wrote: > > From: Jing Liu <jing2.liu@intel.com> > > > > vCPU threads are different from native tasks regarding to the initial XFD > > value. While all native tasks follow a fixed value (init_fpstate::xfd) > > established by the FPU core at boot, vCPU threads need to obey the reset > > value (i.e. ZERO) defined by the specification, to meet the expectation of > > the guest. > > > > Let the caller supply an argument and adjust the host and guest related > > invocations accordingly. > > > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > > If Jing is author, then tglx's SOB should come after Jing's to mean, > tglx handled it further. > > As it is now, it looks wrong. Thanks for pointing it out! Actually this is one area which we didn't get a clear answer from 'submitting-patches.rst' and now possibly a good chance to get it clarified. This patch was handled in an interesting way due to Jing's two absences: Internal version: Jing was the original author Signed-off-by: Jing Liu <jing2.liu@intel.com> --- Jing's first absence --- v1: Yang was the submitter: Signed-off-by: Jing Liu <jing2.liu@intel.com> Signed-off-by: Yang Zhong <yang.zhong@intel.com> Thomas updated it in his personal branch when reviewing v1: From: Jing Liu <jing2.liu@intel.com> Signed-off-by: Jing Liu <jing2.liu@intel.com> Signed-off-by: Yang Zhong <yang.zhong@intel.com> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- Jing was back --- v2-v3: Jing was the submitter: The open here is whether Jing should change the SoB order: Signed-off-by: Yang Zhong <yang.zhong@intel.com> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Jing Liu <jing2.liu@intel.com> or just append her name again: Signed-off-by: Jing Liu <jing2.liu@intel.com> Signed-off-by: Yang Zhong <yang.zhong@intel.com> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Jing Liu <jing2.liu@intel.com> The former was selected for this patch. --- Jing's 2nd absence --- v4-v5: Yang was the submitter. SoB order was partially changed (for Yang's SoB) but forgot to make Jing's SoB as the first. It should be: From: Jing Liu <jing2.liu@intel.com> Signed-off-by: Jing Liu <jing2.liu@intel.com> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Yang Zhong <yang.zhong@intel.com> If the rule is that SoB order should not be changed, then it will be: From: Jing Liu <jing2.liu@intel.com> Signed-off-by: Jing Liu <jing2.liu@intel.com> Signed-off-by: Yang Zhong <yang.zhong@intel.com> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Jing Liu <jing2.liu@intel.com> Signed-off-by: Yang Zhong <yang.zhong@intel.com> > > Ditto for patches 10, 11, 12, 13. > > Also, I wonder if all those Signed-off-by's do mean "handled" or > Co-developed-by but I haven't tracked that particular pile so... > > > Signed-off-by: Jing Liu <jing2.liu@intel.com> > > Signed-off-by: Yang Zhong <yang.zhong@intel.com> > > Message-Id: <20220105123532.12586-6-yang.zhong@intel.com> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Jan 10, 2022 at 05:15:44AM +0000, Tian, Kevin wrote: > Thanks for pointing it out! Actually this is one area which we didn't get > a clear answer from 'submitting-patches.rst' Are you sure? I see "Any further SoBs (Signed-off-by:'s) following the author's SoB are from people handling and transporting the patch, but were not involved in its development. SoB chains should reflect the **real** route a patch took as it was propagated to the maintainers and ultimately to Linus, with the first SoB entry signalling primary authorship of a single author." Now, when you read that paragraph, what do you think is the answer to your question and why? And if that paragraph doesn't make it clear, we would have to improve it... Thx.
On 1/10/22 09:52, Borislav Petkov wrote: > On Mon, Jan 10, 2022 at 05:15:44AM +0000, Tian, Kevin wrote: >> Thanks for pointing it out! Actually this is one area which we didn't get >> a clear answer from 'submitting-patches.rst' > > Are you sure? I see > > "Any further SoBs (Signed-off-by:'s) following the author's SoB are from > people handling and transporting the patch, but were not involved in its > development. SoB chains should reflect the **real** route a patch took > as it was propagated to the maintainers and ultimately to Linus, with > the first SoB entry signalling primary authorship of a single author." Say a patch went A->B->C->A->D and all of {A,B,C} were involved in the development at different times. The above text says "any further SoBs are from people not involved in its development", in other words it doesn't cover the case of multiple people handling different versions of a patch submission. The only clear thing from the text would be "do not remove/move the author's Signed-off-by", but apart from that it's wild wild west and there are contradictions everywhere. For example: 1) checkpatch.pl wants "Co-developed-by" to be immediately followed by "Signed-off-by". Should we imply that all SoB entries preceded by Co-developed-by do not exactly reflect the route that the patch took (since there could be multiple back and forth)? 2) if the author sends the patches but has co-developers, should they be first (because they're the author) or last (because they're the one actually sending the patch out)? Any consistent rules that I could come up with are too baroque to be practical: 1) a sequence consisting of {SoB,Co-developed-by,SoB} does not necessarily reflect a chain from the first signoff to the second signoff 2) if you are a maintainer committing a patch so that it will go to Linus, just add your SoB line. 3) if you pick up someone else's branch or posted series, and you are not in the existing SoB chain, you must add a Co-developed-by and SoB line for yourself. Do not use the document the changes in brackets: that is only done by the maintainers when they make changes and do not repost for review. The maintainers must already have a bad case of Stockholm syndrome for not having automated this kind of routine check, but it would be even worse if we were to inflict this on the developers. In the end, IMHO the real rules that matter are: - there should be a SoB line for the author - the submitter must always have the last SoB line - SoB lines shall never be removed - maintainers should prefer merge commits when moving commits from one tree to the other - merge commits should have a SoB line too Everything else, including the existence of Co-developed-by lines, is an unnecessary complication. Paolo
On Mon, Jan 10, 2022 at 03:18:26PM +0100, Paolo Bonzini wrote: > Say a patch went A->B->C->A->D and all of {A,B,C} were involved in the > development at different times. The above text says "any further SoBs are > from people not involved in its development", in other words it doesn't > cover the case of multiple people handling different versions of a patch > submission. Well, if I'm reading Kevin's mail correctly, it sounds like Thomas updated it (and I'm pretty sure he doesn't care about Co-developed-by) and the rest of the folks simply handled it. In the interest of remaining practical, I'd say one doesn't have to add Co-developed-by when one is doing contextual changes or addressing minor review feedback, or, of course, simply handling the patch as part of a series. > The only clear thing from the text would be "do not remove/move the > author's Signed-off-by", but apart from that it's wild wild west and > there are contradictions everywhere. The main point in that paragraph is that the SOB chain needs to denote how that patch went upstream and who handled it on the way. So you can't simply shorten or reorder the SOBs. What is more, even if you cherry-pick a patch which doesn't have your SOB at the end - for example, tglx applies a patch and I cherry-pick it into a different branch - I must add my SOB because, well, I handled it. > For example: > > 1) checkpatch.pl wants "Co-developed-by" to be immediately followed by > "Signed-off-by". Should we imply that all SoB entries preceded by > Co-developed-by do not exactly reflect the route that the patch took (since > there could be multiple back and forth)? Co-developed-by is to express multiple-people's authorship. And that rule checkpatch enforces is already in submitting-patches: "Since Co-developed-by: denotes authorship, every Co-developed-by: must be immediately followed by a Signed-off-by: of the associated co-author." > > 2) if the author sends the patches but has co-developers, should they be > first (because they're the author) or last (because they're the one actually > sending the patch out)? That is also explained there: "Standard sign-off procedure applies, i.e. the ordering of Signed-off-by: tags should reflect the chronological history of the patch insofar as possible, regardless of whether the author is attributed via From: or Co-developed-by:. Notably, the last Signed-off-by: must always be that of the developer submitting the patch." > Any consistent rules that I could come up with are too baroque to be > practical: > > 1) a sequence consisting of {SoB,Co-developed-by,SoB} does not necessarily > reflect a chain from the first signoff to the second signoff Right, if you want to attribute co-authorship too - at least I think this is what you mean - then you can do that according to the last snippet I pasted. IOW, you'd need to do: Co-developed-by: A SOB: A Co-developed-by: B SOB: B SOB: C etc. Or remain practical and do only an SOB chain. But it all depends on who has co-authored what and what kind of attribution the co-authors/handlers prefer. > 2) if you are a maintainer committing a patch so that it will go to Linus, > just add your SoB line. Ack. > 3) if you pick up someone else's branch or posted series, and you are not in > the existing SoB chain, you must add a Co-developed-by and SoB line for > yourself. Just SOB, as stated above. Because you handled the patch. > The maintainers must already have a bad case of Stockholm syndrome for not > having automated this kind of routine check, but it would be even worse if > we were to inflict this on the developers. Well, usually, the SOB chain is pretty simple and straight-forward. In this particular case, I'd simply add my SOB when I've handled the patch. If I've done big changes and I think I'd like to be listed as an author, I'd do Co-developed but other than that, just the SOB. > In the end, IMHO the real rules > that matter are: > > - there should be a SoB line for the author Yap. > - the submitter must always have the last SoB line Yap. > - SoB lines shall never be removed And their order should be kept too as the path upstream is important, obviously. > - maintainers should prefer merge commits when moving commits from one tree > to the other It depends. > - merge commits should have a SoB line too Yap, we do that in the tip tree and document the reason for the merge commit. > Everything else, including the existence of Co-developed-by lines, is an > unnecessary complication. See above. Thx.
On 1/10/22 16:25, Borislav Petkov wrote: > "Standard sign-off procedure applies, i.e. the ordering of > Signed-off-by: tags should reflect the chronological history of > the patch insofar as possible, regardless of whether the author > is attributed via From: or Co-developed-by:. Notably, the last > Signed-off-by: must always be that of the developer submitting the > patch." So this means that "the author must be the first SoB" is not an absolute rule. In the case of this patch we had: From: Jing Liu <jing2.liu@intel.com> ... Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Jing Liu <jing2.liu@intel.com> Signed-off-by: Yang Zhong <yang.zhong@intel.com> and the possibilities could be: 1) have two SoB lines for Jing (before and after Thomas) 2) add a Co-developed-by for Thomas as the first line 3) do exactly what the gang did ("remain practical and do only an SOB chain") Paolo
On Mon, Jan 10, 2022 at 04:55:01PM +0100, Paolo Bonzini wrote: > So this means that "the author must be the first SoB" is not an absolute > rule. In the case of this patch we had: > > From: Jing Liu <jing2.liu@intel.com> > ... > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Jing Liu <jing2.liu@intel.com> > Signed-off-by: Yang Zhong <yang.zhong@intel.com> Looking at Kevin's explanation, that should be: Signed-off-by: Jing Liu <jing2.liu@intel.com> # author Signed-off-by: Yang Zhong <yang.zhong@intel.com> # v1 submitter Signed-off-by: Thomas Gleixner <tglx@linutronix.de> # handler/reviewer Signed-off-by: Jing Liu <jing2.liu@intel.com> # v2-v3 submitter Signed-off-by: Yang Zhong <yang.zhong@intel.com> # v4-v5 submitter > and the possibilities could be: > > 1) have two SoB lines for Jing (before and after Thomas) > > 2) add a Co-developed-by for Thomas as the first line If Thomas would prefer. But then it becomes: Signed-off-by: Jing Liu <jing2.liu@intel.com> # author Signed-off-by: Yang Zhong <yang.zhong@intel.com> # v1 submitter Co-developed-by: Thomas Gleixner <tglx@linutronix.de> # co-author Signed-off-by: Thomas Gleixner <tglx@linutronix.de> # handler/reviewer Signed-off-by: Jing Liu <jing2.liu@intel.com> # v2-v3 submitter Signed-off-by: Yang Zhong <yang.zhong@intel.com> # v4-v5 submitter and that means, Thomas worked on that patch *after* Yang submitted v1. Which is the exact chronological order, as Kevin writes. > 3) do exactly what the gang did ("remain practical and do only an SOB > chain") Yes, but not change the SOB order. Because if you do that, then it doesn't state what the exact path was the patch took and how it ended up upstream. And due to past fun stories with SCO, we want to track exactly how a patch ended up upstream. And I think this is the most important aspect of those SOB chains. IMNSVHO.
> From: Borislav Petkov <bp@alien8.de> > Sent: Tuesday, January 11, 2022 2:19 AM > > On Mon, Jan 10, 2022 at 04:55:01PM +0100, Paolo Bonzini wrote: > > So this means that "the author must be the first SoB" is not an absolute > > rule. In the case of this patch we had: > > > > From: Jing Liu <jing2.liu@intel.com> > > ... > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > > Signed-off-by: Jing Liu <jing2.liu@intel.com> > > Signed-off-by: Yang Zhong <yang.zhong@intel.com> > > Looking at Kevin's explanation, that should be: > > Signed-off-by: Jing Liu <jing2.liu@intel.com> # author > Signed-off-by: Yang Zhong <yang.zhong@intel.com> # v1 submitter > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> # handler/reviewer > Signed-off-by: Jing Liu <jing2.liu@intel.com> # v2-v3 submitter > Signed-off-by: Yang Zhong <yang.zhong@intel.com> # v4-v5 submitter > > > and the possibilities could be: > > > > 1) have two SoB lines for Jing (before and after Thomas) > > > > 2) add a Co-developed-by for Thomas as the first line > > If Thomas would prefer. But then it becomes: > > Signed-off-by: Jing Liu <jing2.liu@intel.com> # author > Signed-off-by: Yang Zhong <yang.zhong@intel.com> # v1 submitter > Co-developed-by: Thomas Gleixner <tglx@linutronix.de> # co-author > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> # handler/reviewer > Signed-off-by: Jing Liu <jing2.liu@intel.com> # v2-v3 submitter > Signed-off-by: Yang Zhong <yang.zhong@intel.com> # v4-v5 submitter > > and that means, Thomas worked on that patch *after* Yang submitted v1. > Which is the exact chronological order, as Kevin writes. > > > 3) do exactly what the gang did ("remain practical and do only an SOB > > chain") > > Yes, but not change the SOB order. > > Because if you do that, then it doesn't state what the exact path was > the patch took and how it ended up upstream. And due to past fun stories > with SCO, we want to track exactly how a patch ended up upstream. And I > think this is the most important aspect of those SOB chains. > This is exactly what we'd like to get clarified in this very special case. Since Thomas didn't put a Co-developed-by in the first place, I prefer to respecting his choice i.e.: From: Jing Liu <jing2.liu@intel.com> Signed-off-by: Jing Liu <jing2.liu@intel.com> Signed-off-by: Yang Zhong <yang.zhong@intel.com> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Jing Liu <jing2.liu@intel.com> Signed-off-by: Yang Zhong <yang.zhong@intel.com> Following this rule then the SoB order in all other patches also need be fixed, e.g. both Jing's name and Yang's name will occur twice to reflect the chronological order in most patches. But I'm not sure what's the best way to handle it since Paolo already puts the entire series in his tip. Can this series gets an exempt given all participated names already have their SoB in this special case? If must be fixed we can certainly provide updated SoB history for every patch. The burden might be on Paolo's side and if it does happen I really want to say sorry here for not getting this open clarified earlier... Thanks Kevin
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c index eddeeb4ed2f5..a78bc547fc03 100644 --- a/arch/x86/kernel/fpu/core.c +++ b/arch/x86/kernel/fpu/core.c @@ -199,7 +199,7 @@ void fpu_reset_from_exception_fixup(void) } #if IS_ENABLED(CONFIG_KVM) -static void __fpstate_reset(struct fpstate *fpstate); +static void __fpstate_reset(struct fpstate *fpstate, u64 xfd); static void fpu_init_guest_permissions(struct fpu_guest *gfpu) { @@ -231,7 +231,8 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu) if (!fpstate) return false; - __fpstate_reset(fpstate); + /* Leave xfd to 0 (the reset value defined by spec) */ + __fpstate_reset(fpstate, 0); fpstate_init_user(fpstate); fpstate->is_valloc = true; fpstate->is_guest = true; @@ -454,21 +455,21 @@ void fpstate_init_user(struct fpstate *fpstate) fpstate_init_fstate(fpstate); } -static void __fpstate_reset(struct fpstate *fpstate) +static void __fpstate_reset(struct fpstate *fpstate, u64 xfd) { /* Initialize sizes and feature masks */ fpstate->size = fpu_kernel_cfg.default_size; fpstate->user_size = fpu_user_cfg.default_size; fpstate->xfeatures = fpu_kernel_cfg.default_features; fpstate->user_xfeatures = fpu_user_cfg.default_features; - fpstate->xfd = init_fpstate.xfd; + fpstate->xfd = xfd; } void fpstate_reset(struct fpu *fpu) { /* Set the fpstate pointer to the default fpstate */ fpu->fpstate = &fpu->__fpstate; - __fpstate_reset(fpu->fpstate); + __fpstate_reset(fpu->fpstate, init_fpstate.xfd); /* Initialize the permission related info in fpu */ fpu->perm.__state_perm = fpu_kernel_cfg.default_features;