Increase GICV size to 64KB so !4KB page-size kernels are able to initialize KVM

Increase GICV size to 64KB so !4KB page-size kernels are able to initialize KVM

[Itaru Kitayama]

Against the tag v4.7-rc2 in the kvmtree.

---
  drivers/irqchip/irq-gic.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index fbc4ae2..bd463a3 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1425,7 +1425,7 @@ static bool __init gic_validate_dist(struct 
acpi_subtable_header *header,
  #define ACPI_GICV2_DIST_MEM_SIZE       (SZ_4K)
  #define ACPI_GIC_CPU_IF_MEM_SIZE       (SZ_8K)
  #define ACPI_GICV2_VCTRL_MEM_SIZE      (SZ_4K)
-#define ACPI_GICV2_VCPU_MEM_SIZE       (SZ_8K)
+#define ACPI_GICV2_VCPU_MEM_SIZE       (SZ_64K)

  static void __init gic_acpi_setup_kvm_info(void)
  {
-- 
2.7.4

Re: Increase GICV size to 64KB so !4KB page-size kernels are able to initialize KVM

[Marc Zyngier]

On 22/06/16 07:48, Itaru Kitayama wrote:
> Against the tag v4.7-rc2 in the kvmtree.
> 
> ---
>   drivers/irqchip/irq-gic.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index fbc4ae2..bd463a3 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1425,7 +1425,7 @@ static bool __init gic_validate_dist(struct 
> acpi_subtable_header *header,
>   #define ACPI_GICV2_DIST_MEM_SIZE       (SZ_4K)
>   #define ACPI_GIC_CPU_IF_MEM_SIZE       (SZ_8K)
>   #define ACPI_GICV2_VCTRL_MEM_SIZE      (SZ_4K)
> -#define ACPI_GICV2_VCPU_MEM_SIZE       (SZ_8K)
> +#define ACPI_GICV2_VCPU_MEM_SIZE       (SZ_64K)
> 
>   static void __init gic_acpi_setup_kvm_info(void)
>   {
> 

I'm afraid this is the wrong approach. No matter how you look at it, the
size of the GICV region is 8kB, and not 64kB. On some systems the GICV
region is *aliased* over 128kB (first 4kB aliased over the first 64kB
page, second 4kB aliased over the second 64kB page). The usable range
for the GICV region is then the f000-10fff region in order to preserve
the contiguity of the two 4kB blocks.

In that case, ACPI ought to provide the base address of the usable
range, not some dummy address. It would be different if the ACPI tables
would provide the size of the region (where we could make an educated
guess at what is happening, just like with DT), but the information is
simply not available. So my advise here is to fix your ACPI tables (or
get them fixed).

What is actually missing in KVM is a way to describe the offset within a
page at which the GICV region must be mapped. I have patches for that
that I may revive if there is some interest.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: Increase GICV size to 64KB so !4KB page-size kernels are able to initialize KVM

[Itaru Kitayama]

Hi Marc,

Thanks for your comments. I'm definitely interested in your patch set 
that makes KVM to work with large page sizes. I'll ask the vendor to 
update some ACPI tables. (I'm on Overdrive with recent F/W though I 
think the GICV physical address is aligned at 4KB boundary still)

Itaru

On 6/22/16 7:06 PM, Marc Zyngier wrote:
> On 22/06/16 07:48, Itaru Kitayama wrote:
>> Against the tag v4.7-rc2 in the kvmtree.
>>
>> ---
>>   drivers/irqchip/irq-gic.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index fbc4ae2..bd463a3 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -1425,7 +1425,7 @@ static bool __init gic_validate_dist(struct
>> acpi_subtable_header *header,
>>   #define ACPI_GICV2_DIST_MEM_SIZE       (SZ_4K)
>>   #define ACPI_GIC_CPU_IF_MEM_SIZE       (SZ_8K)
>>   #define ACPI_GICV2_VCTRL_MEM_SIZE      (SZ_4K)
>> -#define ACPI_GICV2_VCPU_MEM_SIZE       (SZ_8K)
>> +#define ACPI_GICV2_VCPU_MEM_SIZE       (SZ_64K)
>>
>>   static void __init gic_acpi_setup_kvm_info(void)
>>   {
>>
>
> I'm afraid this is the wrong approach. No matter how you look at it, the
> size of the GICV region is 8kB, and not 64kB. On some systems the GICV
> region is *aliased* over 128kB (first 4kB aliased over the first 64kB
> page, second 4kB aliased over the second 64kB page). The usable range
> for the GICV region is then the f000-10fff region in order to preserve
> the contiguity of the two 4kB blocks.
>
> In that case, ACPI ought to provide the base address of the usable
> range, not some dummy address. It would be different if the ACPI tables
> would provide the size of the region (where we could make an educated
> guess at what is happening, just like with DT), but the information is
> simply not available. So my advise here is to fix your ACPI tables (or
> get them fixed).
>
> What is actually missing in KVM is a way to describe the offset within a
> page at which the GICV region must be mapped. I have patches for that
> that I may revive if there is some interest.
>
> Thanks,
>
> 	M.
>

Re: Increase GICV size to 64KB so !4KB page-size kernels are able to initialize KVM

[Marc Zyngier]

On 22/06/16 12:02, Itaru Kitayama wrote:
> Hi Marc,
> 
> Thanks for your comments. I'm definitely interested in your patch set 
> that makes KVM to work with large page sizes. I'll ask the vendor to 
> update some ACPI tables. (I'm on Overdrive with recent F/W though I 
> think the GICV physical address is aligned at 4KB boundary still)

I'm only using DT, which works just fine on Overdrive, even with 64kB 
pages.

I think I understand the issue you're having, which is not what I 
was thinking of. The issue is that because ACPI doesn't tell us
anything about the size of the GICV region, we have to assume that
it is 8kB, and cannot distinguish a safe platform from an unsafe
one, failing the size test on 64kB.

Oh well.

How about this:

diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index e31405e..8aac59b 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -315,12 +315,10 @@ int vgic_v2_probe(const struct gic_kvm_info *info)
 		return -ENXIO;
 	}
 
-	if (!PAGE_ALIGNED(resource_size(&info->vcpu))) {
-		kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n",
+	if (!PAGE_ALIGNED(resource_size(&info->vcpu)))
+		kvm_warn("GICV size 0x%llx not a multiple of page size 0x%lx, system may be unsafe\n",
 			(unsigned long long)resource_size(&info->vcpu),
 			PAGE_SIZE);
-		return -ENXIO;
-	}
 
 	kvm_vgic_global_state.vctrl_base = ioremap(info->vctrl.start,
 						   resource_size(&info->vctrl));

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: Increase GICV size to 64KB so !4KB page-size kernels are able to initialize KVM

[Itaru Kitayama]

On 6/22/16 9:20 PM, Marc Zyngier wrote:
> I'm only using DT, which works just fine on Overdrive, even with 64kB
> pages.
>
> I think I understand the issue you're having, which is not what I
> was thinking of. The issue is that because ACPI doesn't tell us
> anything about the size of the GICV region, we have to assume that
> it is 8kB, and cannot distinguish a safe platform from an unsafe
> one, failing the size test on 64kB.

Is it likely in the future specification of ACPI the size information
stored in the GIC subtable? In 6.1 or earlier, it seems optional to me.
I also had to allow the not page-aligned physical address of GICV, below
is an excerpt of apic.dsl

[054h 0084   8]     Virtual GIC Base Address : 00000000E116F000
[05Ch 0092   8]  Hypervisor GIC Base Address : 00000000E1140000
[064h 0100   4]        Virtual GIC Interrupt : 00000019
[068h 0104   8]   Redistributor Base Address : 0000000000000000
[070h 0112   8]                    ARM MPIDR : 0000000000000000

>
> Oh well.
>
> How about this:
>
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index e31405e..8aac59b 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -315,12 +315,10 @@ int vgic_v2_probe(const struct gic_kvm_info *info)
>  		return -ENXIO;
>  	}
>
> -	if (!PAGE_ALIGNED(resource_size(&info->vcpu))) {
> -		kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n",
> +	if (!PAGE_ALIGNED(resource_size(&info->vcpu)))
> +		kvm_warn("GICV size 0x%llx not a multiple of page size 0x%lx, system may be unsafe\n",
>  			(unsigned long long)resource_size(&info->vcpu),
>  			PAGE_SIZE);
> -		return -ENXIO;
> -	}
>
>  	kvm_vgic_global_state.vctrl_base = ioremap(info->vctrl.start,
>  						   resource_size(&info->vctrl));
>
> Thanks,
>
> 	M.
>

Re: Increase GICV size to 64KB so !4KB page-size kernels are able to initialize KVM

[Marc Zyngier]

On 23/06/16 04:17, Itaru Kitayama wrote:
> On 6/22/16 9:20 PM, Marc Zyngier wrote:
>> I'm only using DT, which works just fine on Overdrive, even with 64kB
>> pages.
>>
>> I think I understand the issue you're having, which is not what I
>> was thinking of. The issue is that because ACPI doesn't tell us
>> anything about the size of the GICV region, we have to assume that
>> it is 8kB, and cannot distinguish a safe platform from an unsafe
>> one, failing the size test on 64kB.
> 
> Is it likely in the future specification of ACPI the size information
> stored in the GIC subtable? In 6.1 or earlier, it seems optional to me.

I don't see any provision for that, and it is already too late (systems
exist in the wild).

> I also had to allow the not page-aligned physical address of GICV, below
> is an excerpt of apic.dsl
> 
> [054h 0084   8]     Virtual GIC Base Address : 00000000E116F000

Right, this is what I was referring to in my initial reply. The problem
is that we cannot know what is actually safe to map into a guest's
address space without any sizing information.

Also, we have the issue of mapping GICV at an offset in a page, which
userspace needs to be made aware of. So even with your hack, it is
unlikely that you'll be able to boot a guest because userspace has only
1/16th probability to map GICV at offset 0xf000.

Doing so requires reporting the page offset to userspace. I had this patch:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/264852.html

but that requires buy-in from QEMU (though I can do the corresponding
kvmtool change).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...