Re: [RFC 1/2] vhost: IFC VF hardware operation layer

[Jason Wang]

On 2019/10/23 äå6:13, Simon Horman wrote:
> On Tue, Oct 22, 2019 at 09:32:36AM +0800, Jason Wang wrote:
> > On 2019/10/22 äå12:31, Simon Horman wrote:
> > > On Mon, Oct 21, 2019 at 05:55:33PM +0800, Zhu, Lingshan wrote:
> > > > On 10/16/2019 5:53 PM, Simon Horman wrote:
> > > > > Hi Zhu,
> > > > >
> > > > > thanks for your patch.
> > > > >
> > > > > On Wed, Oct 16, 2019 at 09:10:40AM +0800, Zhu Lingshan wrote:
> > > ...
> > >
> > > > > > +static void ifcvf_read_dev_config(struct ifcvf_hw *hw, u64 offset,
> > > > > > +		       void *dst, int length)
> > > > > > +{
> > > > > > +	int i;
> > > > > > +	u8 *p;
> > > > > > +	u8 old_gen, new_gen;
> > > > > > +
> > > > > > +	do {
> > > > > > +		old_gen = ioread8(&hw->common_cfg->config_generation);
> > > > > > +
> > > > > > +		p = dst;
> > > > > > +		for (i = 0; i < length; i++)
> > > > > > +			*p++ = ioread8((u8 *)hw->dev_cfg + offset + i);
> > > > > > +
> > > > > > +		new_gen = ioread8(&hw->common_cfg->config_generation);
> > > > > > +	} while (old_gen != new_gen);
> > > > > Would it be wise to limit the number of iterations of the loop above?
> > > > Thanks but I don't quite get it. This is used to make sure the function
> > > > would get the latest config.
> > > I am worried about the possibility that it will loop forever.
> > > Could that happen?
> > >
> > > ...
> > My understanding is that the function here is similar to virtio config
> > generation [1]. So this can only happen for a buggy hardware.
> Ok, so this circles back to my original question.
> Should we put a bound on the number of times the loop runs
> or should we accept that the kernel locks up if the HW is buggy?

I'm not sure, and similar logic has been used by virtio-pci drivers for 
years. Consider this logic is pretty simple and it should not be the 
only place that virito hardware can lock kernel, we can keep it as is.

Actually, there's no need for hardware to implement generation logic, it 
could be emulated by software or even ignored. In new version of 
virtio-mdev, get_generation() is optional, when it was not implemented, 
0 is simply returned by virtio-mdev transport.

Thanks