Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/01/2020 17:43, Wei Liu wrote:
> On Tue, Jan 07, 2020 at 03:42:14PM +0000, Wei Liu wrote:
>> On Sun, Jan 05, 2020 at 09:57:56PM +0000, Andrew Cooper wrote:
>> [...]
>>>>> The locked bit is probably a good idea, but one aspect missing here is
>>>>> the check to see whether the hypercall page is already enabled, which I
>>>>> expect is for a kexec crash scenario.
>>>>>
>>>>> However, the most important point is the one which describes the #GP
>>>>> properties of the guest trying to modify the page.  This can only be
>>>>> achieved with an EPT/NPT mapping lacking the W permission, which will
>>>>> shatter host superpages.   Therefore, putting it in .text is going to be
>>>>> rather poor, perf wise.
>>>>>
>>>>> I also note that Xen's implementation of the Viridian hypercall page
>>>>> doesn't conform to these properties, and wants fixing.  It is going to
>>>>> need a new kind identification of the page (probably a new p2m type)
>>>>> which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
>>>>>
>>>>> As for suggestions here, I'm struggling to find any memory map details
>>>>> exposed in the Viridian interface, and therefore which gfn is best to
>>>>> choose.  I have a sinking feeling that the answer is ACPI...
>>>> TLFS only says "go find one suitable page yourself" without further
>>>> hints.
>>>>
>>>> Since we're still quite far away from a functioning system, finding a
>>>> most suitable page isn't my top priority at this point. If there is a
>>>> simple way to extrapolate suitable information from ACPI, that would be
>>>> great. If it requires writing a set of functionalities, than that will
>>>> need to wait till later.
>>> To cope with the "one is already established and it is already locked"
>>> case, the only option is to have a fixmap entry which can be set
>>> dynamically.  The problem is that the fixmap region is marked NX and 64G
>>> away from .text.
>>>
>>> Possibly the least bad option is to have some build-time space (so 0 or
>>> 4k depending on CONFIG_HYPERV) between the per-cpu stubs and
>>> XEN_VIRT_END, which operates like the fixmap, but ends up as X/RO mappings.
>>>
>> OK. This is probably not too difficult. 
>>
> I have a closer look at this today and want to make sure I understand
> what you had in mind.
>
> Suppose we set aside some space in linker script. Using the following
> will give you a WAX section. I still need to add CONFIG_GUEST around it,
> but you get the idea.
>
>
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 111edb5360..a7af321139 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -305,6 +305,15 @@ SECTIONS
>         . = ALIGN(POINTER_ALIGN);
>         __bss_end = .;
>    } :text
> +
> +  . = ALIGN(SECTION_ALIGN);
> +  DECL_SECTION(.text.hypercall_page) {
> +       . = ALIGN(PAGE_SIZE);
> +       __hypercall_page_start = .;
> +       . = . + PAGE_SIZE;
> +       __hypercall_page_end = .;
> +  } :text=0x9090
> +
>    _end = . ;
>  
>    . = ALIGN(SECTION_ALIGN);
>
>
> And then? Use map_pages_to_xen(..., PAGE_HYPERVISOR_RX) to point that
> address to (MAXPHYSADDR-PAGE_SIZE)?

Ah no.  This still puts the hypercall page (or at least, space for it)
in the Xen image itself, which is something we are trying to avoid.

What I meant was to basically copy(/extend) the existing fixmap
implementation, calling it fixmap_x() (or something better), and put
FIXMAP_X_SIZE as an additional gap in the calculation
alloc_stub_page().  Even the current fixmap code has an ifdef example
for CONFIG_XEN_GUEST.

You'd then end up with something like
__set_fixmap_x(FIXMAP_X_HYPERV_HYPERCALL_PAGE, mfn) which uses _RX in
the underlying call to map_pages_to_xen()

~Andrew

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