Re: [PATCH v6 4/4] x86/xen: Add "nopv" support for HVM guest

[Boris Ostrovsky]

On 7/9/19 10:07 PM, Zhenzhong Duan wrote:
> On 2019/7/9 22:54, Boris Ostrovsky wrote:
>> On 7/9/19 12:20 AM, Zhenzhong Duan wrote:
>>> -const __initconst struct hypervisor_x86 x86_hyper_xen_hvm = {
>>> +static uint32_t __init xen_platform_hvm(void)
>>> +{
>>> +ÂÂÂÂÂÂ uint32_t xen_domain = xen_cpuid_base();
>>> +ÂÂÂÂÂÂ struct x86_hyper_init *h = &x86_hyper_xen_hvm.init;
>>> +
>>> +ÂÂÂÂÂÂ if (xen_pv_domain())
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return 0;
>>> +
>>> +ÂÂÂÂÂÂ if (xen_pvh_domain() && nopv) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* Guest booting via the Xen-PVH boot entry goes
>>> here */
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_info("\"nopv\" parameter is ignored in PVH
>>> guest\n");
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ nopv = false;
>>> +ÂÂÂÂÂÂ } else if (nopv && xen_domain) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /*
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ * Guest booting via normal boot entry (like via
>>> grub2) goes
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ * here.
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ *
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ * Use interface functions for bare hardware if nopv,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ * xen_hvm_guest_late_init is an exception as we
>>> need to
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ * detect PVH and panic there.
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ */
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ memcpy(h, (void *)&x86_init.hyper,
>>> sizeof(x86_init.hyper));
>>
>> And this worked? I'd think it would fail since h points to RO section.
> Yes, I have below changes in the patch.
>
> -const __initconst struct hypervisor_x86 x86_hyper_xen_hvm = {
> +struct hypervisor_x86 x86_hyper_xen_hvm __initdata = {


But hypervisors[] stays__initconst so that I thought could be a problem.
But apparently it's not.

>
>>
>>
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ memcpy(&x86_hyper_xen_hvm.runtime, (void
>>> *)&x86_platform.hyper,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(x86_platform.hyper));
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ h->guest_late_init = xen_hvm_guest_late_init;
>>
>> To me this still doesn't look right --- you are making assumptions about
>> x86_platform/x86_init.hyper and I don't think you can assume they have
>> not been set to point to another hypervisor, for example.
>
> You mean copy_array() calls in init_hypervisor_platform()? But that
> happens after
>
> detect_hypervisor_vendor() shoose out the prefered hypervisor. In
> detect stage,
>
> x86_platform/x86_init.hyper has default value for bare hardware, or I
> missed something?


Right. My point though is that this ordering is opaque to
hypervisor-specific code and can change. The same is true about making
assumptions about x86_platform/x86_init.hyper values.

-boris