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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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