[05/11] KVM: s390: Support Configuration z/Architecture Mode
diff mbox

Message ID d72ff520-8fac-34d9-e71a-53697a59957c@redhat.com
State New
Headers show

Commit Message

David Hildenbrand Aug. 28, 2017, 7:27 p.m. UTC
>  		rc = handle_sigp_dst(vcpu, order_code, cpu_addr,
> diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
> index 025ea20..181db5b 100644
> --- a/arch/s390/tools/gen_facilities.c
> +++ b/arch/s390/tools/gen_facilities.c
> @@ -80,6 +80,7 @@ static struct facility_def facility_defs[] = {
>  			78, /* enhanced-DAT 2 */
>  			130, /* instruction-execution-protection */
>  			131, /* enhanced-SOP 2 and side-effect */
> +			138, /* configuration z/architecture mode (czam) */
>  			146, /* msa extension 8 */
>  			-1  /* END */
>  		}
> 

Thinking about it, this should be the right thing to do instead of the
last hunk:

        if (MACHINE_HAS_TLB_GUEST) {

That would produce consistent results for very old QEMU.

Comments

Christian Borntraeger Aug. 28, 2017, 7:35 p.m. UTC | #1
On 08/28/2017 09:27 PM, David Hildenbrand wrote:
> 
>>  		rc = handle_sigp_dst(vcpu, order_code, cpu_addr,
>> diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
>> index 025ea20..181db5b 100644
>> --- a/arch/s390/tools/gen_facilities.c
>> +++ b/arch/s390/tools/gen_facilities.c
>> @@ -80,6 +80,7 @@ static struct facility_def facility_defs[] = {
>>  			78, /* enhanced-DAT 2 */
>>  			130, /* instruction-execution-protection */
>>  			131, /* enhanced-SOP 2 and side-effect */
>> +			138, /* configuration z/architecture mode (czam) */
>>  			146, /* msa extension 8 */
>>  			-1  /* END */
>>  		}
>>
> 
> Thinking about it, this should be the right thing to do instead of the
> last hunk:
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 84c069afc02f..39115f5a38df 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1927,6 +1927,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned
> long type)
>         memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask,
>                S390_ARCH_FAC_LIST_SIZE_BYTE);
> 
> +       /* we are always in CZAM mode - even on pre z14 machines */
> +       set_kvm_facility(kvm->arch.model.fac_mask, 138);
> +       set_kvm_facility(kvm->arch.model.fac_list, 138);
> +       /* we emulate STHYI in kvm */
>         set_kvm_facility(kvm->arch.model.fac_mask, 74);
>         set_kvm_facility(kvm->arch.model.fac_list, 74);
>         if (MACHINE_HAS_TLB_GUEST) {
> 
> That would produce consistent results for very old QEMU.
> 

This should be identical as the initial fac_list is populated from the fac_mask,
which is populated by the facility_defs structure.

[..]
       /* Populate the facility list initially. */
        kvm->arch.model.fac_list = kvm->arch.sie_page2->fac_list;
---->   memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask,
               S390_ARCH_FAC_LIST_SIZE_BYTE);

[,..]
Christian Borntraeger Aug. 28, 2017, 7:38 p.m. UTC | #2
On 08/28/2017 09:35 PM, Christian Borntraeger wrote:
> 
> 
> On 08/28/2017 09:27 PM, David Hildenbrand wrote:
>>
>>>  		rc = handle_sigp_dst(vcpu, order_code, cpu_addr,
>>> diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
>>> index 025ea20..181db5b 100644
>>> --- a/arch/s390/tools/gen_facilities.c
>>> +++ b/arch/s390/tools/gen_facilities.c
>>> @@ -80,6 +80,7 @@ static struct facility_def facility_defs[] = {
>>>  			78, /* enhanced-DAT 2 */
>>>  			130, /* instruction-execution-protection */
>>>  			131, /* enhanced-SOP 2 and side-effect */
>>> +			138, /* configuration z/architecture mode (czam) */
>>>  			146, /* msa extension 8 */
>>>  			-1  /* END */
>>>  		}
>>>
>>
>> Thinking about it, this should be the right thing to do instead of the
>> last hunk:
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 84c069afc02f..39115f5a38df 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -1927,6 +1927,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned
>> long type)
>>         memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask,
>>                S390_ARCH_FAC_LIST_SIZE_BYTE);
>>
>> +       /* we are always in CZAM mode - even on pre z14 machines */
>> +       set_kvm_facility(kvm->arch.model.fac_mask, 138);
>> +       set_kvm_facility(kvm->arch.model.fac_list, 138);
>> +       /* we emulate STHYI in kvm */
>>         set_kvm_facility(kvm->arch.model.fac_mask, 74);
>>         set_kvm_facility(kvm->arch.model.fac_list, 74);
>>         if (MACHINE_HAS_TLB_GUEST) {
>>
>> That would produce consistent results for very old QEMU.
>>
> 
> This should be identical as the initial fac_list is populated from the fac_mask,
> which is populated by the facility_defs structure.
> 
> [..]
>        /* Populate the facility list initially. */
>         kvm->arch.model.fac_list = kvm->arch.sie_page2->fac_list;
> ---->   memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask,
>                S390_ARCH_FAC_LIST_SIZE_BYTE);
> 
> [,..]

