Re: [virtio-comment] Re: [PATCH V2 6/6] virtio-pci: implement dirty page tracking

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

On Tue, Nov 07, 2023 at 05:43:44PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 11/6/2023 6:15 PM, Michael S. Tsirkin wrote:
> > On Mon, Nov 06, 2023 at 05:16:43PM +0800, Zhu, Lingshan wrote:
> > > 
> > > On 11/3/2023 10:21 PM, Zhu, Lingshan wrote:
> > > > 
> > > > On 11/3/2023 6:46 PM, Michael S. Tsirkin wrote:
> > > > > On Fri, Nov 03, 2023 at 06:34:37PM +0800, Zhu Lingshan wrote:
> > > > > > +\begin{lstlisting}
> > > > > > +struct virtio_pci_dity_page_track {
> > > > > > +        u8 enable;               /* Read-Write */
> > > > > > +        u8 gra_power;            /* Read-Write */
> > > > > > +        u8 reserved[2];
> > > > > > +        le32 {
> > > > > > +            pasid: 20;           /* Read-Write */
> > > > > > +            reserved: 12;
> > > > > > +        };
> > > > > > +        le64 bitmap_addr;        /* Read-Write */
> > > > > > +        le64 bitmap_length;      /* Read-Write */
> > > > > > +};
> > > > > > +\end{lstlisting}
> > > > > Okay, so it's a simple mailbox in config space.  Which by itself is
> > > > > probably a very reasonable idea - more or less what I suggested.
> > > > > However, using such a generic facility just for the dirty bitmap seems
> > > > > too limited.  Please make it accept arbitrary commands. Reusing admin
> > > > > command structure with a special "device itself" group sounds like one
> > > > > way to do it.
> > > > processing admin cmds in a cap may be too complex and overkill.
> > > > we need to handle variable length of cmds, handle async returned
> > > > results, and so on.
> > > > 
> > > > This struct seems easy and simple. And shall we use platform facilities
> > > > like vt-d
> > > > to track dirty pages?
> > > To demonstrate these issues, suppose we have a struct in a bar to process
> > > admin cmds:
> > > 
> > > struct virtio_admin_cmd {
> > >          u64 in_data_length;
> > >          u8 cmd_in_data[];
> > >          u64 out_data_length;
> > >          u8 cmd_out_data[];
> > >          u8 ret;
> > > };
> > An alternative is do same as you did here, e.g.:
> > struct virtio_admin_cmd {
> >           u64 admin_cmd_pa; /* an out descriptor followed by an in descriptor */
> PA depends on IOMMU and ATS

Not how virtio spec uses it. grep spec for physical address.


> >           u32 pasid : 20;
> > 	 u8 reserved : 11;
> > 	 u8 hardware : 1;
> > };
> > 
> > or we can stick two lengths and addresses straight in the capability.
> Still can only process only one cmd at a time and others are blocked.

Point being? Your patch is the same - thought you wanted simplicity?
We could stick a full queue there for sure - does this seem like
a good idea to you?

> > 
> > 
> > > The problems are:
> > > 1) command_in_data and command_out data have variable length, so how many HW
> > > resource should be reserved in the bar?
> > actually admin commands are truncated by device so just
> > set to length that device understands.
> How the framework know the length? How can the vendor know how many
> HW resource they should place here? I am not sure it is a good
> idea to guess.

I really don't want to repeat the whole admin command infrastructure
explanation I thought it's pretty clear. device knows the length
and truncates. Let's not get started on frameworks please.


> > 
> > > 2) To process the cmds in the bar, the device MAY need to read many
> > > registers in cmd_in_data[] and write many registers in cmd_out_data[],
> > > which can be ineffective, this is not DMA.
> > True. Again if you don't want to depend on pasid that's the
> > only option.
> Yes, this exactly shows how complex and overkill to use
> a cap to handle admin cmds. And maybe admin cmds is not a must.

I don't know. But this patch in question is really just impractical.
We can't implement silly one-off gateways for each new random thing.
You work on this thing so it looks like it's the most important feature
to you maybe, but virtio works very well for most people than you
very much.


> > 
> > 
> > > 3) a bar can only process one cmd at a time, and the driver can only issue
> > > another cmd after received an ret.
> > > This process has to be synchronous IO, one cmd blocks another.
> > Exactly same as what you did though.
> We have implemented a cap in PCI for dirty page tracking, there are no admin
> cmds.

"implemented" as in "posted v1 of a very rough draft". with tradeoffs
like increasing migration payload by a factor of x8. Or try to use
AtomicOps, if you try you will I believe see commands can actually
fail and so you need to report errors.

So maybe it's great for CXL. Should we be able to use platform
capabilities if there?  Of course. This is right in the charter.  Must
we limit ourselves to specific platforms with specific capabilities? if
there are tc members interested in supported limited platforms without
said capabilities, I don't see why we must.


> And for dirty page tracking, we still believe it is better to use platform
> facilities,
> as Jason explained.

So pls don't waste everyone's time trying to review stuff you don't think can work.


> > 
> > > 4) VF implementing a bar processing admin cmds conflicts with PF's admin vq.
> > So just don't create conflicts. It's same as multiple admin vqs
> > really which we already support.
> How to tell the SW don't create conflict?

It's like a single mutex in software or something no?
We already say 
	It is the responsibility of the driver to ensure
	strict request ordering for commands, because they will be
	consumed with no order constraints. 
seems enough for me.


> > 
> > 
> > > So I think a bar or a cap processing admin cmds is way to complex and
> > > overkill.
> > > 
> > > Thanks
> > Sounds like a straw man argument.
> >