* Cornelia Huck <cornelia.h...@de.ibm.com> [2017-05-05 14:11:53 +0200]:
> On Fri, 5 May 2017 04:03:44 +0200
> Dong Jia Shi <bjsdj...@linux.vnet.ibm.com> wrote:
>
> > The S390 virtual css support already has a mechanism to create a
> > virtual subchannel and provide it to the guest. However, to
> > pass-through subchannels to a guest, we need to introduce a new
> > mechanism to create the subchannel according to the real device
> > information. Thus we reconstruct css_create_virtual_sch to a new
> > css_create_sch function to handl all these cases and do allocation
>
> s/handl/handle/
>
Shame on me. :-/
> > and initialization of the subchannel according to the device type
> > and machine configuration.
> >
> > Reviewed-by: Pierre Morel <pmo...@linux.vnet.ibm.com>
> > Signed-off-by: Dong Jia Shi <bjsdj...@linux.vnet.ibm.com>
> > ---
> > hw/s390x/css-bridge.c | 2 ++
> > hw/s390x/css.c | 45
> > ++++++++++++++++++++++++++++++++++++-------
> > hw/s390x/s390-virtio-ccw.c | 12 +++++++++---
> > hw/s390x/virtio-ccw.c | 6 +++++-
> > include/hw/s390x/css-bridge.h | 1 +
> > include/hw/s390x/css.h | 25 ++++++++++++++++--------
> > 6 files changed, 72 insertions(+), 19 deletions(-)
> >
>
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index 748e2ad..1052eea 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -1924,28 +1924,59 @@ PropertyInfo css_devid_ro_propinfo = {
> > .get = get_css_devid,
> > };
> >
> > -SubchDev *css_create_virtual_sch(CssDevId bus_id, Error **errp)
> > +SubchDev *css_create_sch(CssDevId bus_id, bool is_virtio, bool squash_mcss,
> > + Error **errp)
> > {
> > uint16_t schid = 0;
> > SubchDev *sch;
> >
> > if (bus_id.valid) {
> > - /* Enforce use of virtual cssid. */
> > - if (bus_id.cssid != VIRTUAL_CSSID) {
> > - error_setg(errp, "cssid %hhx not valid for virtual devices",
> > - bus_id.cssid);
> > + if (is_virtio != (bus_id.cssid == VIRTUAL_CSSID)) {
> > + error_setg(errp, "cssid %hhx not valid for %s devices",
> > + bus_id.cssid,
> > + (is_virtio ? "virtio" : "non-virtio"));
>
> This reminds me of something else: virtual 3270 devices will use the
> virtual channel subsystem by default. I think this is fine: Even though
> they are not virtio devices, they are fully virtual constructs, and it
> does not make sense to artificially shove them into another css.
VIRTUAL CSS (0xFE) is reserved for virtio devices only, no? This is what
I understood... So we should not put any non-virtio devices into CSS
0xFE.
If my understanding is not right before, I agree that we put non-virtio
(e.g. 3270) devices into CSS 0xFE, and ...
>
> But this means the distinction should be virtual vs. non-virtual and
> not virtio vs. non-virtio, and the squashing is only applicable to
> non-virtual devices. Mainly a naming thing.
... do the following modifications here:
s/virtio/virtual
s/non-virtio/non-virtual
s/is_virtio/is_virtual
>
>
> > return NULL;
> > }
> > + }
> > +
> > + if (bus_id.valid) {
> > + if (squash_mcss) {
> > + bus_id.cssid = channel_subsys.default_cssid;
> > + } else if (!channel_subsys.css[bus_id.cssid]) {
> > + css_create_css_image(bus_id.cssid, false);
> > + }
> > +
> > if (!css_find_free_subch_for_devno(bus_id.cssid, bus_id.ssid,
> > bus_id.devid, &schid, errp)) {
> > return NULL;
> > }
> > - } else {
> > - bus_id.cssid = VIRTUAL_CSSID;
> > + } else if (squash_mcss || is_virtio) {
> > + bus_id.cssid = channel_subsys.default_cssid;
> > +
> > if (!css_find_free_subch_and_devno(bus_id.cssid, &bus_id.ssid,
> > &bus_id.devid, &schid, errp)) {
> > return NULL;
> > }
> > + } else {
> > + for (bus_id.cssid = 0; bus_id.cssid < MAX_CSSID; ++bus_id.cssid) {
> > + if (bus_id.cssid == VIRTUAL_CSSID) {
> > + continue;
> > + }
> > +
> > + if (!channel_subsys.css[bus_id.cssid]) {
> > + css_create_css_image(bus_id.cssid, false);
> > + }
> > +
> > + if (css_find_free_subch_and_devno(bus_id.cssid, &bus_id.ssid,
> > + &bus_id.devid, &schid,
> > + NULL)) {
> > + break ;
>
> extra blanks
>
Shame again...
> > + }
> > + if (bus_id.cssid == MAX_CSSID) {
> > + error_setg(errp, "Virtual channel subsystem is full!");
> > + return NULL;
> > + }
> > + }
> > }
> >
> > sch = g_malloc0(sizeof(*sch));
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index cd007ca..735d66d 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -136,10 +136,16 @@ static void ccw_init(MachineState *machine)
> > kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> > }
> > /*
> > - * Create virtual css and set it as default so that non mcss-e
> > - * enabled guests only see virtio devices.
> > + * Non mcss-e enabled guests only see the devices from the default
> > + * css, which is determined by the value of the squash_mcss property.
> > + * Note: we must not squash non virtio devices to css 0xFE, since
> > + * it's reserved for virtio devices only.
>
> See my virtio vs. virtual argument above.
>
Yes, if my former understanding is wrong, I will also replace "virtio"
with "virtual" here.
> > */
> > - ret = css_create_css_image(VIRTUAL_CSSID, true);
> > + if (css_bus->squash_mcss) {
> > + ret = css_create_css_image(0, true);
> > + } else {
> > + ret = css_create_css_image(VIRTUAL_CSSID, true);
> > + }
> > assert(ret == 0);
> >
> > /* Create VirtIO network adapters */
--
Dong Jia Shi
