Re: [PATCH 2/2] s390x: pv: Fix diag318 PV fencing

[David Hildenbrand]

On 21.10.20 16:18, Christian Borntraeger wrote:
> 
> 
> On 21.10.20 16:14, David Hildenbrand wrote:
>> On 21.10.20 15:43, Janosch Frank wrote:
>>> Diag318 fencing needs to be determined on the current VM PV state and
>>> not on the state that the VM has when we create the CPU model.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
>>> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>>> ---
>>>  hw/s390x/sclp.c    | 10 ++++++++++
>>>  target/s390x/kvm.c |  3 +--
>>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>> index 0cf2290826..69aba402d3 100644
>>> --- a/hw/s390x/sclp.c
>>> +++ b/hw/s390x/sclp.c
>>> @@ -22,6 +22,7 @@
>>>  #include "hw/s390x/event-facility.h"
>>>  #include "hw/s390x/s390-pci-bus.h"
>>>  #include "hw/s390x/ipl.h"
>>> +#include "hw/s390x/pv.h"
>>>  
>>>  static inline SCLPDevice *get_sclp_device(void)
>>>  {
>>> @@ -142,6 +143,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>>      if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
>>>          s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134,
>>>                              &read_info->fac134);
>>> +        /*
>>> +         * Diag318 is not available in protected mode and will result
>>> +         * in an operation exception. As we can dynamically move in
>>> +         * and out of protected mode, we need to fence the feature
>>> +         * here rather than when creating the CPU model.
>>> +         */
>>> +        if (s390_is_pv()) {
>>> +            read_info->fac134 &= ~0x80;
>>> +        }
>>
>> Hmm, I thought firmware would handle exposing cpu features and similar,
>> so we can't temper with it ....
> 
> Only the stfle bits. 
>>
>> Can we move that into s390_get_feat_block instead and check against the
>> feature bit instead?
> 
> No because we want to have this active for !pv and disabled for pv but we 
> switch
> this multiple times when doing boot/reboot. 

Keeping the s390_is_pv() condition of course.

-- 
Thanks,

David / dhildenb