[PATCH] KVM: arm/arm64: drop resource size check for GICV window

[PATCH] KVM: arm/arm64: drop resource size check for GICV window

[Ard Biesheuvel]

When booting a 64 KB pages kernel on a ACPI GICv3 system that
implements support for v2 emulation, the following warning is
produced

  GICV size 0x2000 not a multiple of page size 0x10000

and support for v2 emulation is disabled, preventing GICv2 VMs
from being able to run on such hosts.

The reason is that vgic_v3_probe() performs a sanity check on the
size of the window (it should be a multiple of the page size),
while the ACPI MADT parsing code hardcodes the size of the window
to 8 KB. This makes sense, considering that ACPI does not bother
to describe the size in the first place, under the assumption that
platforms implementing ACPI will follow the architecture and not
put anything else in the same 64 KB window.

So let's just drop the sanity check altogether, and assume that
the window is at least 64 KB in size.

Fixes: 909777324588 ("KVM: arm/arm64: vgic-new: vgic_init: implement kvm_vgic_hyp_init")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 virt/kvm/arm/vgic/vgic-v3.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index bdcf8e7a6161..72fc688c3e9d 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -552,11 +552,6 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
 		pr_warn("GICV physical address 0x%llx not page aligned\n",
 			(unsigned long long)info->vcpu.start);
 		kvm_vgic_global_state.vcpu_base = 0;
