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 <jbeul...@suse.com> 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. Jan
