Re: [Qemu-devel] [PATCH] spapr-vscsi: Adding VSCSI capabilities

[Nikunj A Dadhania]

Alexander Graf <address@hidden> writes:
>> +static int vscsi_send_capabilities(VSCSIState *s, vscsi_req *req)
>> +{
>> +    struct viosrp_capabilities *vcap;
>> +    struct capabilities cap = { };
>> +    uint16_t len, req_len;
>> +    uint64_t buffer;
>> +    int rc;
>> +
>> +    vcap = &req->iu.mad.capabilities;
>> +    req_len = len = be16_to_cpu(vcap->common.length);
>> +    buffer = be64_to_cpu(vcap->buffer);
>> +    if (len > sizeof(cap)) {
>> +        fprintf(stderr, "vscsi_send_capabilities: capabilities size 
>> mismatch !\n");
>> +
>> +        /*
>> +         * Just read and populate the structure that is known.
>> +         * Zero rest of the structure.
>> +         */
>> +        len = sizeof(cap);
>> +    }
>> +    rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
>> +    if (rc)  {
>> +        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");
>
> ... if we fail reading
>
>> +    }
>> +
>> +    /*
>> +     * Current implementation does not suppport any migration or
>> +     * reservation capabilities. Construct the response telling the
>> +     * guest not to use them.
>> +     */
>> +    cap.flags = 0;
>> +    cap.migration.ecl = 0;
>> +    cap.reserve.type = 0;
>> +    cap.migration.common.server_support = 0;
>> +    cap.reserve.common.server_support = 0;
>> +
>> +    rc = spapr_vio_dma_write(&s->vdev, buffer, &cap, len);
>
> ... but then succeed writing back the empty structure we will report
> "success". Is this correct? It's fine with me if it's intended, just
> want to make sure we're making a conscious decision about it.

Yes, that should be fine if it succeeds writing it. If DMA read/write is
an issue, it should fail writing as well and we report back failure.

>
> The rest looks good to me. But please post new patch versions as new
> top level emails, not as inline replies. My scripts use patchwork and
> that can't really handle this case overly well (it works, but is
> additional work). It also makes it hard for people who just follow the
> thread loosely to realize that a new version is there.

Sure, will take care next time.

Regards
Nikunj