Xen devel - Jul 2012 - [PATCH] nestedhvm: fix write access fault on ro mapping

Fix write access fault when host npt is mapped read-only.
In this case let the host handle the #NPF.
Apply host p2mt to hap-on-hap pagetable entry.
This fixes the l2 guest graphic display refresh problem.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
CC: Tim Deegan <tim@xen.org>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

At 16:15 +0200 on 19 Jul (1342714507), Christoph Egger
wrote:
> 
> Fix write access fault when host npt is mapped read-only.
> In this case let the host handle the #NPF.
> Apply host p2mt to hap-on-hap pagetable entry.
> This fixes the l2 guest graphic display refresh problem.
> diff -r ae0e96e156f3 xen/arch/x86/hvm/hvm.c
> --- a/xen/arch/x86/hvm/hvm.c	Thu Jul 19 12:12:12 2012 +0200
> +++ b/xen/arch/x86/hvm/hvm.c	Thu Jul 19 15:30:04 2012 +0200
> @@ -1291,6 +1291,8 @@ int hvm_hap_nested_page_fault(unsigned l
>              if ( !handle_mmio() )
>                  hvm_inject_hw_exception(TRAP_gp_fault, 0);
>              return 1;
> +        case NESTEDHVM_PAGEFAULT_READONLY:
> +            break;
Don''t we have to translate the faulting PA into an L1 address before
letting the rest of this fault handler run?  It explicitly operates on
the hostp2m.  

If we do that, we should probably do it for NESTEDHVM_PAGEFAULT_ERROR,
rather than special-casing READONLY.  That way any other
automatically-fixed types (like the p2m_access magic) will be covered
too.

Cheers,

Tim.

On 07/26/12 20:21, Tim Deegan wrote:
> At 16:15 +0200 on 19 Jul (1342714507), Christoph Egger wrote:
>>
>> Fix write access fault when host npt is mapped read-only.
>> In this case let the host handle the #NPF.
>> Apply host p2mt to hap-on-hap pagetable entry.
>> This fixes the l2 guest graphic display refresh problem.
> 
>> diff -r ae0e96e156f3 xen/arch/x86/hvm/hvm.c
>> --- a/xen/arch/x86/hvm/hvm.c	Thu Jul 19 12:12:12 2012 +0200
>> +++ b/xen/arch/x86/hvm/hvm.c	Thu Jul 19 15:30:04 2012 +0200
>> @@ -1291,6 +1291,8 @@ int hvm_hap_nested_page_fault(unsigned l
>>              if ( !handle_mmio() )
>>                  hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>              return 1;
>> +        case NESTEDHVM_PAGEFAULT_READONLY:
>> +            break;
> 
> Don''t we have to translate the faulting PA into an L1 address
before
> letting the rest of this fault handler run?  It explicitly operates on
> the hostp2m.  
> 
> If we do that, we should probably do it for NESTEDHVM_PAGEFAULT_ERROR,
> rather than special-casing READONLY.  That way any other
> automatically-fixed types (like the p2m_access magic) will be covered
> too.

How do you differentiate if the error happened from walking l1 npt or
host npt ?
In the first case it isn''t possible to provide l1 address.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

At 13:57 +0200 on 27 Jul (1343397454), Christoph Egger
wrote:
> >> @@ -1291,6 +1291,8 @@ int hvm_hap_nested_page_fault(unsigned l
> >>              if ( !handle_mmio() )
> >>                  hvm_inject_hw_exception(TRAP_gp_fault, 0);
> >>              return 1;
> >> +        case NESTEDHVM_PAGEFAULT_READONLY:
> >> +            break;
> > 
> > Don''t we have to translate the faulting PA into an L1 address
before
> > letting the rest of this fault handler run?  It explicitly operates on
> > the hostp2m.  
> > 
> > If we do that, we should probably do it for NESTEDHVM_PAGEFAULT_ERROR,
> > rather than special-casing READONLY.  That way any other
> > automatically-fixed types (like the p2m_access magic) will be covered
> > too.
> 
> How do you differentiate if the error happened from walking l1 npt or
> host npt ?
> In the first case it isn''t possible to provide l1 address.
It must be _possible_; after all we managed to detect the error. :)  In
any case it''s definitely wrong to carry on with this handler with the
wrong address in hand.  So I wonder why this patch actually works for
you.  Does replacing the ''break'' above with ''return
1'' also fix the
problem?

In the short term, do you only care about pages that are read-only for
log-dirty tracking?  For the L1 walk, that should be handled by the PT
walker''s own calls to paging_mark_dirty(), and the nested-p2m handler
could potentially take care of the other case by calling
paging_mark_dirty() (for writes!) before calling nestedhap_walk_L0_p2m().

Tim.

On 08/02/12 12:45, Tim Deegan wrote:
> At 13:57 +0200 on 27 Jul (1343397454), Christoph Egger wrote:
>>>> @@ -1291,6 +1291,8 @@ int hvm_hap_nested_page_fault(unsigned l
>>>>              if ( !handle_mmio() )
>>>>                  hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>>>              return 1;
>>>> +        case NESTEDHVM_PAGEFAULT_READONLY:
>>>> +            break;
>>>
>>> Don''t we have to translate the faulting PA into an L1
address before
>>> letting the rest of this fault handler run?  It explicitly operates
on
>>> the hostp2m.  
>>>
>>> If we do that, we should probably do it for
NESTEDHVM_PAGEFAULT_ERROR,
>>> rather than special-casing READONLY.  That way any other
>>> automatically-fixed types (like the p2m_access magic) will be
covered
>>> too.
>>
>> How do you differentiate if the error happened from walking l1 npt or
>> host npt ?
>> In the first case it isn''t possible to provide l1 address.
> 
> It must be _possible_; after all we managed to detect the error. :)  In
> any case it''s definitely wrong to carry on with this handler with
the
> wrong address in hand.  So I wonder why this patch actually works for
> you.  Does replacing the ''break'' above with
''return 1'' also fix the
> problem?

No. Two things have to happen:

1. Calling paging_mark_dirty() and
2. using the same p2mt from the hostp2m in the nestedp2m.
>
> In the short term, do you only care about pages that are read-only for
> log-dirty tracking?  For the L1 walk, that should be handled by the PT
> walker''s own calls to paging_mark_dirty(), and the nested-p2m
handler
> could potentially take care of the other case by calling
> paging_mark_dirty() (for writes!) before calling nestedhap_walk_L0_p2m().

Ok, I consider this as a performance improvement rather a bugfix.

New version is attached.

Christoph

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

At 14:32 +0200 on 02 Aug (1343917962), Christoph Egger
wrote:
> > It must be _possible_; after all we managed to detect the error. :) 
In
> > any case it''s definitely wrong to carry on with this handler
with the
> > wrong address in hand.  So I wonder why this patch actually works for
> > you.  Does replacing the ''break'' above with
''return 1'' also fix the
> > problem?
> 
> No. Two things have to happen:
> 
> 1. Calling paging_mark_dirty() and
> 2. using the same p2mt from the hostp2m in the nestedp2m.
> 
> >
> 
> > In the short term, do you only care about pages that are read-only for
> > log-dirty tracking?  For the L1 walk, that should be handled by the PT
> > walker''s own calls to paging_mark_dirty(), and the nested-p2m
handler
> > could potentially take care of the other case by calling
> > paging_mark_dirty() (for writes!) before calling
nestedhap_walk_L0_p2m().
> 
> Ok, I consider this as a performance improvement rather a bugfix.
> 
> New version is attached.
Applied, thanks.

Tim.