[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH for-4.16] Revert "x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents"


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Fri, 26 Nov 2021 09:37:48 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Mpspua+0GLswF9Qn3jLXlfTXrpHhfRx8gki9ukV4AGg=; b=cKrjpmDyIdu2y38g9SOW5e7JnF0XyQ06A+yjyH5KRrigQ3hJ320QtDuUt9ce15Z2v5VuWKEAe1j2nCRPAuyXftOgeo313DTqEFejEuoNuFf7c9UIDtbtyuDv7VMkYt+zmrMyZUnvEborEpiRhmGoKxLKh9wvAAcNFQ+a6/XCpsfBAwq8WKhsKNQMlRTFPeE5LxernQch+wW7XYi73/loSgYmvze1H2cidOe57znOHt89Sf7dyogcb2Zs5m5hQNIEAZH2gtbTGUF1OTguzNwUZonGueoVyldpvLKO307hVh8TSbBpvFuibNNt/+c5LDe5Z0PsVj+ixQk4csS1KIiO8Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=k7wn/VGaHiBEFEhN2So8abG6wea3fh6qbr778YxedDQqMWexl0iulOPmvoaGJi8wF1e6arDSRJJqx5NEOTolcllodZCuJmVKubJsf+QwHdiXQ+PDKVuoA70d0DNGZM3vQiyTis2qKASX9vn/Qy/xEjvBkpYdX/Bb4kgq4NT1qaUV5G9m60HiarZGM4d5MONtszWbSk8RTDCck6IH4woQBDhlrelr4cqfs6/4mrAPlbdtilU3+XO0t3m9KNzpDHJB4h8MwUraKCfd1wIKeHe8KM6rLPzY1WsqSfPz+3kcDGfmg8szC5/q6JcuVxNXKMdPAuIkVetFJq+sqZAmjILb0Q==
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Andrew Cooper <amc96@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 26 Nov 2021 08:38:24 +0000
  • Ironport-data: A9a23:VDV3r6DjtAwi0RVW/9Lkw5YqxClBgxIJ4kV8jS/XYbTApD0lhjQGn GsYWmrTa6rcMWH2ftl/OY+/8kpX6JXVy9M3QQY4rX1jcSlH+JHPbTi7wuYcHM8wwunrFh8PA xA2M4GYRCwMo/u1Si6FatANl1ElvU2zbue6WLGs1hxZH1c+EX5400w7wobVv6Yz6TSHK1LV0 T/Ni5W31G+Ng1aY5UpNtspvADs21BjDkGtwUm4WPJinj3eH/5UhN7oNJLnZEpfNatI88thW5 Qr05OrREmvxp3/BAz4++1rxWhVirrX6ZWBihpfKMkQLb9crSiEai84G2PQghUh/ug+LwZd68 fB3n42PUCwDE5HswNoyXEwNe81+FfUuFL7vJHG+tYqYzlHccmuqyPJrZK00FdRGoKAtWzgIr KFGbmBWBvyAr7veLLaTUO5ji95lNMD2FIgepmth3XfSCvNOrZXrHvWUtYUFhGpYasZmLbXhP uUfSj1Vcg3STz8MIlM9Grkbg7L97pX4W2IB8w/EzUYt2EDMyCRh3b6rN8DaEvSaSMMQkkuGq 2bu+2XiHgpcJNGZ0SCC8H+nmqnIhyyTcIMNFpWo+/hymlqRy2cPThoMWjOTo/O0l0q/UNJ3M FEP92wlqq1a3EWuRYijdwaiq3DCuBMAM+e8CMVjtlvLkPCNpV/EWC5UFVatdeDKqucHfz50k VSwuunDFAN2m5e4dVLFyJib+Gba1TcuEUcOYioNTA0g6tbloZ0ugh+ncuuPAJJZnfWuR2iun mniQDwWwuxK0JVVj/nTEUXv2mr0/vD0ohgJChI7t45Pxidwf8abaoOh8jA3Bt4Qfd/CHjFtU JXp8vVyDdzi77nRy0Rho81XRdlFAspp1hWH2jaD+LF7qlyQF4aLJ9w43d2HDB4B3jw4UTHoe lTPngha+YVeOnCnBYcuPdnuW5p3kvm8S4i+PhwxUjaoSsMqHONg1HszDXN8Iki3yBR8+U3BE cvznTmQ4YYyVv08kWveqxY12r433CEurV4/trigpylLJYG2PSbPIZ9caQPmRrlgsMus/VWEm /4CZpDi40gOD4XDjtz/rNd7waYidiNgW/gbaqV/K4a+H+aRMD17VqKKn+p+I9cNcmY8vr6gw 0xRk3RwkTLXrXbGNR+LejZkbrbuVox4tnU1IWonOlPA5pTpSd/HAH43e8RlcL852vZkyPIoH fAJd9/ZWqZESyjd+iRbZp749dQweBOujAOIHiykfDlgIMIwG12XoofpLln16S0DLiurrs9i8 beu4RzWHMgYTAN4AceINP/2lwGtvWIQkf5ZVlfTJoUBY13l9YVncnSjjvI+L8wWBw/Ewz+Wi 1SfDRsC/LGfqI4p6tjZw6uDqt7xQed5G0NbGUjd7Kq3anaGrjbyn9cYXb/RLz7HVW7y9KGzX slvzqnxYK8dgVJHk4tgCLI3n6gw0MTi+u1BxQN+EXSVM1nyUuF8ImOL1NVkv7FWwuMLohO/X 0+C94UIObiNP8+5QlcdKBB8M7aG3PAQ3DLT8e40MAPx4youpOiLVkBbPh+tji1BLeQqbNN5k Ll54MNGuRaijhcKM8qdinEG/muBGXUMTqE7u8xIG4TskAcqlglPbJG05vUaO31ThwGg6nUXH wI=
  • Ironport-hdrordr: A9a23:H14CFah112GFoTKdwsgsDIE+h3BQX0B13DAbv31ZSRFFG/FwyP rAoB1L73PJYWgqNU3I+ergBEGBKUmskqKdxbNhR4tKOzOWxVdATbsSlrcKpgePJ8SQzJ8+6U 4NSdkaNDS0NykHsS+Y2njILz9D+qj/zEnAv463pB0MPGIaG52IrT0JcjpzencGOjWubqBJcq Z0iPA3wwZJLh8sH7uG7zQ+LqL+juyOsKijTQ8NBhYh5gXLpTS06ITiGxzd+hsFSTtAzZor7G CAymXCl+memsD+7iWZ+37Y7pxQltek4txfBPaUgsxQDjn3kA6naKloRrXHljEop+OE7kosjb D30lodFvU2z0mUUnC+oBPr1QWl+DEy60X6wVvdunfnqdyRfkNyN+NxwaZiNjfJ4Uspu99xlI hR2XiCipZRBRTc2Azg+tnhTXhR5wuJiEtntdRWo21UUIMYZrMUh5cY5llpHJAJGz+/wJw7Ed NpENrX6J9tAB2nhkjizypSKeGXLzYO9k/seDlGhiXV6UkYoJlB9TpZ+CRF9U1wsK7USPF/lp L52+pT5fZzp/QtHNBA7dE6MLyK41z2MGHx2V2pUCHa/YE8SjrwQs3Mkf4IDN/DQu198HJ1ou WGbG9l
  • Ironport-sdr: Kht7KxNn02LP5h3Duc4rz/5ru6WPouh9EKCGXWFXOmBECN/R6pbz/V8YfWhW83m7fEyMf27cud BBmovsD7ZZljXMcMUd+bZeUM2WOu3S/A/MsNUwCn+qVCzV+9DNswvwcpwuGUUovMJ4YFm93w+Y RgBrPqt9l0yPE0TXUAtQi/+jfoiBmDWRvlKIsZpZ/cp5qV5GU666KlYd7MUWRmcXu+y0czPX+j BT/vzemPP01kot8dpFmFs4LlgYV/LZm8GvZTAQTlhdRYCqtp3x/Ph9xrybkbQ8ztljytL+ThG5 LxbwOFR5/PhFYeUcStmr0U5R
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Fri, Nov 26, 2021 at 09:22:50AM +0100, Jan Beulich wrote:
> On 25.11.2021 18:28, Andrew Cooper wrote:
> > On 25/11/2021 10:43, Roger Pau Monné wrote:
> >> On Thu, Nov 25, 2021 at 11:25:36AM +0100, Jan Beulich wrote:
> >>> On 24.11.2021 22:11, Andrew Cooper wrote:
> >>>> OSSTest has identified a 3rd regression caused by this change.  Migration
> >>>> between Xen 4.15 and 4.16 on the nocera pair of machines (AMD Opteron 
> >>>> 4133)
> >>>> fails with:
> >>>>
> >>>>   xc: error: Failed to set CPUID policy: leaf 00000000, subleaf 
> >>>> ffffffff, msr ffffffff (22 = Invalid argument): Internal error
> >>>>   xc: error: Restore failed (22 = Invalid argument): Internal error
> >>>>
> >>>> which is a safety check to prevent resuming the guest when the CPUID 
> >>>> data has
> >>>> been truncated.  The problem is caused by shrinking of the max policies, 
> >>>> which
> >>>> is an ABI that needs handling compatibly between different versions of 
> >>>> Xen.
> >>>>
> >>>> Furthermore, shrinking of the default policies also breaks things in some
> >>>> cases, because certain cpuid= settings in a VM config file which used to 
> >>>> have
> >>>> an effect will now be silently discarded.
> >>>>
> >>>> This reverts commit 540d911c2813c3d8f4cdbb3f5672119e5e768a3d, as well as 
> >>>> the
> >>>> partial fix attempt in 81da2b544cbb003a5447c9b14d275746ad22ab37 (which 
> >>>> added
> >>>> one new case where cpuid= settings might not apply correctly) and 
> >>>> restores the
> >>>> same behaviour as Xen 4.15.
> >>>>
> >>>> Fixes: 540d911c2813 ("x86/CPUID: shrink max_{,sub}leaf fields according 
> >>>> to actual leaf contents")
> >>>> Fixes: 81da2b544cbb ("x86/cpuid: prevent shrinking migrated policies max 
> >>>> leaves")
> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >>> While not strictly needed with Roger having given his already,
> >>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>> to signal my (basic) agreement with the course of action taken.
> >>> Nevertheless I fear this is going to become yet one more case where
> >>> future action is promised, but things then die out.
> >> I'm certainly happy to look at newer versions of this patch, but I
> >> think we should consider doing the shrinking only on the toolstack
> >> said, and only after all the manipulations on the policy have been
> >> performed.
> > 
> > Correct.
> > 
> > The max policies cannot be shrunk - they are, by definition, the upper
> > bounds that we audit against.  (More precisely, they must never end up
> > lower than an older Xen used to offer on the same configuration, and
> > must not be lower anything the user may opt in to.)
> 
> I disagree: For one, the user cannot opt in to anything beyond max policy.
> Or else that policy isn't "max" anymore. The user may opt in to a higher
> than useful max (sub)leaf, but that's independent.

Wouldn't it be possible for Xen to offer certain features to guests
that are not part of the native CPUID policy, and that thus might
require setting bit(s) on leaves that could otherwise be empty? That
also requires some explicit checks in Xen in order to assert that
leaves above the max one are empty.

TBH I'm not sure what's the benefit of shrinking the max policies, as
those are not to be used as-is by guests. They are an upper bound, but
should be tailored for each guest usage, and should be shrunk before
being used by guests on a case-by-case basis (ie: by the toolstack).

> I'm also not convinced
> older Xen mistakenly offering too much can be taken as an excuse that we
> can't ever go below that. We've done so in the past iirc, with workarounds
> added elsewhere. Older Xen offering too high a max (sub)leaf again is
> independent. Max (sub)leaf requests from the user should, contrary to my
> view when submitting the original change, be honored. This would then
> automatically include migrating-in guests.
> 
> > The default policies can in principle be shrunk, but only if the
> > toolstack learns to grow max leaf too (which it will need to). 
> > Nevertheless, actually shrinking the default policies is actively
> > unhelpful, because it is wasting time doing something which the
> > toolstack needs to undo later.
> 
> I agree.
> 
> > The policy for new domains should be shrunk, but only after every other
> > adjustment is made.  This is one small aspect of teaching the toolstack
> > to properly understand CPUID (and MSR) policies, and has always been on
> > the plan.
> 
> And this not being the case yet is getting too prominent for my taste
> with the need to raise the max Intel leaves we know of for things like
> AMX or KeyLocker. I didn't get shrinking done right; apologies for that.
> But I continue to think that proper shrinking ought to be a prereq to
> that, without delaying that work (effectively complete for AMX, partial
> for KeyLocker) almost indefinitely.

I have to revisit the patches I have pending for CPUID/MSR interface
cleanup on my side, but I think that the shrinking could be easily
done at the tail of that that series.

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.