Well not identical. The patch actually enables the czam facility only if the host
hardware while your variant enables it always. Interesting discussion if it matters
if we should enable it on z13, z12 and earlier or not
David Hildenbrand Aug. 28, 2017, 7:41 p.m. UTC | #3
On 28.08.2017 21:35, Christian Borntraeger wrote:
> 
> 
> On 08/28/2017 09:27 PM, David Hildenbrand wrote:
>>
>>>  		rc = handle_sigp_dst(vcpu, order_code, cpu_addr,
>>> diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
>>> index 025ea20..181db5b 100644
>>> --- a/arch/s390/tools/gen_facilities.c
>>> +++ b/arch/s390/tools/gen_facilities.c
>>> @@ -80,6 +80,7 @@ static struct facility_def facility_defs[] = {
>>>  			78, /* enhanced-DAT 2 */
>>>  			130, /* instruction-execution-protection */
>>>  			131, /* enhanced-SOP 2 and side-effect */
>>> +			138, /* configuration z/architecture mode (czam) */
>>>  			146, /* msa extension 8 */
>>>  			-1  /* END */
>>>  		}
>>>
>>
>> Thinking about it, this should be the right thing to do instead of the
>> last hunk:
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 84c069afc02f..39115f5a38df 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -1927,6 +1927,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned
>> long type)
>>         memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask,
>>                S390_ARCH_FAC_LIST_SIZE_BYTE);
>>
>> +       /* we are always in CZAM mode - even on pre z14 machines */
>> +       set_kvm_facility(kvm->arch.model.fac_mask, 138);
>> +       set_kvm_facility(kvm->arch.model.fac_list, 138);
>> +       /* we emulate STHYI in kvm */
>>         set_kvm_facility(kvm->arch.model.fac_mask, 74);
>>         set_kvm_facility(kvm->arch.model.fac_list, 74);
>>         if (MACHINE_HAS_TLB_GUEST) {
>>
>> That would produce consistent results for very old QEMU.
>>
> 
> This should be identical as the initial fac_list is populated from the fac_mask,
> which is populated by the facility_defs structure.
> 
> [..]
>        /* Populate the facility list initially. */
>         kvm->arch.model.fac_list = kvm->arch.sie_page2->fac_list;
> ---->   memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask,
>                S390_ARCH_FAC_LIST_SIZE_BYTE);
> 
> [,..]
> 

This would keep it unset on e.g. a z13, no? (that's why I added the
comment regarding z14)


