Re: [Xen-devel] [PATCH v3 3/3] x86/hyperv: L0 assisted TLB flush

[Michael Kelley <mikelley@xxxxxxxxxxxxx>]

From: Wei Liu <wei.liu.xen@xxxxxxxxx> On Behalf Of Wei Liu

[snip]

> diff --git a/xen/arch/x86/guest/hyperv/util.c 
> b/xen/arch/x86/guest/hyperv/util.c
> new file mode 100644
> index 0000000000..0abb37b05f
> --- /dev/null
> +++ b/xen/arch/x86/guest/hyperv/util.c
> @@ -0,0 +1,74 @@
> +/**************************************************************************
> ****
> + * arch/x86/guest/hyperv/util.c
> + *
> + * Hyper-V utility functions
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see https://www.gnu.org/licenses/.
> + *
> + * Copyright (c) 2020 Microsoft.
> + */
> +
> +#include <xen/cpu.h>
> +#include <xen/cpumask.h>
> +#include <xen/errno.h>
> +
> +#include <asm/guest/hyperv.h>
> +#include <asm/guest/hyperv-tlfs.h>
> +
> +#include "private.h"
> +
> +int cpumask_to_vpset(struct hv_vpset *vpset,
> +                     const cpumask_t *mask)
> +{
> +    int nr = 1;
> +    unsigned int cpu, vcpu_bank, vcpu_offset;
> +    unsigned int max_banks = ms_hyperv.max_vp_index / 64;
> +
> +    /* Up to 64 banks can be represented by valid_bank_mask */
> +    if ( max_banks > 64 )
> +        return -E2BIG;
> +
> +    /* Clear all banks to avoid flushing unwanted CPUs */
> +    for ( vcpu_bank = 0; vcpu_bank < max_banks; vcpu_bank++ )
> +        vpset->bank_contents[vcpu_bank] = 0;
> +
> +    vpset->valid_bank_mask = 0;
> +    vpset->format = HV_GENERIC_SET_SPARSE_4K;
> +
> +    for_each_cpu ( cpu, mask )
> +    {
> +        unsigned int vcpu = hv_vp_index(cpu);
> +
> +        vcpu_bank = vcpu / 64;
> +        vcpu_offset = vcpu % 64;
> +
> +        __set_bit(vcpu_offset, &vpset->bank_contents[vcpu_bank]);
> +        __set_bit(vcpu_bank, &vpset->valid_bank_mask);

This approach to setting the bits in the valid_bank_mask causes a bug.
If an entire 64-bit word in the bank_contents array is zero because there
are no CPUs in that range, the corresponding bit in valid_bank_mask still
must be set to tell Hyper-V that the 64-bit word is present in the array
and should be processed, even though the content is zero.  A zero bit
in valid_bank_mask indicates that the corresponding 64-bit word in the
array is not present, and every 64-bit word above it has been shifted down.
That's why the similar Linux function sets valid_bank_mask the way that
it does.

Michael

> +
> +        if ( vcpu_bank >= nr )
> +            nr = vcpu_bank + 1;
> +    }
> +
> +    return nr;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel