| Message ID | 20140328105756.21018.57522.stgit@bahia.local |
|---|---|
| State | New |
| Headers | show |
On Fri, 28 Mar 2014 11:57:56 +0100 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > From: Rusty Russell <rusty@rustcorp.com.au> > > Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p. > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> > [ use per-device needs_byteswap flag, > fix missing tswap32 in virtio_scsi_push_event(), > Greg Kurz <gkurz@linux.vnet.ibm.com> ] > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > hw/scsi/virtio-scsi.c | 38 ++++++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index b0d7517..20d326e 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -18,6 +18,7 @@ > #include <hw/scsi/scsi.h> > #include <block/scsi.h> > #include <hw/virtio/virtio-bus.h> > +#include "hw/virtio/virtio-access.h" > > typedef struct VirtIOSCSIReq { > VirtIOSCSI *dev; > @@ -315,12 +316,12 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status, > req->resp.cmd->response = VIRTIO_SCSI_S_OK; > req->resp.cmd->status = status; > if (req->resp.cmd->status == GOOD) { > - req->resp.cmd->resid = tswap32(resid); > + req->resp.cmd->resid = virtio_tswap32(resid, VIRTIO_DEVICE(s)); > } else { > req->resp.cmd->resid = 0; > sense_len = scsi_req_get_sense(r, req->resp.cmd->sense, > vs->sense_size); > - req->resp.cmd->sense_len = tswap32(sense_len); > + req->resp.cmd->sense_len = virtio_tswap32(sense_len, VIRTIO_DEVICE(s)); > } > virtio_scsi_complete_req(req); > } > @@ -416,16 +417,16 @@ static void virtio_scsi_get_config(VirtIODevice *vdev, > VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config; > VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev); > > - stl_raw(&scsiconf->num_queues, s->conf.num_queues); > - stl_raw(&scsiconf->seg_max, 128 - 2); > - stl_raw(&scsiconf->max_sectors, s->conf.max_sectors); > - stl_raw(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun); > - stl_raw(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent)); > - stl_raw(&scsiconf->sense_size, s->sense_size); > - stl_raw(&scsiconf->cdb_size, s->cdb_size); > - stw_raw(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL); > - stw_raw(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET); > - stl_raw(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN); > + virtio_stl_p(&scsiconf->num_queues, s->conf.num_queues, vdev); > + virtio_stl_p(&scsiconf->seg_max, 128 - 2, vdev); > + virtio_stl_p(&scsiconf->max_sectors, s->conf.max_sectors, vdev); > + virtio_stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun, vdev); > + virtio_stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent), vdev); > + virtio_stl_p(&scsiconf->sense_size, s->sense_size, vdev); > + virtio_stl_p(&scsiconf->cdb_size, s->cdb_size, vdev); > + virtio_stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL, vdev); > + virtio_stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET, vdev); > + virtio_stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN, vdev); > } > > static void virtio_scsi_set_config(VirtIODevice *vdev, > @@ -434,14 +435,15 @@ static void virtio_scsi_set_config(VirtIODevice *vdev, > VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config; > VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); > > - if ((uint32_t) ldl_raw(&scsiconf->sense_size) >= 65536 || > - (uint32_t) ldl_raw(&scsiconf->cdb_size) >= 256) { > + if ((uint32_t) virtio_ldl_p(&scsiconf->sense_size, > + vdev) >= 65536 || > + (uint32_t) virtio_ldl_p(&scsiconf->cdb_size, vdev) >= 256) { > error_report("bad data written to virtio-scsi configuration space"); > exit(1); > } > > - vs->sense_size = ldl_raw(&scsiconf->sense_size); > - vs->cdb_size = ldl_raw(&scsiconf->cdb_size); > + vs->sense_size = virtio_ldl_p(&scsiconf->sense_size, vdev); > + vs->cdb_size = virtio_ldl_p(&scsiconf->cdb_size, vdev); > } > > static uint32_t virtio_scsi_get_features(VirtIODevice *vdev, > @@ -519,8 +521,8 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev, > > evt = req->resp.event; > memset(evt, 0, sizeof(VirtIOSCSIEvent)); > - evt->event = event; > - evt->reason = reason; > + evt->event = virtio_tswap32(event); > + evt->reason = virtio_tswap32(reason); My bad again, it should be: + evt->event = virtio_tswap32(event, vdev); + evt->reason = virtio_tswap32(reason, vdev); > if (!dev) { > assert(event == VIRTIO_SCSI_T_EVENTS_MISSED); > } else { > >
Am 28.03.2014 18:13, schrieb Greg Kurz: > On Fri, 28 Mar 2014 11:57:56 +0100 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: >> @@ -519,8 +521,8 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev, >> >> evt = req->resp.event; >> memset(evt, 0, sizeof(VirtIOSCSIEvent)); >> - evt->event = event; >> - evt->reason = reason; >> + evt->event = virtio_tswap32(event); >> + evt->reason = virtio_tswap32(reason); > > My bad again, it should be: > > + evt->event = virtio_tswap32(event, vdev); > + evt->reason = virtio_tswap32(reason, vdev); Pure bikeshedding, but wouldn't it make sense to have the vdev as first argument? Regards, Andreas
On Fri, 28 Mar 2014 18:21:43 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 28.03.2014 18:13, schrieb Greg Kurz: > > On Fri, 28 Mar 2014 11:57:56 +0100 > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > >> @@ -519,8 +521,8 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev, > >> > >> evt = req->resp.event; > >> memset(evt, 0, sizeof(VirtIOSCSIEvent)); > >> - evt->event = event; > >> - evt->reason = reason; > >> + evt->event = virtio_tswap32(event); > >> + evt->reason = virtio_tswap32(reason); > > > > My bad again, it should be: > > > > + evt->event = virtio_tswap32(event, vdev); > > + evt->reason = virtio_tswap32(reason, vdev); > > Pure bikeshedding, but wouldn't it make sense to have the vdev as first > argument? > I have thought about it also... it would make sense to do the same with all the other helpers in virtio-access.h then. And while we are at it, since we pass &address_space_memory to all occurences of virtio_*_phys() and I don't see why we would change that, maybe we can also move that into the helpers. Thoughts ? > Regards, > Andreas > Thanks.
On 28 March 2014 17:37, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > And while we are at it, since we pass &address_space_memory to all > occurences of virtio_*_phys() and I don't see why we would change > that, maybe we can also move that into the helpers. Thoughts ? In the longer term I'm hoping that references to &address_space_memory go away -- we should be modelling separate address spaces per CPU and per every other thing that can act as a DMA master (ie issue memory transactions). I'm not sure exactly how virtio ought to work since these accesses directly to memory are a total hack, but probably we will end up setting the virtio device up and handing it an AddressSpace* that it should use. thanks -- PMM
On Fri, 28 Mar 2014 17:43:07 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On 28 March 2014 17:37, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > And while we are at it, since we pass &address_space_memory to all > > occurences of virtio_*_phys() and I don't see why we would change > > that, maybe we can also move that into the helpers. Thoughts ? > > In the longer term I'm hoping that references to > &address_space_memory go away -- we should be modelling > separate address spaces per CPU and per every other > thing that can act as a DMA master (ie issue memory > transactions). I'm not sure exactly how virtio ought to > work since these accesses directly to memory are a total > hack, but probably we will end up setting the virtio > device up and handing it an AddressSpace* that it should use. > Ok, I am now convinced. Let's make struct VirtIODevice* be the first argument for all helpers and kill the AddressSpace* one. Unless you envision we could end up with different address spaces accross multiple virtio devices, I would then do as proposed above... Even if we add an AddressSpace* to devices, the API will remain the same. > thanks > -- PMM > Cheers.
On 28 March 2014 18:04, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > Ok, I am now convinced. Let's make struct VirtIODevice* be the > first argument for all helpers and kill the AddressSpace* one. > Unless you envision we could end up with different address spaces > accross multiple virtio devices Well, any one virtio device would always use the same AddressSpace, but consider a system model with two separate models of different PCs in it, each of which has its own self-contained memory, PCI bus and virtio devices... thanks -- PMM
On Fri, 28 Mar 2014 18:14:55 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On 28 March 2014 18:04, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > Ok, I am now convinced. Let's make struct VirtIODevice* be the > > first argument for all helpers and kill the AddressSpace* one. > > Unless you envision we could end up with different address spaces > > accross multiple virtio devices > > Well, any one virtio device would always use the same AddressSpace, > but consider a system model with two separate models of different > PCs in it, each of which has its own self-contained memory, PCI > bus and virtio devices... > A per-device AddressSpace* property would be more appropriate then. Correct ? > thanks > -- PMM >
On 03/28/2014 11:57 AM, Greg Kurz wrote: > From: Rusty Russell <rusty@rustcorp.com.au> > > Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p. > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> > [ use per-device needs_byteswap flag, > fix missing tswap32 in virtio_scsi_push_event(), > Greg Kurz <gkurz@linux.vnet.ibm.com> ] > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > hw/scsi/virtio-scsi.c | 38 ++++++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index b0d7517..20d326e 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -18,6 +18,7 @@ > #include <hw/scsi/scsi.h> > #include <block/scsi.h> > #include <hw/virtio/virtio-bus.h> > +#include "hw/virtio/virtio-access.h" Just a small nit, but I spot some inconsitency in < and " usage here :) Alex
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index b0d7517..20d326e 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -18,6 +18,7 @@ #include <hw/scsi/scsi.h> #include <block/scsi.h> #include <hw/virtio/virtio-bus.h> +#include "hw/virtio/virtio-access.h" typedef struct VirtIOSCSIReq { VirtIOSCSI *dev; @@ -315,12 +316,12 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status, req->resp.cmd->response = VIRTIO_SCSI_S_OK; req->resp.cmd->status = status; if (req->resp.cmd->status == GOOD) { - req->resp.cmd->resid = tswap32(resid); + req->resp.cmd->resid = virtio_tswap32(resid, VIRTIO_DEVICE(s)); } else { req->resp.cmd->resid = 0; sense_len = scsi_req_get_sense(r, req->resp.cmd->sense, vs->sense_size); - req->resp.cmd->sense_len = tswap32(sense_len); + req->resp.cmd->sense_len = virtio_tswap32(sense_len, VIRTIO_DEVICE(s)); } virtio_scsi_complete_req(req); } @@ -416,16 +417,16 @@ static void virtio_scsi_get_config(VirtIODevice *vdev, VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config; VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev); - stl_raw(&scsiconf->num_queues, s->conf.num_queues); - stl_raw(&scsiconf->seg_max, 128 - 2); - stl_raw(&scsiconf->max_sectors, s->conf.max_sectors); - stl_raw(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun); - stl_raw(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent)); - stl_raw(&scsiconf->sense_size, s->sense_size); - stl_raw(&scsiconf->cdb_size, s->cdb_size); - stw_raw(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL); - stw_raw(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET); - stl_raw(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN); + virtio_stl_p(&scsiconf->num_queues, s->conf.num_queues, vdev); + virtio_stl_p(&scsiconf->seg_max, 128 - 2, vdev); + virtio_stl_p(&scsiconf->max_sectors, s->conf.max_sectors, vdev); + virtio_stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun, vdev); + virtio_stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent), vdev); + virtio_stl_p(&scsiconf->sense_size, s->sense_size, vdev); + virtio_stl_p(&scsiconf->cdb_size, s->cdb_size, vdev); + virtio_stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL, vdev); + virtio_stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET, vdev); + virtio_stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN, vdev); } static void virtio_scsi_set_config(VirtIODevice *vdev, @@ -434,14 +435,15 @@ static void virtio_scsi_set_config(VirtIODevice *vdev, VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config; VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); - if ((uint32_t) ldl_raw(&scsiconf->sense_size) >= 65536 || - (uint32_t) ldl_raw(&scsiconf->cdb_size) >= 256) { + if ((uint32_t) virtio_ldl_p(&scsiconf->sense_size, + vdev) >= 65536 || + (uint32_t) virtio_ldl_p(&scsiconf->cdb_size, vdev) >= 256) { error_report("bad data written to virtio-scsi configuration space"); exit(1); } - vs->sense_size = ldl_raw(&scsiconf->sense_size); - vs->cdb_size = ldl_raw(&scsiconf->cdb_size); + vs->sense_size = virtio_ldl_p(&scsiconf->sense_size, vdev); + vs->cdb_size = virtio_ldl_p(&scsiconf->cdb_size, vdev); } static uint32_t virtio_scsi_get_features(VirtIODevice *vdev, @@ -519,8 +521,8 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev, evt = req->resp.event; memset(evt, 0, sizeof(VirtIOSCSIEvent)); - evt->event = event; - evt->reason = reason; + evt->event = virtio_tswap32(event); + evt->reason = virtio_tswap32(reason); if (!dev) { assert(event == VIRTIO_SCSI_T_EVENTS_MISSED); } else {