On 11/13/19 11:28 AM, Thomas Huth wrote:
> On 24/10/2019 13.40, Janosch Frank wrote:
>> Let's add a KVM interface to create and destroy protected VMs.
>>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>> ---
> [...]
>> +int kvm_s390_pv_unpack(struct kvm *kvm, unsigned long addr, unsigned long size,
>> + unsigned long tweak)
>> +{
>> + int i, rc = 0;
>> + struct uv_cb_unp uvcb = {
>> + .header.cmd = UVC_CMD_UNPACK_IMG,
>> + .header.len = sizeof(uvcb),
>> + .guest_handle = kvm_s390_pv_handle(kvm),
>> + .tweak[0] = tweak
>> + };
>> +
>> + if (addr & ~PAGE_MASK || size & ~PAGE_MASK)
>> + return -EINVAL;
>
> Also check for size == 0 ?
Yep
>
>> +
>> +
>
> Remove one of the two empty lines, please.
Yep
>
>> + VM_EVENT(kvm, 3, "PROTVIRT VM UNPACK: start addr %lx size %lx",
>> + addr, size);
>> + for (i = 0; i < size / PAGE_SIZE; i++) {
>> + uvcb.gaddr = addr + i * PAGE_SIZE;
>> + uvcb.tweak[1] = i * PAGE_SIZE;
>> +retry:
>> + rc = uv_call(0, (u64)&uvcb);
>> + if (!rc)
>> + continue;
>> + /* If not yet mapped fault and retry */
>> + if (uvcb.header.rc == 0x10a) {
>> + rc = gmap_fault(kvm->arch.gmap, uvcb.gaddr,
>> + FAULT_FLAG_WRITE);
>> + if (rc)
>> + return rc;
>> + goto retry;
>> + }
>> + VM_EVENT(kvm, 3, "PROTVIRT VM UNPACK: failed addr %llx rc %x rrc %x",
>> + uvcb.gaddr, uvcb.header.rc, uvcb.header.rrc);
>> + break;
>
> A break at the end of the for-loop ... that's really not what I'd expect.
>
> Could you please invert the logic here, i.e.:
>
> if (uvcb.header.rc != 0x10a) {
> VM_EVENT(...)
> break;
> }
> rc = gmap_fault(...)
> ...
>
> I think you might even get rid of that ugly "goto", too, that way?
But without the goto we would increment i, no?
I'll try to find a solution, maybe using while, but then we need to
manage i incrementation.
>
>> + }
>> + VM_EVENT(kvm, 3, "PROTVIRT VM UNPACK: finished with rc %x rrc %x",
>> + uvcb.header.rc, uvcb.header.rrc);
>> + return rc;
>> +}
>
> Thomas
>
