Re: [PATCH] nestedhvm: do not translate INVALID_GFN

[Christoph Egger <Christoph.Egger@amd.com>]

On 08/02/12 13:35, Tim Deegan wrote:

> At 13:28 +0200 on 02 Aug (1343914136), Christoph Egger wrote:
>> On 08/02/12 13:19, Tim Deegan wrote:
>>
>>> Hi,
>>>
>>> At 17:37 +0200 on 31 Jul (1343756240), Christoph Egger wrote:
>>>> Do not translate INVALID_GFN as l2 guest gfn into l1 guest gfn.
>>>
>>> Why not?  l2 gfns don't have any special meaning that we can
>>> dictate from inside Xen.
>>>
>>>> Pass correct pfec for translation into l1 guest gfn.
>>>
>>> This seems like a good idea, but probably should happen for all
>>> entries, not just INVALID_GFN ones -- we shouldn't be returning a PFEC
>>> to the guest that comes from translations outside his control.
>>>
>>> How about this:
>>>
>>> diff -r fdd4b7b36959 xen/arch/x86/mm/p2m.c
>>> --- a/xen/arch/x86/mm/p2m.c	Thu Aug 02 12:04:31 2012 +0100
>>> +++ b/xen/arch/x86/mm/p2m.c	Thu Aug 02 12:17:48 2012 +0100
>>> @@ -1581,6 +1581,7 @@ unsigned long paging_gva_to_gfn(struct v
>>>          unsigned long gfn;
>>>          struct p2m_domain *p2m;
>>>          const struct paging_mode *mode;
>>> +        uint32_t pfec_21 = *pfec;
>>>          uint64_t ncr3 = nhvm_vcpu_hostcr3(v);
>>>  
>>>          /* translate l2 guest va into l2 guest gfn */
>>> @@ -1590,7 +1591,7 @@ unsigned long paging_gva_to_gfn(struct v
>>>  
>>>          /* translate l2 guest gfn into l1 guest gfn */
>>>          return hostmode->p2m_ga_to_gfn(v, hostp2m, ncr3,
>>> -                                       gfn << PAGE_SHIFT, pfec, NULL);
>>> +                                       gfn << PAGE_SHIFT, &pfec_21, NULL);
>>
>>
>> The caller will see the return value of pfec and not from pfec_21.
>> If this is what the caller expects then this is fine with me.
> 
> Yes, I think that is what the caller expects -- the error code is made
> up from the pagetable walk rather than from the p2m table.
> 
> Can I take that as an ack?

Yes.

> And more importantly, does it fix the Hyper-V problem you encountered?

The one you mean is covered with the other patch.
But I found this with Hyper-V when doing MMIO accesses.

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