Re: [Xen-devel] [PATCH 7/7] xen/mm: Clean up share_xen_page_with_guest() API

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/03/18 14:39, Jan Beulich wrote:
>>>> On 13.03.18 at 13:28, <roger.pau@xxxxxxxxxx> wrote:
>> On Fri, Mar 09, 2018 at 01:18:42PM +0000, Andrew Cooper wrote:
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -1187,8 +1187,8 @@ unsigned long domain_get_maximum_gpfn(struct domain 
>>> *d)
>>>      return gfn_x(d->arch.p2m.max_mapped_gfn);
>>>  }
>>>  
>>> -void share_xen_page_with_guest(struct page_info *page,
>>> -                          struct domain *d, int readonly)
>>> +void share_xen_page_with_guest(struct page_info *page, struct domain *d,
>>> +                               enum XENSHARE_flags flags)
>> Naming this _flags feels wrong to me, I would assume flags to be
>> something which can be used as (SHARE_r | SHARE_w) (ie: stacked) and
>> so on. I would maybe name this XENSHARE_options rather than flags.
>>
>> TBH I would be OK with renaming the parameter to "bool ro/readonly"
>> and let the callers use true and false directly. It seems like
>> over-engineering to use an enum for this, or maybe you have further
>> changes in mind that are going to expand the set of options?
> On one hand I agree that an enum like this is somewhat strange
> to have, and a boolean would seem like a better fit. Otoh using
> plain true/false at the call sites would make it pretty unclear
> whether "true" means r/o or r/w. So another option might be
> to have multiple inline wrappers around the actual worker, like
> share_xen_page_with_guest_ro().

Splitting into (SHARE_r | SHARE_w( doesn't make sense because the
underlying implementation take a boolean idea of whether to use PGT_none
or PGT_writable_page.

We've already got share_xen_page_with_privileged_guests() as a wrapper
around share_xen_page_with_guest().  Therefore, we'd end up with a total
of 4 extra wrappers if we wanted _rw and _ro suffixes, which seems over
the top to me.

I agree its not completely great like this, but it is the least bad
option I managed to come up with.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel