| Message ID | 87y57nadua.fsf@abhimanyu.in.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 27.08.2013, at 07:43, Nikunj A Dadhania wrote: > Alexander Graf <agraf@suse.de> writes: > >> On 26.08.2013, at 13:46, Nikunj A Dadhania wrote: >> >>> Alexander Graf <agraf@suse.de> writes: >>> >>>> On 26.08.2013, at 12:58, Nikunj A Dadhania wrote: >>>> >>>>> This implements capabilities exchange between host and client. >>>>> As at the moment no capability is supported, put zero flags everywhere >>>>> and return. >>>>> >>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>>>> --- >>>>> + rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len); >>>>> + if (rc) { >>>>> + fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n"); >>>> >>>> At this point cap contains random host data, no? >>> >>> Yes, and we zero it out in this case. >> >> Then please make this obvious to the reader. Either memset(0) it or do >> cap = { };. But do something that doesn't make me look up the header >> file to check whether you really did catch all the fields there are in >> the struct. > > Here is the patch incorporating the comments, moreover goto is not there > anymore, we are being more accomodative and copying only the size(cap) > structure even if guest is requesting for more. > > From: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > > This implements capabilities exchange between vscsi host and client. As > at the moment no capability is supported, put zero flags everywhere and > return. > > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > --- > hw/scsi/spapr_vscsi.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 54 insertions(+) > > diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c > index e9090e5..030b775 100644 > --- a/hw/scsi/spapr_vscsi.c > +++ b/hw/scsi/spapr_vscsi.c > @@ -858,6 +858,57 @@ static int vscsi_send_adapter_info(VSCSIState *s, vscsi_req *req) > return vscsi_send_iu(s, req, sizeof(*sinfo), VIOSRP_MAD_FORMAT); > } > > +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. 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. Alex
Alexander Graf <agraf@suse.de> 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
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c index e9090e5..030b775 100644 --- a/hw/scsi/spapr_vscsi.c +++ b/hw/scsi/spapr_vscsi.c @@ -858,6 +858,57 @@ static int vscsi_send_adapter_info(VSCSIState *s, vscsi_req *req) return vscsi_send_iu(s, req, sizeof(*sinfo), VIOSRP_MAD_FORMAT); } +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"); + } + + /* + * 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); + if (rc) { + fprintf(stderr, "vscsi_send_capabilities: DMA write failure !\n"); + } + if (req_len > len) { + /* + * Being paranoid and lets not worry about the error code + * here. Actual write of the cap is done above. + */ + spapr_vio_dma_set(&s->vdev, (buffer + len), 0, (req_len - len)); + } + vcap->common.status = rc ? cpu_to_be32(1) : 0; + return vscsi_send_iu(s, req, sizeof(*vcap), VIOSRP_MAD_FORMAT); +} + static int vscsi_handle_mad_req(VSCSIState *s, vscsi_req *req) { union mad_iu *mad = &req->iu.mad; @@ -878,6 +929,9 @@ static int vscsi_handle_mad_req(VSCSIState *s, vscsi_req *req) mad->host_config.common.status = cpu_to_be16(1); vscsi_send_iu(s, req, sizeof(mad->host_config), VIOSRP_MAD_FORMAT); break; + case VIOSRP_CAPABILITIES_TYPE: + vscsi_send_capabilities(s, req); + break; default: fprintf(stderr, "VSCSI: Unknown MAD type %02x\n", be32_to_cpu(mad->empty_iu.common.type));