RE: [virtio-comment] [RFC PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used buffer notification suppression mechanism

[Parav Pandit <parav@nvidia.com>]

> From: Lege Wang <lege.wang@jaguarmicro.com>
> Sent: Wednesday, January 31, 2024 12:51 PM
> To: Parav Pandit <parav@nvidia.com>; virtio-comment@lists.oasis-open.org
> Subject: RE: [virtio-comment] [RFC PATCH]
> VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used buffer notification
> suppression mechanism
> 
> hi,
> 
> 
> > >
> > > > And for that may be current notification can be made 64-bits or as
> > > > suggested in your patch to have new location.
> > > After some thoughts, I decide to just make current notification
> > > extended to 64-bits for simplicity.
> > I think having 2 32-bit notification registers is better.
> > Existing one for avail desc posting.
> > New 32-bit one for used buffer consumption and re-enablement of
> > notification.
> > This way we get good backward compatibility and also the optional behavior.
> > 3rd advantage of keeping them as two is when used buffer notification
> > enablement is done, it does not need to touch the device for avail posting.
> Yes, indeed this is what I have in mind, I did not clarify it clearly.
> These two 32-bits registers are adjacent, and both are 4-byte aligned.
> 
> >
> > In some systems, a 64-bit single PCIe TLP write may not be possible,
> > through rare, it may be there.
> > So having it as separate 32-bit used buffer notification is good.
> OK, I see.
> 
> >
> > > Yes, I understand your concerns. Initially I intended to add new PCI
> > > capability to describe this new notification area, but found that
> > > this new capability would be similar to virtio_pci_notify_cap, and
> > > will introduce many duplicate descriptions in virtio spec doc. If we
> > > choose to extent current notification
> > area,
> > > there're no needs to touch current pci capability, the corresponding
> > > queue notify address to enable used buffer notification within a BAR
> > > for a virtqueue would look like below:
> > >   cap.offset + queue_notify_off * notify_off_multiplier + 4
> > >
> > Just to double check,
> >
> > If avail buffer notification is at offset = X Used buffer notification
> > is at offset X + 4.
> > Right?
> Yes.
> 
> >
> > > > If this multiplier is not useful, maybe it should be added only if needed.
> > > > Did you add it because to match the current flexibility of the
> > > > spec, or your hw actually needs this multiplier?
> > > Yes, just to match the current spec ??
> > >
> > Ok. that explains.
> > Yes lets keep things simple as X, and X+4 offsets?
> Yes, I think so.
> 
Ok. Look forward to v1 with these small changes.
Thanks.