Re: [Xen-devel] [PATCH v4 2/7] x86/hyperv: setup hypercall page

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.01.2020 21:23, Wei Liu wrote:
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -36,6 +36,22 @@ boolean_param("e820-verbose", e820_verbose);
>  struct e820map e820;
>  struct e820map __initdata e820_raw;
>  
> +static unsigned int find_phys_addr_bits(void)
> +{
> +    uint32_t eax;
> +    unsigned int phys_bits = 36;
> +
> +    eax = cpuid_eax(0x80000000);
> +    if ( (eax >> 16) == 0x8000 && eax >= 0x80000008 )
> +    {
> +        phys_bits = (uint8_t)cpuid_eax(0x80000008);
> +        if ( phys_bits > PADDR_BITS )
> +            phys_bits = PADDR_BITS;
> +    }
> +
> +    return phys_bits;
> +}

Instead of this, how about pulling further ahead the call to
early_cpu_init() in setup.c? (Otherwise the function wants to
be __init at least.)

> @@ -357,6 +373,21 @@ static unsigned long __init find_max_pfn(void)
>              max_pfn = end;
>      }
>  
> +#ifdef CONFIG_HYPERV_GUEST
> +    {
> +     /*
> +      * We reserve the top-most page for hypercall page. Adjust
> +      * max_pfn if necessary.
> +      */
> +        unsigned int phys_bits = find_phys_addr_bits();
> +        unsigned long hcall_pfn =
> +          ((1ull << phys_bits) - 1) >> PAGE_SHIFT;
> +
> +        if ( max_pfn >= hcall_pfn )
> +          max_pfn = hcall_pfn - 1;
> +    }
> +#endif

This wants abstracting away: There shouldn't be Hyper-V specific
code in here if at all possible, and the adjustment also shouldn't
be made unless actually running on Hyper-V.

> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -18,17 +18,27 @@
>   *
>   * Copyright (c) 2019 Microsoft.
>   */
> +#include <xen/version.h>
>  #include <xen/init.h>

Please sort alphabetically.

> +#include <asm/fixmap.h>
>  #include <asm/guest.h>
>  #include <asm/guest/hyperv-tlfs.h>
> +#include <asm/processor.h>
>  
>  struct ms_hyperv_info __read_mostly ms_hyperv;
>  
> -static const struct hypervisor_ops ops = {
> -    .name = "Hyper-V",
> -};
> +static uint64_t generate_guest_id(void)
> +{
> +    uint64_t id = 0;

Pointless initializer.

> +    id = (uint64_t)HV_XEN_VENDOR_ID << 48;
> +    id |= (xen_major_version() << 16) | xen_minor_version();
> +
> +    return id;
> +}
>  
> +static const struct hypervisor_ops ops;
>  const struct hypervisor_ops *__init hyperv_probe(void)

Blank line between these two please (if you can't get away without
this declaration in the first place).

> @@ -72,6 +82,43 @@ const struct hypervisor_ops *__init hyperv_probe(void)
>      return &ops;
>  }
>  
> +static void __init setup_hypercall_page(void)
> +{
> +    union hv_x64_msr_hypercall_contents hypercall_msr;
> +    union hv_guest_os_id guest_id;
> +    unsigned long mfn;
> +
> +    rdmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
> +    if ( !guest_id.raw )
> +    {
> +        guest_id.raw = generate_guest_id();
> +        wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
> +    }
> +
> +    rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +    if ( !hypercall_msr.enable )
> +    {
> +        mfn = ((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT;

Along the lines of the abstracting-away request above: How is
anyone to notice what else needs changing if it is decided
that this page gets moved elsewhere?

> +        hypercall_msr.enable = 1;
> +        hypercall_msr.guest_physical_address = mfn;
> +        wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);

So on Hyper-V the hypervisor magically populates this page as
a side effect of the MSR write?

Jan

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