Re: [kvm-unit-tests PATCH v2 16/24] arm/arm64: Share memregions

[Eric Auger <eric.auger@redhat.com>]

On 2/1/24 15:21, Andrew Jones wrote:
> On Thu, Feb 01, 2024 at 01:03:54PM +0100, Eric Auger wrote:
>> Hi Drew,
>>
>> On 1/26/24 15:23, Andrew Jones wrote:
> ...
>>> -static void mem_regions_add_assumed(void)
>>> -{
>>> -	phys_addr_t code_end = (phys_addr_t)(unsigned long)&_etext;
>>> -	struct mem_region *r;
>>> -
>>> -	r = mem_region_find(code_end - 1);
>>> -	assert(r);
>>> +	struct mem_region *code, *data;
>>>  
>>>  	/* Split the region with the code into two regions; code and data */
>>> -	mem_region_add(&(struct mem_region){
>>> -		.start = code_end,
>>> -		.end = r->end,
>>> -	});
>>> -	*r = (struct mem_region){
>>> -		.start = r->start,
>>> -		.end = code_end,
>>> -		.flags = MR_F_CODE,
>>> -	};
>>> +	memregions_split((unsigned long)&_etext, &code, &data);
>>> +	assert(code);
>>> +	code->flags |= MR_F_CODE;
>> I think this would deserve to be split into several patches, esp. this
>> change in the implementation of
>>
>> mem_regions_add_assumed and the init changes. At the moment this is pretty difficult to review
>>
> Darn, you called me out on this one :-) I had a feeling I should split out
> the introduction of memregions_split(), since it was sneaking a bit more
> into the patch than just code motion as advertised, but then I hoped I
> get away with putting a bit more burden on the reviewer instead. If you
> haven't already convinced yourself that the new function is equivalent to
> the old code, then I'll respin with the splitting and also create a new
> patch for the 'mem_region' to 'memregions' rename while at it (so there
> will be three patches instead of one). But, if you're already good with
> it, then I'll leave it as is, since patch splitting is a pain...
frankly I would prefer you split. But maybe somebody smarter than me
will be able to review as is, maybe just wait a little bit until you
respin ;-)

Eric
>
> Thanks,
> drew
>