* Cornelia Huck [2018-02-27 18:32:51 +0100]:
> The license text currently specifies "any version" of the GPL. It
> is unlikely that GPL v1 was ever intended; change this to the
> standard "or any later version" text.
>
> Cc: Dong Jia Shi
> Cc: Xiao Feng Re
t?
>
> I'm not sure about the later. What prevents the guest from believing
> the (to use your terminology shared) chp 0.00 has become unavailable
> if 0.00 becomes unavailable in the host?
>
> Would that adversely affect the virtual devices? It would at
> least look strange.
AFAI read the code, this series does not hurt the vritual devices, which
always have fixed path information in SCHIB.
The strange thing is, the chp 0.00 is both virtual and non-virtual with
mix use of virtual and non-virtual devices. And any existing problem is
not caused or enhanced by this work. If we can not fix the problem
without MCSS support or chp re-modelling in the near future, we should
document the problem and the effect.
I will do some test on this.
>
>>
>> We'd have to document this either I think.
>>
>>> Note: this is another unpleasant effect of not having MCSSE in the
>>> guest. The original design was to have a separate css for vfio, and
>>> then we would not have this kind of a problem. (Virtualization of
>>> chps would still be good for migration.)
>> Nod. BTW, I don't say that I do not want channel path virtualization in
>> the long run. It's just I want a minimal function set more currently
>> from what I'm focusing perspective.
>>
>> THANKS for these insightful questions!
>>
--
Dong Jia Shi
* Dong Jia Shi [2018-01-30 11:37:43 +0800]:
Damn.. Please just ignore the above mail. It's not the right draft.
> Halil Pasic writes:
>
> Hi Halil,
>
> As you may noticed, Conny replied to this thread on my mail. Some of her
> comments there could answer your questi
up being ignored)?
>> You mean the clash that sharing path 0.00 between a physical dasd and
>> the virtio ccw devices? The problem I saw is in the checking of the
>> chpid.is_virtual in css_do_rchp(). For a virtual chp, we should generate
>> INIT CRWs; while for a non-virtual one, we shouldn't. If the chp is
>> shared with virtual and non-virtual device, I don't know what to do
>> then...
>>
>> I don't think we can fix this before we re-modelled the channel path.
>> But this is neither introduced nor aggravated by this series, right?
>
> I'm not sure about the later. What prevents the guest from believing
> the (to use your terminology shared) chp 0.00 has become unavailable
> if 0.00 becomes unavailable in the host?
>
> Would that adversely affect the virtual devices? It would at
> least look strange.
>
>>
>> We'd have to document this either I think.
>>
>>> Note: this is another unpleasant effect of not having MCSSE in the
>>> guest. The original design was to have a separate css for vfio, and
>>> then we would not have this kind of a problem. (Virtualization of
>>> chps would still be good for migration.)
>> Nod. BTW, I don't say that I do not want channel path virtualization in
>> the long run. It's just I want a minimal function set more currently
>> from what I'm focusing perspective.
>>
>> THANKS for these insightful questions!
>>
--
Dong Jia Shi
* Halil Pasic [2018-01-16 16:57:13 +0100]:
>
>
> On 01/15/2018 09:59 AM, Dong Jia Shi wrote:
> > * Halil Pasic [2018-01-12 19:10:20 +0100]:
> >
> >>
> >>
> >> On 01/11/2018 04:04 AM, Dong Jia Shi wrote:
> >>> What are still missing
* Pierre Morel [2018-01-15 11:21:47 +0100]:
> On 15/01/2018 09:57, Dong Jia Shi wrote:
> >* Cornelia Huck [2018-01-11 11:54:22 +0100]:
> >
> >>On Thu, 11 Jan 2018 04:04:18 +0100
> >>Dong Jia Shi wrote:
> >>
> >>>Hi Folks,
> >>>
* Cornelia Huck [2018-01-15 13:24:22 +0100]:
> On Mon, 15 Jan 2018 10:50:00 +0100
> Pierre Morel wrote:
>
> > On 11/01/2018 04:04, Dong Jia Shi wrote:
> > > This introduces a new region for vfio-ccw to provide subchannel
> > > information for user space.
> &
* Halil Pasic [2018-01-12 19:10:20 +0100]:
>
>
> On 01/11/2018 04:04 AM, Dong Jia Shi wrote:
> > What are still missing, thus need to be offered in the next version are:
> > - I/O termination and FSM state handling if currently we have I/O on the
> > status
> &g
* Cornelia Huck [2018-01-11 11:54:22 +0100]:
> On Thu, 11 Jan 2018 04:04:18 +0100
> Dong Jia Shi wrote:
>
> > Hi Folks,
> >
> > Background
> > ==
> >
> > Some days ago, we had a discussion on the topic of channel path
> > virtua
* Cornelia Huck [2018-01-11 15:16:59 +0100]:
Hi Conny,
> On Thu, 11 Jan 2018 04:04:19 +0100
> Dong Jia Shi wrote:
>
> > This introduces a new region for vfio-ccw to provide subchannel
> > information for user space.
> >
> > Signed-off-by: Dong Jia S
le we are at it, touch one line of the comment around by
adding white space to make it aligned.
Signed-off-by: Dong Jia Shi
---
hw/s390x/css.c | 83 -
hw/s390x/s390-ccw.c | 20 +++
hw/vfio/ccw.c
path information update, we only do update if we were signaled.
We sets scsw.pno and pmcw.pnom to indicate path related event
for a guest. This is a bit lazy, but it works.
Signed-off-by: Dong Jia Shi
---
hw/s390x/css.c | 14 +--
hw/s390x/s390-ccw.c | 12 +
hw
the schib likeness.
Signed-off-by: Dong Jia Shi
---
hw/vfio/ccw.c | 24 +++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 16713f2c52..e673695509 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -32,6 +32,10 @@ typedef
Hi Folks,
This is the QEMU couterpart for the "basic channel path event handling" series.
For more information, please refer to the kernel counterpart.
Dong Jia Shi (5):
vfio: linux-headers update for vfio-ccw
vfio/ccw: get schib region info
vfio/ccw: get irq info and set up h
This adds channel path related event handler for vfio-ccw.
This also signals userland when there is a chp event.
Signed-off-by: Dong Jia Shi
---
drivers/s390/cio/vfio_ccw_drv.c | 51 +
drivers/s390/cio/vfio_ccw_fsm.c | 22
drivers
path information
out once got the eventfd signal, and do further process.
Signed-off-by: Dong Jia Shi
---
hw/vfio/ccw.c | 92 +--
1 file changed, 70 insertions(+), 22 deletions(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index e673695509
This is a placeholder for a linux-headers update.
Signed-off-by: Dong Jia Shi
---
linux-headers/linux/vfio.h | 2 ++
linux-headers/linux/vfio_ccw.h | 6 ++
2 files changed, 8 insertions(+)
diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index 4312e961ff..24d85be022
This introduces a new irq for vfio-ccw to provide channel path
related event for userland.
Signed-off-by: Dong Jia Shi
---
drivers/s390/cio/vfio_ccw_ops.c | 29 +
drivers/s390/cio/vfio_ccw_private.h | 2 ++
include/uapi/linux/vfio.h | 1 +
3 files
Use PIM PAM POM CHPIDs
--
0.0.3f3f 0.0.0002 3390/0c 3990/e9 f0 f0 ff 42434445
#Notice: PAM changed again.
Dong Jia Shi (3):
vfio: ccw: introduce schib region
vfio: ccw: introduce channel path irq
vfio: ccw: handle chp event
drivers/s390/cio/
This introduces a new region for vfio-ccw to provide subchannel
information for user space.
Signed-off-by: Dong Jia Shi
---
drivers/s390/cio/vfio_ccw_fsm.c | 21 ++
drivers/s390/cio/vfio_ccw_ops.c | 79 +++--
drivers/s390/cio/vfio_ccw_private.h
* Dong Jia Shi [2017-12-05 15:43:00 +0800]:
> * Cornelia Huck [2017-12-04 17:11:24 +0100]:
>
> [...]
>
> This one looks good to me too, so:
> Reviewed-by: Dong Jia Shi
>
> >
> > (...)
> >
> > > > Looks sane. We should put a note into th
* Dong Jia Shi [2017-12-05 15:43:00 +0800]:
> * Cornelia Huck [2017-12-04 17:11:24 +0100]:
>
> [...]
>
> This one looks good to me too, so:
> Reviewed-by: Dong Jia Shi
>
BTW, since we are deprecating s390-squash-mcss, I think any more
improment on it (e.g. more accu
* Cornelia Huck [2017-12-04 17:11:24 +0100]:
[...]
This one looks good to me too, so:
Reviewed-by: Dong Jia Shi
>
> (...)
>
> > > Looks sane. We should put a note into the 2.12 changelog as well.
> > >
> >
> > I agree. Who would be responsi
^
> >
>
> Nod.
>
> >> +" or not (read only, always true)",
>
> Do we need "." here ^ ?
>
> >> +NULL);
> >> }
> >>
> >> static const TypeInfo virtual_css_bridge_info = {
> >
> > Looks reasonable. If this works as expected, I'll squash it into the
> > previous patch.
> >
>
> I've just asked Shalini to verify the libvirt perspective.
>
> Supposed we verify this works as expected, I read I don't have to spin
> a v2 and you are going to fix the issues found yourself. Right?
Supposed this works as expected, you have my r-b. ;)
--
Dong Jia Shi
* Halil Pasic [2017-12-01 15:31:34 +0100]:
[...]
No comment for the message part.
The code looks good to me. So after squashing with patch #2:
Reviewed-by: Dong Jia Shi
> ---
> hw/s390x/3270-ccw.c| 2 +-
> hw/s390x/css.c | 28
&g
> > The differences of the test results between w and w/o this patch are in
> > the "squashing off" cases. I think these are things that we can accept.
>
> Yes, I think that makes sense. If you want reliable migration, you need
> to be specific with your ids. I'd just don't want us to break things
> explicitly.
>
Fair enough. Got the message.
--
Dong Jia Shi
* Halil Pasic [2017-11-29 17:30:15 +0100]:
>
>
> On 11/29/2017 12:47 PM, Cornelia Huck wrote:
> > On Wed, 29 Nov 2017 16:17:35 +0800
> > Dong Jia Shi wrote:
> >
> >> * Halil Pasic [2017-11-28 14:07:58 +0100]:
> >>
> >> [...]
> >&g
* Dong Jia Shi [2017-11-29 16:17:35 +0800]:
[...]
> T6. squashing off + explicit given id
> qemu-system-s390x: vmstate: get_nullptr expected VMS_NULLPTR_MARKER
> qemu-system-s390x: Failed to load s390_css:css
> qemu-system-s390x: error while loading state for instance 0
wn savevm section or instance '/00.0.0003/virtio-rng' 0
qemu-system-s390x: load of migration failed: Invalid argument
[Fail due to busid mismatch.]
T8. squashing on + explicit given id
Succeed.
Notice:
The differences of the test results between w and w/o this patch are in
the "squashing off" cases. I think these are things that we can accept.
[...]
--
Dong Jia Shi
* Cornelia Huck [2017-11-27 13:58:16 +0100]:
> On Mon, 27 Nov 2017 10:20:56 +0800
> Dong Jia Shi wrote:
>
> > * Halil Pasic [2017-11-24 17:39:04 +0100]:
> >
> > >
> > >
> > > On 11/24/2017 05:15 PM, Cornelia Huck wrote:
> > >
e can be put
> anywhere. This approach feels wrong to me (the non-restriction is not a
> property of the individual device, but of the whole css resp. the
> machine), so I would continue to veto this.
>
> Proposal 2: Export the default cssid as a machine property. If this
> property exists, it also implies that devices can be put into any css
> image (although it makes the most sense to put them into the default
> css image as indicated by the property). Can be made r/w later if it is
> too much for 2.12.
>
> Proposal 3: Add a machine property that indicates cssids are not
> restricted. Later, optionally add a second property that exposes the
> default cssid and makes it configurable.
>
> Personally, I prefer 2 (especially as this also allows to find out
> where the best place to put devices for non-mcss-e enabled guests is),
> but I could live with 3 as well (if making this r/w later would be
> problematic for libvirt.)
>
> (In every case, we would deprecate and later remove the squash
> parameter.)
>
> [As an aside, how hard is it to make the default cssid configurable? At
> a glance, it does not seem too bad to me.]
>
--
Dong Jia Shi
e operation of setting it's value to
true.
> >
> > (Unless we simply make this a "default cssid" prop after all - then it
> > would be more than just a simple indication for libvirt...)
> >
>
> We are now talking about the "cssid-unrestricted" property. The default
> cssid is not something I would like to do any time soon.
--
Dong Jia Shi
e cds_read. What we
want would be surely more than this. So to extend the coverage, we need
to design more modes (aka, test cases), right?
If nobody disagrees with the basic idea of this series, I can start a
review then. ;)
[...]
--
Dong Jia Shi
* Cornelia Huck [2017-11-14 11:50:14 +0100]:
Hallo Conny,
After spending some time, just some updates for this one.
> On Tue, 14 Nov 2017 16:25:47 +0800
> Dong Jia Shi wrote:
>
> > Dear Conny,
> >
> > Good day!
> >
> > Just now, our Libvirt folks
also for QEMU cmd line users. And we are wondering if there is some
way to improve it.
Thanks,
--
Dong Jia Shi
> >>> -ret = -EBUSY;
> >>> -goto out;
> >>> +return IOINST_CC_STATUS_PRESENT;
> >>> }
> >>
> >> ... but here you change -EBUSY (which got mapped to CC=2) to
> >> CC_STATUS_PRESENT which means CC=1. So that's a change in behavior. i.e.
> >> this is either a bug, or you should update the patch description with a
> >> justification for this change in behavior.
> >
> > Indeed, that's a bug. We still want cc 2.
> >
>
> Agree, it is a bug. It was OK in v1 but was already buggy in
> v2.
>
> Conny can you fix this up as well please?
>
> Thanks in advance!
>
I saw Conny fixed this in her branch. So:
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
n;
> }
> -setcc(cpu, cc);
> +setcc(cpu, css_do_hsch(sch));
> }
>
> static int ioinst_schib_valid(SCHIB *schib)
> --
> 2.13.5
>
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
gt; --
> 2.13.5
>
If we agree to replace 3 with IOINST_CC_NOT_OPERATIONAL, maybe Conny
could fix it. Or we can do that in a following cleanup patch?
W/ or w/o the fix, for now:
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
ENODEV:
> -cc = 3;
> -break;
> -case -EBUSY:
> -cc = 2;
> -break;
> -case 0:
> - cc = 0;
> - break;
> -default:
> -cc = 1;
> -break;
> +if (!sch || !css_subch_visible(sch)) {
> +setcc(cpu, 3);
?:
s/3/IOINST_CC_NOT_OPERATIONAL/
This also applies to other alike cases.
> +return;
> }
> -setcc(cpu, cc);
> +setcc(cpu, css_do_xsch(sch));
> }
>
> void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
> --
> 2.13.5
>
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
;
Agree for what already planned to fix when applying.
LGTM:
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
* Cornelia Huck [2017-10-18 11:53:10 +0200]:
> On Wed, 18 Oct 2017 16:23:47 +0800
> Dong Jia Shi wrote:
>
> > * Halil Pasic [2017-10-17 18:19:20 +0200]:
> >
> > [...]
> >
> > > >> Testing
> > > >> ===
> > > >&
IOINST_CC_BUSY = 2,
> +/* inst. ineffective because not operational */
> +IOINST_CC_NOT_OPERATIONAL = 3
> +} IOInstEnding;
> +
> typedef struct SubchDev SubchDev;
> struct SubchDev {
> /* channel-subsystem related things: */
> --
> 2.13.5
>
With what ha
* Halil Pasic [2017-10-17 18:19:20 +0200]:
[...]
> >> Testing
> >> ===
> >>
> >> Nothing happened since v2 except for a quick smoke test. Dong Jia gave v2
> >> a spin with a focus on vfio-ccw. @Dong Jia I would appreciate some proper
> >
* Halil Pasic [2017-10-11 12:54:51 +0200]:
>
>
> On 10/11/2017 05:47 AM, Dong Jia Shi wrote:
> > * Halil Pasic [2017-10-04 17:41:39 +0200]:
> >
> >> Simplify the error handling of the SSCH and RSCH handler avoiding
> >> arbitrary and cryptic error codes
* Halil Pasic [2017-10-10 12:06:23 +0200]:
>
>
> On 10/10/2017 10:13 AM, Dong Jia Shi wrote:
> > * Halil Pasic [2017-10-04 17:41:39 +0200]:
> >
> > [...]
> >
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index 4f47dbc8b0..b2978c
upt(sch);
> +return (IOInstEnding){.cc = 0};
> }
> -
> -return region->ret_code;
> }
>
> static void vfio_ccw_reset(DeviceState *dev)
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 66916b6546..2116c6b3c7 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
[...]
> @@ -197,13 +208,14 @@ SubchDev *css_find_subch(uint8_t m, uint8_t cssid,
> uint8_t ssid,
> uint16_t schid);
> bool css_subch_visible(SubchDev *sch);
> void css_conditional_io_interrupt(SubchDev *sch);
> +
Extra change.
> int css_do_stsch(SubchDev *sch, SCHIB *schib);
> bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
> int css_do_msch(SubchDev *sch, const SCHIB *schib);
[...]
--
Dong Jia Shi
> +IOInstEnding s390_ccw_cmd_request(SubchDev *sch)
> {
> -S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(data);
> +S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
>
> -if (cdc->handle_request) {
> -return cdc->handle_request(orb, scsw, data);
> -} else {
> -return -ENOSYS;
> +if (!cdc->handle_request) {
> +return (IOInstEnding){.cc = 1};
Same consideration as the last comment.
> }
> +return cdc->handle_request(sch);
> }
>
> static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
[...]
--
Dong Jia Shi
>do_subchannel_work) {
> -return sch->do_subchannel_work(sch);
> -} else {
> +if (!sch->do_subchannel_work) {
> return -EINVAL;
> }
> +g_assert(sch->curr_status.scsw.ctrl & SCSW_CTRL_MASK_FCTL);
> +return sch->do_subchannel_work(sch);
> }
>
> static void copy_pmcw_to_guest(PMCW *dest, const PMCW *src)
With the fix of the message:
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
* Dong Jia Shi [2017-09-26 15:48:56 +0800]:
[...]
> > >
> > > Tried to test with the following method:
> > > 1. Start g1 (first level guest on kvm a host) with a virtio blk device
> > >defined:
> > > -drive
> > > file=/dev/disk/by-path
* Halil Pasic [2017-09-25 12:57:31 +0200]:
[restored Cc:]
>
>
> On 09/25/2017 09:31 AM, Dong Jia Shi wrote:
> > * Cornelia Huck [2017-09-08 11:59:50 +0200]:
> >
> >> On Fri, 8 Sep 2017 11:21:57 +0200
> >> Halil Pasic wrote:
> >>
> >>
* Cornelia Huck [2017-09-21 10:54:02 +0200]:
> On Thu, 21 Sep 2017 16:45:47 +0800
> Dong Jia Shi wrote:
>
> > * Cornelia Huck [2017-09-07 10:08:17 +0200]:
> >
> > [...]
> >
> > > > I'm thinking of a method these days:
> > > >
* Cornelia Huck [2017-09-08 11:59:50 +0200]:
> On Fri, 8 Sep 2017 11:21:57 +0200
> Halil Pasic wrote:
>
> > On 09/08/2017 05:41 AM, Dong Jia Shi wrote:
> > > Let' me summarize here, in case I misunderstand things. Now we have
> > > two ways to cho
* Cornelia Huck [2017-09-08 12:02:38 +0200]:
> On Fri, 8 Sep 2017 11:41:00 +0800
> Dong Jia Shi wrote:
>
> > I think the difficult part is in the Qemu side. For either A or B, in
> > Qemu, we'd need to return a cc0 to indicate the channel program is
> > accepted
; ---
> hw/s390x/css.c | 114
> -
> 1 file changed, 113 insertions(+), 1 deletion(-)
>
LGTM:
Reviewed-by: Dong Jia Shi
[...]
--
Dong Jia Shi
cds);
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 078356e94c..69b374730e 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -87,6 +87,7 @@ typedef struct CcwDataStream {
> #define CDS_F_MIDA 0x02
> #define CDS_F_I2K 0x04
> #define CDS_F_C64 0x08
> +#define CDS_F_FMT 0x10 /* CCW format-1 */
> #define CDS_F_STREAM_BROKEN 0x80
> uint8_t flags;
> uint8_t at_idaw;
> --
> 2.13.5
>
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
pt in a
synchronzing manner. And this causes g1's I/O interrupt handler getting
the interrupt and then signaling the Qemu instance on g1 with the I/O
result, even before return of the pwrite().
But, using gdb on the kvm host, I do see several ssch successfully
executed. I will dig the root reason, and see if there is some way to
fix the issue.
--
Dong Jia Shi
* Halil Pasic [2017-09-20 13:13:01 +0200]:
>
>
> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 15:42:38 +0800
> > Dong Jia Shi wrote:
> >
> >> * Halil Pasic [2017-09-19 20:27:45 +0200]:
> >>
> >>> Let'
* Halil Pasic [2017-09-20 18:46:57 +0200]:
>
>
> On 09/20/2017 01:18 PM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 13:13:01 +0200
> > Halil Pasic wrote:
> >
> >> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> >>> On Wed, 20 S
* Halil Pasic [2017-09-20 13:02:59 +0200]:
>
>
> On 09/20/2017 10:25 AM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 15:47:51 +0800
> > Dong Jia Shi wrote:
> >
> >> * Halil Pasic [2017-09-19 20:27:44 +0200]:
> >
> >>> @@ -828,7 +8
gt; --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -87,6 +87,7 @@ typedef struct CcwDataStream {
> #define CDS_F_MIDA 0x02
> #define CDS_F_I2K 0x04
> #define CDS_F_C64 0x08
> +#define CDS_F_FMT 0x10 /* CCW format-1 */
> #define CDS_F_STREAM_BROKEN 0x80
> uint8_t flags;
> uint8_t at_idaw;
> --
> 2.13.5
>
--
Dong Jia Shi
CcwDataStreamOp op)
> +{
> +uint64_t bsz = ccw_ida_block_size(cds->flags);
> +int ret = 0;
> + uint16_t cont_left, iter_len;
> +const bool idaw_fmt2 = cds->flags & CDS_F_C64;
> +bool ccw_fmt1 = cds->flags & CDS_F_FMT;
Use 'const bool' either? Although I doubt the value of using const here.
;)
> +
> +ret = cds_check_len(cds, len);
> +if (ret <= 0) {
> +return ret;
> +}
[...]
--
Dong Jia Shi
--
> hw/s390x/virtio-ccw.c | 157
> +++---
> 1 file changed, 46 insertions(+), 111 deletions(-)
>
Reviewed-by: Dong Jia Shi
[...]
--
Dong Jia Shi
ks good to me.
> +} CcwDataStreamOp;
> +
>
[...]
--
Dong Jia Shi
).
>
> Reference:
> PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and
> "Invalid Data Address".
>
> How shall we proceed?
>
> Halil
>
> >>>>
> >>>>> +return -EINVAL; /* channel program check */
> >>>>> +
> >>>>> +}
> >>>>> +ret = address_space_rw(&address_space_memory, idaw_addr,
> >>>>> + MEMTXATTRS_UNSPECIFIED, (void *)
> >>>>> &idaw.fmt1,
> >>>>> + sizeof(idaw.fmt1), false);
> >>>>> +cds->cda = be64_to_cpu(idaw.fmt1);>>>>> +}
> >>>>> +++(cds->at_idaw);
> >>>>> +if (ret != MEMTX_OK) {
> >>>>> +/* assume inaccessible address */
> >>>>> +return -EINVAL; /* channel program check */
> >>>>> +
> >>>>> +}
> >>>>> +return 0;
> >>>>> +}
--
Dong Jia Shi
t;>>> Nit.
> >>>>
> >> Maybe checkpatch wanted it this way. My memories are blurry.
> > I'd just leave it like that, tbh.
>
> Yes, if I remove the space before the } checkpatch.pl complains.
>
> Going to keep it as is for v3.
My bad. :-/
>
> Halil
--
Dong Jia Shi
* Halil Pasic [2017-09-19 15:30:29 +0200]:
>
>
> On 09/19/2017 11:06 AM, Cornelia Huck wrote:
> > On Tue, 19 Sep 2017 11:37:30 +0800
> > Dong Jia Shi wrote:
> >
> >> * Halil Pasic [2017-09-13 13:50:28 +0200]:
> >>
> >>> Replace di
tion authors are busy of other stuff these days. :()
>
> Generally, yes I agree. If it's trivial I will probably do it myself
> for v2. I need to do a v2 anyway.
>
> Halil
I saw you are working on this. So leave it to you. ;)
--
Dong Jia Shi
err;
> +}
> +cont_left = bsz;
> +} while (true);
> +return ret;
> +err:
> + cds->flags |= CDS_F_STREAM_BROKEN;
> +return ret;
> +}
> +
> void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> {
> /*
> @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const
> *ccw, ORB const *orb)
> if (!(cds->flags & CDS_F_IDA)) {
> cds->op_handler = ccw_dstream_rw_noflags;
> } else {
> -assert(false);
> +cds->op_handler = ccw_dstream_rw_ida;
> }
> }
>
> --
> 2.13.5
>
Generally, the logic looks fine to me.
--
Dong Jia Shi
emory,
> - ccw.cda + sizeof(revinfo.revision),
> - MEMTXATTRS_UNSPECIFIED, NULL);
> +ccw_dstream_read_buf(&sch->cds, &revinfo, 4);
^
A magic number? O:)
> +be16_to_cpus(&revinfo.revision);
> +be16_to_cpus(&revinfo.length);
> if (ccw.count < len + revinfo.length ||
> (check_len && ccw.count > len + revinfo.length)) {
> ret = -EINVAL;
> --
> 2.13.5
>
Otherwise, looks good.
--
Dong Jia Shi
ret_ccw(SubchDev *sch, hwaddr
> ccw_addr,
> } else {
> sense_id.reserved = 0;
> }
> -cpu_physical_memory_write(ccw.cda, &sense_id, len);
> -sch->curr_status.scsw.count = ccw.count - len;
> +ccw_dstream_write_buf(&sch-
translating 'A' as append in my mind...)
> +} CcwDataStreamOp;
> +
[...]
> +static inline uint16_t ccw_dstream_avail(CcwDataStream *cds)
> +{
> +return ccw_dstream_good(cds) ? ccw_dstream_residual_count(cds) : 0;
^^
Nit.
> +}
[
t like such modification in the Qemu side, then A is not the
way to go.
And for B, we need to update the IRB area of the I/O region in the three
cases listed in the former mail, and:
1. We need to set the ret_code as 0 for them, so that Qemu could map it
to cc0.
2. We need to signal Qemu to fetch the IRB we generated with the checks.
All of these are doable I think.
Any comment for the above thoughts?
--
Dong Jia Shi
* Cornelia Huck [2017-09-07 12:52:20 +0200]:
> On Thu, 7 Sep 2017 12:21:50 +0200
> Halil Pasic wrote:
>
> > On 09/07/2017 10:08 AM, Cornelia Huck wrote:
> > > On Thu, 7 Sep 2017 15:31:09 +0800
> > > Dong Jia Shi wrote:
>
> > >> I'm
ussion is with a unit exception.
> >>
> >> Does that make sense?
> >
> > I think the situation is slightly different here, though. For the orb
> > flags, we reject something out of hand because we have not implemented
> > it, and for that, unit exception
* Cornelia Huck [2017-09-06 13:25:38 +0200]:
> On Wed, 6 Sep 2017 16:27:20 +0800
> Dong Jia Shi wrote:
>
> > * Halil Pasic [2017-09-05 19:20:43 +0200]:
> >
> > >
> > >
> > > On 09/05/2017 05:46 PM, Cornelia Huck wrote:
> > > >
gh. She stucked with the problem that the level 1 kvm host handling
a senseid IDAL ccw as a Direct ccw.
Maybe I could try to pass through a virtio ccw device. I don't think of
any obvious problem that would lead to fail. Any comment?
--
Dong Jia Shi
> If converting the reporting to a device status is straightforward
> enough, let's do that. I'm fine with postponing this and waiting for a
> real fix as well (I don't really have spare cycles to actually write
> vfio-ccw code currently...)
>
I can do this after this series.
[...]
--
Dong Jia Shi
nd I agree that's best handled
> with comment in code.
Using unit check, with bit 3 byte 0 of the sense data set to 1, to
indicate an 'Equipment check', sounds a bit more proper than unit
exception.
> 2) The poor user/programmer is trying to figure out why things
> don't work (why are we getting the unit exception)? I think that's
> best remedied with producing something for the log (maybe a warning
> with warn_report which states that the implementation vfio-ccw requires
> the given flags).
Fine with me.
[...]
--
Dong Jia Shi
never reach the code in question.
Agree.
> So I suppose we can do better.
As the comment said, I'm (still) in the state of 'wondering'.
>
> Adding Ren. @Ren: Do you agree with my analysis. If you do,
> I could come up with a proposal what to do -- after some reading.
If you have a better idea, and time, why not? ;)
>
> Regards,
> Halil
--
Dong Jia Shi
Let's use a macro for the ERC (error recover code) when generating a
Channel Subsystem Event-information pending CRW (channel report word).
While we are at it, let's also add all other ERCs.
Signed-off-by: Dong Jia Shi
Reviewed-by: Halil Pasic
---
hw/s390x/css.c| 2 +-
A successful completion of rchp should signal a solicited channel path
initialized CRW (channel report word), while the current implementation
always generates an un-solicited one. Let's fix this.
Reported-by: Halil Pasic
Signed-off-by: Dong Jia Shi
Reviewed-by: Halil Pasic
---
hw/s390x/
sage update.
Patch #2:
Rename the new added parameter to more speaking name -- solicited.
Dong Jia Shi (2):
s390x/css: use macro for event-information pending error recover code
s390x/css: generate solicited crw for rchp completion signaling
hw/s390x/css.c| 16 ++--
i
* Halil Pasic [2017-08-01 17:16:37 +0200]:
>
>
> On 08/01/2017 09:57 AM, Dong Jia Shi wrote:
> [..]
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -1745,10 +1745,10 @@ int css_do_rchp(uint8_t cssid, uint8_t chpid)
> > }
> >
> >
* Halil Pasic [2017-08-01 17:24:10 +0200]:
>
>
> On 08/01/2017 09:57 AM, Dong Jia Shi wrote:
> > Let's use a macro for the ERC (error recover code) when generating a
> > Channel Subsystem Event-information pending CRW (channel report word).
> >
> > While
A successful completion of rchp should signal a solicited channel path
initialized CRW (channel report word), while the current implementation
always generates an un-solicited one. Let's fix this.
Reported-by: Halil Pasic
Signed-off-by: Dong Jia Shi
---
hw/s390x/css.c
This series is trying to:
1. clear up ERC related code
2. bugfix for channel path related CRW generation
Change log
--
v1->v2:
Patch #1:
Add all ERCs.
Commit message update.
Patch #2:
Rename the new added parameter to more speaking name -- solicited.
Dong Jia Shi (2):
s390x/
Let's use a macro for the ERC (error recover code) when generating a
Channel Subsystem Event-information pending CRW (channel report word).
While we are at it, let's also add all other ERCs.
Signed-off-by: Dong Jia Shi
---
hw/s390x/css.c| 2 +-
include/hw/s390x/ioi
* Cornelia Huck [2017-08-01 09:24:20 +0200]:
> On Tue, 1 Aug 2017 10:29:10 +0800
> Dong Jia Shi wrote:
>
> > * Cornelia Huck [2017-07-31 13:13:02 +0200]:
> >
> > > On Mon, 31 Jul 2017 11:51:37 +0800
> > > Dong Jia Shi wrote:
>
> > >
* Cornelia Huck [2017-07-31 13:13:02 +0200]:
> On Mon, 31 Jul 2017 11:51:37 +0800
> Dong Jia Shi wrote:
>
> > * Cornelia Huck [2017-07-27 13:59:10 +0200]:
> >
> > > On Thu, 27 Jul 2017 03:54:18 +0200
> > > Dong Jia Shi wrote:
> > >
>
* Cornelia Huck [2017-07-31 10:54:47 +0200]:
> On Fri, 28 Jul 2017 23:50:48 +0800
> Dong Jia Shi wrote:
>
> > * Cornelia Huck [2017-07-28 13:53:01 +0200]:
>
> > > > > You're bound to get different kinds of notifications: via a CRW with
> > >
* Halil Pasic [2017-07-31 14:30:32 +0200]:
>
>
> On 07/31/2017 01:13 PM, Cornelia Huck wrote:
> > On Mon, 31 Jul 2017 11:51:37 +0800
> > Dong Jia Shi wrote:
> >
> >> * Cornelia Huck [2017-07-27 13:59:10 +0200]:
> >>
> >>> On
* Cornelia Huck [2017-07-31 10:41:47 +0200]:
> On Mon, 31 Jul 2017 09:46:17 +0800
> Dong Jia Shi wrote:
>
> > * Cornelia Huck [2017-07-28 14:58:19 +0200]:
>
> > > Exposing real channel paths to the guest means that the guest OS needs
> > > to be able t
* Cornelia Huck [2017-07-27 13:59:10 +0200]:
> On Thu, 27 Jul 2017 03:54:18 +0200
> Dong Jia Shi wrote:
>
> > When a channel path is hot plugged into a CSS, we should generate
> > a channel path initialized CRW (channel report word). The current
> > code does not
support a guest OS that
> does not also run under LPAR, I'd prefer that way.
>
My poor English... Sorry, I don't undersatnd the last sentence...
[...]
--
Dong Jia Shi
case CRW_ERC_INIT:
> > if (!chp_is_registered(chpid))
> > chp_new(chpid);
> > chsc_chp_online(chpid);
> >
> > Notice:
> > At the very beginning, I replaced CRW_ERC_IPARM with CRW_ERC_INIT. But
> > Sebstian Ott suggested:
> > "I don't know of a machine that actually implements a CRW
> > at all when a chpid is configured online on the SE/HMC.
> >
> > Because of potential regressions I don't want to remove CRW_ERC_IPARM
> > here. I'm good with adding CRW_ERC_INIT though."
>
> Yeah, that makes sense, especially with the confusing state channel
> path machine check handling is in from the architecture side.
>
> >
> > >
> > > I'll double check with how I'd interpret the PoP today.
> > >
> > Thanks.
>
> I have read through the PoP and the outcome is a bit disappointing.
> Much of it is a bit vague. I still think that you can err on the side
> of overindication, though.
>
I agree. Unless somebody tells me it's forbidden by the PoP explicitly,
or it will break Linux guest from working properly, I will would this as
the way to go.
--
Dong Jia Shi
* Cornelia Huck [2017-07-27 11:46:03 +0200]:
> On Thu, 27 Jul 2017 03:54:15 +0200
> Dong Jia Shi wrote:
>
> > This series is trying to:
> > 1. clear up CRW related code.
> > 2. generate the right channel path related CRW at the right time.
> >
> > I did
gt; -void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid);
> > +void css_queue_crw(uint8_t rsc, uint8_t erc, int s, int chain, uint16_t
> > rsid);
> > void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
> > int hotplugged, int add);
> > void css_generate_chp_crws(uint8_t cssid, uint8_t chpid);
>
> Otherwise, patch looks good.
Thanks.
So, I only need to s/s/solicited for the new version of this one?
>
--
Dong Jia Shi
it */
#define CRW_ERC_PERRI0x07 /* perm. error, facility init */
#define CRW_ERC_PMOD 0x08 /* installed parameters modified */
Want the comment or not? I like them.
> >
> > #define CRW_RSC_SUBCH 0x3
> > #define CRW_RSC_CHP 0x4
>
--
Dong Jia Shi
commit message of that change relies the
> changes done in this patch.
Nothing I could comment. So leave this to Conny.
>
> v1 -> v2
> * fixed condition (precedence was wrong -- thanks Dong Jia)
> * check on each iteration when chaining (every ccw of the channel
> program
ic int css_interpret_ccw(SubchDev *sch, hwaddr
> > ccw_addr,
> > ret = -EINVAL;
> > break;
> > }
> > -if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
> > +if (ccw.flags || ccw.count) {
> > +/* We have already sa
1 - 100 of 374 matches
Mail list logo