On 04/02/2020 14.08, Claudio Imbrenda wrote:
> On Tue, 4 Feb 2020 13:48:47 +0100
> Thomas Huth <thuth@xxxxxxxxxx> wrote:
>
>> On 04/02/2020 12.41, Claudio Imbrenda wrote:
>>> On Tue, 4 Feb 2020 11:37:42 +0100
>>> Thomas Huth <thuth@xxxxxxxxxx> wrote:
>>>
>>> [...]
>>>
>>>>> ---
>>>>> arch/s390/kernel/pgm_check.S | 4 +-
>>>>> arch/s390/mm/fault.c | 87
>>>>> ++++++++++++++++++++++++++++++++++++ 2 files changed, 89
>>>>> insertions(+), 2 deletions(-)
>>>> [...]
>>>>> +void do_non_secure_storage_access(struct pt_regs *regs)
>>>>> +{
>>>>> + unsigned long gaddr = regs->int_parm_long &
>>>>> __FAIL_ADDR_MASK;
>>>>> + struct gmap *gmap = (struct gmap *)S390_lowcore.gmap;
>>>>> + struct uv_cb_cts uvcb = {
>>>>> + .header.cmd = UVC_CMD_CONV_TO_SEC_STOR,
>>>>> + .header.len = sizeof(uvcb),
>>>>> + .guest_handle = gmap->se_handle,
>>>>> + .gaddr = gaddr,
>>>>> + };
>>>>> + int rc;
>>>>> +
>>>>> + if (get_fault_type(regs) != GMAP_FAULT) {
>>>>> + do_fault_error(regs, VM_READ | VM_WRITE,
>>>>> VM_FAULT_BADMAP);
>>>>> + WARN_ON_ONCE(1);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + rc = uv_make_secure(gmap, gaddr, &uvcb, 0);
>>>>> + if (rc == -EINVAL && uvcb.header.rc != 0x104)
>>>>> + send_sig(SIGSEGV, current, 0);
>>>>> +}
>>>>
>>>> What about the other rc beside 0x104 that could happen here? They
>>>> go unnoticed?
>>>
>>> no, they are handled in the uv_make_secure, and return an
>>> appropriate error code.
>> Hmm, in patch 05/37, I basically see:
>>
>> +static int make_secure_pte(pte_t *ptep, unsigned long addr, void
>> *data) +{
>> [...]
>> + rc = uv_call(0, (u64)params->uvcb);
>> + page_ref_unfreeze(page, expected);
>> + if (rc)
>> + rc = (params->uvcb->rc == 0x10a) ? -ENXIO : -EINVAL;
>> + return rc;
>> +}
>>
>> +int uv_make_secure(struct gmap *gmap, unsigned long gaddr, void
>> *uvcb, int pins)
>> +{
>> [...]
>> + lock_page(params.page);
>> + rc = apply_to_page_range(gmap->mm, uaddr, PAGE_SIZE,
>> make_secure_pte, ¶ms);
>> + unlock_page(params.page);
>> +out:
>> + up_read(&gmap->mm->mmap_sem);
>> +
>> + if (rc == -EBUSY) {
>> + if (local_drain) {
>> + lru_add_drain_all();
>> + return -EAGAIN;
>> + }
>> + lru_add_drain();
>> + local_drain = 1;
>> + goto again;
>> + } else if (rc == -ENXIO) {
>> + if (gmap_fault(gmap, gaddr, FAULT_FLAG_WRITE))
>> + return -EFAULT;
>> + return -EAGAIN;
>> + }
>> + return rc;
>> +}
>>
>> So 0x10a result in -ENXIO and is handled ==> OK.
>> And 0x104 is handled in do_non_secure_storage_access ==> OK.
>>
>> But what about the other possible error codes? make_secure_pte()
>> returns -EINVAL in that case, but uv_make_secure() does not care
>> about that error code, and do_non_secure_storage_access() only cares
>> if uvcb.header.rc was 0x104 ... what did I miss?
>
> basically, any error value that is not handled by uv_make_secure is
> passed as-is from make_secure_pte directly to the caller of
> uv_make_secure .
> any RC value that is not explicitly handled here will
> result in -EINVAL. The caller has then to check for -EINVAL and check
> the RC value, like do_non_secure_storage_access does.
>
> so anything else that goes wrong is passed as-is to the caller.
> for some things, like interrupt handlers, we simply don't care; if we
> need to try again, we will try again, if we notice we can't continue,
> the VM will get killed.
In that case a comment in do_non_secure_storage_access() would be
helpful which states why we do not care about the other error codes in
uvcb.header.rc.
Thomas
