Re: [PATCH v4 0/9] add xenalyze to staging

["Roger Pau Monné" <roger.pau@citrix.com>]

El 15/06/15 a les 15.46, George Dunlap ha escrit:
> On 06/15/2015 01:44 PM, Julien Grall wrote:
>> Hi George,
>>
>> On 15/06/2015 10:23, George Dunlap wrote:
>>> On 06/10/2015 12:21 AM, Julien Grall wrote:
>>>> On 09/06/2015 07:38, George Dunlap wrote:
>>>>> On 06/09/2015 12:31 PM, Julien Grall wrote:
>>>>>> On 09/06/2015 06:18, Olaf Hering wrote:
>>>>>>> On Wed, Jun 03, Julien Grall wrote:
>>>>>>>
>>>>>>>> xentrace is not working as we don't have the infrastructure for ARM
>>>>>>>> in Xen.
>>>>>>>
>>>>>>> Why is that? After a very quick look __trace_var is available
>>>>>>> unconditionally.
>>>>>>
>>>>>> Multiple reasons, the shared page for trace is not correctly exposed,
>>>>>> __trace_var/__trace_hypercall is not even used on ARM. I may miss some
>>>>>> other bugs that I didn't spot while looking to the code.
>>>>>
>>>>> Code in xen/common will generate traces (e.g., the scheduling code), so
>>>>> useful traces are already in place for ARM, even if no arm-specific
>>>>> code
>>>>> has trace points yet.
>>>>>
>>>>>> So I don't think we should build a non-working binary until someone
>>>>>> fixes the various problem of the trace system on ARM.
>>>>>
>>>>> If the shared page really doesn't work, then yes, we should probably
>>>>> not
>>>>> build it on ARM for the release.
>>>>>
>>>>> Would it be very difficult to get working?
>>>>
>>>> I don't think this is very difficult. But it's not trivial and will
>>>> require some work in order to properly bring up xentrace on ARM.
>>>>
>>>> There was a thread about it last year [1] with Globallogic. I gave some
>>>> insight [2] on what to fix in order to get xentrace for ARM.
>>>>
>>>> Note that on x86 for foreign mapping, the check for xen domid is open
>>>> opencode the check for xen domid (see p2m_add_foreign).
>>>>
>>>> Also, one things I forgot to mention last year is foreign mapping is
>>>> always RW in the stage 2 P2M. Although, AFAIU, trace buffer should be
>>>> mapped RO in order to avoid the guest writing in it (see
>>>> share_xen_page_with_guest).
>>>
>>> On the contrary, the trace buffer uses the traditional Xen
>>> producer/consumer interface for transferring data.  The user-space
>>> consumer needs to modify the consumer pointer to let Xen know that there
>>> is space to produce more trace records.  The tracing code in the
>>> hypervisor is programmed defensively to make sure that a buggy consumer
>>> can't crash the hypervisor.
>>
>> I think we are not talking about the same things. My comment was a
>> reference to the call to xen_share_page_with_privileged_guests in
>> xen/common/trace.c
>>
>> AFAIU, the code request to share read-only the page (i.e the guest can't
>> write in this memory).
>>
>> The page will be mapped with the foreign mapping hypercall (the foreign
>> domid is xen_dom) always with RW not matter the shared attribute (i.e RW
>> or RO).
> 
> Oh, right -- the trace *buffers* are read-write, but there's a "trace
> info" struct that contains an array of the mfns that make up the
> buffers.  *That* is read-only, since there's no reason for the guest to
> modify it.
> 
> Is this an issue because for ARM you have to attach Xen's mfn to a pfn
> in the guest?  Is this also an issue with PVH?

TBH I've never tried to use xentrace on PVH, so I'm not sure what's missing.

I see that foreign mappings are always RW, maybe we should check if the
page we are going to add to the p2m has write permissions set and use
that instead of always using RW? On x86 this can be achieved in
p2m_add_foreign without much fuss, but I'm not sure what side-effects it
might have.

Roger.