Re: [virtio-comment] RE: [PATCH v2 1/2] transport-pci: Introduce legacy registers access commands

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, May 17, 2023 at 07:32:33PM +0000, Parav Pandit wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, May 17, 2023 1:45 AM
> > 
> > On Sat, May 06, 2023 at 03:01:34AM +0300, Parav Pandit wrote:
> > > +\begin{lstlisting}
> > > +struct virtio_admin_cmd_lreg_wr_data {
> > > +	u8 offset; /* Starting byte offset of the register(s) to write */
> > > +	u8 register[];
> > > +};
> > > +\end{lstlisting}
> > 
> > So to summarize, I think the main comment here was to have separate
> > commands for access to common versus device config. Just today I thought
> > some more and came up with another reason why this is a good idea: if we ever
> > want legacy MMIO emulation, we can reuse the device config command.
> > 
> MMIO is actually simpler than PCI because all the registers are contiguous regardless of msix.
> So single command with offset can service just fine.

yes but then you need to implement both MMIO and PCI in hardware.
with a flag you can implement legacy MMIO in software on top of
legacy PCI in hardware.

> More below.
> 
> > Jason also feels the common config command can be shared with vq transport
> > effort. We can discuss this, for sure, I guess its putting code in hardware versus
> > in hypervisor, but if there are hardware vendors who strictly want this code in
> > hardware I don't have a big problem with this even if I don't exactly get why.
> > 
> > With this idea all hypervisor has to do is subtract the offset from device config
> > access, sounds like a small price to pay.  
> It is more than the subtraction cost.
> 
> Hypervisor needs to inspect MSI-X enable/disable settings to decide when to issue command #1 vs #3 for the requested offset.

I thought that for now you wanted to ignore MSI-X disable case?
Because if you do care then we need INTx emulation.


> > Does this sound like a reasonable
> > compromize to you?
> 
> Splitting proposed one command to two commands,
> 1. one for accessing legacy common config
> 2. second for accessing legacy device specific config 
> 
> seems fine to me as below.
> 
> So we will have total 5 commands (instead of 3).
> 
> 1. legacy common config read
> 2. legacy common config write
> 
> 3. legacy device config read
> 4. legacy device config write
> 5. query device notification area
> 
> #1 and #3 same cmd signature but different opcode.
> #2 and #4 same cmd signature but different opcode.
> 

Sounds reasonable. Jason?

notification thing needs more thought I feel though.
It feels weirdly bolted on, but I can't put my finger
on what's wrong exactly yet. Will think it over.


> 
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
> 
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
> 
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/