Xen project Mailing List

On Fri, Aug 8, 2014 at 10:04 AM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:

On 08/08/2014 08:00, Jan Beulich wrote:
>>>> On 08.08.14 at 01:12, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2725,6 +2725,8 @@ void hvm_inject_page_fault(int errcode, unsigned long cr2)
>> int hvm_hap_nested_page_fault(paddr_t gpa,
>> bool_t gla_valid,
>> unsigned long gla,
>> + bool_t fault_in_gpt,
>> + bool_t fault_gla,
>> bool_t access_r,
>> bool_t access_w,
>> bool_t access_x)
> Afaic it is out of question to have a function with _six_ boolean
> parameters. This needs to be consolidated into a single flags field. I
> have actually done that already, in a patch serving a different
> purpose (see attached), as discussed recently on this list. I would
> very much appreciate if you either re-based yours on top of that or
> modified it along those lines.
>
>> @@ -2371,11 +2372,19 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
>> }
>>
>> if ( qualification & EPT_GLA_VALID )
>> + {
>> __vmread(GUEST_LINEAR_ADDRESS, &gla);
>> + fault_gla = !!(qualification & EPT_GLA_FAULT);
>> + fault_in_gpt = !fault_gla;
> I am actually not agreeing with Andrew regarding the need for two
> flags here, if we already know that SVM also properly expresses the
> distinction between faults on page table accesses and faults on the
> actual translation. The attached patch is also coded in this way, and
> I agree with your earlier arguing for just a single flag.

I also agree. My suggestion for two flags was on the (wrong) assumption
that AMD didn't currently support providing this information (although Is
should have picked up on this and retracted my suggestion).

~Andrew

I actually disagree here because there is still a slight difference between the Intel and AMD implementation. In Intel's case gla_fault is only available iff gla_valid is set. AMD doesn't have gla_valid. So in the abstracted code, having gla_fault 0 could mean potentially different things. That's certainly not good for abstraction. The AMD side could set gla_valid = 1 if either if their two-bits that we will aggregate into gla_fault is set, but without actually providing a gla.. and that could lead to its own set of problems. So if we want to keep the bit aggregated in gla_fault, we would need a bool_t gla_fault_valid.... which is just going back to square one of having two bool_t fields as this patch does already.

Re: [Xen-devel] [PATCH v3 1/2] x86/mem_event: Deliver gla fault EPT violation information