Re: [Qemu-devel] [PATCH 0/3] vfio-ccw: support hsch/csch (kernel part)

[Halil Pasic]

On Tue, 4 Dec 2018 14:11:30 +0100
Cornelia Huck <address@hidden> wrote:

> On Tue, 4 Dec 2018 13:38:10 +0100
> Halil Pasic <address@hidden> wrote:
> 
> > On Thu, 22 Nov 2018 17:54:29 +0100
> > Cornelia Huck <address@hidden> wrote:
> > 
> > > [This is the Linux kernel part, git tree is available at
> > > https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git 
> > > vfio-ccw-caps
> > > 
> > > The companion QEMU patches are available at
> > > https://github.com/cohuck/qemu vfio-ccw-caps]
> > > 
> > > Currently, vfio-ccw only relays START SUBCHANNEL requests to the real
> > > device. This tends to work well for the most common 'good path' scenarios;
> > > however, as we emulate {HALT,CLEAR} SUBCHANNEL in QEMU, things like
> > > clearing pending requests at the device is currently not supported.
> > > This may be a problem for e.g. error recovery.  
> > 
> > I'm wondering: what about MODIFY SUBCHANNEL? Do we plan to add MSCH
> > as well or is it supposed to remain 'userspace emulated'? AFAIR MSCH
> > may have an effect on error recovery as well.
> 
> I think that would require a deeper change, as we have the requirement
> to enable the subchannel before handing it to userspace. IOW, the guest
> does not cause the subchannel to be enabled/disabled, but the host does.
> 

My point is, when the subchannel is disabled, 'firmware' is responsible
for suppressing interrupts and error conditions, and also for
doing the appropriate recovery procedure, so to say under the hood.

I think Jason has discovered some problems related to this while doing
his DASD IPL with vfio-ccw work, but I don't quite remember any more.

IMHO it may be possible to emulate enable/disable, but it seems way more
error prone and complicated, than letting the guest enable/disable the
host subchannel.

I have no idea what was the reason for going with the initial design.
I would appreciate any hints or explanations, but I'm well aware that it
was a long time ago.


> Parameters (like for channel measurements) are a different game. It is
> something we should look into, but it will need a different region.

Yes emulation only channel measurements seem even less likely than proper
enable/disable. And 'that would need a different' region helps me
understanding the scope of async_cmd_region. Maybe we should reconsider
the comment '+ * @cmd_region: MMIO region for asynchronous I/O commands
other than START'.

> 
> > BTW I would like to have the concurrency discussion sorted out before
> > I proceed with my review, because reviewing the stuff without a fair idea
> > of what exactly are we trying to achieve would yield poor results.
> 
> I'm not sure what is unclear about what we're trying to achieve (enable
> the guest to issue halt/clear on real hardware)?

Yeah, that is perfectly clear, but it ain't the complete story. E.g.
are subsequent commands blocking until the preceding command finishes
is part of the interface. And what is good implementation depends on the
answer. What I mean, I first need to understand how things are supposed
to work (together) so I can double check that against the
implementation. Otherwise all I can do is nitpicking.

To get more tangible: we are in the middle of processing an SSCH request
(e.g. doing the translation) when a HSCH comes in. What should happen?
Should we start processing HSCH after he instruction part of SSCH is
done -- which currently includes translation? Or should we -EBUSY? Or do
we abort START related activities and do the HALT stuff?


> 
> But yes, we need to sort out that concurrency thing; I'm currently
> unsure if the core should do some things as well or if it's more of a
> vendor-driver thing.
> 

By core you mean vfio-mdev core? If yes, I think it is a vendor-driver
thing: limiting concurrency for all vfio-mdev  does not make sense IMHO.

Regards,
Halil