-	} else if (!PAGE_ALIGNED(resource_size(&info->vcpu))) {
-		pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
-			(unsigned long long)resource_size(&info->vcpu),
-			PAGE_SIZE);
-		kvm_vgic_global_state.vcpu_base = 0;
 	} else {
 		kvm_vgic_global_state.vcpu_base = info->vcpu.start;
 		kvm_vgic_global_state.can_emulate_gicv2 = true;
-- 
2.17.0

[PATCH] KVM: arm/arm64: drop resource size check for GICV window

[Christoffer Dall]

On Fri, Jun 01, 2018 at 05:06:28PM +0200, Ard Biesheuvel wrote:
> When booting a 64 KB pages kernel on a ACPI GICv3 system that
> implements support for v2 emulation, the following warning is
> produced
> 
>   GICV size 0x2000 not a multiple of page size 0x10000
> 
> and support for v2 emulation is disabled, preventing GICv2 VMs
> from being able to run on such hosts.
> 
> The reason is that vgic_v3_probe() performs a sanity check on the
> size of the window (it should be a multiple of the page size),
> while the ACPI MADT parsing code hardcodes the size of the window
> to 8 KB. This makes sense, considering that ACPI does not bother
> to describe the size in the first place, under the assumption that
> platforms implementing ACPI will follow the architecture and not
> put anything else in the same 64 KB window.

Does the architecture actually say that anywhere?

> 
> So let's just drop the sanity check altogether, and assume that
> the window is at least 64 KB in size.

This could obviously be dangerous if broken systems actually exist.
Marc may know more about that than me.  An alternative would be to
modify the ACPI code to assume max(8 KB, page size) instead, and/or a
command line parameter to override this check.

That said, I'm not directly opposed to this patch, but I'll let Marc
have a look as well.

Thanks,
-Christoffer

> 
> Fixes: 909777324588 ("KVM: arm/arm64: vgic-new: vgic_init: implement kvm_vgic_hyp_init")
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  virt/kvm/arm/vgic/vgic-v3.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index bdcf8e7a6161..72fc688c3e9d 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -552,11 +552,6 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
>  		pr_warn("GICV physical address 0x%llx not page aligned\n",
>  			(unsigned long long)info->vcpu.start);
>  		kvm_vgic_global_state.vcpu_base = 0;
> -	} else if (!PAGE_ALIGNED(resource_size(&info->vcpu))) {
> -		pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
> -			(unsigned long long)resource_size(&info->vcpu),
> -			PAGE_SIZE);
> -		kvm_vgic_global_state.vcpu_base = 0;
>  	} else {
>  		kvm_vgic_global_state.vcpu_base = info->vcpu.start;
>  		kvm_vgic_global_state.can_emulate_gicv2 = true;
> -- 
> 2.17.0
> 

[PATCH] KVM: arm/arm64: drop resource size check for GICV window

[Ard Biesheuvel]

> On 9 Jun 2018, at 12:06, Christoffer Dall <christoffer.dall@arm.com> wrote:
> 
>> On Fri, Jun 01, 2018 at 05:06:28PM +0200, Ard Biesheuvel wrote:
>> When booting a 64 KB pages kernel on a ACPI GICv3 system that
>> implements support for v2 emulation, the following warning is
>> produced
>> 
>>  GICV size 0x2000 not a multiple of page size 0x10000
>> 
>> and support for v2 emulation is disabled, preventing GICv2 VMs
>> from being able to run on such hosts.
>> 
>> The reason is that vgic_v3_probe() performs a sanity check on the
>> size of the window (it should be a multiple of the page size),
>> while the ACPI MADT parsing code hardcodes the size of the window
>> to 8 KB. This makes sense, considering that ACPI does not bother
>> to describe the size in the first place, under the assumption that
>> platforms implementing ACPI will follow the architecture and not
>> put anything else in the same 64 KB window.
> 
> Does the architecture actually say that anywhere?
> 
>> 
>> So let's just drop the sanity check altogether, and assume that
>> the window is at least 64 KB in size.
> 
> This could obviously be dangerous if broken systems actually exist.
> Marc may know more about that than me.  An alternative would be to
> modify the ACPI code to assume max(8 KB, page size) instead, and/or a
> command line parameter to override this check.
> 
> That said, I'm not directly opposed to this patch, but I'll let Marc
> have a look as well.
> 

This approach was actually Marc?s idea, and he already applied the patch to the queue branch afaik.


> 
>> 
>> Fixes: 909777324588 ("KVM: arm/arm64: vgic-new: vgic_init: implement kvm_vgic_hyp_init")
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> virt/kvm/arm/vgic/vgic-v3.c | 5 -----
>> 1 file changed, 5 deletions(-)
>> 
>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>> index bdcf8e7a6161..72fc688c3e9d 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -552,11 +552,6 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
>>        pr_warn("GICV physical address 0x%llx not page aligned\n",
>>            (unsigned long long)info->vcpu.start);
>>        kvm_vgic_global_state.vcpu_base = 0;
>> -    } else if (!PAGE_ALIGNED(resource_size(&info->vcpu))) {
>> -        pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
>> -            (unsigned long long)resource_size(&info->vcpu),
>> -            PAGE_SIZE);
>> -        kvm_vgic_global_state.vcpu_base = 0;
>>    } else {
>>        kvm_vgic_global_state.vcpu_base = info->vcpu.start;
>>        kvm_vgic_global_state.can_emulate_gicv2 = true;
>> -- 
>> 2.17.0
>> 

[PATCH] KVM: arm/arm64: drop resource size check for GICV window

[Christoffer Dall]

On Sat, Jun 09, 2018 at 12:30:14PM +0200, Ard Biesheuvel wrote:
> 
> 
> > On 9 Jun 2018, at 12:06, Christoffer Dall <christoffer.dall@arm.com> wrote:
> > 
> >> On Fri, Jun 01, 2018 at 05:06:28PM +0200, Ard Biesheuvel wrote:
> >> When booting a 64 KB pages kernel on a ACPI GICv3 system that
> >> implements support for v2 emulation, the following warning is
> >> produced
> >> 
> >>  GICV size 0x2000 not a multiple of page size 0x10000
> >> 
> >> and support for v2 emulation is disabled, preventing GICv2 VMs
> >> from being able to run on such hosts.
> >> 
> >> The reason is that vgic_v3_probe() performs a sanity check on the
> >> size of the window (it should be a multiple of the page size),
> >> while the ACPI MADT parsing code hardcodes the size of the window
> >> to 8 KB. This makes sense, considering that ACPI does not bother
> >> to describe the size in the first place, under the assumption that
> >> platforms implementing ACPI will follow the architecture and not
> >> put anything else in the same 64 KB window.
> > 
> > Does the architecture actually say that anywhere?
> > 
> >> 
> >> So let's just drop the sanity check altogether, and assume that
> >> the window is at least 64 KB in size.
> > 
> > This could obviously be dangerous if broken systems actually exist.
> > Marc may know more about that than me.  An alternative would be to
> > modify the ACPI code to assume max(8 KB, page size) instead, and/or a
> > command line parameter to override this check.
> > 
> > That said, I'm not directly opposed to this patch, but I'll let Marc
> > have a look as well.
> > 
> 
> This approach was actually Marc?s idea, and he already applied the patch to the queue branch afaik.
> 

Hmmm, ok.
-Christoffer

[PATCH] KVM: arm/arm64: drop resource size check for GICV window

[Marc Zyngier]

On Sat, 09 Jun 2018 11:06:57 +0100,
Christoffer Dall wrote:
> 
> On Fri, Jun 01, 2018 at 05:06:28PM +0200, Ard Biesheuvel wrote:
> > When booting a 64 KB pages kernel on a ACPI GICv3 system that
> > implements support for v2 emulation, the following warning is
> > produced
> > 
> >   GICV size 0x2000 not a multiple of page size 0x10000
> > 
> > and support for v2 emulation is disabled, preventing GICv2 VMs
> > from being able to run on such hosts.
> > 
> > The reason is that vgic_v3_probe() performs a sanity check on the
> > size of the window (it should be a multiple of the page size),
> > while the ACPI MADT parsing code hardcodes the size of the window
> > to 8 KB. This makes sense, considering that ACPI does not bother
> > to describe the size in the first place, under the assumption that
> > platforms implementing ACPI will follow the architecture and not
> > put anything else in the same 64 KB window.
> 
> Does the architecture actually say that anywhere?

It implies it in section 8.14 of the GICv3 spec:

<quote>
To enable use of 64KB pages, the GICV_* memory map must ensure that:

* The base address of the GICV_* registers is 64KB aligned.

* An alias of the GICV_* registers is provided starting at offset
  0xF000 from the start of the page such that a second copy of
  GICV_DIR exists at the start of the next 64KB page.  This provides
  support for both 4KB and 64KB pages.
</quote>

> > So let's just drop the sanity check altogether, and assume that
> > the window is at least 64 KB in size.
> 
> This could obviously be dangerous if broken systems actually exist.
> Marc may know more about that than me.  An alternative would be to
> modify the ACPI code to assume max(8 KB, page size) instead, and/or a
> command line parameter to override this check.

While the above is in effect very similar to the corresponding GICv2
requirements with the ARMv8 architecture (described in SBSA, which
everybody and their dog are unfortunately making a point in ignoring),
this is implemented in the CPU, meaning that integrators do not have
the opportunity to fsck it up. Hooray!

And as far as I know, this is only implemented on A35, A53, A57, A72
and A73 (all the other ARMv8 CPUs are purely GICv3, and no other
architectural licensee ever shipped a system with the compat
interface).

> That said, I'm not directly opposed to this patch, but I'll let Marc
> have a look as well.

My take on this is that we should play it as per the architecture, and
only add more checks if we're presented with a non-compliant
implementation.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

[PATCH] KVM: arm/arm64: drop resource size check for GICV window

[Christoffer Dall]

On Sat, Jun 09, 2018 at 01:09:32PM +0100, Marc Zyngier wrote:
> On Sat, 09 Jun 2018 11:06:57 +0100,
> Christoffer Dall wrote:
> > 
> > On Fri, Jun 01, 2018 at 05:06:28PM +0200, Ard Biesheuvel wrote:
> > > When booting a 64 KB pages kernel on a ACPI GICv3 system that
> > > implements support for v2 emulation, the following warning is
> > > produced
> > > 
> > >   GICV size 0x2000 not a multiple of page size 0x10000
> > > 
> > > and support for v2 emulation is disabled, preventing GICv2 VMs
> > > from being able to run on such hosts.
> > > 
> > > The reason is that vgic_v3_probe() performs a sanity check on the
> > > size of the window (it should be a multiple of the page size),
> > > while the ACPI MADT parsing code hardcodes the size of the window
> > > to 8 KB. This makes sense, considering that ACPI does not bother
> > > to describe the size in the first place, under the assumption that
> > > platforms implementing ACPI will follow the architecture and not
> > > put anything else in the same 64 KB window.
> > 
> > Does the architecture actually say that anywhere?
> 
> It implies it in section 8.14 of the GICv3 spec:
> 
> <quote>
> To enable use of 64KB pages, the GICV_* memory map must ensure that:
> 
> * The base address of the GICV_* registers is 64KB aligned.
> 
> * An alias of the GICV_* registers is provided starting at offset
>   0xF000 from the start of the page such that a second copy of
>   GICV_DIR exists at the start of the next 64KB page.  This provides
>   support for both 4KB and 64KB pages.
> </quote>
> 
> > > So let's just drop the sanity check altogether, and assume that
> > > the window is at least 64 KB in size.
> > 
> > This could obviously be dangerous if broken systems actually exist.
> > Marc may know more about that than me.  An alternative would be to
> > modify the ACPI code to assume max(8 KB, page size) instead, and/or a
> > command line parameter to override this check.
> 
> While the above is in effect very similar to the corresponding GICv2
> requirements with the ARMv8 architecture (described in SBSA, which
> everybody and their dog are unfortunately making a point in ignoring),
> this is implemented in the CPU, meaning that integrators do not have
> the opportunity to fsck it up. Hooray!
> 
> And as far as I know, this is only implemented on A35, A53, A57, A72
> and A73 (all the other ARMv8 CPUs are purely GICv3, and no other
> architectural licensee ever shipped a system with the compat
> interface).
> 
> > That said, I'm not directly opposed to this patch, but I'll let Marc
> > have a look as well.
> 
> My take on this is that we should play it as per the architecture, and
> only add more checks if we're presented with a non-compliant
> implementation.
> 

I had some faint memory that we had actually seen that, but I'm clearly
mixing things up.

Thanks for the extended explanation.
-Christoffer