Re: [Xen-devel] [PATCH v2] mem_access: sanitize code around sending vm_event request

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Tue, Aug 2, 2016 at 9:23 AM, Tamas K Lengyel
<tamas.lengyel@xxxxxxxxxxxx> wrote:
> On Aug 2, 2016 06:45, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>
>> >>> On 01.08.16 at 18:52, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/hvm/hvm.c
>> > +++ b/xen/arch/x86/hvm/hvm.c
>> > @@ -1707,7 +1707,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>> > unsigned long gla,
>> >      int rc, fall_through = 0, paged = 0;
>> >      int sharing_enomem = 0;
>> >      vm_event_request_t *req_ptr = NULL;
>> > -    bool_t ap2m_active;
>> > +    bool_t ap2m_active, sync = 0;
>> >
>> >      /* On Nested Virtualization, walk the guest page table.
>> >       * If this succeeds, all is fine.
>> > @@ -1846,11 +1846,12 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>> > unsigned long gla,
>> >                  }
>> >              }
>> >
>> > -            if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) )
>> > -            {
>> > +            sync = p2m_mem_access_check(gpa, gla, npfec, &req_ptr);
>> > +
>> > +            if ( !sync ) {
>>
>> Coding style. If you dropped the brace entirely, you could at once
>> adjust ...
>>
>> >                  fall_through = 1;
>> >              } else {
>>
>> ... coding style here.
>>
>> > -                /* Rights not promoted, vcpu paused, work here is done
>> > */
>> > +                /* Rights not promoted (aka. sync event), work here is
>> > done */
>>
>> Comment style. More of these elsewhere.
>>
>> > +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync,
>>
>> Coding style.
>>
>> > +                           vm_event_request_t *req)
>> > +{
>> > +    return monitor_traps(v, sync, req);
>> > +}
>>
>> Overall - is this a useful wrapper? Why can't the caller(s) call
>> monitor_traps() directly? And if you really want to keep it, it would
>> probably better be an inline one.
>>
>
> The reason for this wrapper is to avoid having to include the common monitor
> header here. I can move it into the hvm monitor header as inline, that's no
> problem.
>

Actually, making it into inline would require that hvm/monitor.h
include the common monitor.h as well, at which point having the
wrapper would be useless as hvm.c would have effectively include
common monitor.h too. So yea, the goal is to avoid having to include
both common/monitor and hvm/monitor in hvm.c and it needs this kind of
wrapper.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel