Re: [RFC PATCH 03/12] arm64: kvm: Cleanup VTCR_EL2/VTTBR computation
On Wed, 16 Mar 2016 15:37:18 + "Suzuki K. Poulose" wrote: > On 16/03/16 15:01, Marc Zyngier wrote: > > On 14/03/16 16:53, Suzuki K Poulose wrote: > >> No functional changes. Group the common bits for VCTR_EL2 > >> initialisation for better readability. The granule size > >> and the entry level are controlled by the page size. > > >> > >> diff --git a/arch/arm64/include/asm/kvm_arm.h > >> b/arch/arm64/include/asm/kvm_arm.h > >> index b7d61e4..d49dd50 100644 > >> --- a/arch/arm64/include/asm/kvm_arm.h > >> +++ b/arch/arm64/include/asm/kvm_arm.h > >> @@ -139,32 +139,30 @@ > >>* The magic numbers used for VTTBR_X in this patch can be found in > >> Tables > >>* D4-23 and D4-25 in ARM DDI 0487A.b. > >>*/ > > ... > > >> > >> +#define VTCR_EL2_FLAGS(VTCR_EL2_TGRAN_FLAGS | > >> VTCR_EL2_COMMON_BITS) > >> +#define VTTBR_X ((VTTBR_X_TGRAN_MAGIC) - > >> VTCR_EL2_T0SZ_40B) > > > > Nit: spurious brackets. > > Will remove them. > > > It would be nice to add an ARMv8 ARM reference to where the "magic" > > value is coming from. > > That reference already exists in the code, see above. Ah, good point! Thanks, M. -- Jazz is not dead. It just smells funny. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH 02/12] arm64: kvm: Fix {V}TCR_EL2_TG0 mask
On 14/03/16 16:53, Suzuki K Poulose wrote:
> {V}TCR_EL2_TG0 is a 2bit wide field, where:
>
> 00 - 4K
> 01 - 64K
> 10 - 16K
>
> But we use only 1 bit, which has worked well so far since
> we never cared about 16K. Fix it for 16K support.
>
> Cc: Catalin Marinas
> Cc: Will Deacon
> Cc: Marc Zyngier
> Cc: Christoffer Dall
> Cc: kvmarm@lists.cs.columbia.edu
> Acked-by: Mark Rutland
> Signed-off-by: Suzuki K Poulose
> ---
> arch/arm64/include/asm/kvm_arm.h |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_arm.h
> b/arch/arm64/include/asm/kvm_arm.h
> index d201d4b..b7d61e4 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -99,7 +99,7 @@
> #define TCR_EL2_TBI (1 << 20)
> #define TCR_EL2_PS (7 << 16)
> #define TCR_EL2_PS_40B (2 << 16)
> -#define TCR_EL2_TG0 (1 << 14)
> +#define TCR_EL2_TG0 (3 << 14)
> #define TCR_EL2_SH0 (3 << 12)
> #define TCR_EL2_ORGN0(3 << 10)
> #define TCR_EL2_IRGN0(3 << 8)
> @@ -110,7 +110,7 @@
> /* VTCR_EL2 Registers bits */
> #define VTCR_EL2_RES1(1 << 31)
> #define VTCR_EL2_PS_MASK (7 << 16)
> -#define VTCR_EL2_TG0_MASK(1 << 14)
> +#define VTCR_EL2_TG0_MASK(3 << 14)
> #define VTCR_EL2_TG0_4K (0 << 14)
> #define VTCR_EL2_TG0_64K (1 << 14)
> #define VTCR_EL2_SH0_MASK(3 << 12)
>
As we already have arch/arm64/include/asm/pgtable-hwdef.h defining
TCR_TG0_{4,16,64}K, would it make sense to reuse those and drop the
locally defined values? Something like:
#define TCR_EL2_TG0_4K TCR_TG0_4K
?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 5/9] irqchip/gic-v3: Gather all ACPI specific data in a single structure
On Tue, Mar 15, 2016 at 12:26:29PM +, Julien Grall wrote: > Hi Christoffer, > > On 09/03/16 05:39, Christoffer Dall wrote: > >On Tue, Mar 08, 2016 at 11:29:29AM +, Julien Grall wrote: > >>Even though all the variables aren't marked with __initdata, they are > >>only used during initialization. So the structure is marked with > >>__initdata. > > > >Not sure I understand this commit message. > > > >As I see it, this commit includes two changes: > > > >1. Mark the variables only used during init with __initdata > > > >2. Move the variables into a structure > > > >If I get that right, can you argue for both changes? > > What about: > > "The ACPI code requires to use global variables in order to collect > information from the tables. > > To make clear those variables are ACPI specific, gather all of them > in a single structure. > > Furthermore, even if some of the variables are not marked with > __initdata, they are all only used during the initialization. > Therefore, the new variable, which hold the structure, can be marked > __initdata." > Looks ok (the part about some of the variables not marked with __initdata is confusing but I think I understand what you mean). Thanks, -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 00/16] KVM: arm64: GICv3 ITS emulation
On Mon, Mar 14, 2016 at 05:29:44PM +, Peter Maydell wrote: > On 14 March 2016 at 11:13, Andre Przywara wrote: > > So I see two ways to fix this: > > 1.) we find a KVM specific way of letting userland save and restore the > > ITS tables directly > > 2.) we implement the BASER registers, but still use our "cache" for > > normal operations. On demand we would serialize KVM's virtual ITS data > > structures and put them into the guest's memory, so they could be > > saved/restored from there. > > I feel like we're rehashing a bunch of design choices we talked > through way back in the last-but-one Connect. I don't suppose > anybody wrote down our rationales from back then? Someone (not me) had the task to write it down, I don't recall if that happened or not :) > > (In particular I forget whether we decided the ITS tables were > large enough to need to allow some sort of before-the-VM-stops > migration of the data, which would be relatively doable with > option 2 but painful under option 1.) I think we concluded that it's not so much data that applying dirty bitmaps stuff on there is strictly necessary, but that being able to do this was probably a plus, and not very hard to do. I am quite sure that we dismissed option 1, and were decided on option 2 though. -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 00/16] KVM: arm64: GICv3 ITS emulation
On 18 March 2016 at 09:40, Christoffer Dall wrote: > On Mon, Mar 14, 2016 at 06:20:36PM +, Andre Przywara wrote: >> Well, probably there is not so much difference. I was just wondering if >> it would be easier to treat that data as an opaque blob. >> But you are probably right that it would just mean the difference >> between documenting the format or not. > Even ignoring the migrate-to-TCG case, you cannot treat it as a blob, > because you want to be able to migrate between KVM on kernel version X > and version Y. You could require userspace to treat it as an opaque blob, and transparently handle any version-upgrade within the kernel. I think having it be documented-to-userspace ABI makes it clearer that any format changes are a Big Deal, though. thanks -- PMM ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH 03/12] arm64: kvm: Cleanup VTCR_EL2/VTTBR computation
On 14/03/16 16:53, Suzuki K Poulose wrote: > No functional changes. Group the common bits for VCTR_EL2 > initialisation for better readability. The granule size > and the entry level are controlled by the page size. > > Cc: Christoffer Dall > Cc: Marc Zyngier > Cc: kvmarm@lists.cs.columbia.edu > Signed-off-by: Suzuki K Poulose > --- > arch/arm64/include/asm/kvm_arm.h | 22 ++ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_arm.h > b/arch/arm64/include/asm/kvm_arm.h > index b7d61e4..d49dd50 100644 > --- a/arch/arm64/include/asm/kvm_arm.h > +++ b/arch/arm64/include/asm/kvm_arm.h > @@ -139,32 +139,30 @@ > * The magic numbers used for VTTBR_X in this patch can be found in Tables > * D4-23 and D4-25 in ARM DDI 0487A.b. > */ > +#define VTCR_EL2_COMMON_BITS (VTCR_EL2_SH0_INNER | VTCR_EL2_ORGN0_WBWA | \ > + VTCR_EL2_IRGN0_WBWA | VTCR_EL2_SL0_LVL1 | \ > + VTCR_EL2_RES1 | VTCR_EL2_T0SZ_40B) > #ifdef CONFIG_ARM64_64K_PAGES > /* > * Stage2 translation configuration: > - * 40bits input (T0SZ = 24) > * 64kB pages (TG0 = 1) > * 2 level page tables (SL = 1) > */ > -#define VTCR_EL2_FLAGS (VTCR_EL2_TG0_64K | VTCR_EL2_SH0_INNER > | \ > - VTCR_EL2_ORGN0_WBWA | VTCR_EL2_IRGN0_WBWA | \ > - VTCR_EL2_SL0_LVL1 | VTCR_EL2_T0SZ_40B | \ > - VTCR_EL2_RES1) > -#define VTTBR_X (38 - VTCR_EL2_T0SZ_40B) > +#define VTCR_EL2_TGRAN_FLAGS (VTCR_EL2_TG0_64K | VTCR_EL2_SL0_LVL1) > +#define VTTBR_X_TGRAN_MAGIC 38 > #else > /* > * Stage2 translation configuration: > - * 40bits input (T0SZ = 24) > * 4kB pages (TG0 = 0) > * 3 level page tables (SL = 1) > */ > -#define VTCR_EL2_FLAGS (VTCR_EL2_TG0_4K | VTCR_EL2_SH0_INNER | > \ > - VTCR_EL2_ORGN0_WBWA | VTCR_EL2_IRGN0_WBWA | \ > - VTCR_EL2_SL0_LVL1 | VTCR_EL2_T0SZ_40B | \ > - VTCR_EL2_RES1) > -#define VTTBR_X (37 - VTCR_EL2_T0SZ_40B) > +#define VTCR_EL2_TGRAN_FLAGS (VTCR_EL2_TG0_4K | VTCR_EL2_SL0_LVL1) > +#define VTTBR_X_TGRAN_MAGIC 37 > #endif > > +#define VTCR_EL2_FLAGS (VTCR_EL2_TGRAN_FLAGS | > VTCR_EL2_COMMON_BITS) > +#define VTTBR_X ((VTTBR_X_TGRAN_MAGIC) - > VTCR_EL2_T0SZ_40B) Nit: spurious brackets. It would be nice to add an ARMv8 ARM reference to where the "magic" value is coming from. > + > #define VTTBR_BADDR_SHIFT (VTTBR_X - 1) > #define VTTBR_BADDR_MASK (((UL(1) << (PHYS_MASK_SHIFT - VTTBR_X)) - 1) << > VTTBR_BADDR_SHIFT) > #define VTTBR_VMID_SHIFT (UL(48)) > Otherwise: Acked-by: Marc Zyngier M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH 03/12] arm64: kvm: Cleanup VTCR_EL2/VTTBR computation
On 16/03/16 15:01, Marc Zyngier wrote: On 14/03/16 16:53, Suzuki K Poulose wrote: No functional changes. Group the common bits for VCTR_EL2 initialisation for better readability. The granule size and the entry level are controlled by the page size. diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h index b7d61e4..d49dd50 100644 --- a/arch/arm64/include/asm/kvm_arm.h +++ b/arch/arm64/include/asm/kvm_arm.h @@ -139,32 +139,30 @@ * The magic numbers used for VTTBR_X in this patch can be found in Tables * D4-23 and D4-25 in ARM DDI 0487A.b. */ ... +#define VTCR_EL2_FLAGS (VTCR_EL2_TGRAN_FLAGS | VTCR_EL2_COMMON_BITS) +#define VTTBR_X((VTTBR_X_TGRAN_MAGIC) - VTCR_EL2_T0SZ_40B) Nit: spurious brackets. Will remove them. It would be nice to add an ARMv8 ARM reference to where the "magic" value is coming from. That reference already exists in the code, see above. + #define VTTBR_BADDR_SHIFT (VTTBR_X - 1) #define VTTBR_BADDR_MASK (((UL(1) << (PHYS_MASK_SHIFT - VTTBR_X)) - 1) << VTTBR_BADDR_SHIFT) #define VTTBR_VMID_SHIFT (UL(48)) Otherwise: Acked-by: Marc Zyngier Thanks Suzuki ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH 02/12] arm64: kvm: Fix {V}TCR_EL2_TG0 mask
On 16/03/16 14:54, Marc Zyngier wrote:
On 14/03/16 16:53, Suzuki K Poulose wrote:
{V}TCR_EL2_TG0 is a 2bit wide field, where:
00 - 4K
01 - 64K
10 - 16K
But we use only 1 bit, which has worked well so far since
we never cared about 16K. Fix it for 16K support.
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -99,7 +99,7 @@
#define TCR_EL2_TBI (1 << 20)
#define TCR_EL2_PS(7 << 16)
#define TCR_EL2_PS_40B(2 << 16)
-#define TCR_EL2_TG0(1 << 14)
+#define TCR_EL2_TG0(3 << 14)
#define TCR_EL2_SH0 (3 << 12)
#define TCR_EL2_ORGN0 (3 << 10)
#define TCR_EL2_IRGN0 (3 << 8)
@@ -110,7 +110,7 @@
/* VTCR_EL2 Registers bits */
#define VTCR_EL2_RES1 (1 << 31)
#define VTCR_EL2_PS_MASK (7 << 16)
-#define VTCR_EL2_TG0_MASK (1 << 14)
+#define VTCR_EL2_TG0_MASK (3 << 14)
#define VTCR_EL2_TG0_4K (0 << 14)
#define VTCR_EL2_TG0_64K (1 << 14)
#define VTCR_EL2_SH0_MASK (3 << 12)
As we already have arch/arm64/include/asm/pgtable-hwdef.h defining
TCR_TG0_{4,16,64}K, would it make sense to reuse those and drop the
locally defined values? Something like:
#define TCR_EL2_TG0_4K TCR_TG0_4K
?
We could do that for both TCR_EL2 and VTCR_EL2.
Btw, since this patch doesn't touch any of those fields and fixes an issue,
I will keep that change separate. I can squash it to the following cleanup
patch in this series.
Cheers
Suzuki
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 00/16] KVM: arm64: GICv3 ITS emulation
On Mon, Mar 14, 2016 at 06:20:36PM +, Andre Przywara wrote: > Hi, > > On 14/03/16 17:54, Marc Zyngier wrote: > > On 14/03/16 17:29, Peter Maydell wrote: > >> On 14 March 2016 at 11:13, Andre Przywara wrote: > >>> So I see two ways to fix this: > >>> 1.) we find a KVM specific way of letting userland save and restore the > >>> ITS tables directly > >>> 2.) we implement the BASER registers, but still use our "cache" for > >>> normal operations. On demand we would serialize KVM's virtual ITS data > >>> structures and put them into the guest's memory, so they could be > >>> saved/restored from there. > >> > >> I feel like we're rehashing a bunch of design choices we talked > >> through way back in the last-but-one Connect. I don't suppose > >> anybody wrote down our rationales from back then? > >> > >> (In particular I forget whether we decided the ITS tables were > >> large enough to need to allow some sort of before-the-VM-stops > >> migration of the data, which would be relatively doable with > >> option 2 but painful under option 1.) > > > > I think only option 2 is valid here, and we must be able to shove most > > of the routing information in the device/collection/IT tables. Common HW > > seems to use 64bit of data per entry per table, so we should be able to > > do the same with KVM. > > All right, just skimmed over this and it looks doable. > For the collection table we will most likely even get away with 32 bits > per entry (compressed MPIDR or even VCPUIDs). > Would the IPA of the ITTE suffice for each device table entry? > > I will work out the details later. > > Only caveat there I think was that we had to decide on a storage format > in those memory regions, to allow QEMU to understand the state and to > ensure back/forwards compatibility between KVM versions. > >>> > >>> Do we need QEMU to actually understand this? Can't we just leave this > >>> all to the kernel and QEMU just passes on the data? That would still > >>> require some ABI stability between kernel versions in this respect, but > >>> it's less problematic than exposing the data format to userland at all. > >> > >> This would preclude ever being able to migrate a VM from KVM to > >> TCG QEMU, which seems a shame. (That doesn't work right now, but > >> I'm a bit wary of shutting the door to it forever.) > > > > If the format of the migrated tables becomes ABI for KVM, it also > > becomes ABI for userspace (anything that comes out of the kernel *is* > > ABI). Andre, can you please explain what you mean? > > Well, probably there is not so much difference. I was just wondering if > it would be easier to treat that data as an opaque blob. > But you are probably right that it would just mean the difference > between documenting the format or not. > Even ignoring the migrate-to-TCG case, you cannot treat it as a blob, because you want to be able to migrate between KVM on kernel version X and version Y. -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE
Hi Marc,
On 18/03/16 17:25, Marc Zyngier wrote:
When running with VHE, there is no need to translate kernel pointers
to the EL2 memory space, since we're already there (and we have a much
saner memory map to start with).
Unfortunately, kvm_ksym_ref is getting in the way, and the first
call into the "hypervisor" section is going to end up in fireworks,
since we're now branching into nowhereland. Meh.
A potential solution is to test if VHE is engaged or not, and only
perform the translation in the negative case. With this in place,
VHE is able to run again.
Signed-off-by: Marc Zyngier
---
arch/arm64/include/asm/kvm_asm.h | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 226f49d..282f907 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -26,7 +26,13 @@
#define KVM_ARM64_DEBUG_DIRTY_SHIFT 0
#define KVM_ARM64_DEBUG_DIRTY (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
-#define kvm_ksym_ref(sym) phys_to_virt((u64)&sym - kimage_voffset)
+#define kvm_ksym_ref(sym) \
+ ({ \
+ void *val = sym;\
+ if (!is_kernel_in_hyp_mode()) \
+ val = phys_to_virt((u64)&sym - kimage_voffset); \
Is it definitely OK to evaluate sym twice here?
Robin.
+ val;\
+})
#ifndef __ASSEMBLY__
struct kvm;
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE
When running with VHE, there is no need to translate kernel pointers
to the EL2 memory space, since we're already there (and we have a much
saner memory map to start with).
Unfortunately, kvm_ksym_ref is getting in the way, and the first
call into the "hypervisor" section is going to end up in fireworks,
since we're now branching into nowhereland. Meh.
A potential solution is to test if VHE is engaged or not, and only
perform the translation in the negative case. With this in place,
VHE is able to run again.
Signed-off-by: Marc Zyngier
---
arch/arm64/include/asm/kvm_asm.h | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 226f49d..282f907 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -26,7 +26,13 @@
#define KVM_ARM64_DEBUG_DIRTY_SHIFT0
#define KVM_ARM64_DEBUG_DIRTY (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
-#define kvm_ksym_ref(sym) phys_to_virt((u64)&sym - kimage_voffset)
+#define kvm_ksym_ref(sym) \
+ ({ \
+ void *val = sym;\
+ if (!is_kernel_in_hyp_mode()) \
+ val = phys_to_virt((u64)&sym - kimage_voffset); \
+ val;\
+})
#ifndef __ASSEMBLY__
struct kvm;
--
2.1.4
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
