Re: [PULL 1/1] vfio-ccw: Don't assume there are more ccws after a TIC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 20 Feb 2019 11:25:46 -0500
Eric Farman <farman@xxxxxxxxxxxxx> wrote:

> On 02/20/2019 07:44 AM, Cornelia Huck wrote:
> > On Wed, 20 Feb 2019 06:29:38 -0500
> > Eric Farman <farman@xxxxxxxxxxxxx> wrote:
> >   
> >> On 02/20/2019 04:48 AM, Cornelia Huck wrote:  
> >>> On Tue, 19 Feb 2019 21:49:07 -0500
> >>> Eric Farman <farman@xxxxxxxxxxxxx> wrote:
> >>>      
> >>>> Hi Connie, Farhan,
> >>>>
> >>>> On 02/04/2019 12:06 PM, Cornelia Huck wrote:  
> >>>>> From: Farhan Ali <alifm@xxxxxxxxxxxxx>
> >>>>>
> >>>>> When trying to calculate the length of a ccw chain, we assume
> >>>>> there are ccws after a TIC. This can lead to overcounting and
> >>>>> copying garbage data from guest memory.
> >>>>>
> >>>>> Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
> >>>>> Message-Id: <d63748c1f1b03147bcbf401596638627a5e35ef7.1548082107.git.alifm@xxxxxxxxxxxxx>
> >>>>> Reviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
> >>>>> Signed-off-by: Cornelia Huck <cohuck@xxxxxxxxxx>
> >>>>> ---
> >>>>>     drivers/s390/cio/vfio_ccw_cp.c | 2 +-
> >>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> >>>>> index 70a006ba4d05..ba08fe137c2e 100644
> >>>>> --- a/drivers/s390/cio/vfio_ccw_cp.c
> >>>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> >>>>> @@ -392,7 +392,7 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
> >>>>>     			return -EOPNOTSUPP;
> >>>>>     		}
> >>>>>     
> >>>>> -		if ((!ccw_is_chain(ccw)) && (!ccw_is_tic(ccw)))
> >>>>> +		if (!ccw_is_chain(ccw))  
> > 
> > OK, this function now returns the length of the chain excluding the
> > last tic.

Hm, not so sure about that anymore. I'm a bit tired, please apply salt
where needed to what I'm seeing.

> >   
> >>>>>     			break;
> >>>>>     
> >>>>>     		ccw++;
> >>>>>       
> > 
> > Now, cp_init will not copy the last tic to the chain. When it then
> > looks for tics in that new chain, it won't find any, and stop copying.  
(...)
> >>>> I'm not going to claim to know exactly how this is failing,
> >>>> because it's too late for coffee and reading IPL records is
> >>>> difficult enough.  But in the working case (without this patch),
> >>>> cp_init() calls ccwchain_calc_length() and gets a count of four
> >>>> CCWs (SEEK + SIDE + TIC
> >>>> + READ), which is broken up into two chains.  One for the
> >>>> SEEK/SIDE/TIC, and another for the READ.  In the failing case
> >>>> (with this patch), cp_init() only counts three CCWs, we never read
> >>>> the IPL2 address (the second chain in the working case), and our
> >>>> boot fails.  
> > 
> > I think "we don't copy the tic to the chain where we search for tics"
> > fits those symptoms.  
> 
> Indeed.  Hrm...
(...)
> > See above. We may need two chains: One without the trailing tic, and
> > one to process to see where that tic points to... or rework the way to
> > follow the tic? Not sure.  
> 
> I hate the idea of two chains, because how much storage do we end up 
> needing to consume?  But maybe it's inevitable, or maybe I can think up 
> a way to rework it without.

Yeah, that sucks. If we really need to track tics separately (and I'm
not so sure, see above), there should be a better way for that...

(...)

> > I'm wondering whether we should keep this patch and fix on top of it,
> > or revert it for now... I'm not sure tic processing is working at all
> > right now.  
> 
> I've been wondering this too.  Yeah, the bios code isn't merged yet, but 
> this patch means there's no point in merging it until we get TIC fixed 
> properly.  Which I'd hope is 5.1, but I'm not sure how deep this bowl of 
> spaghetti is.

We also need to make sure that this is not a bug in the bios instead...

> 
> > 
> > Maybe we need a tool for testing that throws random channel programs at
> > the device :)
> >   
> 
> Are you peeking at my todo/wish list?  :)

:)

Might be something for kvm unit tests? Or do I misunderstand what they
can do?



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux