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
>
