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 ?
> +
> +
Remove one of the two empty lines, please.
> + 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?
> + }
> + VM_EVENT(kvm, 3, "PROTVIRT VM UNPACK: finished with rc %x rrc %x",
> + uvcb.header.rc, uvcb.header.rrc);
> + return rc;
> +}
Thomas
