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

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

On Thu, Aug 4, 2016 at 12:29 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 03.08.16 at 20:41, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> The two functions monitor_traps and mem_access_send_req duplicate some of the
>> same functionality. The mem_access_send_req however leaves a lot of the
>> standard vm_event fields to be filled by other functions.
>>
>> Remove mem_access_send_req() completely, making use of monitor_traps() to
>> put
>> requests into the monitor ring.  This in turn causes some cleanup around the
>> old callsites of mem_access_send_req(). We also update monitor_traps to now
>> include setting the common vcpu_id field so that all other call-sites can
>> ommit
>> this step.
>>
>> Finally, this change identifies that errors from mem_access_send_req() were
>> never checked.  As errors constitute a problem with the monitor ring,
>> crashing the domain is the most appropriate action to take.
>>
>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Acked-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>>
>> v3: reduce the code movement and sanitization performed to a minimum
>
> Doesn't this invalidate prior reviews and acks?

I don't think so, the difference is pretty much cosmetic.

>
>> @@ -1846,11 +1846,15 @@ 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 )
>>                  fall_through = 1;
>> -            } else {
>> -                /* Rights not promoted, vcpu paused, work here is done */
>> +            else
>> +            {
>> +                /*
>> +                 * Rights not promoted (aka. sync event), work here is done
>> +                 */
>
> Comment style.

Alright, you had an issue with the pre-existing comment style but you
also have an issue with this. What exactly is the issue here?

>
>> @@ -1750,23 +1745,12 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned 
>> long gla,
>>          req->u.mem_access.flags |= npfec.read_access    ? MEM_ACCESS_R : 0;
>>          req->u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W : 0;
>>          req->u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;
>> -        req->vcpu_id = v->vcpu_id;
>> -
>> -        vm_event_fill_regs(req);
>> -
>> -        if ( altp2m_active(v->domain) )
>> -        {
>> -            req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M;
>> -            req->altp2m_idx = vcpu_altp2m(v).p2midx;
>> -        }
>>      }
>>
>> -    /* Pause the current VCPU */
>> -    if ( p2ma != p2m_access_n2rwx )
>> -        vm_event_vcpu_pause(v);
>> -
>> -    /* VCPU may be paused, return whether we promoted automatically */
>> -    return (p2ma == p2m_access_n2rwx);
>> +    /*
>> +     * Return whether vCPU pause is required (aka. sync event)
>> +     */
>
> Again.

Again..

>
>> +    return (p2ma != p2m_access_n2rwx);
>
> Pointless parentheses.
>

The parentheses were already there, I'm trying no to do style
adjustments in this patch.

Tamas

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