Linux Virtualization - Apr 2023 - [PATCH resend 1/2] vdpa/snet: support getting and setting VQ state

Hi Jason,
> > +       /* Overwrite the control register with the new buffer size (in
4B words) */
> > +       snet_write_ctrl(regs, buf_words);
> > +       /* Use a memory barrier, this must be written before the
opcode register. */
> > +       wmb();
> 
> 
> At least you need to use smp_wmb() but if you want to serialize MMIO
> writes you can simply use spinlocks after the removal of mmiowb
> work[1].
> 
I'm not sure how a spinlock can help in this case.
The entire control mechanism is protected by the spinlock snet->ctrl_lock, so
multiple CPUs won't use it simultaneously.
> Note that Documentation/memory-barriers.txt said:
> 
>         1. All readX() and writeX() accesses to the same peripheral are
ordered
>            with respect to each other. This ensures that MMIO register
accesses
>            by the same CPU thread to a particular device will arrive in
program
>            order.
> 
So it will arrive to the pci subsystem in program order, but the pci subsystem
may reorder, right?
I can just issue a read after the writes, something like:

write_control
read_control
write op
read op

What do you think?
> > +       /* Clear the control register - clear the error code if
previous control operation failed */
> > +       snet_write_ctrl(regs, 0);
> > +
> > +       /* Write opcode and VQ idx */
> > +       val = opcode | (vq_idx << 16);
> > +       snet_write_op(regs, val);
> 
> I guess we need to serialize two writes here as well.
Same here.

Thanks

> I'm not sure how a spinlock can help in this case.
> The entire control mechanism is protected by the spinlock
snet->ctrl_lock, so multiple CPUs won't use it simultaneously.
Sorry, snet->ctrl_lock is a mutex, not a spinlock.

On Mon, Apr 3, 2023 at 5:18?PM Alvaro Karsz <alvaro.karsz at
solid-run.com> wrote:
>
> Hi Jason,
>
> > > +       /* Overwrite the control register with the new buffer
size (in 4B words) */
> > > +       snet_write_ctrl(regs, buf_words);
> > > +       /* Use a memory barrier, this must be written before the
opcode register. */
> > > +       wmb();
> >
> >
> > At least you need to use smp_wmb() but if you want to serialize MMIO
> > writes you can simply use spinlocks after the removal of mmiowb
> > work[1].
> >
>
> I'm not sure how a spinlock can help in this case.
> The entire control mechanism is protected by the spinlock
snet->ctrl_lock, so multiple CPUs won't use it simultaneously.
>
> > Note that Documentation/memory-barriers.txt said:
> >
> >         1. All readX() and writeX() accesses to the same peripheral
are ordered
> >            with respect to each other. This ensures that MMIO register
accesses
> >            by the same CPU thread to a particular device will arrive
in program
> >            order.
> >
>
> So it will arrive to the pci subsystem in program order, but the pci
subsystem may reorder, right?
This is not what I read from the above doc. It said "to a particular
device will arrive in program order".
> I can just issue a read after the writes, something like:
>
> write_control
> read_control
> write op
> read op
>
> What do you think?
This should work.

Thanks
>
> > > +       /* Clear the control register - clear the error code if
previous control operation failed */
> > > +       snet_write_ctrl(regs, 0);
> > > +
> > > +       /* Write opcode and VQ idx */
> > > +       val = opcode | (vq_idx << 16);
> > > +       snet_write_op(regs, val);
> >
> > I guess we need to serialize two writes here as well.
>
> Same here.
>
> Thanks
>
>