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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 13, 2020 at 12:20:33PM +0000, Wei Liu wrote:
> On Wed, Feb 12, 2020 at 06:43:47PM +0100, Roger Pau Monné wrote:
> > On Wed, Feb 12, 2020 at 04:09:18PM +0000, Wei Liu wrote:
> > > +static uint64_t flush_tlb_ex(const cpumask_t *mask, const void *va,
> > > +                             unsigned int flags)
> > > +{
> > > +    struct hv_tlb_flush_ex *flush = this_cpu(hv_input_page);
> > > +    int nr_banks;
> > > +    unsigned int max_gvas;
> > > +    unsigned int order = flags & FLUSH_ORDER_MASK;
> > > +    uint64_t ret;
> > > +
> > > +    ASSERT(flush);
> > > +    ASSERT(!local_irq_is_enabled());
> > 
> > Can you turn this into an if condition with ASSERT_UNREACHABLE and
> > return ~0ULL? (as I think that signals an error).
> > 
> 
> There is no need for that. This function will always be internal to
> Hyper-V in the foreseeable future. If it is ever called with IRQ enabled
> something is wrong with the code.

But iff it ever manages to be called violating one of those conditions
things will go badly I assume?

It would be better to stay on the safe side and simply return an error
when the conditions are no meet, and assert in the debug build.

> 
> > > +
> > > +    if ( !(ms_hyperv.hints & HV_X64_EX_PROCESSOR_MASKS_RECOMMENDED) )
> > > +        return ~0ULL;
> > > +
> > > +    flush->address_space = 0;
> > > +    flush->flags = HV_FLUSH_ALL_VIRTUAL_ADDRESS_SPACES;
> > > +    if ( !(flags & FLUSH_TLB_GLOBAL) )
> > > +        flush->flags |= HV_FLUSH_NON_GLOBAL_MAPPINGS_ONLY;
> > > +
> > > +    flush->hv_vp_set.valid_bank_mask = 0;
> > > +    flush->hv_vp_set.format = HV_GENERIC_SET_SPARSE_4K;
> > > +
> > > +    nr_banks = cpumask_to_vpset(&flush->hv_vp_set, mask);
> > > +    if ( nr_banks < 0 )
> > > +        return ~0ULL;
> > > +
> > > +    max_gvas =
> > > +        (PAGE_SIZE - sizeof(*flush) - nr_banks *
> > > +         sizeof(flush->hv_vp_set.bank_contents[0])) /
> > > +        sizeof(uint64_t);       /* gva is represented as uint64_t */
> > > +
> > > +    /*
> > > +     * Flush the entire address space if va is NULL or if there is not
> > > +     * enough space for gva_list.
> > > +     */
> > > +    if ( !va || (ORDER_TO_BYTES(order) / HV_TLB_FLUSH_UNIT) > max_gvas )
> > > +        ret = hv_do_rep_hypercall(HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE_EX, 
> > > 0,
> > > +                                  nr_banks, virt_to_maddr(flush), 0);
> > > +    else
> > > +    {
> > > +        uint64_t *gva_list = (uint64_t *)flush + sizeof(*flush) + 
> > > nr_banks;
> > 
> > Don't you need nr_banks * sizeof(flush->hv_vp_set.bank_contents) in
> > order to calculate the position of the gva_list?
> > 
> 
> The pointer arithmetic is done on uint64_t pointers so it already takes
> into account sizeof(bank_contents[0]).

Oh, then the sizeof(*flush) should be divided by sizeof(uint64_t)?

> > > +        unsigned int gvas = fill_gva_list(gva_list, va, order);
> > > +
> > > +        ret = hv_do_rep_hypercall(HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST_EX,
> > > +                                  gvas, nr_banks, virt_to_maddr(flush), 
> > > 0);
> > > +    }
> > > +
> > > +    return ret;
> > > +}
> > > +
> > >  int hyperv_flush_tlb(const cpumask_t *mask, const void *va,
> > >                       unsigned int flags)
> > >  {
> > > -    return -EOPNOTSUPP;
> > > +    unsigned long irq_flags;
> > > +    struct hv_tlb_flush *flush = this_cpu(hv_input_page);
> > > +    uint64_t ret;
> > > +    unsigned int order = flags & FLUSH_ORDER_MASK;
> > > +    unsigned int max_gvas;
> > > +
> > > +    ASSERT(flush);
> > > +    ASSERT(!cpumask_empty(mask));
> > > +
> > > +    local_irq_save(irq_flags);
> > > +
> > > +    flush->address_space = 0;
> > > +    flush->flags = HV_FLUSH_ALL_VIRTUAL_ADDRESS_SPACES;
> > > +    flush->processor_mask = 0;
> > > +    if ( !(flags & FLUSH_TLB_GLOBAL) )
> > > +        flush->flags |= HV_FLUSH_NON_GLOBAL_MAPPINGS_ONLY;
> > > +
> > > +    if ( cpumask_equal(mask, &cpu_online_map) )
> > > +        flush->flags |= HV_FLUSH_ALL_PROCESSORS;
> > > +    else
> > > +    {
> > > +        int cpu;
> > 
> > unsigned int.
> > 
> 
> I picked int here and above because all the cpumask functions return
> int. I don't mind changing it to unsigned int -- it makes no practical
> difference.

Those should likely return unsigned ints also, as I don't think
cpumask can return errors. I prefer unsigned int, since negative cpu
values make no sense.

> > > +
> > > +        /*
> > > +         * Normally VP indices are in ascending order and match Xen's
> > > +         * idea of CPU ids. Check the last index to see if VP index is
> > > +         * >= 64. If so, we can skip setting up parameters for
> > > +         * non-applicable hypercalls without looking further.
> > > +         */
> > > +        if ( hv_vp_index(cpumask_last(mask)) >= 64 )
> > > +            goto do_ex_hypercall;
> > > +
> > > +        for_each_cpu ( cpu, mask )
> > > +        {
> > > +            uint32_t vpid = hv_vp_index(cpu);
> > > +
> > > +            if ( vpid > ms_hyperv.max_vp_index )
> > > +            {
> > > +                local_irq_restore(irq_flags);
> > > +                return -ENXIO;
> > > +            }
> > > +
> > > +            if ( vpid >= 64 )
> > > +                goto do_ex_hypercall;
> > > +
> > > +            __set_bit(vpid, &flush->processor_mask);
> > > +        }
> > > +    }
> > > +
> > > +    max_gvas = (PAGE_SIZE - sizeof(*flush)) / sizeof(flush->gva_list[0]);
> > > +
> > > +    /*
> > > +     * Flush the entire address space if va is NULL or if there is not
> > > +     * enough space for gva_list.
> > > +     */
> > > +    if ( !va || (ORDER_TO_BYTES(order) / HV_TLB_FLUSH_UNIT) > max_gvas )
> > > +        ret = hv_do_hypercall(HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE,
> > > +                              virt_to_maddr(flush), 0);
> > > +    else
> > > +    {
> > > +        unsigned int gvas = fill_gva_list(flush->gva_list, va, order);
> > > +
> > > +        ret = hv_do_rep_hypercall(HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST, 
> > > gvas, 0,
> > > +                                  virt_to_maddr(flush), 0);
> > > +    }
> > > +
> > > +    goto done;
> > > +
> > > + do_ex_hypercall:
> > > +    ret = flush_tlb_ex(mask, va, flags);
> > > +
> > > + done:
> > > +    local_irq_restore(irq_flags);
> > > +
> > > +    return ret & HV_HYPERCALL_RESULT_MASK;
> > 
> > Will this return an error code that uses the same space as Xen's errno
> > values?
> > 
> 
> No, it won't. It returns Hyper-V's status code (0 still means success).
> 
> I didn't think that was a big deal because non-zero values meant errors.
> And the upper layer didn't care about the exact error values (yet).

Hm, I would rather have this return an error value in the errno.h
range. ie:

return ret & HV_HYPERCALL_RESULT_MASK ? -EINVAL : 0;

Or something along this lines, but long term you will need some kind
of mapping between HyperV and Xen error codes IMO.

Thanks, Roger.

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