| Message ID | 20220203181023.1554-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/guest: Fix comment regarding CPUID compatibility | expand |
On 03.02.2022 19:10, Andrew Cooper wrote: > It was Xen 4.14 where CPUID data was added to the migration stream, and 4.13 > that we need to worry about with regards to compatibility. Xen 4.12 isn't > relevant. > > Expand and correct the commentary. > > Fixes: 111c8c33a8a1 ("x86/cpuid: do not expand max leaves on restore") But doesn't this commit amend 685e922d6f30 ("tools/libxc: Rework xc_cpuid_apply_policy() to use {get,set}_cpu_policy()"), which is where DEF_MAX_* disappeared? That was a 4.13 change, and iirc that was also why the commit above moved the 4.13/4.14 boundary related comment from outside of the if() to inside it. While looking at this, wasn't Roger's change incomplete, in that for Intel the extended leaf upper bound was 0x80000008 in 4.12? Jan
On 04/02/2022 08:31, Jan Beulich wrote: > On 03.02.2022 19:10, Andrew Cooper wrote: >> It was Xen 4.14 where CPUID data was added to the migration stream, and 4.13 >> that we need to worry about with regards to compatibility. Xen 4.12 isn't >> relevant. >> >> Expand and correct the commentary. >> >> Fixes: 111c8c33a8a1 ("x86/cpuid: do not expand max leaves on restore") > But doesn't this commit amend 685e922d6f30 ("tools/libxc: Rework > xc_cpuid_apply_policy() to use {get,set}_cpu_policy()"), which is > where DEF_MAX_* disappeared? No. All that happened in that change was that we switched to using cpuid.h:89:#define CPUID_GUEST_NR_EXTD_AMD instead, which remained the same size until Xen 4.15 when e9b4fe26364 bumped it. > While looking at this, wasn't Roger's change incomplete, in that > for Intel the extended leaf upper bound was 0x80000008 in 4.12? CPUID_GUEST_NR_EXTD_INTEL is still 8, so this is all fine. Even if hardware has more, we'll still limit to 8 on Intel. That said, to this date Intel haven't bumped the limit, so we're approaching 2 decades without change. Should this change, then yes, we'd have have to adjust the compat path. ~Andrew
On 04.02.2022 13:12, Andrew Cooper wrote: > On 04/02/2022 08:31, Jan Beulich wrote: >> On 03.02.2022 19:10, Andrew Cooper wrote: >>> It was Xen 4.14 where CPUID data was added to the migration stream, and 4.13 >>> that we need to worry about with regards to compatibility. Xen 4.12 isn't >>> relevant. >>> >>> Expand and correct the commentary. >>> >>> Fixes: 111c8c33a8a1 ("x86/cpuid: do not expand max leaves on restore") >> But doesn't this commit amend 685e922d6f30 ("tools/libxc: Rework >> xc_cpuid_apply_policy() to use {get,set}_cpu_policy()"), which is >> where DEF_MAX_* disappeared? > > No. All that happened in that change was that we switched to using > > cpuid.h:89:#define CPUID_GUEST_NR_EXTD_AMD > > instead, which remained the same size until Xen 4.15 when e9b4fe26364 > bumped it. Oh, right. I did try to look for a replacement, but managed to miss this. But then, as much as 4.12 isn't relevant, isn't it the case that the fact that CPUID data was added to the stream in 4.14 isn't relevant here either, and it's instead the bumping in 4.15 which is? IOW while I've been fine with the comment adjustment anyway, there would still want to be an adjustment to the description. >> While looking at this, wasn't Roger's change incomplete, in that >> for Intel the extended leaf upper bound was 0x80000008 in 4.12? > > CPUID_GUEST_NR_EXTD_INTEL is still 8, so this is all fine. Again, somehow I did manage to miss the replacement defines. Jan
On 04/02/2022 13:09, Jan Beulich wrote: > On 04.02.2022 13:12, Andrew Cooper wrote: >> On 04/02/2022 08:31, Jan Beulich wrote: >>> On 03.02.2022 19:10, Andrew Cooper wrote: >>>> It was Xen 4.14 where CPUID data was added to the migration stream, and 4.13 >>>> that we need to worry about with regards to compatibility. Xen 4.12 isn't >>>> relevant. >>>> >>>> Expand and correct the commentary. >>>> >>>> Fixes: 111c8c33a8a1 ("x86/cpuid: do not expand max leaves on restore") >>> But doesn't this commit amend 685e922d6f30 ("tools/libxc: Rework >>> xc_cpuid_apply_policy() to use {get,set}_cpu_policy()"), which is >>> where DEF_MAX_* disappeared? >> No. All that happened in that change was that we switched to using >> >> cpuid.h:89:#define CPUID_GUEST_NR_EXTD_AMD >> >> instead, which remained the same size until Xen 4.15 when e9b4fe26364 >> bumped it. > Oh, right. I did try to look for a replacement, but managed to miss > this. But then, as much as 4.12 isn't relevant, isn't it the case > that the fact that CPUID data was added to the stream in 4.14 isn't > relevant here either, and it's instead the bumping in 4.15 which is? The fact that the bump happened is relevant, by virtue of the fact there logic added to cope. The fact it was in 4.15 is not relevant - this isn't a list of every ABI-relevant change. CPUID data being added to the stream is critically important, because that's the point after which we never enter this compatibility path. ~Andrew
On 04.02.2022 14:34, Andrew Cooper wrote: > On 04/02/2022 13:09, Jan Beulich wrote: >> On 04.02.2022 13:12, Andrew Cooper wrote: >>> On 04/02/2022 08:31, Jan Beulich wrote: >>>> On 03.02.2022 19:10, Andrew Cooper wrote: >>>>> It was Xen 4.14 where CPUID data was added to the migration stream, and 4.13 >>>>> that we need to worry about with regards to compatibility. Xen 4.12 isn't >>>>> relevant. >>>>> >>>>> Expand and correct the commentary. >>>>> >>>>> Fixes: 111c8c33a8a1 ("x86/cpuid: do not expand max leaves on restore") >>>> But doesn't this commit amend 685e922d6f30 ("tools/libxc: Rework >>>> xc_cpuid_apply_policy() to use {get,set}_cpu_policy()"), which is >>>> where DEF_MAX_* disappeared? >>> No. All that happened in that change was that we switched to using >>> >>> cpuid.h:89:#define CPUID_GUEST_NR_EXTD_AMD >>> >>> instead, which remained the same size until Xen 4.15 when e9b4fe26364 >>> bumped it. >> Oh, right. I did try to look for a replacement, but managed to miss >> this. But then, as much as 4.12 isn't relevant, isn't it the case >> that the fact that CPUID data was added to the stream in 4.14 isn't >> relevant here either, and it's instead the bumping in 4.15 which is? > > The fact that the bump happened is relevant, by virtue of the fact there > logic added to cope. The fact it was in 4.15 is not relevant - this > isn't a list of every ABI-relevant change. > > CPUID data being added to the stream is critically important, because > that's the point after which we never enter this compatibility path. If the bump happened before CPUID data was added to the stream, logic to cope with migrating-in guests would have been required too, wouldn't it. But anyway, just to be done with this: Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On 04/02/2022 13:46, Jan Beulich wrote: > On 04.02.2022 14:34, Andrew Cooper wrote: >> On 04/02/2022 13:09, Jan Beulich wrote: >>> On 04.02.2022 13:12, Andrew Cooper wrote: >>>> On 04/02/2022 08:31, Jan Beulich wrote: >>>>> On 03.02.2022 19:10, Andrew Cooper wrote: >>>>>> It was Xen 4.14 where CPUID data was added to the migration stream, and 4.13 >>>>>> that we need to worry about with regards to compatibility. Xen 4.12 isn't >>>>>> relevant. >>>>>> >>>>>> Expand and correct the commentary. >>>>>> >>>>>> Fixes: 111c8c33a8a1 ("x86/cpuid: do not expand max leaves on restore") >>>>> But doesn't this commit amend 685e922d6f30 ("tools/libxc: Rework >>>>> xc_cpuid_apply_policy() to use {get,set}_cpu_policy()"), which is >>>>> where DEF_MAX_* disappeared? >>>> No. All that happened in that change was that we switched to using >>>> >>>> cpuid.h:89:#define CPUID_GUEST_NR_EXTD_AMD >>>> >>>> instead, which remained the same size until Xen 4.15 when e9b4fe26364 >>>> bumped it. >>> Oh, right. I did try to look for a replacement, but managed to miss >>> this. But then, as much as 4.12 isn't relevant, isn't it the case >>> that the fact that CPUID data was added to the stream in 4.14 isn't >>> relevant here either, and it's instead the bumping in 4.15 which is? >> The fact that the bump happened is relevant, by virtue of the fact there >> logic added to cope. The fact it was in 4.15 is not relevant - this >> isn't a list of every ABI-relevant change. >> >> CPUID data being added to the stream is critically important, because >> that's the point after which we never enter this compatibility path. > If the bump happened before CPUID data was added to the stream, logic to > cope with migrating-in guests would have been required too, wouldn't it. Yes, it would have been. It wasn't an accident that none of the max leaves changed while doing the Xen CPUID work. We're unfortunately a long way behind on Intel CPUID leaves, but all(?) of the new leaves need more complicated migration safely logic than the toolstack currently knows how to do. > But anyway, just to be done with this: > Acked-by: Jan Beulich <jbeulich@suse.com> Thanks. ~Andrew
On Fri, Feb 04, 2022 at 02:10:03PM +0000, Andrew Cooper wrote: > On 04/02/2022 13:46, Jan Beulich wrote: > > On 04.02.2022 14:34, Andrew Cooper wrote: > >> On 04/02/2022 13:09, Jan Beulich wrote: > >>> On 04.02.2022 13:12, Andrew Cooper wrote: > >>>> On 04/02/2022 08:31, Jan Beulich wrote: > >>>>> On 03.02.2022 19:10, Andrew Cooper wrote: > >>>>>> It was Xen 4.14 where CPUID data was added to the migration stream, and 4.13 > >>>>>> that we need to worry about with regards to compatibility. Xen 4.12 isn't > >>>>>> relevant. > >>>>>> > >>>>>> Expand and correct the commentary. > >>>>>> > >>>>>> Fixes: 111c8c33a8a1 ("x86/cpuid: do not expand max leaves on restore") > >>>>> But doesn't this commit amend 685e922d6f30 ("tools/libxc: Rework > >>>>> xc_cpuid_apply_policy() to use {get,set}_cpu_policy()"), which is > >>>>> where DEF_MAX_* disappeared? > >>>> No. All that happened in that change was that we switched to using > >>>> > >>>> cpuid.h:89:#define CPUID_GUEST_NR_EXTD_AMD > >>>> > >>>> instead, which remained the same size until Xen 4.15 when e9b4fe26364 > >>>> bumped it. > >>> Oh, right. I did try to look for a replacement, but managed to miss > >>> this. But then, as much as 4.12 isn't relevant, isn't it the case > >>> that the fact that CPUID data was added to the stream in 4.14 isn't > >>> relevant here either, and it's instead the bumping in 4.15 which is? > >> The fact that the bump happened is relevant, by virtue of the fact there > >> logic added to cope. The fact it was in 4.15 is not relevant - this > >> isn't a list of every ABI-relevant change. > >> > >> CPUID data being added to the stream is critically important, because > >> that's the point after which we never enter this compatibility path. > > If the bump happened before CPUID data was added to the stream, logic to > > cope with migrating-in guests would have been required too, wouldn't it. > > Yes, it would have been. > > It wasn't an accident that none of the max leaves changed while doing > the Xen CPUID work. > > We're unfortunately a long way behind on Intel CPUID leaves, but all(?) > of the new leaves need more complicated migration safely logic than the > toolstack currently knows how to do. > > > But anyway, just to be done with this: > > Acked-by: Jan Beulich <jbeulich@suse.com> > > Thanks. Will rebase my CPUID series on top of this, but I will wait for further comments before sending a new version. Roger.
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c index b9e827ce7eb0..57f81eb0a082 100644 --- a/tools/libs/guest/xg_cpuid_x86.c +++ b/tools/libs/guest/xg_cpuid_x86.c @@ -497,9 +497,19 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore, if ( restore ) { /* - * Account for feature which have been disabled by default since Xen 4.13, - * so migrated-in VM's don't risk seeing features disappearing. + * Xen 4.14 introduced support to move the guest's CPUID data in the + * migration stream. Previously, the destination side would invent a + * policy out of thin air in the hopes that it was ok. + * + * This restore path is used for incoming VMs with no CPUID data + * i.e. originated on Xen 4.13 or earlier. We must invent a policy + * compatible with what Xen 4.13 would have done on the same hardware. + * + * Specifically: + * - Clamp max leaves. + * - Re-enable features which have become (possibly) off by default. */ + p->basic.rdrand = test_bit(X86_FEATURE_RDRAND, host_featureset); p->feat.hle = test_bit(X86_FEATURE_HLE, host_featureset); p->feat.rtm = test_bit(X86_FEATURE_RTM, host_featureset); @@ -509,7 +519,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore, p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset); } - /* Clamp maximum leaves to the ones supported on 4.12. */ p->basic.max_leaf = min(p->basic.max_leaf, 0xdu); p->feat.max_subleaf = 0; p->extd.max_leaf = min(p->extd.max_leaf, 0x8000001c);
It was Xen 4.14 where CPUID data was added to the migration stream, and 4.13 that we need to worry about with regards to compatibility. Xen 4.12 isn't relevant. Expand and correct the commentary. Fixes: 111c8c33a8a1 ("x86/cpuid: do not expand max leaves on restore") Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Anthony PERARD <anthony.perard@citrix.com> CC: Juergen Gross <jgross@suse.com> Roger, this also has a knock-on effect on your CPUID series. The 4.12 had been nagging me for a while before I figured out why. --- tools/libs/guest/xg_cpuid_x86.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)