Re: [PATCH v4 02/16] s390x: protvirt: Add diag308 subcodes 8 - 10

[Janosch Frank]

On 2/21/20 10:49 AM, Cornelia Huck wrote:
> On Thu, 20 Feb 2020 07:56:24 -0500
> Janosch Frank <address@hidden> 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      | 48 ++++++++++++++++++++++++++++++++++++++++++---
>>  hw/s390x/ipl.h      | 31 +++++++++++++++++++++++++++++
>>  target/s390x/diag.c | 27 ++++++++++++++++++++++---
>>  3 files changed, 100 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 7773499d7f..e92d989813 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -538,15 +538,56 @@ 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)
>> +{
>> +    int i;
>> +    IPLBlockPV *ipib_pv = &iplb->pv;
>> +
>> +    if (ipib_pv->num_comp == 0) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    for (i = 0; i < ipib_pv->num_comp; i++) {
>> +
>> +        /* Addr must be 4k aligned */
>> +        if (ipib_pv->components[i].addr & ~TARGET_PAGE_MASK) {
>> +            return -EINVAL;
>> +        }
>> +
>> +        /* Tweak prefix is monotonously increasing with each component */
>> +        if (i < ipib_pv->num_comp - 1 &&
>> +            ipib_pv->components[i].tweak_pref >
>> +            ipib_pv->components[i + 1].tweak_pref) {
>> +            return -EINVAL;
>> +        }
>> +    }
>> +    return 1;
> 
> Any reason why you return 1 here? 0 vs negative error is the more usual
> pattern.

I honestly have no idea, will fix :)

> 
>> +}
>> +
> 
> (...)
> 
>> @@ -117,7 +123,8 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, 
>> uint64_t r3, uintptr_t ra)
>>  
>>          cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>>  
>> -        if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb)) {
>> +        if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb) &&
>> +            !(iplb_valid_pv(iplb) && s390_ipl_pv_check_components(iplb) >= 
>> 0)) {
> 
> !s390_ipl_pv_check_components() would also read nicer IMHO :)
> 
>>              env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>>              goto out;
>>          }
> 
> Otherwise, looks good to me.
> 
>

signature.asc
Description: OpenPGP digital signature