Re: [PATCH v6 02/18] s390x: protvirt: Add diag308 subcodes 8 - 10

[David Hildenbrand]

On 05.03.20 13:24, Janosch Frank wrote:
> On 3/5/20 1:04 PM, Janosch Frank wrote:
>> On 3/4/20 6:04 PM, David Hildenbrand wrote:
>>> On 04.03.20 12:42, Janosch Frank wrote:
>>>> For diag308 subcodes 8 - 10 we have a new ipib of type 5. The ipib
>>>> holds the address and length of the secure execution header, as well
>>>> as a list of guest components.
>>>>
>>>> Each component is a block of memory, for example kernel or initrd,
>>>> which needs to be decrypted by the Ultravisor in order to run a
>>>> protected VM. The secure execution header instructs the Ultravisor on
>>>> how to handle the protected VM and its components.
>>>>
>>>> Subcodes 8 and 9 are similiar to 5 and 6 and subcode 10 will finally
>>>> start the protected guest.
>>>>
>>>> Subcodes 8-10 are not valid in protected mode, we have to do a subcode
>>>> 3 and then the 8 and 10 combination for a protected reboot.
>>>>
>>>> Signed-off-by: Janosch Frank <address@hidden>
>>>> ---
>>>>  hw/s390x/ipl.c      | 47 ++++++++++++++++++++++++++++++++++++++++++---
>>>>  hw/s390x/ipl.h      | 32 ++++++++++++++++++++++++++++++
>>>>  target/s390x/diag.c | 26 ++++++++++++++++++++++---
>>>>  3 files changed, 99 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>>> index 9c1ecd423c..80c6ab233a 100644
>>>> --- a/hw/s390x/ipl.c
>>>> +++ b/hw/s390x/ipl.c
>>>> @@ -538,15 +538,55 @@ static bool is_virtio_scsi_device(IplParameterBlock 
>>>> *iplb)
>>>>      return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_SCSI);
>>>>  }
>>>>  
>>>> +int s390_ipl_pv_check_components(IplParameterBlock *iplb)
>>>
>>> What about making this
>>>
>>> bool s390_ipl_pv_valid(IplParameterBlock *iplb)
>>>
>>> and return true/false?
>>
>> We already have iplb_valid_pv() and ipl->iplb_valid_pv.
>> Do you have any other more expressive name we could use?
> 
> I think it makes more sense to rip out these tiny functions and
> consolidate them like this:
> 
> +static inline bool iplb_valid(IplParameterBlock *iplb)
>  {
> -    return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
> -           iplb->pbt == S390_IPL_TYPE_FCP;
> +    switch (iplb->pbt) {
> +        case S390_IPL_TYPE_FCP:
> +            return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN &&
> +                    iplb->pbt == S390_IPL_TYPE_FCP);
> +        case S390_IPL_TYPE_CCW:
> +            return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN &&
> +                    iplb->pbt == S390_IPL_TYPE_CCW);
> +        case S390_IPL_TYPE_PV:
> +            if(be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN ||
> +               iplb->pbt != S390_IPL_TYPE_PV) {
> +                return false;
> +            }
> +            return s390_ipl_pv_check_components(iplb);

yeah, and maybe even inline s390_ipl_pv_check_components().

-- 
Thanks,

David / dhildenb