From mboxrd@z Thu Jan  1 00:00:00 1970
From: Razvan Cojocaru <rcojocaru@bitdefender.com>
Subject: Re: [PATCH RFC V5 4/5] xen,
 libxc: Request page fault injection via libxc
Date: Fri, 08 Aug 2014 17:55:03 +0300
Message-ID: <53E4E4C7.5070404@bitdefender.com>
References: <1407340738-3374-1-git-send-email-rcojocaru@bitdefender.com>	<1407340738-3374-4-git-send-email-rcojocaru@bitdefender.com>
	<53E4FF56020000780002AA71@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <53E4FF56020000780002AA71@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: kevin.tian@intel.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, eddie.dong@intel.com, xen-devel@lists.xen.org, jun.nakajima@intel.com, ian.jackson@eu.citrix.com
List-Id: xen-devel@lists.xenproject.org

On 08/08/2014 05:48 PM, Jan Beulich wrote:
>> +    if ( seg.attr.fields.dpl != 3 ) /* Guest is not in user mode */
>> +        return 0;
>> +
>> +
> 
> Double blank line.Didn't I ask you to clean up your patches in this
> regard already?

Not me, but point taken. :)

>> +    if ( curr->arch.hvm_vcpu.guest_cr[3]
>> +         != d->arch.hvm_domain.fault_info.address_space )
> 
> Do you really mean to compare the full CR3 value here, rather than
> just bits 12...51? In which case the address_space field likely would
> better be a GPFN.

You're right, I'll compare the values shifted by PAGE_SHIFT.

>> +}
>> +
>> +static void vmx_inject_pf(void)
>> +{
>> +    struct vcpu *curr = current;
>> +    struct domain *d = curr->domain;
>> +    int errcode = PFEC_user_mode;
>> +    uint64_t virtual_address = d->arch.hvm_domain.fault_info.virtual_address;
>> +    uint32_t write_access = d->arch.hvm_domain.fault_info.write_access;
>> +
>> +    d->arch.hvm_domain.fault_info.address_space = 0;
>> +    d->arch.hvm_domain.fault_info.virtual_address = 0;
>> +    d->arch.hvm_domain.fault_info.write_access = 0;
> 
> Are these necessary? Because ...
> 
>> +    d->arch.hvm_domain.fault_info.valid = 0;
> 
> ... I would hope that this one is properly guarding all uses of the
> other fields.

No, they're not necessary anymore. Thank you for pointing that out.


Thanks,
Razvan Cojocaru