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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 15.03.18 at 21:25, <andrew.cooper3@citrix.com> wrote:
> On 13/03/18 14:39, Jan Beulich wrote:
>>>>> On 13.03.18 at 13:28, <roger.pau@citrix.com> 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.

Well, without wanting put under question the ack I've already
given, the question of course is whether the code is much
better after the change than it was before. If there's no really
good shape to put this in, leaving things as they are is certainly
also an option.

Jan


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