| Message ID | 20220517153127.40276-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | amd/msr: implement MSR_VIRT_SPEC_CTRL for HVM guests | expand |
On 17.05.2022 17:31, Roger Pau Monne wrote: > Hello, > > The following series implements support for MSR_VIRT_SPEC_CTRL > (VIRT_SSBD) on different AMD CPU families. > > Note that the support is added backwards, starting with the newer CPUs > that support MSR_SPEC_CTRL and moving to the older ones either using > MSR_VIRT_SPEC_CTRL or the SSBD bit in LS_CFG. > > Xen is still free to use it's own SSBD setting, as the selection is > context switched on vm{entry,exit}. > > On Zen2 and later, SPEC_CTRL.SSBD should exist and should be used in > preference to VIRT_SPEC_CTRL.SSBD. However, for migration > compatibility, Xen offers VIRT_SSBD to guests (in the max cpuid policy, > not default) implemented in terms of SPEC_CTRL.SSBD. > > On Fam15h thru Zen1, Xen exposes VIRT_SSBD to guests by default to > abstract away the model and/or hypervisor specific differences in > MSR_LS_CFG/MSR_VIRT_SPEC_CTRL. > > So the implementation of VIRT_SSBD exposed to HVM guests will use one of > the following underlying mechanisms, in the preference order listed > below: > > * SPEC_CTRL.SSBD: patch 1 > * VIRT_SPEC_CTRL.SSBD: patch 2. > * Non-architectural way using LS_CFG: patch 3. > > NB: patch 3 introduces some logic in GIF=0 context, such logic has been > kept to a minimum due to the special context it's running on. > > Roger Pau Monne (3): > amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL > amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests > amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD FTAOD, while besides a missing ack for ... > CHANGELOG.md | 3 + ... this addition the series would now look to be ready to go in, I'd like to have some form of confirmation by you, Andrew, that you now view this as meeting the comments you gave on an earlier version. Jan
Hi Jan, Roger and Andrew, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > > On 17.05.2022 17:31, Roger Pau Monne wrote: > > Hello, > > > > The following series implements support for MSR_VIRT_SPEC_CTRL > > (VIRT_SSBD) on different AMD CPU families. > > > > Note that the support is added backwards, starting with the newer CPUs > > that support MSR_SPEC_CTRL and moving to the older ones either using > > MSR_VIRT_SPEC_CTRL or the SSBD bit in LS_CFG. > > > > Xen is still free to use it's own SSBD setting, as the selection is > > context switched on vm{entry,exit}. > > > > On Zen2 and later, SPEC_CTRL.SSBD should exist and should be used in > > preference to VIRT_SPEC_CTRL.SSBD. However, for migration > > compatibility, Xen offers VIRT_SSBD to guests (in the max cpuid policy, > > not default) implemented in terms of SPEC_CTRL.SSBD. > > > > On Fam15h thru Zen1, Xen exposes VIRT_SSBD to guests by default to > > abstract away the model and/or hypervisor specific differences in > > MSR_LS_CFG/MSR_VIRT_SPEC_CTRL. > > > > So the implementation of VIRT_SSBD exposed to HVM guests will use one > of > > the following underlying mechanisms, in the preference order listed > > below: > > > > * SPEC_CTRL.SSBD: patch 1 > > * VIRT_SPEC_CTRL.SSBD: patch 2. > > * Non-architectural way using LS_CFG: patch 3. > > > > NB: patch 3 introduces some logic in GIF=0 context, such logic has been > > kept to a minimum due to the special context it's running on. > > > > Roger Pau Monne (3): > > amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of > SPEC_CTRL > > amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests > > amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD > > FTAOD, while besides a missing ack for ... > > > CHANGELOG.md | 3 + > > ... this addition the series would now look to be ready to go in, > I'd like to have some form of confirmation by you, Andrew, that > you now view this as meeting the comments you gave on an earlier > version. Not completely sure if I am proper to do that but for the CHANGELOG.md change: Acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry > > Jan
On 18.05.2022 11:51, Henry Wang wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> >> On 17.05.2022 17:31, Roger Pau Monne wrote: >>> Roger Pau Monne (3): >>> amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of >> SPEC_CTRL >>> amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests >>> amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD >> >> FTAOD, while besides a missing ack for ... >> >>> CHANGELOG.md | 3 + >> >> ... this addition the series would now look to be ready to go in, >> I'd like to have some form of confirmation by you, Andrew, that >> you now view this as meeting the comments you gave on an earlier >> version. > > Not completely sure if I am proper to do that but for the CHANGELOG.md > change: Well, no-one except you actually can ack changes to this file, as per ./MAINTAINERS. > Acked-by: Henry Wang <Henry.Wang@arm.com> Thanks. Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > > On 18.05.2022 11:51, Henry Wang wrote: > >> -----Original Message----- > >> From: Jan Beulich <jbeulich@suse.com> > >> > >> On 17.05.2022 17:31, Roger Pau Monne wrote: > >>> Roger Pau Monne (3): > >>> amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of > >> SPEC_CTRL > >>> amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests > >>> amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy > SSBD > >> > >> FTAOD, while besides a missing ack for ... > >> > >>> CHANGELOG.md | 3 + > >> > >> ... this addition the series would now look to be ready to go in, > >> I'd like to have some form of confirmation by you, Andrew, that > >> you now view this as meeting the comments you gave on an earlier > >> version. > > > > Not completely sure if I am proper to do that but for the CHANGELOG.md > > change: > > Well, no-one except you actually can ack changes to this file, as per > ./MAINTAINERS. Thanks for confirming and sending the reminder to help me to understand that I should ack the changes in CHANGELOG.md for this series, I will keep the information in mind and I guess I am gradually acquiring experiences :) Kind regards, Henry > > > Acked-by: Henry Wang <Henry.Wang@arm.com> > > Thanks. > > Jan
On 17.05.2022 17:31, Roger Pau Monne wrote: > Roger Pau Monne (3): > amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL > amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests > amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD While, somewhat different from Jürgen's series, here the delay is more voluntary (in that I had told Roger that I'd prefer to commit this only with your (perhaps informal) agreement, I think we also can't wait much longer. I'm willing to give it until the end of next week, so I guess I'd move forward with committing early the week after, unless I hear substantial arguments against doing so (at which point the two of us would likely need to decide who's going to pick up this work while Roger is away). Once again thanks for your understanding, Jan
On 17.05.2022 17:31, Roger Pau Monne wrote: > Roger Pau Monne (3): > amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL > amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests > amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD I came to realize that I had announced that I would commit this about a month ago. I've done so now, but there was quite a bit of re-basing necessary, to a fair degree because of this delay that I did introduce by oversight. I hope I didn't screw up anywhere. Jan
On 15/08/2022 09:01, Jan Beulich wrote: > On 17.05.2022 17:31, Roger Pau Monne wrote: >> Roger Pau Monne (3): >> amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL >> amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests >> amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD > I came to realize that I had announced that I would commit this about a > month ago. I've done so now, but there was quite a bit of re-basing > necessary, to a fair degree because of this delay that I did introduce > by oversight. I hope I didn't screw up anywhere. Revert them, or I will. There has not been adequate review or testing. Patch 2 in particular is firmly nacked, because the only thing I have ever suggested in that area is deleting the patch. ~Andrew
On 15.08.2022 10:15, Andrew Cooper wrote: > On 15/08/2022 09:01, Jan Beulich wrote: >> On 17.05.2022 17:31, Roger Pau Monne wrote: >>> Roger Pau Monne (3): >>> amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL >>> amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests >>> amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD >> I came to realize that I had announced that I would commit this about a >> month ago. I've done so now, but there was quite a bit of re-basing >> necessary, to a fair degree because of this delay that I did introduce >> by oversight. I hope I didn't screw up anywhere. > > Revert them, or I will. There is no basis for reverting, at this point at least. May I remind you of the fact that Xen is a community project? I know you've done reverts in the past without even waiting for a discussion to settle, but this wasn't okay back then and is not going to be okay this time round, nor going forward. If you see issues with a series, and in particular one which is otherwise fully qualified for committing, you ought to voice these concerns. You cannot expect people to guess that you're still not happy with the adjustments which were made in an attempt to address earlier voiced concerns. > There has not been adequate review or testing. No adequate review? Am I a 2nd class citizen all of the sudden? In my reviews I've tried hard to account for the few comments you gave (or should I say that Roger was able to shake out of you)? Plus I've said more than once that I would prefer to not commit this without you having given it a (perhaps just informal) look over. Yet no feedback ever surfaced. I don't recall you going ... > Patch 2 in particular is firmly nacked, because the only thing I have > ever suggested in that area is deleting the patch. ... this far (and in particular not for the later versions of the series), but I do recall Roger re-working the patch to (try to) address your concerns. Going from just my mailbox (which goes back only to v3) I see no replies from you _at all_ on this patch. There was a longish reply to 0/3, but nothing on v4 or newer, despite pings which were sent your way. Knowing this has happened in the past - is your reaction based on v6 or rather on the last version you've actually looked at (presumably v3)? Jan
On 15/08/2022 10:14, Jan Beulich wrote: > On 15.08.2022 10:15, Andrew Cooper wrote: >> On 15/08/2022 09:01, Jan Beulich wrote: >>> On 17.05.2022 17:31, Roger Pau Monne wrote: >>>> Roger Pau Monne (3): >>>> amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL >>>> amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests >>>> amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD >>> I came to realize that I had announced that I would commit this about a >>> month ago. I've done so now, but there was quite a bit of re-basing >>> necessary, to a fair degree because of this delay that I did introduce >>> by oversight. I hope I didn't screw up anywhere. >> Revert them, or I will. > There is no basis for reverting You have falsified tags from me, which is a consequence of the series not having been reviewed correctly. ~Andrew
On 15.08.2022 20:49, Andrew Cooper wrote: > On 15/08/2022 10:14, Jan Beulich wrote: >> On 15.08.2022 10:15, Andrew Cooper wrote: >>> On 15/08/2022 09:01, Jan Beulich wrote: >>>> On 17.05.2022 17:31, Roger Pau Monne wrote: >>>>> Roger Pau Monne (3): >>>>> amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL >>>>> amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests >>>>> amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD >>>> I came to realize that I had announced that I would commit this about a >>>> month ago. I've done so now, but there was quite a bit of re-basing >>>> necessary, to a fair degree because of this delay that I did introduce >>>> by oversight. I hope I didn't screw up anywhere. >>> Revert them, or I will. >> There is no basis for reverting > > You have falsified tags from me, which is a consequence of the series > not been reviewed correctly. Andrew, please, come on. I have not added any tags from you, hence I cannot possibly have falsified any. The Suggested-by tags had been there all the time. I also cannot see any evidence of "the series not [having] been reviewed correctly". Please can you support your claims by actual facts? As to the series itself and its possible reverting (or fixing) - can you please reply with technical comments on the problematic patch(es) of the most recent version? Henry - you may want to add this to your list of things to monitor. The situation will need resolving one way or another, but obviously we first need to determine the most suitable way. Jan
On Mon, Aug 15, 2022 at 06:49:08PM +0000, Andrew Cooper wrote: > On 15/08/2022 10:14, Jan Beulich wrote: > > On 15.08.2022 10:15, Andrew Cooper wrote: > >> On 15/08/2022 09:01, Jan Beulich wrote: > >>> On 17.05.2022 17:31, Roger Pau Monne wrote: > >>>> Roger Pau Monne (3): > >>>> amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL > >>>> amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests > >>>> amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD > >>> I came to realize that I had announced that I would commit this about a > >>> month ago. I've done so now, but there was quite a bit of re-basing > >>> necessary, to a fair degree because of this delay that I did introduce > >>> by oversight. I hope I didn't screw up anywhere. > >> Revert them, or I will. > > There is no basis for reverting > > You have falsified tags from me, which is a consequence of the series > not having been reviewed correctly. Seeing the changes done to the commits, I guess the problem was the 'Suggested-by' tag. This was added by me, and has been there since v1 because it was you who suggested to do this work, and additionally provided guidance on how the implementation should look like in: https://lore.kernel.org/xen-devel/4457dcd5-6a64-355a-b794-6b404cf90335@citrix.com/ I'm sorry if this turned out to not look like you expected/wanted. It's possible we had informal conversations about this where we discussed changes, but TBH I have quite a big queue of patches, so it's likely I've forgotten about. I'm happy to make any further adjustments to the code, but I will need to be pointed out at issues. Thanks, Roger.
Hello all, This thread was brought to the attention of the Code of Conduct team because it contains some fairly serious accusations. Having looked into the matter, and given the people involved a chance to respond, we’d like to set the record straight (to the best of our current knowledge). First of all, we’d like to acknowledge that the issue addressed in this series has been a long-term source of frustration: Andrew first mentioned the need for this functionality in 2018, and from then until January 2022 repeatedly mentioned it as something critical to get in but which he simply didn’t have time to personally address. That said, while his frustration is understandable, there were a number of accusations made which, as far as we can tell, were unfounded; so we want to set the record straight. In particular, having looked into the history of this series and had discussions with all the parties, we have concluded the following: * Roger attempted to address all of the feedback Andrew gave; and not only was the minimum check-in process followed, but Jan and Roger made every effort to get Andrew's feedback before checking in the patch. * The "Suggested-by" tags were reasonable for Roger to have put in, and reasonable for Jan to have retained. This is not the kind of community we strive to be. In particular, we’d like to exhort people to avoid charged language and attempt to lay out the facts — with references — such that they speak for themselves. We are also attempting to improve communication and shared understanding, to hopefully keep frustration levels low in the future. Thanks, -The Xen Project Conduct Team
Hello, The following series implements support for MSR_VIRT_SPEC_CTRL (VIRT_SSBD) on different AMD CPU families. Note that the support is added backwards, starting with the newer CPUs that support MSR_SPEC_CTRL and moving to the older ones either using MSR_VIRT_SPEC_CTRL or the SSBD bit in LS_CFG. Xen is still free to use it's own SSBD setting, as the selection is context switched on vm{entry,exit}. On Zen2 and later, SPEC_CTRL.SSBD should exist and should be used in preference to VIRT_SPEC_CTRL.SSBD. However, for migration compatibility, Xen offers VIRT_SSBD to guests (in the max cpuid policy, not default) implemented in terms of SPEC_CTRL.SSBD. On Fam15h thru Zen1, Xen exposes VIRT_SSBD to guests by default to abstract away the model and/or hypervisor specific differences in MSR_LS_CFG/MSR_VIRT_SPEC_CTRL. So the implementation of VIRT_SSBD exposed to HVM guests will use one of the following underlying mechanisms, in the preference order listed below: * SPEC_CTRL.SSBD: patch 1 * VIRT_SPEC_CTRL.SSBD: patch 2. * Non-architectural way using LS_CFG: patch 3. NB: patch 3 introduces some logic in GIF=0 context, such logic has been kept to a minimum due to the special context it's running on. Roger Pau Monne (3): amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD CHANGELOG.md | 3 + xen/arch/x86/cpu/amd.c | 121 +++++++++++++++++--- xen/arch/x86/cpuid.c | 21 ++++ xen/arch/x86/hvm/hvm.c | 1 + xen/arch/x86/hvm/svm/entry.S | 8 ++ xen/arch/x86/hvm/svm/svm.c | 39 +++++++ xen/arch/x86/include/asm/amd.h | 4 + xen/arch/x86/include/asm/cpufeatures.h | 1 + xen/arch/x86/include/asm/msr.h | 14 +++ xen/arch/x86/msr.c | 26 +++++ xen/arch/x86/spec_ctrl.c | 12 +- xen/include/public/arch-x86/cpufeatureset.h | 2 +- 12 files changed, 233 insertions(+), 19 deletions(-)