Re: [Qemu-devel] [PATCH v7 05/13] s390x/css: realize css_create_sch

[Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>]

* Cornelia Huck <cornelia.huck@de.ibm.com> [2017-05-05 14:11:53 +0200]:

> On Fri,  5 May 2017 04:03:44 +0200
> Dong Jia Shi <bjsdjshi@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 <pmorel@linux.vnet.ibm.com>
> > Signed-off-by: Dong Jia Shi <bjsdjshi@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