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(-)

-- 
2.36.0

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

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

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

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