Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids

[Cornelia Huck]

On Tue, 21 Nov 2017 16:47:29 +0100
Halil Pasic <address@hidden> wrote:

> On 11/21/2017 02:44 PM, Cornelia Huck wrote:
> > On Tue, 21 Nov 2017 12:18:25 +0100
> > Halil Pasic <address@hidden> wrote:
> > 
> > Subject: s/unresrict/unrestrict/  
> 
> Sure!
> 
> >   
> >> The default css 0xFE is currently restricted to virtual subchannel
> >> devices. The hope when the decision was made was, that non-virtual
> >> subchannel devices will come around when guest can exploit multiple
> >> channel subsystems. Since the guests generally don't do, the pain
> >> of the partitioned (cssid) namespace outweighs the gain.
> >>
> >> Let us remove the corresponding restrictions (virtual devices
> >> can be put only in 0xFE and non-virtual devices in any css except
> >> the 0xFE), and inform management software property on all ccw
> >> devices.  
> > 
> > I do not really like dropping the restrictions while still keeping the
> > default cssid as 0xfe. Putting virtual devices into css 0 seems
> > completely fine, but putting non-virtual devices into css 0xfe still
> > feels a bit wrong. What about:
> > 
> > - Add a property to specify the default cssid. Compat machines use a
> >   default cssid of 0xfe.
> > - Default to a cssid of 0.
> > - (optional) Warn when putting a non-virtual device into css 0xfe,
> >   unless it is the default css.
> >   
> 
> Please see Christians response. IMHO the libvirt perspective, and
> especially keeping the domain xmls as they are today viable is the
> key to a good solution.
> 
> AFAIU one should probably use vfio-ccw as devices having their own
> xml managed via attach-device and detach-device, as they are not
> migratable (thus need to be removed before migrating). If we want
> to be able to attach such devices to domains especially created
> with vfio-ccw in mind putting all the devices into 0xfe seems to
> be the only sane way.
> 
> Something like mcsse-squash isn't really a good solution, because
> doing it behind of the back of the user in libvirt feels wrong,
> and if we have to make the user put it in the domain xml then
> we are back at special domain definitions problem.

See my response in the other thread as well. I'm not really opposed to
keeping 0xfe as the default.

> 
> >>
> >> The adverse effect on migration should not be too severe as
> >> vfio-ccw devices are not live-migratable yet, and for virtual
> >> devices using the extra freedom would only make sense with
> >> the aforementioned guest support in place.
> >>
> >> Signed-off-by: Halil Pasic <address@hidden>
> >> Acked-by: Christian Borntraeger <address@hidden>
> >> Reviewed-by: Dong Jia Shi <address@hidden>
> >>
> >> ---
> >> Hi!
> >>
> >> About indicating this at the ccw devices instead of, e.g. as a machine
> >> property (or otherwise globally), was requested by our libvirt guys. I
> >> have no strong opinion regarding in this matter.  
> > 
> > I would like to understand why. It feels very odd.
> >   
> 
> @Boris: I would like to delegate explaining to you. I did understand
> your arguments, but I'm not confident about being able to reproduce them
> authentically.
> 
> >>
> >> This patch is intended as a discussion starter. I would at least like to
> >> get a Tested-by by Shalini before promoting it to non-RFC (provided the
> >> discussion goes well).
> >>
> >> TODOs:
> >> * Consider adding a description for the new property.  
> > 
> > As it is, it is rather incomprehensible. But see below.
> >   
> 
> OK. The idea is that this property should be used for libvirt.

Yes, but what for? That was my problem.

> 
> Comprehensibility is a general problem as the user should not
> really have to care about mcss-e at all (see PoP). How should we
> explain mcss-e to the user? AFAIR this is what triggered the usability
> discussion.

I think we can expect an admin wanting to set up machines understand
the fact that there are multiple cssids, but only the default one can
be seen by most guests.

> 
> >> * Consider renaming VIRTUAL_CSSID.  
> > 
> > Why? This is still reserved for virtual devices in the architecture.
> > You just change qemu policy (and possibly what the default cssid is).
> >  
> 
> I don't think so. Where is it reserved in the architecture? The
> only reference I've found is our VSM book. Sorry I really can't
> find it.

It was reserved with some architecture folks; Christian should know (I
obviously don't have access to anything anymore).

>  
> >> * Consider changing the bus-id generation scheme (when
> >> devno is not specified by the user). his patch keep the current scheme in
> >> place: they won't go into the default css (but slots are filled up
> >> starting at cssid 0). This is theoretically good for migration
> >> compatibility same command line same addresses.  Practically since there
> >> is no migratable non-virtual ccw device, we should consider using the
> >> same bus-id generation scheme for virtual and non-virtual devices.  
> > 
> > How does this interact with the squash parameter?  
> 
> 
> With squash it would not change anything: we would start at default cssid 
> which is 0
> with squash. Without squash it would have the effect that we first
> fill the default css and then proceed to the next one. Would change
> behavior. The hope is that nobody used non-virtual devices without
> squash on, as they are useless that way since there is no mcss-e
> guest around.
> 
> The expected benefit is that the devices would show up in the guest
> instead of being effectively inaccessible -- sane defaults.
> 
> I forgot to write, but I would actually like to deprecate the squash.
> I see it as something on top though.

