[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] s390: use sync regs for register transfer
From: |
Christian Borntraeger |
Subject: |
Re: [Qemu-devel] [PATCH] s390: use sync regs for register transfer |
Date: |
Thu, 20 Sep 2012 17:21:50 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20120827 Thunderbird/15.0 |
On 20/09/12 16:13, Alexander Graf wrote:
>
>
> On 20.09.2012, at 14:49, Christian Borntraeger <address@hidden> wrote:
>
>> [...]
>>>> --- a/target-s390x/kvm.c
>>>> +++ b/target-s390x/kvm.c
>>>> @@ -88,50 +88,77 @@ void kvm_arch_reset_vcpu(CPUS390XState *env)
>>>> /* FIXME: add code to reset vcpu. */
>>>> }
>>>>
>>>> +/* we want to have the prefix, the GPRS, the ACRS and the CRS up to date
>>>> */
>>>> +#define QEMU_NEEDED_REGS (KVM_SYNC_PREFIX | KVM_SYNC_GPRS | \
>>>> + KVM_SYNC_ACRS | KVM_SYNC_CRS)
>>>> +
>>>> +/* But qemu only changes the GPRS */
>>>> +#define QEMU_DIRTY_REGS (KVM_SYNC_GPRS)
>>>> +
>>>> int kvm_arch_put_registers(CPUS390XState *env, int level)
>>>> {
>>>> struct kvm_regs regs;
>>>> int ret;
>>>> int i;
>>>>
>>>> - ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, ®s);
>>>> - if (ret < 0) {
>>>> - return ret;
>>>> - }
>>>> -
>>>> - for (i = 0; i < 16; i++) {
>>>> - regs.gprs[i] = env->regs[i];
>>>> - }
>>>> -
>>>> - ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, ®s);
>>>> - if (ret < 0) {
>>>> - return ret;
>>>> - }
>>>> -
>>>> env->kvm_run->psw_addr = env->psw.addr;
>>>> env->kvm_run->psw_mask = env->psw.mask;
>>>>
>>>> - return ret;
>>>> + if ((env->kvm_run->kvm_valid_regs & QEMU_NEEDED_REGS) ==
>>>> QEMU_NEEDED_REGS) {
>>>
>>> Isn't this also missing a check for KVM_CAP_SYNC_REGS?
>>
>> I ommitted the check for two reasons:
>> - since kvm_check_extension is an ioctl by itself, this would counter the
>> original idea of sync regs
>> - the kvm_run structure contains zeroes at the sync reg location in kernels
>> that dont support that,
>> (the area was unused and the page was allocated with GFP_ZERO)
>> So by having any of the kvm_valid_regs != 0 we know that SYNC REGS must be
>> supported
>
> But the spec explicitly says that the fields are only valid when the CAP is
> present. So if you think that is unnecessary, please patch the spec.
Maybe I should do both. Update the spec that the CAP defines if this is any
useful, if not it will start out with zeroes,but also having a single CAP check
during startup to have the code cleaner.
> The way we get around constant ioctls for cap checks is that we usually check
> only once and remember the result in a global variable. Please check other
> arch's kvm.c files for reference.
I looked for that in i386, but seems that only ppc does it. So I will look more
into ppc....
>
>>
>>
>>> Also, on reset we probably also want to write the other registers back,
>>> right?
>>
>> Well, on reset we call the initial cpu reset ioctl, which does reset the
>> other registers mandated
>> by a cpu reset. Furthermore, it is not different to the current
>> implementation....
>
> The difference is that get_registers fetches more registers, so not putting
> them looks awkward.
Yep, obviously the commenting was not enough to explain the why.
>
>>
>> What we might want to do is to also consider a clear reset (which also
>> zeroes the FPRs and ARs as
>> well as memory etc) as an additional option. But this should not be the
>> default, e.g. to make
>> kdump or standalone dump possible.
>> I think this would then be an addon-patch by itself.
>
> What does reset on normal systems look like? Does it clear register state?
A reset does not, a reset clear does.
But you just got me convinced, that we should really follow the level statement
for the put function and simply write everything for KVM_PUT_FULL_STATE and
KVM_PUT_RESET_STATE. The KVM_EXIT_S390_RESET handler must then do the right
thing (e.g. making sure that the general purpose register content for a
non-clear reset is not changed). Will look if that works out.
Christian