On Tue, Jan 26, 2021 at 01:20:36AM +0000, Michael Kelley wrote:
> From: Wei Liu <wei.liu@xxxxxxxxxx> Sent: Wednesday, January 20, 2021 4:01 AM
[...]
> > +#include <asm/trace/hyperv.h>
> > +
> > +#define HV_DEPOSIT_MAX_ORDER (8)
> > +#define HV_DEPOSIT_MAX (1 << HV_DEPOSIT_MAX_ORDER)
>
> Is there any reason to not let the maximum be 511, which is
> how many entries will fit on the hypercall input page? The
> max could be define in terms of HY_HYP_PAGE_SIZE so that
> the logical dependency is fully expressed.
Let me try changing this. This file is largely authored by Lilian and
Nuno. I don't see a particular reason why the value can't be larger.
I've updated the value to the following.
/*
* See struct hv_deposit_memory. The first u64 is partition ID, the rest
* are GPAs.
*/
#define HV_DEPOSIT_MAX (HV_HYP_PAGE_SIZE / sizeof(u64) - 1)
Let's see how that goes. I will test it once I fix other places.
>
> > +
> > +/*
> > + * Deposits exact number of pages
> > + * Must be called with interrupts enabled
> > + * Max 256 pages
> > + */
> > +int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
> > +{
> > + struct page **pages;
> > + int *counts;
> > + int num_allocations;
> > + int i, j, page_count;
> > + int order;
> > + int desired_order;
> > + u16 status;
> > + int ret;
> > + u64 base_pfn;
> > + struct hv_deposit_memory *input_page;
> > + unsigned long flags;
> > +
> > + if (num_pages > HV_DEPOSIT_MAX)
> > + return -E2BIG;
> > + if (!num_pages)
> > + return 0;
> > +
> > + /* One buffer for page pointers and counts */
> > + pages = page_address(alloc_page(GFP_KERNEL));
> > + if (!pages)
>
> Does the above check work? If alloc_pages() returns NULL, it looks like
> page_address() might fault.
>
Good catch. Fixed.
> > + return -ENOMEM;
> > +
> > + counts = kcalloc(HV_DEPOSIT_MAX, sizeof(int), GFP_KERNEL);
> > + if (!counts) {
> > + free_page((unsigned long)pages);
> > + return -ENOMEM;
> > + }
> > +
> > + /* Allocate all the pages before disabling interrupts */
> > + num_allocations = 0;
> > + i = 0;
> > + order = HV_DEPOSIT_MAX_ORDER;
> > +
> > + while (num_pages) {
> > + /* Find highest order we can actually allocate */
> > + desired_order = 31 - __builtin_clz(num_pages);
> > + order = min(desired_order, order);
>
> The above seems redundant since request sizes larger than the
> max have already been rejected.
>
min(...) can be dropped.
> > + do {
> > + pages[i] = alloc_pages_node(node, GFP_KERNEL, order);
> > + if (!pages[i]) {
> > + if (!order) {
> > + ret = -ENOMEM;
> > + goto err_free_allocations;
> > + }
> > + --order;
> > + }
> > + } while (!pages[i]);
>
> The duplicative test of !pages[i] is somewhat annoying. How about
> this:
>
> while{!pages[i] = alloc_pages_node(node, GFP_KERNEL, order) {
> if (!order) {
> ret = -ENOMEM;
> goto err_free_allocations;
> }
> --order;
> }
>
> or if you don't like doing an assignment in the while test:
>
> while(1) {
> pages[i] = alloc_pages_node(node, GFP_KERNEL, order);
> if (page[i])
> break;
> if (!order) {
> ret = -ENOMEM;
> goto err_free_allocations;
> }
> --order;
> }
>
I will use this variant.
> > +
> > + split_page(pages[i], order);
> > + counts[i] = 1 << order;
> > + num_pages -= counts[i];
> > + i++;
> > + num_allocations++;
>
> Incrementing both I and num_allocations in the loop seems
> redundant, especially since num_allocations isn't used in the loop.
> Could num_allocations be assigned the value of i once the loop
> is exited? (and num_allocations would not need to be initialized to 0.)
> Would also have to do the assignment in the error case.
>
Yes. That can be done.
> > + }
> > +
> > + local_irq_save(flags);
> > +
> > + input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
> > +
> > + input_page->partition_id = partition_id;
> > +
> > + /* Populate gpa_page_list - these will fit on the input page */
> > + for (i = 0, page_count = 0; i < num_allocations; ++i) {
> > + base_pfn = page_to_pfn(pages[i]);
> > + for (j = 0; j < counts[i]; ++j, ++page_count)
> > + input_page->gpa_page_list[page_count] = base_pfn + j;
> > + }
> > + status = hv_do_rep_hypercall(HVCALL_DEPOSIT_MEMORY,
> > + page_count, 0, input_page,
> > + NULL) & HV_HYPERCALL_RESULT_MASK;
>
> Similar comment about how hypercall status is checked.
>
Fixed.
> > + local_irq_restore(flags);
> > +
> > + if (status != HV_STATUS_SUCCESS) {
> > + pr_err("Failed to deposit pages: %d\n", status);
> > + ret = status;
> > + goto err_free_allocations;
> > + }
> > +
> > + ret = 0;
> > + goto free_buf;
> > +
> > +err_free_allocations:
> > + for (i = 0; i < num_allocations; ++i) {
> > + base_pfn = page_to_pfn(pages[i]);
> > + for (j = 0; j < counts[i]; ++j)
> > + __free_page(pfn_to_page(base_pfn + j));
> > + }
> > +
> > +free_buf:
> > + free_page((unsigned long)pages);
> > + kfree(counts);
> > + return ret;
> > +}
> > +
> > +int hv_call_add_logical_proc(int node, u32 lp_index, u32 apic_id)
> > +{
> > + struct hv_add_logical_processor_in *input;
> > + struct hv_add_logical_processor_out *output;
> > + int status;
> > + unsigned long flags;
> > + int ret = 0;
> > +#ifdef CONFIG_ACPI_NUMA
> > + int pxm = node_to_pxm(node);
> > +#else
> > + int pxm = 0;
> > +#endif
>
> It seems like the above #ifdef'ery might be better fixed in
> include/acpi/acpi_numa.h, where there's already a null definition
> of pxm_to_node() in case CONFIG_ACPI_NUMA isn't defined. There
> should also be a null definition of node_to_pxm() in that file.
>
Sure.
> > +
> > + /*
> > + * When adding a logical processor, the hypervisor may return
> > + * HV_STATUS_INSUFFICIENT_MEMORY. When that happens, we deposit more
> > + * pages and retry.
> > + */
> > + do {
> > + local_irq_save(flags);
> > +
> > + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> > + /* We don't do anything with the output right now */
> > + output = *this_cpu_ptr(hyperv_pcpu_output_arg);
> > +
> > + input->lp_index = lp_index;
> > + input->apic_id = apic_id;
> > + input->flags = 0;
> > + input->proximity_domain_info.domain_id = pxm;
> > + input->proximity_domain_info.flags.reserved = 0;
> > + input->proximity_domain_info.flags.proximity_info_valid = 1;
> > + input->proximity_domain_info.flags.proximity_preferred = 1;
> > + status = hv_do_hypercall(HVCALL_ADD_LOGICAL_PROCESSOR,
> > + input, output);
> > + local_irq_restore(flags);
> > +
> > + if (status != HV_STATUS_INSUFFICIENT_MEMORY) {
>
> The 'and' with HV_HYPERCALL_RESULT_MASK isn't coded anywhere for this
> hypercall, and 'status' is declared as 'int'.
>
Fixed.
> > + if (status != HV_STATUS_SUCCESS) {
> > + pr_err("%s: cpu %u apic ID %u, %d\n", __func__,
> > + lp_index, apic_id, status);
> > + ret = status;
> > + }
> > + break;
> > + }
> > + ret = hv_call_deposit_pages(node, hv_current_partition_id, 1);
> > + } while (!ret);
> > +
> > + return ret;
> > +}
> > +
> > +int hv_call_create_vp(int node, u64 partition_id, u32 vp_index, u32 flags)
> > +{
> > + struct hv_create_vp *input;
> > + u16 status;
> > + unsigned long irq_flags;
> > + int ret = 0;
> > +#ifdef CONFIG_ACPI_NUMA
> > + int pxm = node_to_pxm(node);
> > +#else
> > + int pxm = 0;
> > +#endif
>
> Same comment.
>
> > +
> > + /* Root VPs don't seem to need pages deposited */
> > + if (partition_id != hv_current_partition_id) {
> > + ret = hv_call_deposit_pages(node, partition_id, 90);
>
> Perhaps add a comment about the value "90". Was it
> empirically determined?
I think so. I will add a comment.
>
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + do {
> > + local_irq_save(irq_flags);
> > +
> > + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> > +
> > + input->partition_id = partition_id;
> > + input->vp_index = vp_index;
> > + input->flags = flags;
> > + input->subnode_type = HvSubnodeAny;
> > + if (node != NUMA_NO_NODE) {
> > + input->proximity_domain_info.domain_id = pxm;
> > + input->proximity_domain_info.flags.reserved = 0;
> > + input->proximity_domain_info.flags.proximity_info_valid = 1;
> > + input->proximity_domain_info.flags.proximity_preferred = 1;
> > + } else {
> > + input->proximity_domain_info.as_uint64 = 0;
> > + }
> > + status = hv_do_hypercall(HVCALL_CREATE_VP, input, NULL);
> > + local_irq_restore(irq_flags);
> > +
> > + if (status != HV_STATUS_INSUFFICIENT_MEMORY) {
>
> Same problems with the status check.
Fixed.
>
> > + if (status != HV_STATUS_SUCCESS) {
> > + pr_err("%s: vcpu %u, lp %u, %d\n", __func__,
> > + vp_index, flags, status);
> > + ret = status;
> > + }
> > + break;
> > + }
> > + ret = hv_call_deposit_pages(node, partition_id, 1);
> > +
> > + } while (!ret);
> > +
> > + return ret;
> > +}
> > +
> > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> > index 67f5d35a73d3..4e590a167160 100644
> > --- a/arch/x86/include/asm/mshyperv.h
> > +++ b/arch/x86/include/asm/mshyperv.h
[...]
> > +/* HvAddLogicalProcessor hypercall */
> > +struct hv_add_logical_processor_in {
> > + u32 lp_index;
> > + u32 apic_id;
> > + union hv_proximity_domain_info proximity_domain_info;
> > + u64 flags;
> > +};
>
> __packed is missing from this struct definition
>
Fixed.
> > +
> > +struct hv_add_logical_processor_out {
> > + struct hv_lp_startup_status startup_status;
> > +} __packed;
> > +
> > +enum HV_SUBNODE_TYPE
> > +{
> > + HvSubnodeAny = 0,
> > + HvSubnodeSocket,
> > + HvSubnodeAmdNode,
> > + HvSubnodeL3,
> > + HvSubnodeCount,
> > + HvSubnodeInvalid = -1
> > +};
>
> Are these values defined by Hyper-V? If so, explicitly coding the
> value of each enum member might be better.
Fixed.
Wei.
