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

Re: [PATCH 1/8] x86/msr: Fix migration compatibility issue with MSR_SPEC_CTRL


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 26 Jan 2022 13:17:27 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; 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=Pu5v7E6wpEaDLESZ29unUYXoSklMxrmo4g/8dJDRus0=; b=HCA0Xi0k2RszNKo5D9ep64qhkzwIh6acbQ6Ny2XpTGgf5TpCJZUl41mxyC43hyMIQTXpf9CSfAgU6lom7nhPgJlgEKEywvdec1G6DevxIqXpQf12sHkpztVaCXu3zYFatiYUPGdj1TJmStrL2/j5gzQysLC4MtZpOcPCkrS7pdAC6Pe5IrX2lT5ofa3x+88DAW9rXoz1Iu05HkgyfE1CIPlJz+N0nBuS2E+1fsJ8CcZqHYRgBL9J5/NAGUPn/I3iSi+1SYHD3h1IbobuY1yQoMgzLP5M4TKRRvHdQ3RI2GnnHYr2rLniOv82e+5g0UlamOO7dK2AXzA+DUkesfR0ew==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Qk6imrqB89TI7Tv8e+EOVpkW63rmbJpUy9XhSZByneTg1HqPhjdygwxFhzMdeXOnKBfAZKE3QmE3ngVk+9g5ZVzLGGvnyZa+LaTPs8wh3J4ik8GKVfWKy9UP05L/5T/VC5uxMryibi0BTKUNf2+D1ITL5ZNP5kRXe30sHgv79oE1kHt1outT5ffRZgjTOocXXOLiB61uR3QLqRZzlp7l1YfWrJF1/AuI/kEWOhHFWsXPtXCafwWhg+ZyUz4G6SWIKrMnqw2PsgLBVQRNzsSxaauO+gMv5eHBOWaqeGu66eFIf005OxX5pUNHLuMYq6veppM2D8kgCZ36BQW7l7PmNw==
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Wed, 26 Jan 2022 12:18:06 +0000
  • Ironport-data: A9a23:VOFpu6sce3N+Pb0n4B/FyZ2vxufnVLBZMUV32f8akzHdYApBsoF/q tZmKWCEOv2JMDT1eYsjOoSzpENXsMSHm9RrHVM4/ngzEH8R+JbJXdiXEBz9bniYRiHhoOOLz Cm8hv3odp1coqr0/0/1WlTZQP0VOZigHtIQMsadUsxKbVIiGHdJZS5LwbZj2NYx24bhWWthh PupyyHhEA79s9JLGjp8B5Kr8HuDa9yr5Vv0FnRnDRx6lAe2e0s9VfrzFonoR5fMeaFGH/bSe gr25OrRElU1XfsaIojNfr7TKiXmS1NJVOSEoiI+t6OK2nCuqsGuu0qS2TV1hUp/0l20c95NJ NplhJWAUxYILJT1kctADChpCChxHo1Z0eqSSZS/mZT7I0zudnLtx7NlDV0sPJ1e8eFyaY1M3 aVGcnZXNEnF3r/ohuLgIgVvrp1LwM3DJoQQt2sm1TjEJf0nXYrCU+PB4towMDIY2JwfQ6+AN pNxhTxHXh7HalpWKkgtBbUMgvaHv37Cdg1ipwfAzUYwyzeKl1EguFT3C/LKfvSaSMMTmVyXz krk1WnkBhARNPSE1CGItHmrg4fnjS79HY4fCrC83vprm0GIgHweDgUMUlm2quX/jVSxM++zM GRNpHBo9/JrshX2EJ+tBHVUvUJooDYtUupKAswo0TiE5feL0jeCD2cnbiFoPYlOWNANeRQm0 VqAntXMDDNpsaGIRX/1yop4vQ9eKgBOczZcOHZsoR8tpoC6/dpt1k6nosNLTfbt5uAZDw0c1 NxjQMIWo7wIxfAG2Kyglbwsq2L9/8OZJuLZC+i+Y45E0u+bTNL0D2BLwQKChRqlEGp/ZgPQ1 JTjs5PGhN3i9bnXyESwrBwlRdlFHcqtPjzGmkJIFJI87Tmr8HPLVdkOvGonfxo3bppZKWCBj KrvVeV5vs470JyCNvcfXm5MI55ykfiI+SrNC5g4keaikrAuLVTarUmClGab3nz3kVhErE3ME czzTCpYNl5DUf4P5GPvH481iOZ3rghjmz+7bc2lnnyPjOrPDFbIGOxtGAbfMYgEAFas/V+9H yB3bZXakn2ykYTWP0HqzGLkBQladCdgXcGv9ZU/myzqClMOJVzNwsT5mNsJU4dkg75UhqHP+ HS8UVVf013xmTvMLgDiV5ypQOqHsU9XoS1pMCoyE0yv3nR/M4+j4L1GL8k8fKU99fwlxvlxF qFXd8KFC/VJazLG5zVCMsWt8N08LEym1VCUIi6oQDkjZJo8FQbHzcDpI1n0/y4UAyvp6cZn+ ++81hnWSIYoThh5CJqEc+qmyl685CBPmO97U0bSDMNUfUHgrNpjJyDr16dlKMAQMxTTgDCd0 l/OUxsfoODMpa4z8cXI2v/Y/9v4TbMmExMDTWfB7LuwOS3LxUaZwNdNALSSYDTQdGLo46H+N +9b+O7xba8cl1FQvosiT7sylfAi58HirqNxxxh/GCmZdEyiD75tLyXU3cRLsaERlLZVtRHvB xCK89hef76IJNnkABgaIw98NraP0vQdmz/z6/UpIRqluH8rreTfCUgCbQORjCF9LaduNNJ3y Ogsj8ca9gijh0d4Kd2BlC1VqzyBI3Fov3/LbX3G7FsHUjYW92w=
  • Ironport-hdrordr: A9a23:TJTq0q9XKEm3OnpksoVuk+FAdb1zdoMgy1knxilNoENuHfBwxv rDoB1E73LJYVYqOU3Jmbi7Sc69qFfnhORICO4qTMqftWjdyRCVxeRZg7cKrAeQeREWmtQtsJ uINpIOdOEYbmIK/PoSgjPIaurIqePvmMvD5Za8854ud3ATV0gJ1XYGNu/xKDwReOApP+tcKH LKjfA32AZINE5nJfiTNz0gZazuttfLnJXpbVovAAMm0hCHiXeN5KThGxaV8x8CW3cXqI1SvF Ttokjc3OGOovu7whjT2yv66IlXosLozp9mCNaXgsYYBz3wgkKDZZhnWZeFoDcpydvfomoCoZ 3pmVMNLs5z43TeciWcpgbs4RDp1HIU53rr2Taj8A3eiP28YAh/J9tKhIpffBecwVEnpstA3K VC2H/cn4ZLDDvb9R6NqeTgZlVPrA6ZsHAimekcgzh0So0FcoJcqoQZ4Qd8DIoAJiTn84oqed MeQ/003MwmMW9yUkqp/VWGmLeXLzYO91a9MwQ/U/WuonlrdCsT9Tpc+CQd9k1wg67VBaM0o9 gsCZ4Y542mePVmGZ6VNN1xMfdfNVa9My4kEFjiaGgPR5t3c04klfbMkcAIDaeRCds18Kc=
  • Ironport-sdr: DeEVe60V6dTAMMjtEmxVfhbRBEL1KS/YpGG0WIIPSVwSyi1Ji/zEsySO1+4VgSCO2Ia+QIzwDy 5V3SiS55RVv4nrRwaU92sM6BwbJ9M4xqLoSqrsCHNRKNi1E/56xxSBFVVKBzkY/MbIZSL4TJ/U IbxfvB7k7M5gLjaYvrkeWU2wyu9tNWOues6QVZFaZkE1j8MDtFtBGl4GRWeLWMK9dPwl866BcA CSXmsB63tIxFSivNF70hF/I8pVR5H6TSVy0WsHj3H8Wo4pT+GYCkeINkhYVJyVjdNI34uf4Qn8 QsPmNBhCHC1g+2lRvMQ/V7GX
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Wed, Jan 26, 2022 at 08:44:45AM +0000, Andrew Cooper wrote:
> This bug existed in early in 2018 between MSR_SPEC_CTRL arriving in microcode,
> and SSBD arriving a few months later.  It went unnoticed presumably because
> everyone was busy rebooting everything.
> 
> The same bug will reappear when adding PSFD support.
> 
> Clamp the guest MSR_SPEC_CTRL value to that permitted by CPUID on migrate.
> The guest is already playing with reserved bits at this point, and clamping
> the value will prevent a migration to a less capable host from failing.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c         | 25 +++++++++++++++++++++++--
>  xen/arch/x86/include/asm/msr.h |  2 ++
>  xen/arch/x86/msr.c             | 33 +++++++++++++++++++++------------
>  3 files changed, 46 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index d7d3299b431e..c4ddb8607d9c 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1340,6 +1340,7 @@ static const uint32_t msrs_to_send[] = {
>  
>  static int hvm_save_cpu_msrs(struct vcpu *v, hvm_domain_context_t *h)
>  {
> +    const struct domain *d = v->domain;
>      struct hvm_save_descriptor *desc = _p(&h->data[h->cur]);
>      struct hvm_msr *ctxt;
>      unsigned int i;
> @@ -1355,7 +1356,8 @@ static int hvm_save_cpu_msrs(struct vcpu *v, 
> hvm_domain_context_t *h)
>      for ( i = 0; i < ARRAY_SIZE(msrs_to_send); ++i )
>      {
>          uint64_t val;
> -        int rc = guest_rdmsr(v, msrs_to_send[i], &val);
> +        unsigned int msr = msrs_to_send[i];
> +        int rc = guest_rdmsr(v, msr, &val);
>  
>          /*
>           * It is the programmers responsibility to ensure that
> @@ -1375,7 +1377,26 @@ static int hvm_save_cpu_msrs(struct vcpu *v, 
> hvm_domain_context_t *h)
>          if ( !val )
>              continue; /* Skip empty MSRs. */
>  
> -        ctxt->msr[ctxt->count].index = msrs_to_send[i];
> +        /*
> +         * Guests are given full access to certain MSRs for performance
> +         * reasons.  A consequence is that Xen is unable to enforce that all
> +         * bits disallowed by the CPUID policy yield #GP, and an enterprising
> +         * guest may be able to set and use a bit it ought to leave alone.
> +         *
> +         * When migrating from a more capable host to a less capable one, 
> such
> +         * bits may be rejected by the destination, and the migration failed.
> +         *
> +         * Discard such bits here on the source side.  Such bits have 
> reserved
> +         * behaviour, and the guest has only itself to blame.
> +         */
> +        switch ( msr )
> +        {
> +        case MSR_SPEC_CTRL:
> +            val &= msr_spec_ctrl_valid_bits(d->arch.cpuid);
> +            break;
> +        }

Should you move the check for !val here, in case the clearing done
here zeros the MSR?

> +
> +        ctxt->msr[ctxt->count].index = msr;
>          ctxt->msr[ctxt->count++].val = val;
>      }
>  
> diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
> index 10039c2d227b..657a3295613d 100644
> --- a/xen/arch/x86/include/asm/msr.h
> +++ b/xen/arch/x86/include/asm/msr.h
> @@ -277,6 +277,8 @@ static inline void wrmsr_tsc_aux(uint32_t val)
>      }
>  }
>  
> +uint64_t msr_spec_ctrl_valid_bits(const struct cpuid_policy *cp);
> +
>  extern struct msr_policy     raw_msr_policy,
>                              host_msr_policy,
>                            pv_max_msr_policy,
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 2cc355575d45..5e80c8b47c21 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -435,6 +435,24 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
> *val)
>      return X86EMUL_EXCEPTION;
>  }
>  
> +/*
> + * Caller to confirm that MSR_SPEC_CTRL is available.  Intel and AMD have
> + * separate CPUID features for this functionality, but only set will be
> + * active.
> + */
> +uint64_t msr_spec_ctrl_valid_bits(const struct cpuid_policy *cp)
> +{
> +    bool ssbd = cp->feat.ssbd;
> +
> +    /*
> +     * Note: SPEC_CTRL_STIBP is specified as safe to use (i.e. ignored)
> +     * when STIBP isn't enumerated in hardware.
> +     */
> +    return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
> +            (ssbd       ? SPEC_CTRL_SSBD       : 0) |
> +            0);

The format here looks weird to me, and I don't get why you
unconditionally or a 0 at the end?

I would also be fine with using cp->feat.ssbd directly here.

Thanks, Roger.



 


Rackspace

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