[PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[Pavel Fedin]

This function takes stage-II physical addresses (A.K.A. IPA), on input, not
real physical addresses. This causes kvm_is_device_pfn() to return wrong
values, depending on how much guest and host memory maps match. This
results in completely broken KVM on some boards. The problem has been
caught on Samsung proprietary hardware.

Cc: stable@vger.kernel.org
Fixes: e6fab5442345 ("ARM/arm64: KVM: test properly for a PTE's uncachedness")

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm/kvm/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 7dace90..51ad98f 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -310,7 +310,8 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
 
 	pte = pte_offset_kernel(pmd, addr);
 	do {
-		if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
+		if (!pte_none(*pte) &&
+		    (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
 			kvm_flush_dcache_pte(*pte);
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 }
-- 
2.4.4

[PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[Pavel Fedin]

This function takes stage-II physical addresses (A.K.A. IPA), on input, not
real physical addresses. This causes kvm_is_device_pfn() to return wrong
values, depending on how much guest and host memory maps match. This
results in completely broken KVM on some boards. The problem has been
caught on Samsung proprietary hardware.

Cc: stable@vger.kernel.org
Fixes: e6fab5442345 ("ARM/arm64: KVM: test properly for a PTE's uncachedness")

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm/kvm/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 7dace90..51ad98f 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -310,7 +310,8 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
 
 	pte = pte_offset_kernel(pmd, addr);
 	do {
-		if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
+		if (!pte_none(*pte) &&
+		    (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
 			kvm_flush_dcache_pte(*pte);
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 }
-- 
2.4.4

Re: [PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[Ard Biesheuvel]

Hi Pavel,

Thanks for getting to the bottom of this.

On 1 December 2015 at 14:03, Pavel Fedin <p.fedin@samsung.com> wrote:
> This function takes stage-II physical addresses (A.K.A. IPA), on input, not
> real physical addresses. This causes kvm_is_device_pfn() to return wrong
> values, depending on how much guest and host memory maps match. This
> results in completely broken KVM on some boards. The problem has been
> caught on Samsung proprietary hardware.
>
> Cc: stable@vger.kernel.org
> Fixes: e6fab5442345 ("ARM/arm64: KVM: test properly for a PTE's uncachedness")
>

That commit is not in a release yet, so no need for cc stable

> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm/kvm/mmu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 7dace90..51ad98f 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -310,7 +310,8 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
>
>         pte = pte_offset_kernel(pmd, addr);
>         do {
> -               if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
> +               if (!pte_none(*pte) &&
> +                   (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)

I think your analysis is correct, but does that not apply to both instances?
And instead of reverting, could we fix this properly instead?

>                         kvm_flush_dcache_pte(*pte);
>         } while (pte++, addr += PAGE_SIZE, addr != end);
>  }
> --
> 2.4.4
>

RE: [PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[Pavel Fedin]

 Hello!

> > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> > index 7dace90..51ad98f 100644
> > --- a/arch/arm/kvm/mmu.c
> > +++ b/arch/arm/kvm/mmu.c
> > @@ -310,7 +310,8 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
> >
> >         pte = pte_offset_kernel(pmd, addr);
> >         do {
> > -               if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
> > +               if (!pte_none(*pte) &&
> > +                   (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> 
> I think your analysis is correct, but does that not apply to both instances?

 No no, another one is correct, since it operates on real PFN (at least looks like so). I have verified my fix against the original problem (crash on Exynos5410 without generic timer), and it still works fine there.

> And instead of reverting, could we fix this properly instead?

 Of course, i'm not against alternate approaches, feel free to. I've just suggested what i could, to fix things quickly. I'm indeed no expert in KVM memory management yet. After all, this is what mailing lists are for.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[Ard Biesheuvel]

On 3 December 2015 at 08:14, Pavel Fedin <p.fedin@samsung.com> wrote:
>  Hello!
>
>> > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> > index 7dace90..51ad98f 100644
>> > --- a/arch/arm/kvm/mmu.c
>> > +++ b/arch/arm/kvm/mmu.c
>> > @@ -310,7 +310,8 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
>> >
>> >         pte = pte_offset_kernel(pmd, addr);
>> >         do {
>> > -               if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
>> > +               if (!pte_none(*pte) &&
>> > +                   (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
>>
>> I think your analysis is correct, but does that not apply to both instances?
>
>  No no, another one is correct, since it operates on real PFN (at least looks like so). I have verified my fix against the original problem (crash on Exynos5410 without generic timer), and it still works fine there.
>

I don't think so. Regardless of whether you are manipulating HYP
mappings or stage-2 mappings, the physical address is always the
output, not the input of the translation, so addr is always either a
virtual address or a intermediate physical address, whereas
pfn_valid() operates on host physical addresses.

>> And instead of reverting, could we fix this properly instead?
>
>  Of course, i'm not against alternate approaches, feel free to. I've just suggested what i could, to fix things quickly. I'm indeed no expert in KVM memory management yet. After all, this is what mailing lists are for.
>

OK. I will follow up with a patch, as Christoffer requested. I'd
appreciate it if you could test to see if it also fixes the current
issue, and the original arch timer issue.

Thanks,
Ard.

RE: [PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[Pavel Fedin]

 Hello!

> >> I think your analysis is correct, but does that not apply to both instances?
> >
> >  No no, another one is correct, since it operates on real PFN (at least looks like so). I
> have verified my fix against the original problem (crash on Exynos5410 without generic timer),
> and it still works fine there.
> >
> 
> I don't think so. Regardless of whether you are manipulating HYP
> mappings or stage-2 mappings, the physical address is always the
> output, not the input of the translation, so addr is always either a
> virtual address or a intermediate physical address, whereas
> pfn_valid() operates on host physical addresses.

 Yes, you are right. I have reviewed this more carefully, and indeed, unmap_range() is also called by unmap_stage2_range(), so it can be both IPA and real PA.

> OK. I will follow up with a patch, as Christoffer requested. I'd
> appreciate it if you could test to see if it also fixes the current
> issue, and the original arch timer issue.

 I have just made the same patch, and currently testing it on all my boards. Also i'll test it on my ARM64 too, just in case. I was about to finish the testing and send the patch in maybe one or two hours.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

On Wed, 2015-12-02 at 18:41 +0100, Ard Biesheuvel wrote:
> Hi Pavel,
> 
> Thanks for getting to the bottom of this.
> 
> On 1 December 2015 at 14:03, Pavel Fedin <p.fedin@samsung.com> wrote:
> > This function takes stage-II physical addresses (A.K.A. IPA), on input, not
> > real physical addresses. This causes kvm_is_device_pfn() to return wrong
> > values, depending on how much guest and host memory maps match. This
> > results in completely broken KVM on some boards. The problem has been
> > caught on Samsung proprietary hardware.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: e6fab5442345 ("ARM/arm64: KVM: test properly for a PTE's uncachedness")
> > 
> 
> That commit is not in a release yet, so no need for cc stable
[...]

But it is cc'd to stable, so unless it is going to be nacked at review
stage, any subsequent fixes should also be cc'd.

Ben.

-- 
Ben Hutchings
For every action, there is an equal and opposite criticism. - Harrison

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

RE: [PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[Pavel Fedin]

 Hello!

> > > Cc: stable@vger.kernel.org
> > > Fixes: e6fab5442345 ("ARM/arm64: KVM: test properly for a PTE's uncachedness")
> > >
> >
> > That commit is not in a release yet, so no need for cc stable
> [...]
> 
> But it is cc'd to stable, so unless it is going to be nacked at review
> stage, any subsequent fixes should also be cc'd.

 Sorry guys for messing things up a bit, but the affected commit actually is in stable branch (4.4-rc3), so i decided to Cc: stable, just in case, because the breakage is quite big IMHO.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[Ard Biesheuvel]

On 4 December 2015 at 02:58, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Wed, 2015-12-02 at 18:41 +0100, Ard Biesheuvel wrote:
>> Hi Pavel,
>>
>> Thanks for getting to the bottom of this.
>>
>> On 1 December 2015 at 14:03, Pavel Fedin <p.fedin@samsung.com> wrote:
>> > This function takes stage-II physical addresses (A.K.A. IPA), on input, not
>> > real physical addresses. This causes kvm_is_device_pfn() to return wrong
>> > values, depending on how much guest and host memory maps match. This
>> > results in completely broken KVM on some boards. The problem has been
>> > caught on Samsung proprietary hardware.
>> >
>> > Cc: stable@vger.kernel.org
>> > Fixes: e6fab5442345 ("ARM/arm64: KVM: test properly for a PTE's uncachedness")
>> >
>>
>> That commit is not in a release yet, so no need for cc stable
> [...]
>
> But it is cc'd to stable, so unless it is going to be nacked at review
> stage, any subsequent fixes should also be cc'd.
>

Ah yes, thanks for pointing that out.

But please, don't cc your proposed patches straight to
stable@vger.kernel.org. I usually leave it up to the maintainer that
merges the patch to add the Cc: line to the commit log.

Re: [PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[Christoffer Dall]

On Tue, Dec 01, 2015 at 04:03:52PM +0300, Pavel Fedin wrote:
> This function takes stage-II physical addresses (A.K.A. IPA), on input, not
> real physical addresses. This causes kvm_is_device_pfn() to return wrong
> values, depending on how much guest and host memory maps match. This
> results in completely broken KVM on some boards. The problem has been
> caught on Samsung proprietary hardware.
> 
> Cc: stable@vger.kernel.org

cc'ing stable doesn't make sense here as the bug was introduced in
v4.4-rc3 and we didn't release v4.4 yet...

> Fixes: e6fab5442345 ("ARM/arm64: KVM: test properly for a PTE's uncachedness")
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm/kvm/mmu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 7dace90..51ad98f 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -310,7 +310,8 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
>  
>  	pte = pte_offset_kernel(pmd, addr);
>  	do {
> -		if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
> +		if (!pte_none(*pte) &&
> +		    (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
>  			kvm_flush_dcache_pte(*pte);
>  	} while (pte++, addr += PAGE_SIZE, addr != end);
>  }

You are right that there was a bug in the fix, but your fix is not the
right one.

Either we have to apply an actual mask and the compare against the value
(yes, I know, because of the UXN bit we get lucky so far, but that's too
brittle), or we should do a translation fo the gfn to a pfn.  Is there
anything preventing us to do the following?

if (!pte_none(*pte) && !kvm_is_device_pfn(pte_pfn(*pte)))

-Christoffer

Re: [PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[Ard Biesheuvel]

On 2 December 2015 at 19:50, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Tue, Dec 01, 2015 at 04:03:52PM +0300, Pavel Fedin wrote:
>> This function takes stage-II physical addresses (A.K.A. IPA), on input, not
>> real physical addresses. This causes kvm_is_device_pfn() to return wrong
>> values, depending on how much guest and host memory maps match. This
>> results in completely broken KVM on some boards. The problem has been
>> caught on Samsung proprietary hardware.
>>
>> Cc: stable@vger.kernel.org
>
> cc'ing stable doesn't make sense here as the bug was introduced in
> v4.4-rc3 and we didn't release v4.4 yet...
>
>> Fixes: e6fab5442345 ("ARM/arm64: KVM: test properly for a PTE's uncachedness")
>>
>> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
>> ---
>>  arch/arm/kvm/mmu.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 7dace90..51ad98f 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -310,7 +310,8 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
>>
>>       pte = pte_offset_kernel(pmd, addr);
>>       do {
>> -             if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
>> +             if (!pte_none(*pte) &&
>> +                 (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
>>                       kvm_flush_dcache_pte(*pte);
>>       } while (pte++, addr += PAGE_SIZE, addr != end);
>>  }
>
> You are right that there was a bug in the fix, but your fix is not the
> right one.
>
> Either we have to apply an actual mask and the compare against the value
> (yes, I know, because of the UXN bit we get lucky so far, but that's too
> brittle), or we should do a translation fo the gfn to a pfn.  Is there
> anything preventing us to do the following?
>
> if (!pte_none(*pte) && !kvm_is_device_pfn(pte_pfn(*pte)))
>

Yes, that looks better. I got confused by addr being a 'phys_addr_t'
but obviously, the address inside the PTE is the one we need to test
for device-ness, so I think we should replace both instances with this

-- 
Ard.

Re: [PATCH] KVM: arm/arm64: Revert to old way of checking for device mapping in stage2_flush_ptes().

[Christoffer Dall]

On Wed, Dec 02, 2015 at 08:04:42PM +0100, Ard Biesheuvel wrote:
> On 2 December 2015 at 19:50, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > On Tue, Dec 01, 2015 at 04:03:52PM +0300, Pavel Fedin wrote:
> >> This function takes stage-II physical addresses (A.K.A. IPA), on input, not
> >> real physical addresses. This causes kvm_is_device_pfn() to return wrong
> >> values, depending on how much guest and host memory maps match. This
> >> results in completely broken KVM on some boards. The problem has been
> >> caught on Samsung proprietary hardware.
> >>
> >> Cc: stable@vger.kernel.org
> >
> > cc'ing stable doesn't make sense here as the bug was introduced in
> > v4.4-rc3 and we didn't release v4.4 yet...
> >
> >> Fixes: e6fab5442345 ("ARM/arm64: KVM: test properly for a PTE's uncachedness")
> >>
> >> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> >> ---
> >>  arch/arm/kvm/mmu.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> >> index 7dace90..51ad98f 100644
> >> --- a/arch/arm/kvm/mmu.c
> >> +++ b/arch/arm/kvm/mmu.c
> >> @@ -310,7 +310,8 @@ static void stage2_flush_ptes(struct kvm *kvm, pmd_t *pmd,
> >>
> >>       pte = pte_offset_kernel(pmd, addr);
> >>       do {
> >> -             if (!pte_none(*pte) && !kvm_is_device_pfn(__phys_to_pfn(addr)))
> >> +             if (!pte_none(*pte) &&
> >> +                 (pte_val(*pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> >>                       kvm_flush_dcache_pte(*pte);
> >>       } while (pte++, addr += PAGE_SIZE, addr != end);
> >>  }
> >
> > You are right that there was a bug in the fix, but your fix is not the
> > right one.
> >
> > Either we have to apply an actual mask and the compare against the value
> > (yes, I know, because of the UXN bit we get lucky so far, but that's too
> > brittle), or we should do a translation fo the gfn to a pfn.  Is there
> > anything preventing us to do the following?
> >
> > if (!pte_none(*pte) && !kvm_is_device_pfn(pte_pfn(*pte)))
> >
> 
> Yes, that looks better. I got confused by addr being a 'phys_addr_t'

Yeah, that's what I thought when I saw this.  Admittedly we could have a
typedef for the IPA, but oh well...

> but obviously, the address inside the PTE is the one we need to test
> for device-ness, so I think we should replace both instances with this
> 

care to send a patch by any chance?

-Christoffer