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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/03/2018 07:43, Jan Beulich wrote:
>>>> On 15.03.18 at 21:25, <andrew.cooper3@xxxxxxxxxx> wrote:
>> 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.
> 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.

The code is in better shape with the change, than without it (common
prototypes, share_xen_page_with_privileged_guests() being a wrapper,
fewer linebreaks, etc).  I just can't see a way of getting it into a
yet-better state.

~Andrew

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