By always enabling it we would behave exactly like QEMU.
David Hildenbrand Aug. 28, 2017, 7:42 p.m. UTC | #4
On 28.08.2017 21:38, Christian Borntraeger wrote:
> 
> 
> On 08/28/2017 09:35 PM, Christian Borntraeger wrote:
>>
>>
>> On 08/28/2017 09:27 PM, David Hildenbrand wrote:
>>>
>>>>  		rc = handle_sigp_dst(vcpu, order_code, cpu_addr,
>>>> diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
>>>> index 025ea20..181db5b 100644
>>>> --- a/arch/s390/tools/gen_facilities.c
>>>> +++ b/arch/s390/tools/gen_facilities.c
>>>> @@ -80,6 +80,7 @@ static struct facility_def facility_defs[] = {
>>>>  			78, /* enhanced-DAT 2 */
>>>>  			130, /* instruction-execution-protection */
>>>>  			131, /* enhanced-SOP 2 and side-effect */
>>>> +			138, /* configuration z/architecture mode (czam) */
>>>>  			146, /* msa extension 8 */
>>>>  			-1  /* END */
>>>>  		}
>>>>
>>>
>>> Thinking about it, this should be the right thing to do instead of the
>>> last hunk:
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index 84c069afc02f..39115f5a38df 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -1927,6 +1927,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned
>>> long type)
>>>         memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask,
>>>                S390_ARCH_FAC_LIST_SIZE_BYTE);
>>>
>>> +       /* we are always in CZAM mode - even on pre z14 machines */
>>> +       set_kvm_facility(kvm->arch.model.fac_mask, 138);
>>> +       set_kvm_facility(kvm->arch.model.fac_list, 138);
>>> +       /* we emulate STHYI in kvm */
>>>         set_kvm_facility(kvm->arch.model.fac_mask, 74);
>>>         set_kvm_facility(kvm->arch.model.fac_list, 74);
>>>         if (MACHINE_HAS_TLB_GUEST) {
>>>
>>> That would produce consistent results for very old QEMU.
>>>
>>
>> This should be identical as the initial fac_list is populated from the fac_mask,
>> which is populated by the facility_defs structure.
>>
>> [..]
>>        /* Populate the facility list initially. */
>>         kvm->arch.model.fac_list = kvm->arch.sie_page2->fac_list;
>> ---->   memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask,
>>                S390_ARCH_FAC_LIST_SIZE_BYTE);
>>
>> [,..]
> 
> Well not identical. The patch actually enables the czam facility only if the host
> hardware while your variant enables it always. Interesting discussion if it matters
> if we should enable it on z13, z12 and earlier or not
> 

