[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Me
From: |
Alexey Kardashevskiy |
Subject: |
Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability |
Date: |
Tue, 12 Dec 2017 17:28:33 +1100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
On 12/12/17 16:54, David Gibson wrote:
> On Tue, Dec 12, 2017 at 03:06:33PM +1100, Alexey Kardashevskiy wrote:
>> On 12/12/17 14:40, David Gibson wrote:
>>> On Tue, Dec 12, 2017 at 02:28:33PM +1100, Alexey Kardashevskiy wrote:
>>>> On 12/12/17 11:20, David Gibson wrote:
>>>>> On Mon, Dec 11, 2017 at 08:20:30PM +1100, Alexey Kardashevskiy wrote:
>>>>>> On 11/12/17 18:08, David Gibson wrote:
>>>>>>> This adds an spapr capability bit for Hardware Transactional Memory. By
>>>>>>> default it is enabled for all machine type versions with POWER8 and
>>>>>>> later
>>>>>>> CPUs.
>>>>>>>
>>>>>>> This means that "qemu -machine pseries,accel=tcg =cpu POWER8" will now
>>>>>>> fail to start, however that configuration was already broken in that it
>>>>>>> would provide a nonstandard environment to the guest, which could break
>>>>>>> migration.
>>>>>>>
>>>>>>> Signed-off-by: David Gibson <address@hidden>
>>>>>>>
>>>>>>> # Conflicts:
>>>>>>> # hw/ppc/spapr_caps.c
>>>>>>> ---
>>>>>>> hw/ppc/spapr.c | 13 +++++++------
>>>>>>> hw/ppc/spapr_caps.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>>>>>> include/hw/ppc/spapr.h | 3 +++
>>>>>>> 3 files changed, 46 insertions(+), 11 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>>> index a921feeb03..0487d67894 100644
>>>>>>> --- a/hw/ppc/spapr.c
>>>>>>> +++ b/hw/ppc/spapr.c
>>>>>>> @@ -253,7 +253,8 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int
>>>>>>> offset, PowerPCCPU *cpu)
>>>>>>> }
>>>>>>>
>>>>>>> /* Populate the "ibm,pa-features" property */
>>>>>>> -static void spapr_populate_pa_features(PowerPCCPU *cpu, void *fdt, int
>>>>>>> offset,
>>>>>>> +static void spapr_populate_pa_features(sPAPRMachineState *spapr,
>>>>>>> PowerPCCPU *cpu,
>>>>>>> + void *fdt, int offset,
>>>>>>> bool legacy_guest)
>>>>>>> {
>>>>>>> CPUPPCState *env = &cpu->env;
>>>>>>> @@ -318,7 +319,7 @@ static void spapr_populate_pa_features(PowerPCCPU
>>>>>>> *cpu, void *fdt, int offset,
>>>>>>> */
>>>>>>> pa_features[3] |= 0x20;
>>>>>>> }
>>>>>>> - if (kvmppc_has_cap_htm() && pa_size > 24) {
>>>>>>> + if (spapr_has_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {
>>>>>>> pa_features[24] |= 0x80; /* Transactional memory support */
>>>>>>> }
>>>>>>> if (legacy_guest && pa_size > 40) {
>>>>>>> @@ -384,8 +385,8 @@ static int spapr_fixup_cpu_dt(void *fdt,
>>>>>>> sPAPRMachineState *spapr)
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> - spapr_populate_pa_features(cpu, fdt, offset,
>>>>>>> -
>>>>>>> spapr->cas_legacy_guest_workaround);
>>>>>>> + spapr_populate_pa_features(spapr, cpu, fdt, offset,
>>>>>>> + spapr->cas_legacy_guest_workaround);
>>>>>>> }
>>>>>>> return ret;
>>>>>>> }
>>>>>>> @@ -579,7 +580,7 @@ static void spapr_populate_cpu_dt(CPUState *cs,
>>>>>>> void *fdt, int offset,
>>>>>>> page_sizes_prop, page_sizes_prop_size)));
>>>>>>> }
>>>>>>>
>>>>>>> - spapr_populate_pa_features(cpu, fdt, offset, false);
>>>>>>> + spapr_populate_pa_features(spapr, cpu, fdt, offset, false);
>>>>>>>
>>>>>>> _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
>>>>>>> cs->cpu_index / vcpus_per_socket)));
>>>>>>> @@ -3823,7 +3824,7 @@ static void spapr_machine_class_init(ObjectClass
>>>>>>> *oc, void *data)
>>>>>>> */
>>>>>>> mc->numa_mem_align_shift = 28;
>>>>>>>
>>>>>>> - smc->default_caps = spapr_caps(0);
>>>>>>> + smc->default_caps = spapr_caps(SPAPR_CAP_HTM);
>>>>>>> spapr_capability_properties(smc, &error_abort);
>>>>>>> }
>>>>>>>
>>>>>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>>>>>>> index f1721cc68f..5afb4d9d5f 100644
>>>>>>> --- a/hw/ppc/spapr_caps.c
>>>>>>> +++ b/hw/ppc/spapr_caps.c
>>>>>>> @@ -24,6 +24,10 @@
>>>>>>> #include "qemu/osdep.h"
>>>>>>> #include "qapi/error.h"
>>>>>>> #include "qapi/visitor.h"
>>>>>>> +#include "sysemu/hw_accel.h"
>>>>>>> +#include "target/ppc/cpu.h"
>>>>>>> +#include "cpu-models.h"
>>>>>>> +#include "kvm_ppc.h"
>>>>>>>
>>>>>>> #include "hw/ppc/spapr.h"
>>>>>>>
>>>>>>> @@ -34,30 +38,57 @@ typedef struct sPAPRCapabilityInfo {
>>>>>>> } sPAPRCapabilityInfo;
>>>>>>>
>>>>>>> static sPAPRCapabilityInfo capability_table[] = {
>>>>>>> + {
>>>>>>> + .name = "htm",
>>>>>>> + .description = "Allow Hardware Transactional Memory (HTM)",
>>>>>>> + .bit = SPAPR_CAP_HTM,
>>>>>>> + },
>>>>>>> };
>>>>>>>
>>>>>>> static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState
>>>>>>> *spapr, CPUState *cs)
>>>>>>> {
>>>>>>> sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>>>>>>> + PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
>>>>>>> sPAPRCapabilities caps;
>>>>>>>
>>>>>>> caps = smc->default_caps;
>>>>>>>
>>>>>>> - /* TODO: clamp according to cpu model */
>>>>>>> + if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_07,
>>>>>>> + 0, spapr->max_compat_pvr)) {
>>>>>>> + caps.mask &= ~SPAPR_CAP_HTM;
>>>>>>> + }
>>>>>>>
>>>>>>> return caps;
>>>>>>> }
>>>>>>>
>>>>>>> static void spapr_allow_caps(sPAPRMachineState *spapr, Error **errp)
>>>>>>> {
>>>>>>> - /* TODO: make sure all requested caps are allowed with the current
>>>>>>> - * accelerator, cpu etc. */
>>>>>>> + Error *err = NULL;
>>>>>>> +
>>>>>>> + if (spapr_has_cap(spapr, SPAPR_CAP_HTM)) {
>>>>>>> + if (tcg_enabled()) {
>>>>>>> + error_setg(&err,
>>>>>>> +"No Transactional Memory support in TCG, try cap-htm=off"
>>>>>>> + );
>>>>>>> + goto out;
>>>>>>> + } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
>>>>>>> + error_setg(&err,
>>>>>>> +"KVM implementation does not support Transactional Memory, try
>>>>>>> cap-htm=off"
>>>>>>> + );
>>>>>>> + }
>>>>>>> + }
>>>>>>> +
>>>>>>> +out:
>>>>>>> + if (err) {
>>>>>>> + error_propagate(errp, err);
>>>>>>> + }
>>>>>>> }
>>>>>>>
>>>>>>> static void spapr_enforce_caps(sPAPRMachineState *spapr, Error **errp)
>>>>>>> {
>>>>>>> - /* TODO: to the extent possible, prevent the guest from accessing
>>>>>>> - * features controlled by disabled caps */
>>>>>>> + if (!spapr_has_cap(spapr, SPAPR_CAP_HTM)) {
>>>>>>> + /* TODO: Tell KVM not to allow HTM instructions */
>>>>>>
>>>>>>
>>>>>> Are the capabilities expected to be enabled by default in the KVM? There
>>>>>> is
>>>>>> no such "TODO" in spapr_allow_caps while it should probably be.
>>>>>
>>>>> In short, yes. At present this is a "read only" cap in KVM. It is
>>>>> always enabled when the host can support HTM and can't be changed. We
>>>>> could - and probably should - add a way to disable the capability, but
>>>>> we couldn't change it to being off by default without breaking
>>>>> compatibility.
>>>>>
>>>>>> I would rather implement spapr_get_supported_caps() which would tell full
>>>>>> list of what the accelerator can do for the spapr machine; and
>>>>>> spapr_set_caps() which would compare supported caps what we got from
>>>>>> defaults and the command line and then enable requested or disable
>>>>>> forbidden caps, explicitly.
>>>>>
>>>>> Hrm. I don't really see how this helps. We'd still need cap-by-cap
>>>>> logic to actually implement the enabling and/or disabling: the
>>>>> mechanism need not be the same for all caps. And in fact we'd still
>>>>> need the cap by cap logic to implement spapr_get_supported_caps().
>>>>
>>>>
>>>> I'd think AccelClass is the place to implement
>>>> is_cap_supported()/enable_cap()/disable_cap(), and spapr would not have to
>>>> go via these kvm_enabled()/tcg_enabled() but it is probably out of scope.
>>>
>>> Not really. The 3 caps so far are related to the cpu and/or
>>> accelerator, but even there they only really make sense in a context
>>> where the hypervisor can block access to features. They wouldn't
>>> really make sense for powernv (or, say, ppce500) where the guest OS
>>> controls the HFSCR or equivalent ways of controlling feature access.
>>
>> ok, fair enough.
>>
>>
>>>>> If
>>>>> we have to do that, we might as well check for failures at the same
>>>>> time as doing the actual enabling and disabling,
>>>>>
>>>>>> Right now both spapr_allow_caps() and
>>>>>> spapr_enforce_caps() both have to check for spapr_has_cap() and (at least
>>>>>> what TODO suggests) for kvm_enabled() and do something to KVM in
>>>>>> this regard.
>>>>>
>>>>> Why is that bad?
>>>>
>>>>
>>>> If let's say we have a capability which is off by default but I want to
>>>> enable it anyway, then should both _allow() and _enforce() try to enable
>>>> it?
>>>
>>> No, if the feature is on, _allow() makes sure it can be implemented
>>> with the cpu/accel, _enforce() does nothing.
>>>
>>> If the feature os off, _allow() does nothing, _enforce makes a best
>>> effort to prevent it from working.
>>
>> Ah. _allow() can only enable, _enforce() can only disable. ok.
>>
>> This is what I meant elsewhere - the "enforce" word sounds to me (one bad
>> engligh speaker :) ) more like "enable" (forcefully) than "disable".
>> "forbid" seems better but I do not insist.
>
> Right, but I wanted to avoid "forbid" because I was already using it
> for forbidden_caps which has different semantics. forbidden_caps is
> those caps the _user_ says shouldn't be there
Sounds like a plain "disabled" to me :)
> and are therefore
> excluded from the "effective caps" regardless of other defaults.
> "enforce" is about taking the effective caps and matching them to
> actual hardware capabilities. i.e. they're operating at different
> levels. Maybe this makes it clearer:
>
> USER QEMU STATE HARDWARE
>
> caps_reset() allow/enforce
> forced_caps -----------> effective_caps ------------> HFSCR
> forbidden_caps ^ ^ KVM caps
> | | ...
> (defaults) ---/ |
> (hw abilities) ---/
>
>>>> It just looks nicer to have a single place which enables or disables a
>>>> capability. Dunno...
>>>
>>> I could put both the allowing and enforcing in one function, say
>>> spapr_caps_apply(). But then I'd need an alternative mechanism for
>>> reporting fatal vs. non-fatal errors. Maybe worth it, I'll have a
>>> look.
>>
>> And now it is assumed that all failures in _allow() are non-fatal?
>
> No, failures in _allow() are always fatal. Failures in _enforce() aren't.
Ah. Ok. So if we cannot disable TM in KVM, we print a message and continue?
And we only fail if the user specifically asked for something and we could
not enable it? What about preselecting 2.06-compat - should not _enforce()
fail fatally if it fails to disable TM?
--
Alexey
signature.asc
Description: OpenPGP digital signature
- [Qemu-ppc] [RFC for-2.12 0/8] spapr: Add optional capabilities, David Gibson, 2017/12/11
- [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, David Gibson, 2017/12/11
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, Alexey Kardashevskiy, 2017/12/11
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, David Gibson, 2017/12/11
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, Alexey Kardashevskiy, 2017/12/11
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, David Gibson, 2017/12/11
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, Alexey Kardashevskiy, 2017/12/11
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, David Gibson, 2017/12/12
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability,
Alexey Kardashevskiy <=
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, David Gibson, 2017/12/12
Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, Daniel Henrique Barboza, 2017/12/11
Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, Michael Roth, 2017/12/11
Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, Suraj Jitindar Singh, 2017/12/11
[Qemu-ppc] [RFC for-2.12 2/8] spapr: Capabilities infrastructure, David Gibson, 2017/12/11