| Message ID | 1424934286-7099-5-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 26 Feb 2015 15:04:39 +0800 Jason Wang <jasowang@redhat.com> wrote: > Instead of depending on marco, using a bus specific limit. > > Cc: Alexander Graf <agraf@suse.de> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > Cc: Richard Henderson <rth@twiddle.net> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/s390x/s390-virtio-ccw.c | 7 +++++-- > hw/s390x/virtio-ccw.c | 13 +++++++------ > include/hw/virtio/virtio.h | 1 + > 3 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 71bafe0..6aeb815 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -43,6 +43,7 @@ void io_subsystem_reset(void) > > static int virtio_ccw_hcall_notify(const uint64_t *args) > { > + VirtIODevice *vdev; > uint64_t subch_id = args[0]; > uint64_t queue = args[1]; > SubchDev *sch; > @@ -55,10 +56,12 @@ static int virtio_ccw_hcall_notify(const uint64_t *args) > if (!sch || !css_subch_visible(sch)) { > return -EINVAL; > } > - if (queue >= VIRTIO_PCI_QUEUE_MAX) { > + > + vdev = virtio_ccw_get_vdev(sch); > + if (queue >= virtio_get_queue_max(vdev)) { But we already know we have a virtio_ccw device, right? So why not just check against VIRTIO_CCW_QUEUE_MAX? > return -EINVAL; > } > - virtio_queue_notify(virtio_ccw_get_vdev(sch), queue); > + virtio_queue_notify(vdev, queue); > return 0; > > } (...) > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 9fe0d92..04ad532 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -93,6 +93,7 @@ typedef struct VirtQueueElement > } VirtQueueElement; > > #define VIRTIO_PCI_QUEUE_MAX 64 > +#define VIRTIO_CCW_QUEUE_MAX 64 Does generic code need to know about the ccw limit? > > #define VIRTIO_NO_VECTOR 0xffff >
On Thu, Feb 26, 2015 at 9:02 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Thu, 26 Feb 2015 15:04:39 +0800 > Jason Wang <jasowang@redhat.com> wrote: > >> Instead of depending on marco, using a bus specific limit. >> >> Cc: Alexander Graf <agraf@suse.de> >> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> >> Cc: Christian Borntraeger <borntraeger@de.ibm.com> >> Cc: Richard Henderson <rth@twiddle.net> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 7 +++++-- >> hw/s390x/virtio-ccw.c | 13 +++++++------ >> include/hw/virtio/virtio.h | 1 + >> 3 files changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 71bafe0..6aeb815 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -43,6 +43,7 @@ void io_subsystem_reset(void) >> >> static int virtio_ccw_hcall_notify(const uint64_t *args) >> { >> + VirtIODevice *vdev; >> uint64_t subch_id = args[0]; >> uint64_t queue = args[1]; >> SubchDev *sch; >> @@ -55,10 +56,12 @@ static int virtio_ccw_hcall_notify(const >> uint64_t *args) >> if (!sch || !css_subch_visible(sch)) { >> return -EINVAL; >> } >> - if (queue >= VIRTIO_PCI_QUEUE_MAX) { >> + >> + vdev = virtio_ccw_get_vdev(sch); >> + if (queue >= virtio_get_queue_max(vdev)) { > > But we already know we have a virtio_ccw device, right? So why not > just > check against VIRTIO_CCW_QUEUE_MAX? The problem is whether or not you want to increase the max queue for ccw. If yes, for migration compatibility, you could not just use a marco here since legacy machine type will also get increased. Something dynamically like this is need so the machine initialization code can change this limit. > > >> return -EINVAL; >> } >> - virtio_queue_notify(virtio_ccw_get_vdev(sch), queue); >> + virtio_queue_notify(vdev, queue); >> return 0; >> >> } > > (...) > >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >> index 9fe0d92..04ad532 100644 >> --- a/include/hw/virtio/virtio.h >> +++ b/include/hw/virtio/virtio.h >> @@ -93,6 +93,7 @@ typedef struct VirtQueueElement >> } VirtQueueElement; >> >> #define VIRTIO_PCI_QUEUE_MAX 64 >> +#define VIRTIO_CCW_QUEUE_MAX 64 > > Does generic code need to know about the ccw limit? Seems not, will move it to ccw file. > > >> >> #define VIRTIO_NO_VECTOR 0xffff >> >
On Fri, 27 Feb 2015 03:46:25 +0008 Jason Wang <jasowang@redhat.com> wrote: > On Thu, Feb 26, 2015 at 9:02 PM, Cornelia Huck > <cornelia.huck@de.ibm.com> wrote: > > On Thu, 26 Feb 2015 15:04:39 +0800 > > Jason Wang <jasowang@redhat.com> wrote: > > > >> Instead of depending on marco, using a bus specific limit. > >> > >> Cc: Alexander Graf <agraf@suse.de> > >> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > >> Cc: Christian Borntraeger <borntraeger@de.ibm.com> > >> Cc: Richard Henderson <rth@twiddle.net> > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > >> --- > >> hw/s390x/s390-virtio-ccw.c | 7 +++++-- > >> hw/s390x/virtio-ccw.c | 13 +++++++------ > >> include/hw/virtio/virtio.h | 1 + > >> 3 files changed, 13 insertions(+), 8 deletions(-) > >> > >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > >> index 71bafe0..6aeb815 100644 > >> --- a/hw/s390x/s390-virtio-ccw.c > >> +++ b/hw/s390x/s390-virtio-ccw.c > >> @@ -43,6 +43,7 @@ void io_subsystem_reset(void) > >> > >> static int virtio_ccw_hcall_notify(const uint64_t *args) > >> { > >> + VirtIODevice *vdev; > >> uint64_t subch_id = args[0]; > >> uint64_t queue = args[1]; > >> SubchDev *sch; > >> @@ -55,10 +56,12 @@ static int virtio_ccw_hcall_notify(const > >> uint64_t *args) > >> if (!sch || !css_subch_visible(sch)) { > >> return -EINVAL; > >> } > >> - if (queue >= VIRTIO_PCI_QUEUE_MAX) { > >> + > >> + vdev = virtio_ccw_get_vdev(sch); > >> + if (queue >= virtio_get_queue_max(vdev)) { > > > > But we already know we have a virtio_ccw device, right? So why not > > just > > check against VIRTIO_CCW_QUEUE_MAX? > > The problem is whether or not you want to increase the max queue for > ccw. If yes, for migration compatibility, you could not just use a > marco here since legacy machine type will also get increased. Confused. With "legacy machine type", do you mean s390-virtio? It does not register this hcall. > Something > dynamically like this is need so the machine initialization code can > change this limit. I fear I don't understand how the machine code comes into play here. Is the virtio-ccw device code supposed to inherit the limit from the machine? Anyway, if we move the queue limit to the VirtIODevice, checking the limit there instead of using a #define should work out fine.
On Fri, Feb 27, 2015 at 5:41 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Fri, 27 Feb 2015 03:46:25 +0008 > Jason Wang <jasowang@redhat.com> wrote: > >> On Thu, Feb 26, 2015 at 9:02 PM, Cornelia Huck >> <cornelia.huck@de.ibm.com> wrote: >> > On Thu, 26 Feb 2015 15:04:39 +0800 >> > Jason Wang <jasowang@redhat.com> wrote: >> > >> >> Instead of depending on marco, using a bus specific limit. >> >> >> >> Cc: Alexander Graf <agraf@suse.de> >> >> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> >> >> Cc: Christian Borntraeger <borntraeger@de.ibm.com> >> >> Cc: Richard Henderson <rth@twiddle.net> >> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> >> --- >> >> hw/s390x/s390-virtio-ccw.c | 7 +++++-- >> >> hw/s390x/virtio-ccw.c | 13 +++++++------ >> >> include/hw/virtio/virtio.h | 1 + >> >> 3 files changed, 13 insertions(+), 8 deletions(-) >> >> >> >> diff --git a/hw/s390x/s390-virtio-ccw.c >> b/hw/s390x/s390-virtio-ccw.c >> >> index 71bafe0..6aeb815 100644 >> >> --- a/hw/s390x/s390-virtio-ccw.c >> >> +++ b/hw/s390x/s390-virtio-ccw.c >> >> @@ -43,6 +43,7 @@ void io_subsystem_reset(void) >> >> >> >> static int virtio_ccw_hcall_notify(const uint64_t *args) >> >> { >> >> + VirtIODevice *vdev; >> >> uint64_t subch_id = args[0]; >> >> uint64_t queue = args[1]; >> >> SubchDev *sch; >> >> @@ -55,10 +56,12 @@ static int virtio_ccw_hcall_notify(const >> >> uint64_t *args) >> >> if (!sch || !css_subch_visible(sch)) { >> >> return -EINVAL; >> >> } >> >> - if (queue >= VIRTIO_PCI_QUEUE_MAX) { >> >> + >> >> + vdev = virtio_ccw_get_vdev(sch); >> >> + if (queue >= virtio_get_queue_max(vdev)) { >> > >> > But we already know we have a virtio_ccw device, right? So why >> not >> > just >> > check against VIRTIO_CCW_QUEUE_MAX? >> >> The problem is whether or not you want to increase the max queue >> for >> ccw. If yes, for migration compatibility, you could not just use a >> marco here since legacy machine type will also get increased. > > Confused. With "legacy machine type", do you mean s390-virtio? It does > not register this hcall. Ok, I thought s390 may have something like pc had to keep the migration compatibility for migration. But seems not. > > >> Something >> dynamically like this is need so the machine initialization code >> can >> change this limit. > > I fear I don't understand how the machine code comes into play here. > Is > the virtio-ccw device code supposed to inherit the limit from the > machine? As I said in the previous reply. The machine code is used to keep migration compatibility. We don't want to break the migration from 2.3 to 2.2. If the limit of ccw will be increased in the future and we want to keep migration compatibility. The machine codes may do something similar to virtio pci. > > Anyway, if we move the queue limit to the VirtIODevice, checking the > limit there instead of using a #define should work out fine. > >
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 71bafe0..6aeb815 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -43,6 +43,7 @@ void io_subsystem_reset(void) static int virtio_ccw_hcall_notify(const uint64_t *args) { + VirtIODevice *vdev; uint64_t subch_id = args[0]; uint64_t queue = args[1]; SubchDev *sch; @@ -55,10 +56,12 @@ static int virtio_ccw_hcall_notify(const uint64_t *args) if (!sch || !css_subch_visible(sch)) { return -EINVAL; } - if (queue >= VIRTIO_PCI_QUEUE_MAX) { + + vdev = virtio_ccw_get_vdev(sch); + if (queue >= virtio_get_queue_max(vdev)) { return -EINVAL; } - virtio_queue_notify(virtio_ccw_get_vdev(sch), queue); + virtio_queue_notify(vdev, queue); return 0; } diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 4874622..98a1a90 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -170,7 +170,7 @@ static void virtio_ccw_start_ioeventfd(VirtioCcwDevice *dev) return; } vdev = virtio_bus_get_device(&dev->bus); - for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) { + for (n = 0; n < virtio_get_queue_max(vdev); n++) { if (!virtio_queue_get_num(vdev, n)) { continue; } @@ -205,7 +205,7 @@ static void virtio_ccw_stop_ioeventfd(VirtioCcwDevice *dev) return; } vdev = virtio_bus_get_device(&dev->bus); - for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) { + for (n = 0; n < virtio_get_queue_max(vdev); n++) { if (!virtio_queue_get_num(vdev, n)) { continue; } @@ -265,8 +265,9 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align, uint16_t index, uint16_t num) { VirtIODevice *vdev = virtio_ccw_get_vdev(sch); + int queue_max = virtio_get_queue_max(vdev); - if (index > VIRTIO_PCI_QUEUE_MAX) { + if (index > queue_max) { return -EINVAL; } @@ -291,7 +292,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align, virtio_queue_set_vector(vdev, index, index); } /* tell notify handler in case of config change */ - vdev->config_vector = VIRTIO_PCI_QUEUE_MAX; + vdev->config_vector = queue_max; return 0; } @@ -1026,7 +1027,7 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector) return; } - if (vector < VIRTIO_PCI_QUEUE_MAX) { + if (vector < virtio_get_queue_max(virtio_bus_get_device(&dev->bus))) { if (!dev->indicators) { return; } @@ -1695,7 +1696,7 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data) k->load_queue = virtio_ccw_load_queue; k->save_config = virtio_ccw_save_config; k->load_config = virtio_ccw_load_config; - k->queue_max = VIRTIO_PCI_QUEUE_MAX; + k->queue_max = VIRTIO_CCW_QUEUE_MAX; } static const TypeInfo virtio_ccw_bus_info = { diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 9fe0d92..04ad532 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -93,6 +93,7 @@ typedef struct VirtQueueElement } VirtQueueElement; #define VIRTIO_PCI_QUEUE_MAX 64 +#define VIRTIO_CCW_QUEUE_MAX 64 #define VIRTIO_NO_VECTOR 0xffff
Instead of depending on marco, using a bus specific limit. Cc: Alexander Graf <agraf@suse.de> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Cc: Richard Henderson <rth@twiddle.net> Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/s390x/s390-virtio-ccw.c | 7 +++++-- hw/s390x/virtio-ccw.c | 13 +++++++------ include/hw/virtio/virtio.h | 1 + 3 files changed, 13 insertions(+), 8 deletions(-)