Re: [Xen-devel] [PATCH 09/14] livepatch: Add per-function applied/reverted state tracking marker

From: Julien Grall <julien.grall@arm.com>
To: "Wieczorkiewicz, Pawel" <wipawel@amazon.de>
Cc: "Tim Deegan" <tim@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	xen-devel <xen-devel@lists.xen.org>,
	"Pohlack, Martin" <mpohlack@amazon.de>,
	"Ross Lagerwall" <ross.lagerwall@citrix.com>,
	"Jan Beulich" <jbeulich@suse.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 09/14] livepatch: Add per-function applied/reverted state tracking marker
Date: Thu, 22 Aug 2019 11:07:29 +0100	[thread overview]
Message-ID: <5526b464-24a6-6aca-a9f3-095492abecdd@arm.com> (raw)
In-Reply-To: <BEEC1E7D-4A4D-406E-97C3-D5433BAE8CAF@amazon.com>

Hi,

On 22/08/2019 08:44, Wieczorkiewicz, Pawel wrote:
> 
>> On 21. Aug 2019, at 23:34, Julien Grall <julien.grall@arm.com 
>> <mailto:julien.grall@arm.com>> wrote:
>> On 8/21/19 9:19 AM, Pawel Wieczorkiewicz wrote:
>>> Livepatch only tracks an entire payload applied/reverted state. But,
>>> with an option to supply the apply_payload() and/or revert_payload()
>>> functions as optional hooks, it becomes possible to intermix the
>>> execution of the original apply_payload()/revert_payload() functions
>>> with their dynamically supplied counterparts.
>>> It is important then to track the current state of every function
>>> being patched and prevent situations of unintentional double-apply
>>> or unapplied revert.
>>> To support that, it is necessary to extend public interface of the
>>> livepatch. The struct livepatch_func gets additional field holding
>>> the applied/reverted state marker.
>>> To reflect the livepatch payload ABI change, bump the version flag
>>> LIVEPATCH_PAYLOAD_VERSION up to 2.
>>> The above solution only applies to x86 architecture for now.
>>
>> Is this going to break livepatch on Arm? If so, do you have any plan to fix it?
>>
> 
> No, I do not think it is. But, I am unable to test on Arm (No access to HW and SW),
> so I took the conservative approach here.
Arm provides decent free model (see FoundationModel) that you can use for basic 
testing. Alternatively, you QEMU also support virtualization extension.

Let me have a look at the code (I will answer separately) to see if I can spot 
anything.

> 
>> [...]
>>
>>> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
>>> index 2aec532ee2..a93126f631 100644
>>> --- a/xen/include/xen/livepatch.h
>>> +++ b/xen/include/xen/livepatch.h
>>> @@ -117,7 +117,7 @@ int arch_livepatch_quiesce(void);
>>>  void arch_livepatch_revive(void);
>>>    void arch_livepatch_apply(struct livepatch_func *func);
>>> -void arch_livepatch_revert(const struct livepatch_func *func);
>>> +void arch_livepatch_revert(struct livepatch_func *func);
>>
>> I doubt livepatch on Arm will compile after this change.
>>
> 
> What would be you suggestion then?

Cross-compiler are nowadays widely available. So build testing your changes in 
common code would be the minimum.

In this case, as you dropped the const from the prototype, you will need to do 
the same in the declaration.

> Shall I limit the change to X86 everywhere
>   Or maybe drop the compilation flag completely?

I am a bit confused. Which compilation flag do you refer to?

Cheers,

-- 
Julien Grall

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