On Tue, Sep 15, 2020 at 01:00:55PM +0200, Vitaly Kuznetsov wrote:
> Wei Liu <wei.liu@xxxxxxxxxx> writes:
>
> > They are used to deposit pages into Microsoft Hypervisor and bring up
> > logical and virtual processors.
> >
> > Signed-off-by: Lillian Grassin-Drake <ligrassi@xxxxxxxxxxxxx>
> > Signed-off-by: Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxx>
> > Signed-off-by: Nuno Das Neves <nudasnev@xxxxxxxxxxxxx>
> > Co-Developed-by: Lillian Grassin-Drake <ligrassi@xxxxxxxxxxxxx>
> > Co-Developed-by: Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxx>
> > Co-Developed-by: Nuno Das Neves <nudasnev@xxxxxxxxxxxxx>
> > Signed-off-by: Wei Liu <wei.liu@xxxxxxxxxx>
> > ---
> > arch/x86/hyperv/Makefile | 2 +-
> > arch/x86/hyperv/hv_proc.c | 209 ++++++++++++++++++++++++++++++
> > arch/x86/include/asm/mshyperv.h | 4 +
> > include/asm-generic/hyperv-tlfs.h | 56 ++++++++
> > 4 files changed, 270 insertions(+), 1 deletion(-)
> > create mode 100644 arch/x86/hyperv/hv_proc.c
> >
> > diff --git a/arch/x86/hyperv/Makefile b/arch/x86/hyperv/Makefile
> > index 89b1f74d3225..565358020921 100644
> > --- a/arch/x86/hyperv/Makefile
> > +++ b/arch/x86/hyperv/Makefile
> > @@ -1,6 +1,6 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > obj-y := hv_init.o mmu.o nested.o
> > -obj-$(CONFIG_X86_64) += hv_apic.o
> > +obj-$(CONFIG_X86_64) += hv_apic.o hv_proc.o
> >
> > ifdef CONFIG_X86_64
> > obj-$(CONFIG_PARAVIRT_SPINLOCKS) += hv_spinlock.o
> > diff --git a/arch/x86/hyperv/hv_proc.c b/arch/x86/hyperv/hv_proc.c
> > new file mode 100644
> > index 000000000000..847c72465d0e
> > --- /dev/null
> > +++ b/arch/x86/hyperv/hv_proc.c
> > @@ -0,0 +1,209 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/types.h>
> > +#include <linux/version.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/mm.h>
> > +#include <linux/clockchips.h>
> > +#include <linux/acpi.h>
> > +#include <linux/hyperv.h>
> > +#include <linux/slab.h>
> > +#include <linux/cpuhotplug.h>
> > +#include <asm/hypervisor.h>
> > +#include <asm/mshyperv.h>
> > +#include <asm/apic.h>
> > +
> > +#include <asm/trace/hyperv.h>
> > +
> > +#define HV_DEPOSIT_MAX_ORDER (8)
> > +#define HV_DEPOSIT_MAX (1 << HV_DEPOSIT_MAX_ORDER)
> > +
> > +#define MAX(a, b) ((a) > (b) ? (a) : (b))
> > +#define MIN(a, b) ((a) < (b) ? (a) : (b))
>
> Nit: include/linux/kernel.h defines min() and max() macros with type
> checking.
Fixed.
>
> > +
> > +/*
> > + * 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;
> > + int status;
> > + int ret;
> > + u64 base_pfn;
> > + struct hv_deposit_memory *input_page;
> > + unsigned long flags;
> > +
> > + if (num_pages > HV_DEPOSIT_MAX)
> > + return -EINVAL;
> > + if (!num_pages)
> > + return 0;
> > +
> > + ret = -ENOMEM;
> > +
> > + /* One buffer for page pointers and counts */
> > + pages = page_address(alloc_page(GFP_KERNEL));
> > + if (!pages)
> > + goto free_buf;
>
> There is nothing to free, just do 'return -ENOMEM' here;
>
> > + counts = (int *)&pages[256];
> > +
>
> Oh this is weird. So 'pages' is an array of 512 'struct page *' items
> and we use its second half (pages[256]) for an array of signed(!)
> integers(!). Can we use a locally defined struct or something better for
> that?
>
This can be changed. I will make the counts array have its own buffer.
> > + /* 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);
> > + do {
> > + pages[i] = alloc_pages_node(node, GFP_KERNEL, order);
> > + if (!pages[i]) {
> > + if (!order) {
> > + goto err_free_allocations;
> > + }
> > + --order;
> > + }
> > + } while (!pages[i]);
> > +
> > + split_page(pages[i], order);
> > + counts[i] = 1 << order;
> > + num_pages -= counts[i];
> > + i++;
>
> So here we believe we will never overrun the 2048 bytes we 'allocated'
> for 'counts' above. While 'if (num_pages > HV_DEPOSIT_MAX)' presumably
> guarantees that, this is not really obvious.
>
This is moot since counts is going to have its own buffer allocated with
kcalloc(HV_DEPOSIT_MAX, sizeof(int), ...).
> > + num_allocations++;
> > + }
> > +
> > + 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;
> > + local_irq_restore(flags);
> > +
> > + if (status != HV_STATUS_SUCCESS) {
>
> Nit: same like in one ov the previous patches, status can be 'u16'.
>
Fixed.
> > + 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);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_call_deposit_pages);
> > +
> > +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;
> > +
> > + 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 = node_to_pxm(node);
> > + 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) {
> > + if (status != HV_STATUS_SUCCESS) {
> > + pr_err("%s: cpu %u apic ID %u, %d\n", __func__,
> > + lp_index, apic_id, status);
> > + ret = status;
> > + }
> > + break;
>
> So if status == HV_STATUS_SUCCESS we break and avoid
> hv_call_deposit_pages() below?
>
Yes, that means adding the logical processor has succeeded. There is
nothing more to do.
> > + }
> > + ret = hv_call_deposit_pages(node, hv_current_partition_id, 1);
> > +
> > + } while (!ret);
>
> And if hv_call_deposit_pages() returns '0' we keep doing something? Sorry
> but I'm probably missing something important in the 'depositing'
> process, could you please add a comment explaining what's going on here?
>
We only get here because 1) there isn't sufficient memory in the last
iteration, 2) we've succeeded in adding a bit more memory. In this case
we will want to retry adding the logical processor.
I will add a comment before the loop.
> > +
> > + return ret;
> > +}
> > +
[...]
> > diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
> > index 87b1a79b19eb..2b05bed712c0 100644
> > --- a/include/asm-generic/hyperv-tlfs.h
> > +++ b/include/asm-generic/hyperv-tlfs.h
> > @@ -142,6 +142,8 @@ struct ms_hyperv_tsc_page {
> > #define HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST_EX 0x0014
> > #define HVCALL_SEND_IPI_EX 0x0015
> > #define HVCALL_GET_PARTITION_ID 0x0046
> > +#define HVCALL_DEPOSIT_MEMORY 0x0048
> > +#define HVCALL_CREATE_VP 0x004e
> > #define HVCALL_GET_VP_REGISTERS 0x0050
> > #define HVCALL_SET_VP_REGISTERS 0x0051
> > #define HVCALL_POST_MESSAGE 0x005c
> > @@ -149,6 +151,7 @@ struct ms_hyperv_tsc_page {
> > #define HVCALL_POST_DEBUG_DATA 0x0069
> > #define HVCALL_RETRIEVE_DEBUG_DATA 0x006a
> > #define HVCALL_RESET_DEBUG_SESSION 0x006b
> > +#define HVCALL_ADD_LOGICAL_PROCESSOR 0x0076
> > #define HVCALL_RETARGET_INTERRUPT 0x007e
> > #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE 0x00af
> > #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_LIST 0x00b0
> > @@ -413,6 +416,59 @@ struct hv_get_partition_id {
> > u64 partition_id;
> > } __packed;
> >
> > +/* HvDepositMemory hypercall */
> > +struct hv_deposit_memory {
> > + u64 partition_id;
> > + u64 gpa_page_list[];
> > +};
>
> Other structures above have '__packed' and I remember there were
> different opinions if it is needed or not (for properly padded
> structures). I'd suggest we stay consitent and keep adding it unless we
> decide to get rid of them (but you've added it to the newly introduced
> hv_get_partition_id above).
Fixed.
> > +
> > +
> > +struct hv_proximity_domain_flags {
> > + u32 proximity_preferred : 1;
> > + u32 reserved : 30;
> > + u32 proximity_info_valid : 1;
> > +};
> > +
> > +/* Not a union in windows but useful for zeroing */
> > +union hv_proximity_domain_info {
> > + struct {
> > + u32 domain_id;
> > + struct hv_proximity_domain_flags flags;
> > + };
> > + u64 as_uint64;
> > +};
> > +
> > +struct hv_lp_startup_status {
> > + u64 hv_status;
> > + u64 substatus1;
> > + u64 substatus2;
> > + u64 substatus3;
> > + u64 substatus4;
> > + u64 substatus5;
> > + u64 substatus6;
> > +};
> > +
> > +/* HvAddLogicalProcessor hypercalls */
>
> s/hypercalls/hypercall/
Fixed.
Wei.