I'd vote for getting rid of it as soon as possible.

> 
> > 
> > If we force the default css to 0xfe for compat machines, we should be
> > fine if we autogenerate to the default css. If you start at css 0
> > regardless of the default css, you might end up with a configuration
> > that the user did not expect at all.
> >   
> 
> I don't force anything in the compat machines here. So I don't understand
> your question.

It refers to my suggestion above (specifying a default css).

> 
> 
> >>
> >> ---
> >>  hw/s390x/ccw-device.c | 6 ++++++
> >>  hw/s390x/css.c        | 9 ---------
> >>  2 files changed, 6 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
> >> index f9bfa154d6..2167ccea5d 100644
> >> --- a/hw/s390x/ccw-device.c
> >> +++ b/hw/s390x/ccw-device.c
> >> @@ -40,6 +40,10 @@ static Property ccw_device_properties[] = {
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> >>  
> >> +static bool prop_get_true(Object *obj, Error **errp)
> >> +{
> >> +    return true;
> >> +}
> >>  static void ccw_device_class_init(ObjectClass *klass, void *data)
> >>  {
> >>      DeviceClass *dc = DEVICE_CLASS(klass);
> >> @@ -48,6 +52,8 @@ static void ccw_device_class_init(ObjectClass *klass, 
> >> void *data)
> >>      k->realize = ccw_device_realize;
> >>      k->refill_ids = ccw_device_refill_ids;
> >>      dc->props = ccw_device_properties;
> >> +    object_class_property_add_bool(klass, "cssid-unrestricted",
> >> +                                   prop_get_true, NULL, NULL);  
> > 
> > This looks really, really strange. This is a property that is always
> > true if it exists.
> >   
> 
> Won't argue about that. The libvirt guys are actually not interested
> int he value at all: only that there is such a property.

So what about a machine property? Wouldn't that help as well?

Or a css object?

> 
> > What about compat machines? This qemu won't have the restriction, but
> > old qemus will.
> >   
> 
> Very true. But as the commit message implies it should not be a problem.

How is that supposed to play with libvirt detection, then?

> 
> > Also, I'd consider this a property of the machine, not of the
> > individual devices. Or of the ChannelSubsystem structure, which is not
> > qom'ified.
> >   
> 
> I've said the exact same thing to Boris. He said from libvirt perspective
> a device property is better.
> 
> > As an alternative, I think providing a machine default_cssid parameter
> > makes more sense: It is understandable, and you keep compatibility. I
> > think we want this in the long run anyway.
> >   
> 
> I think most of us had the idea. I support this idea fully (expose default
> cssid to the user (rw)). I see it as something that can be done on top
> and is not a pressing issue, but rather a nice to have.

TBH, this weird property is what I like least about this patch.

> 
> >>  }
> >>  
> >>  const VMStateDescription vmstate_ccw_dev = {
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index f6b5c807cd..957cb9ec90 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -2377,15 +2377,6 @@ SubchDev *css_create_sch(CssDevId bus_id, bool 
> >> is_virtual, bool squash_mcss,
> >>      SubchDev *sch;
> >>  
> >>      if (bus_id.valid) {
> >> -        if (is_virtual != (bus_id.cssid == VIRTUAL_CSSID)) {
> >> -            error_setg(errp, "cssid %hhx not valid for %s devices",
> >> -                       bus_id.cssid,
> >> -                       (is_virtual ? "virtual" : "non-virtual"));
> >> -            return NULL;
> >> -        }
> >> -    }  
> > 
> > This allows building a commandline in a compat machine that will not
> > work with old qemus, no?
> >   
> 
> Yes. We have considered this. I was convinced by Christian that
> it ain't too bad. In the end, if we don't want non-virtual device
> aware domains (see above), then there is no way to make this work
> with libvirt. Actually to make the migration work with libvirt with
> old qemus the only way would be to use sqash. But libvirt does not
> want that.
> 
> Also consider that vfio-ccw (AFAIK the only non-virtual css device
> type) is not migratable. So having them on the command line and live
> migrating is a lost case already.
> 
> Yes, having a pre- 2.12 binary version and a post 2.12 binary version
> way to use vfio-ccw is ugly to some extent. The recommendation would
> be don't use it with pre 2.12 (libvirt is going to plainly refuse).
> 
> And yes with this one is going to be able to write a 2.12 command
> line which does not work with 2.11 but that is normal.
> 
> This patch keeps the squash so the 2.10 definition will still be
> viable for 2.12. Should we sometime get rid of the squash, then
> that would be a breaking change.

Removing squash is easy to detect. I'm a bit worried about
not-obvious-at-the-start breakage.

> 
> Regards,
> Halil
> 
> >> -
> >> -    if (bus_id.valid) {
> >>          if (squash_mcss) {
> >>              bus_id.cssid = channel_subsys.default_cssid;
> >>          } else if (!channel_subsys.css[bus_id.cssid]) {  
> >   
>