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

[Zhenzhong Duan]

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 = {

> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 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?

Just realized I can use memset to zero instead of memcpy which looks 
more rational.

> Would modifying all x86_hyper_xen_hvm's ops (except, I guess,
> xen_hvm_guest_late_init()) to immediately return if nopv is set work?

I think so, Let me try it.

Zhenzhong