On 21.09.2011, at 08:35, David Gibson wrote:
> Sufficiently recent PAPR specifications define properties "ibm,vmx"
> and "ibm,dfp" on the CPU node which advertise whether the VMX vector
> extensions (or the later VSX version) and/or the Decimal Floating
> Point operations from IBM's recent POWER CPUs are available.
>
> Currently we do not put these in the guest device tree and the guest
> kernel will consequently assume they are not available. This is good,
> because they are not supported under TCG. VMX is similar enough to
> Altivec that it might be trivial to support, but VSX and DFP would
> both require significant work to support in TCG.
>
> However, when running under kvm on a host which supports these
> instructions, there's no reason not to let the guest use them. This
> patch, therefore, checks for the relevant support on the host CPU
> and, if present, advertises them to the guest as well.
>
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
> hw/spapr.c | 13 ++++++++++
> target-ppc/kvm.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
> target-ppc/kvm_ppc.h | 12 ++++++++++
> 3 files changed, 85 insertions(+), 0 deletions(-)
>
> diff --git a/hw/spapr.c b/hw/spapr.c
> index b118975..573392d 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -168,6 +168,9 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
> 0xffffffff, 0xffffffff};
> uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TIMEBASE_FREQ;
> uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() :
> 1000000000;
> + /* Currently TCG doesn't implement VMX or DFP instructions */
> + uint32_t vmx = kvm_enabled() ? kvmppc_get_vmx() : 0;
> + uint32_t dfp = kvm_enabled() ? kvmppc_get_dfp() : 0;
This should also take the target CPU into account. If the target CPU can't do
VMX/DFP, it shouldn' be announced. Imagine running a guest with the
compatibility mode bit set, so we're virtualization the previous generation.
When did dfp get introduced? Was that POWER6 or POWER7? Also, isn't there this
twice-as-wide VMX extension too?
>
> if (asprintf(&nodename, "%s@%x", modelname, index) < 0) {
> fprintf(stderr, "Allocation failure\n");
> @@ -202,6 +205,16 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
> segs, sizeof(segs))));
> }
>
> + /* Advertise VMX/VSX (vector extensions) if available */
> + if (vmx) {
> + _FDT((fdt_property_cell(fdt, "ibm,vmx", vmx)));
> + }
> +
> + /* Advertise DFP (Decimal Floating Point) if available */
> + if (dfp) {
> + _FDT((fdt_property_cell(fdt, "ibm,dfp", dfp)));
> + }
> +
> _FDT((fdt_end_node(fdt)));
> }
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 35a6f10..397a803 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -673,6 +673,66 @@ uint64_t kvmppc_get_clockfreq(void)
> return 0;
> }
>
> +uint32_t kvmppc_get_vmx(void)
> +{
> + char buf[512];
> + uint32_t vmx;
> + FILE *f;
> + int len;
> +
> + if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
> + return 0;
> + }
> +
> + strncat(buf, "/ibm,vmx", sizeof(buf) - strlen(buf));
> +
> + f = fopen(buf, "rb");
> + if (!f) {
> + /* Assume -ENOENT, which indicates that VMX is not available */
> + return 0;
> + }
> +
> + len = fread(&vmx, sizeof(vmx), 1, f);
> + fclose(f);
> +
> + if (len != 1) {
> + /* Malformed ibm,vmx property, assume no vmx or vsx */
> + return 0;
> + }
> +
> + return be32_to_cpu(vmx);
> +}
> +
> +uint32_t kvmppc_get_dfp(void)
> +{
> + char buf[512];
> + uint32_t dfp;
> + FILE *f;
> + int len;
> +
> + if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
> + return 0;
> + }
> +
> + strncat(buf, "/ibm,dfp", sizeof(buf) - strlen(buf));
> +
> + f = fopen(buf, "rb");
> + if (!f) {
> + /* Assume -ENOENT, which indicates that DFP is not available */
> + return 0;
> + }
> +
> + len = fread(&dfp, sizeof(dfp), 1, f);
> + fclose(f);
> +
> + if (len != 1) {
> + /* Malformed ibm,dfp property, assume no DFP */
> + return 0;
> + }
> +
> + return be32_to_cpu(dfp);
> +}
Could you please fold those two into a single helper function to read out a
32-bit dt property and then just use that? :)
Please also document somewhere in the code what the return value means (0 =
unavailable, 1 = available).
Apart from that, nice patch!
Alex
