Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode

From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
To: Anchal Agarwal <anchalag@amazon.com>
Cc: eduval@amazon.com, len.brown@intel.com, peterz@infradead.org,
	benh@kernel.crashing.org, x86@kernel.org, linux-mm@kvack.org,
	pavel@ucw.cz, hpa@zytor.com, sstabellini@kernel.org,
	kamatam@amazon.com, mingo@redhat.com,
	xen-devel@lists.xenproject.org, sblbir@amazon.com,
	axboe@kernel.dk, konrad.wilk@oracle.com, bp@alien8.de,
	tglx@linutronix.de, jgross@suse.com, netdev@vger.kernel.org,
	linux-pm@vger.kernel.org, rjw@rjwysocki.net,
	linux-kernel@vger.kernel.org, vkuznets@redhat.com,
	davem@davemloft.net, dwmw@amazon.co.uk, roger.pau@citrix.com
Subject: Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
Date: Tue, 21 Jul 2020 17:48:45 -0400	[thread overview]
Message-ID: <408d3ce9-2510-2950-d28d-fdfe8ee41a54@oracle.com> (raw)
In-Reply-To: <20200721000348.GA19610@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com>

>>>>>> +static int xen_setup_pm_notifier(void)
>>>>>> +{
>>>>>> +     if (!xen_hvm_domain())
>>>>>> +             return -ENODEV;
>>>>>>
>>>>>> I forgot --- what did we decide about non-x86 (i.e. ARM)?
>>>>> It would be great to support that however, its  out of
>>>>> scope for this patch set.
>>>>> I’ll be happy to discuss it separately.
>>>>
>>>> I wasn't implying that this *should* work on ARM but rather whether this
>>>> will break ARM somehow (because xen_hvm_domain() is true there).
>>>>
>>>>
>>> Ok makes sense. TBH, I haven't tested this part of code on ARM and the series
>>> was only support x86 guests hibernation.
>>> Moreover, this notifier is there to distinguish between 2 PM
>>> events PM SUSPEND and PM hibernation. Now since we only care about PM
>>> HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state.
>>> However, I may have to fix other patches in the series where this check may
>>> appear and cater it only for x86 right?
>>
>>
>> I don't know what would happen if ARM guest tries to handle hibernation
>> callbacks. The only ones that you are introducing are in block and net
>> fronts and that's arch-independent.
>>
>>
>> You do add a bunch of x86-specific code though (syscore ops), would
>> something similar be needed for ARM?
>>
>>
> I don't expect this to work out of the box on ARM. To start with something
> similar will be needed for ARM too.
> We may still want to keep the driver code as-is.
> 
> I understand the concern here wrt ARM, however, currently the support is only
> proposed for x86 guests here and similar work could be carried out for ARM.
> Also, if regular hibernation works correctly on arm, then all is needed is to
> fix Xen side of things.
> 
> I am not sure what could be done to achieve any assurances on arm side as far as
> this series is concerned.

If you are not sure what the effects are (or sure that it won't work) on
ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.

if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())
	return -ENODEV;

(plus '|| xen_initial_domain()' for PVH dom0 as Roger suggested.)

-boris