Re: [Xen-devel] [PATCH 03/14] xen/x86: Make mfn_to_gfn typesafe

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 5/20/19 4:13 PM, Julien Grall wrote:
> Hi,
> 
> On 10/05/2019 14:25, Julien Grall wrote:
>>
>>
>> On 10/05/2019 14:24, Jan Beulich wrote:
>>>>>> On 10.05.19 at 15:02, <julien.grall@xxxxxxx> wrote:
>>>
>>>>
>>>> On 10/05/2019 12:35, Jan Beulich wrote:
>>>>>>>> On 07.05.19 at 17:14, <julien.grall@xxxxxxx> wrote:
>>>>>> --- a/xen/arch/x86/mm/shadow/common.c
>>>>>> +++ b/xen/arch/x86/mm/shadow/common.c
>>>>>> @@ -474,7 +474,8 @@ static inline void trace_resync(int event,
>>>>>> mfn_t gmfn)
>>>>>>        if ( tb_init_done )
>>>>>>        {
>>>>>>            /* Convert gmfn to gfn */
>>>>>> -        unsigned long gfn = mfn_to_gfn(current->domain, gmfn);
>>>>>> +        unsigned long gfn = gfn_x(mfn_to_gfn(current->domain,
>>>>>> gmfn));
>>>>>> +
>>>>>>            __trace_var(event, 0/*!tsc*/, sizeof(gfn), &gfn);
>>>>>>        }
>>>>>
>>>>> Can't you use gfn_t here, and hence avoid the gfn_x()? Same again
>>>>> further
>>>>> down.
>>>> Because __trace_var will export the value to the guest. I wasn't sure
>>>> whether we
>>>> can safely consider that gfn_t is exactly the same as unsigned long in
>>>> debug-build.
>>>
>>> Hmm, well - see the definition of gfn_t. George, what do you think?
>>
>> I know what's the current definition. My point is we never made that
>> assumption before. In all honesty, sure assumption would definitely
>> help in a few places, but I think we ought to safeguard with
>> BUILD_BUG(...).
> 
> George, do you have any opinions?

Sorry, not sure how I missed this question earlier.

The __trace_var() call here is designed to be generic: whatever type or
size gfn is, it will copy the whole thing.  Tracing is explicitly not
meant to be a stable interface -- the toolstack needs to track the
hypervisor in terms of what it's going to kick out.

So, having gfn be a gfn_t in this case should be fine; in fact it should
be _better_ than unsigned long, since if gfn_t ever does change size,
the trace record will change size appropriately.  If that happens,
xenalyze will need to be modified to understand how to deal with the new
size, but that's expected.

All that to say: it looks like Jan's suggestion of having gfn_t here
would be better.

 -George

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