Re: [PATCH 2/7] kvmtool: virt_queue: handle guest endianness
On 11/10/13 15:50, Will Deacon wrote:
On Fri, Oct 11, 2013 at 03:36:30PM +0100, Marc Zyngier wrote:
Wrap all accesses to virt_queue data structures shared between
host and guest with byte swapping helpers.
Should the architecture only support one endianness, these helpers
are reduced to the identity function.
Cc: Pekka Enberg penb...@kernel.org
Cc: Will Deacon will.dea...@arm.com
Signed-off-by: Marc Zyngier marc.zyng...@arm.com
---
tools/kvm/include/kvm/virtio.h | 189 -
tools/kvm/virtio/core.c| 59 +++--
2 files changed, 219 insertions(+), 29 deletions(-)
diff --git a/tools/kvm/include/kvm/virtio.h b/tools/kvm/include/kvm/virtio.h
index d6b0f47..04ec137 100644
--- a/tools/kvm/include/kvm/virtio.h
+++ b/tools/kvm/include/kvm/virtio.h
@@ -1,6 +1,8 @@
#ifndef KVM__VIRTIO_H
#define KVM__VIRTIO_H
+#include endian.h
+
#include linux/virtio_ring.h
#include linux/virtio_pci.h
@@ -29,15 +31,194 @@ struct virt_queue {
u16 endian;
};
+/*
+ * The default policy is not to cope with the guest endianness.
+ * It also helps not breaking archs that do not care about supporting
+ * such a configuration.
+ */
Jesus Marc, are you *trying* to crash my preprocessor? Seriously though,
maybe this is better done as a block:
#if __BYTE_ORDER == __BIG_ENDIAN
#define virtio_le16toh(x) le16toh(x)
#define virtio_be16toh(x)
[...]
The preprocessor magic to turn the functions into one-liners is pretty
gruesome in itself, though...
+#ifndef VIRTIO_RING_ENDIAN
+#define VIRTIO_RING_ENDIAN 0
+#endif
+
+#if (VIRTIO_RING_ENDIAN ((1UL VIRTIO_RING_F_GUEST_LE) | (1UL
VIRTIO_RING_F_GUEST_BE)))
+
+#ifndef __BYTE_ORDER
+#error No byteorder? Giving up...
+#endif
#ifdef __BYTE_ORDER
#if __BYTE_ORDER == __BIG_ENDIAN
#define BYTEORDER_TOKEN be
#define ENDIAN_OPPOSITE VIRTIO_ENDIAN_LE
#else
#define BYTEORDER_TOKEN le
#define ENDIAN_OPPOSITE VIRTIO_ENDIAN_BE
#endif
#define _CAT3(a,b,c)a##b##c
#define CAT3(a,b,c) _CAT3(a,b,c)
#define vio_gtoh(size, val) (endian==ENDIAN_OPPOSITE) ?\
CAT3(BYTEORDER_TOKEN,size,toh(val)) : val
#define vio_htog(size, val) (endian==ENDIAN_OPPOSITE) ?\
CAT3(hto,BYTEORDER_TOKEN,size(val)) : val
#else
#error No byteorder? Giving up...
#endif
+
+
+static inline __u16 __virtio_guest_to_host_u16(u16 endian, __u16 val)
+{
+#if __BYTE_ORDER == __BIG_ENDIAN
+ if (endian == VIRTIO_ENDIAN_LE)
+ return le16toh(val);
+#else
+ if (endian == VIRTIO_ENDIAN_BE)
+ return be16toh(val);
+#endif
{
return vio_gtoh(16, val);
}
On the upside however, it does remove all the duplication and keep all
the mess in one place.
Robin.
Then you can just use the endian parameter to do the right thing.
Will
___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] kvmtool: Enable overriding Generic Timer frequency
This patch series allows (but discourages) overriding the Generic Timer frequency for device tree-based guest OSes, to work around systems with broken secure firmware that fails to program CNTFRQ correctly. Robin Murphy (2): kvmtool: Support unsigned int options kvmtool/arm: Add option to override Generic Timer frequency tools/kvm/arm/include/arm-common/kvm-config-arch.h | 15 ++- tools/kvm/arm/timer.c |2 ++ tools/kvm/include/kvm/parse-options.h |9 + 3 files changed, 21 insertions(+), 5 deletions(-) -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] kvmtool: Support unsigned int options
Add support for unsigned int command-line options by implementing the
OPT_UINTEGER macro.
Signed-off-by: Robin Murphy robin.mur...@arm.com
Acked-by: Will Deacon will.dea...@arm.com
---
tools/kvm/include/kvm/parse-options.h |9 +
1 file changed, 9 insertions(+)
diff --git a/tools/kvm/include/kvm/parse-options.h
b/tools/kvm/include/kvm/parse-options.h
index 09a5fca..b03f151 100644
--- a/tools/kvm/include/kvm/parse-options.h
+++ b/tools/kvm/include/kvm/parse-options.h
@@ -109,6 +109,15 @@ struct option {
.help = (h) \
}
+#define OPT_UINTEGER(s, l, v, h)\
+{ \
+ .type = OPTION_UINTEGER,\
+ .short_name = (s), \
+ .long_name = (l), \
+ .value = check_vtype(v, unsigned int *), \
+ .help = (h) \
+}
+
#define OPT_U64(s, l, v, h) \
{ \
.type = OPTION_U64, \
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] kvmtool/arm: Add option to override Generic Timer frequency
Some platforms have secure firmware which does not correctly set the
CNTFRQ register on boot, preventing the use of the Generic Timer.
This patch allows mirroring the necessary host workaround by specifying
the clock-frequency property in the guest DT.
This should only be considered a means of KVM bring-up on such systems,
such that vendors may be convinced to properly implement their firmware
to support the virtualisation capabilities of their hardware.
Signed-off-by: Robin Murphy robin.mur...@arm.com
Acked-by: Will Deacon will.dea...@arm.com
---
tools/kvm/arm/include/arm-common/kvm-config-arch.h | 15 ++-
tools/kvm/arm/timer.c |2 ++
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/tools/kvm/arm/include/arm-common/kvm-config-arch.h
b/tools/kvm/arm/include/arm-common/kvm-config-arch.h
index 7ac6f6e..f3baf39 100644
--- a/tools/kvm/arm/include/arm-common/kvm-config-arch.h
+++ b/tools/kvm/arm/include/arm-common/kvm-config-arch.h
@@ -5,13 +5,18 @@
struct kvm_config_arch {
const char *dump_dtb_filename;
+ unsigned int force_cntfrq;
bool aarch32_guest;
};
-#define OPT_ARCH_RUN(pfx, cfg) \
- pfx,\
- ARM_OPT_ARCH_RUN(cfg) \
- OPT_STRING('\0', dump-dtb, (cfg)-dump_dtb_filename, \
- .dtb file, Dump generated .dtb to specified file),
+#define OPT_ARCH_RUN(pfx, cfg)
\
+ pfx,
\
+ ARM_OPT_ARCH_RUN(cfg)
\
+ OPT_STRING('\0', dump-dtb, (cfg)-dump_dtb_filename,
\
+ .dtb file, Dump generated .dtb to specified file),
\
+ OPT_UINTEGER('\0', override-bad-firmware-cntfrq,
(cfg)-force_cntfrq,\
+Specify Generic Timer frequency in guest DT to
\
+work around buggy secure firmware *Firmware should be
\
+updated to program CNTFRQ correctly*),
#endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
diff --git a/tools/kvm/arm/timer.c b/tools/kvm/arm/timer.c
index bd6a0bb..d757c1d 100644
--- a/tools/kvm/arm/timer.c
+++ b/tools/kvm/arm/timer.c
@@ -33,6 +33,8 @@ void timer__generate_fdt_nodes(void *fdt, struct kvm *kvm,
int *irqs)
_FDT(fdt_begin_node(fdt, timer));
_FDT(fdt_property(fdt, compatible, compatible, sizeof(compatible)));
_FDT(fdt_property(fdt, interrupts, irq_prop, sizeof(irq_prop)));
+ if (kvm-cfg.arch.force_cntfrq 0)
+ _FDT(fdt_property_cell(fdt, clock-frequency,
kvm-cfg.arch.force_cntfrq));
_FDT(fdt_end_node(fdt));
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] kvmtool/arm: Add option to override Generic Timer frequency
On 17/12/13 20:39, Alexander Graf wrote: On 17.12.2013, at 19:31, Robin Murphy robin.mur...@arm.com wrote: Some platforms have secure firmware which does not correctly set the CNTFRQ register on boot, preventing the use of the Generic Timer. This patch allows mirroring the necessary host workaround by specifying the clock-frequency property in the guest DT. This should only be considered a means of KVM bring-up on such systems, such that vendors may be convinced to properly implement their firmware to support the virtualisation capabilities of their hardware. Signed-off-by: Robin Murphy robin.mur...@arm.com Acked-by: Will Deacon will.dea...@arm.com How does it encourage a vendor to properly implement their firmware if there's a workaround? Alex Hi Alex, In short, by enabling the users to create the demand. Yes, like any workaround there's potential for abuse, but having *something* that makes it work is the difference between I want virtualisation[1] and Dear vendor, I've tried virtualisation on your chip/board and it's great, but it tells me I need new firmware, where do I get that? Having the specs tell them what to do clearly isn't sufficient, so let's give the integrators and consumers incentive to shout at them too. The sooner proper support is commonplace and we can deprecate clock-frequency hacks altogether, the better. Robin. [1] http://www.theregister.co.uk/2013/12/12/virtualisation_on_mobile_phones_is_coming/ -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] kvmtool/arm: Add option to override Generic Timer frequency
On 18/12/13 14:07, Alexander Graf wrote: [...] How does it encourage a vendor to properly implement their firmware if there's a workaround? Alex Hi Alex, In short, by enabling the users to create the demand. Yes, like any workaround there's potential for abuse, but having *something* that makes it work is the difference between I want virtualisation[1] and Dear vendor, I've tried virtualisation on your chip/board and it's great, but it tells me I need new firmware, where do I get that? Having the specs tell them what to do clearly isn't sufficient, so let's give the integrators and consumers incentive to shout at them too. The sooner proper support is commonplace and we can deprecate clock-frequency hacks altogether, the better. Oh, I'm all for hacks. But please don't fall under the illusion that this will push vendors to fix their firmware. It will have the opposite effect - vendors will just point to the workaround and say but it works :). If vendors already aren't bothering to support functionality available in their flagship hardware, workarounds hardly make that worse, and are a win for the user. If it can drive adoption enough to get vendors to see the value in at least fixing future products, that's only good. Either way, this hack is basically required because you can't program CNTFRQ because it's controlled by secure firmware, right? So the host os already needs to know about this and probably does have a clock-frequency value in its device tree entry already to know how fast its clock ticks. In some cases, yes. In others they don't explicitly use the arch timer at all thus have no frequency set anywhere. In the case of the board I have on my desk, it took hacking the non-secure part of the bootloader, writing a shim to throw away the securely-booted non-hyp cpu0 and fire up a secondary, and hacking a timer node into the host DT to even get as far as having an issue with kvmtool. Couldn't we search for that host entry and automatically pass it into the guest if it's there? That way the whole thing becomes seamless and even less of an issue. In theory that would be an ideal solution, yes. In practice it means either making KVM dependent on PROC_DEVICETREE (yuck) or cooking up some kernel interface to expose the system timer frequency to userspace (double yuck). Not just global solution to local problem, but global solution to local problem-that-shouldn't-even-exist-and-you-want-to-go-away-as-soon-as-possible-without-leaving-a-legacy. Besides, that would probably just reinforce the equally wrong behaviour of putting the frequency in the host DT instead of fixing the firmware ;) Robin. Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] vfio/type1: Do not support IOMMUs that allow bypass
On 29/10/15 18:28, Will Deacon wrote: On Tue, Oct 27, 2015 at 10:00:11AM -0600, Alex Williamson wrote: On Tue, 2015-10-27 at 15:40 +, Will Deacon wrote: On Fri, Oct 16, 2015 at 09:51:22AM -0600, Alex Williamson wrote: Would it be possible to add iommu_domain_geometry support to arm-smmu.c? In addition to this test to verify that DMA cannot bypass the IOMMU, I'd eventually like to pass the aperture information out through the VFIO API. Thanks, The slight snag here is that we don't know which SMMU in the system the domain is attached to at the point when the geometry is queried, so I can't give you an upper bound on the aperture. For example, if there is an SMMU with a 32-bit input address and another with a 48-bit input address. We could play the same horrible games that we do with the pgsize bitmap, and truncate some global aperture everytime we probe an SMMU device, but I'd really like to have fewer hacks like that if possible. The root of the problem is still that domains are allocated for a bus, rather than an IOMMU instance. Yes, Intel VT-d has this issue as well. In theory we can have heterogeneous IOMMU hardware units (DRHDs) in a system and the upper bound of the geometry could be diminished if we add a less capable DRHD into the domain. I suspect this is more a theoretical problem than a practical one though as we're typically mixing similar DRHDs and I think we're still capable of 39-bit addressing in the least capable version per the spec. In any case, I really want to start testing geometry.force_aperture, even if we're not yet comfortable to expose the IOMMU limits to the user. The vfio type1 shouldn't be enabled at all for underlying hardware that allows DMA bypass. Thanks, Ok, I'll put it on my list of things to look at under the assumption that the actual aperture limits don't need to be accurate as long as DMA to an arbitrary unmapped address always faults. I'm pretty sure we'd only ever set the aperture to the full input address range anyway (since we're not a GART-type thing), in which case we should only need to worry about unmatched streams that don't hit in a domain at all. Doesn't the disable_bypass option already cover that? (FWIW I hacked it up for v2 a while back, too[0]). Robin. [0]:http://www.linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=23a251189fa3330b799a837bd8eb1023aa2dcea4 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] arm64: KVM: Fix AArch32 to AArch64 register mapping
Hi Marc,
On 16/11/15 10:28, Marc Zyngier wrote:
When running a 32bit guest under a 64bit hypervisor, the ARMv8
architecture defines a mapping of the 32bit registers in the 64bit
space. This includes banked registers that are being demultiplexed
over the 64bit ones.
On exception caused by an operation involving a 32bit register, the
HW exposes the register number in the ESR_EL2 register. It was so
far understood that SW had to compute which register was AArch64
register was used (based on the current AArch32 mode and register
number).
It turns out that I misinterpreted the ARM ARM, and the clue is in
D1.20.1: "For some exceptions, the exception syndrome given in the
ESR_ELx identifies one or more register numbers from the issued
instruction that generated the exception. Where the exception is
taken from an Exception level using AArch32 these register numbers
give the AArch64 view of the register."
Which means that the HW is already giving us the translated version,
and that we shouldn't try to interpret it at all (for example, doing
an MMIO operation from the IRQ mode using the LR register leads to
very unexpected behaviours).
The fix is thus not to perform a call to vcpu_reg32() at all from
vcpu_reg(), and use whatever register number is supplied directly.
The only case we need to find out about the mapping is when we
actively generate a register access, which only occurs when injecting
a fault in a guest.
Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
arch/arm64/include/asm/kvm_emulate.h | 8 +---
arch/arm64/kvm/inject_fault.c| 2 +-
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_emulate.h
b/arch/arm64/include/asm/kvm_emulate.h
index 17e92f0..3ca894e 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -99,11 +99,13 @@ static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
*vcpu_cpsr(vcpu) |= COMPAT_PSR_T_BIT;
}
+/*
+ * vcpu_reg should always be passed a register number coming from a
+ * read of ESR_EL2. Otherwise, it may give the wrong result on AArch32
+ * with banked registers.
+ */
static inline unsigned long *vcpu_reg(const struct kvm_vcpu *vcpu, u8 reg_num)
{
- if (vcpu_mode_is_32bit(vcpu))
- return vcpu_reg32(vcpu, reg_num);
-
return (unsigned long *)_gp_regs(vcpu)->regs.regs[reg_num];
}
diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
index 85c5715..648112e 100644
--- a/arch/arm64/kvm/inject_fault.c
+++ b/arch/arm64/kvm/inject_fault.c
@@ -48,7 +48,7 @@ static void prepare_fault32(struct kvm_vcpu *vcpu, u32 mode,
u32 vect_offset)
/* Note: These now point to the banked copies */
*vcpu_spsr(vcpu) = new_spsr_value;
- *vcpu_reg(vcpu, 14) = *vcpu_pc(vcpu) + return_offset;
+ *vcpu_reg32(vcpu, 14) = *vcpu_pc(vcpu) + return_offset;
To the best of my knowledge after picking through all the uses of
vcpu_reg, particularly in the shared 32-bit code, this does seem to be
the only one which involves a potentially-banked register number that
didn't originally come from an ESR read, and thus needs translation.
Reviewed-by: Robin Murphy <robin.mur...@arm.com>
(unfortunately I don't have an actual test-case as it was already a
third-hand report when I started trying to look into it).
Thanks for picking this up,
Robin.
/* Branch to exception vector */
if (sctlr & (1 << 13))
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
