Re: [PATCH 2/3] xen/virtual-region: Include rodata pointers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.03.2024 12:31, Andrew Cooper wrote:
> On 07/03/2024 7:39 am, Jan Beulich wrote:
>> On 06.03.2024 18:21, Andrew Cooper wrote:
>>> On 06/03/2024 5:09 pm, Ross Lagerwall wrote:
>>>> On Tue, Mar 5, 2024 at 2:17 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>> On 05.03.2024 13:11, Andrew Cooper wrote:
>>>>>> --- a/xen/include/xen/virtual_region.h
>>>>>> +++ b/xen/include/xen/virtual_region.h
>>>>>> @@ -16,6 +16,9 @@ struct virtual_region
>>>>>>      const void *text_start;                /* .text virtual address 
>>>>>> start. */
>>>>>>      const void *text_end;                  /* .text virtual address 
>>>>>> end. */
>>>>>>
>>>>>> +    const void *rodata_start;              /* .rodata virtual address 
>>>>>> start (optional). */
>>>>>> +    const void *rodata_end;                /* .rodata virtual address 
>>>>>> end. */
>>>>>> +
>>>>>>      /* If this is NULL the default lookup mechanism is used. */
>>>>>>      symbols_lookup_t *symbols_lookup;
>>>>> While perhaps the least bad one can do without quite a bit more churn,
>>>>> I'm still irritated by a virtual region (singular) suddenly covering
>>>>> two ranges of VA space. At the very least I think the description should
>>>>> say a word of justification in this regard. An alternative, after all,
>>>>> could have been for livepatch code to register separate regions for
>>>>> rodata (if present in a patch).
>>>>>
>>>>> A follow-on question then would be why ordinary data isn't reflected in
>>>>> a virtual region. Aiui that's just because livepatch presently has no
>>>>> need for it.
>>>>>
>>>>> Underlying question to both: Is the virtual region concept indeed meant
>>>>> to be fully tied to livepatch and its needs?
>>>>>
>>>> Virtual regions were introduced for live patching but I don't think it
>>>> is completely tied to live patching. It was introduced so that any code
>>>> can participate in symbol lookup, bug frame and exception table entry
>>>> search, rather than special casing "if livepatch" in many places.
>>>>
>>>> I agree that the virtual region concept is being abused here - it's just
>>>> being used as a convenient place to store rodata start/end and doesn't
>>>> really have much to do with the text start & end / bug frame / exception
>>>> table entry search that the virtual region is intended for.
>>>>
>>>> Maybe Andrew can explain why he used this approach?
>>> I feel the simplicity and obviousness of patch 3 speaks for itself.
>>>
>>> How do you propose fixing it differently.
>> I'm not opposed to doing it the way you do, but I think it then needs
>> clarifying (up front) what a virtual region really is. It looks to be
>> morphing into a module description instead ... One easy option might
>> be to have a comment next to the struct additions here making clear
>> that this is rather an abuse, but chosen to be this way to keep things
>> simple elsewhere.
> 
> The thing called virtual_region already describes 6 ranges, and I'm
> adding a 7th.

Hmm, yes, in a way you're right.

> It has been a module-ish description right from the very outset.  I
> don't think it is fair to describe this as an abuse at all.
> 
> Is this going to satisfy the outstanding concerns?

Yes. And thank you for bearing with me.

Jan

> diff --git a/xen/include/xen/virtual_region.h
> b/xen/include/xen/virtual_region.h
> index d05362071135..9d150beb8a87 100644
> --- a/xen/include/xen/virtual_region.h
> +++ b/xen/include/xen/virtual_region.h
> @@ -9,6 +9,12 @@
>  #include <xen/list.h>
>  #include <xen/symbols.h>
>  
> +/*
> + * Despite it's name, this is module(ish) description.
> + *
> + * There's one region for .text/etc, one region for .init during boot only,
> + * and one region per livepatch.
> + */
>  struct virtual_region
>  {
>      struct list_head list;
> 
> ~Andrew