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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxx>]

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?

Ross