This is also what we do on the QEMU side and what I meant by "consistent".
Christian Borntraeger Aug. 29, 2017, 7:18 a.m. UTC | #5
On 08/28/2017 09:42 PM, David Hildenbrand wrote:
> On 28.08.2017 21:38, Christian Borntraeger wrote:
>>
>>
>> On 08/28/2017 09:35 PM, Christian Borntraeger wrote:
>>>
>>>
>>> On 08/28/2017 09:27 PM, David Hildenbrand wrote:
>>>>
>>>>>  		rc = handle_sigp_dst(vcpu, order_code, cpu_addr,
>>>>> diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
>>>>> index 025ea20..181db5b 100644
>>>>> --- a/arch/s390/tools/gen_facilities.c
>>>>> +++ b/arch/s390/tools/gen_facilities.c
>>>>> @@ -80,6 +80,7 @@ static struct facility_def facility_defs[] = {
>>>>>  			78, /* enhanced-DAT 2 */
>>>>>  			130, /* instruction-execution-protection */
>>>>>  			131, /* enhanced-SOP 2 and side-effect */
>>>>> +			138, /* configuration z/architecture mode (czam) */
>>>>>  			146, /* msa extension 8 */
>>>>>  			-1  /* END */
>>>>>  		}
>>>>>
>>>>
>>>> Thinking about it, this should be the right thing to do instead of the
>>>> last hunk:
>>>>
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index 84c069afc02f..39115f5a38df 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -1927,6 +1927,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned
>>>> long type)
>>>>         memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask,
>>>>                S390_ARCH_FAC_LIST_SIZE_BYTE);
>>>>
>>>> +       /* we are always in CZAM mode - even on pre z14 machines */
>>>> +       set_kvm_facility(kvm->arch.model.fac_mask, 138);
>>>> +       set_kvm_facility(kvm->arch.model.fac_list, 138);
>>>> +       /* we emulate STHYI in kvm */
>>>>         set_kvm_facility(kvm->arch.model.fac_mask, 74);
>>>>         set_kvm_facility(kvm->arch.model.fac_list, 74);
>>>>         if (MACHINE_HAS_TLB_GUEST) {
>>>>
>>>> That would produce consistent results for very old QEMU.
>>>>
>>>
>>> This should be identical as the initial fac_list is populated from the fac_mask,
>>> which is populated by the facility_defs structure.
>>>
>>> [..]
>>>        /* Populate the facility list initially. */
>>>         kvm->arch.model.fac_list = kvm->arch.sie_page2->fac_list;
>>> ---->   memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask,
>>>                S390_ARCH_FAC_LIST_SIZE_BYTE);
>>>
>>> [,..]
>>
>> Well not identical. The patch actually enables the czam facility only if the host
>> hardware while your variant enables it always. Interesting discussion if it matters
>> if we should enable it on z13, z12 and earlier or not
>>
> 
> This is also what we do on the QEMU side and what I meant by "consistent".

Yes, I agree. QEMU will always enable stfle.138. So should the kernel.
Shall I fixup the patch with your change proposal or do you want to send an on-top patch?
David Hildenbrand Aug. 29, 2017, 12:08 p.m. UTC | #6
>> This is also what we do on the QEMU side and what I meant by "consistent".
> 
> Yes, I agree. QEMU will always enable stfle.138. So should the kernel.
> Shall I fixup the patch with your change proposal or do you want to send an on-top patch?
> 

Whatever you prefer!
Christian Borntraeger Aug. 29, 2017, 12:21 p.m. UTC | #7
On 08/29/2017 02:08 PM, David Hildenbrand wrote:
> 
>>> This is also what we do on the QEMU side and what I meant by "consistent".
>>
>> Yes, I agree. QEMU will always enable stfle.138. So should the kernel.
>> Shall I fixup the patch with your change proposal or do you want to send an on-top patch?
>>
> 
> Whatever you prefer!

Can you send a fixup patch on top?
David Hildenbrand Aug. 29, 2017, 12:24 p.m. UTC | #8
On 29.08.2017 14:21, Christian Borntraeger wrote:
> 
> 
> On 08/29/2017 02:08 PM, David Hildenbrand wrote:
>>
>>>> This is also what we do on the QEMU side and what I meant by "consistent".
>>>
>>> Yes, I agree. QEMU will always enable stfle.138. So should the kernel.
>>> Shall I fixup the patch with your change proposal or do you want to send an on-top patch?
>>>
>>
>> Whatever you prefer!
> 
> Can you send a fixup patch on top?
> 

Will do!

Patch
diff mbox

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 84c069afc02f..39115f5a38df 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1927,6 +1927,10 @@  int kvm_arch_init_vm(struct kvm *kvm, unsigned
long type)
        memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask,
               S390_ARCH_FAC_LIST_SIZE_BYTE);

+       /* we are always in CZAM mode - even on pre z14 machines */
+       set_kvm_facility(kvm->arch.model.fac_mask, 138);
+       set_kvm_facility(kvm->arch.model.fac_list, 138);
+       /* we emulate STHYI in kvm */
        set_kvm_facility(kvm->arch.model.fac_mask, 74);
        set_kvm_facility(kvm->arch.model.fac_list, 74);