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

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, May 23, 2023 2:49 PM
> > > iokit is from my previous work experience not virtio.
> > > For virtio we had a lot of trouble with hacks like this on windows.
> > I don't know why windows driver need to talk to PF driver until now when the
> AQ doesn't exists.
> 
> What is it you want to know?
>
Why a VF driver needs to talk to a PF driver in windows without AQ?
 
> > > That's an important platform.
> > >
> > I am not sure how important is it.
> > What I learnt that a decade has passed and even after that virtio drivers are
> not part of the OS distro.
> 
> Because with QEMU they are so easy to side-load. So we never bothered.
> 
I don't understand your comment.
I was saying to my knowledge a Windows OS do not have a virtio drivers in their distro which you said is important.

> > It doesn't need to because guest is anyway going to do whatever it does.
> 
> yes. But hypervisor can do work-arounds or detect broken functionality and
> stop cleanly.
>
You mentioned this several times.
Below requirement still holds fine.
 
> > The clear requirement is: to not fix legacy but to support legacy.
> > Fixing legacy is role of 1.x. Not the role of legacy interface itself using
> AQ/MMIO.
>
 > And for purpose of BAR mapping, such BAR in the VF can be mapped too.
> > You didn't answer why VF BAR is a problem and suggest PF BAR.
> 
> VF BAR by itself is not a problem. Discovering VF BAR mapping offset through PF
> is.
> 
At the end of the email you right that "AQ is fine" but here you write discovering that through the PF AQ is.
How to interpret your comment?

> > If notification for VF is in VF -> hence other config registers also in VF, such
> blanket statements does not help.
> 
> not other config registers. what's needed in VF driver should be retrievable
> through VF.
>
This applies to the modern device, for legacy emulation, I don't see this a mandatory.

> > Your comments and suggestions are helpful.
> > Some of them follow the loop that doesn't help.
> > Again, thanks a lot for the inputs and reviews.
> >
> > The design discussion helped to produce two solutions here than what was
> started in v0.
> 
> OK good. Now I suggest you make an effort to come up with some compromise
> that will make this thread stop. I really can't say I even remember what we said
> in the beginning.

> You had this idea of starting with a problem statement document how about
> doing that? Because I feel every time people bring up some idea and a problem
> this idea would solve, you go "this is out of scope". Let's decide what the scope
> is?
>
The requirements are simple and straightforward at start of the cover letter.
i.e. for quick reference in short: A PCI VF passthrough to guest VM to support legacy + 1.x.

I propose:
Method-1:
1. VF legacy registers access over AQ of PF
2. VF driver notifications using MMIO of VF

Method-2:
1. legacy registers access using two MMIO registers (data and addr)
2. VF driver notifications using MMIO of VF

> > I am asking a 1.x PCI device has the VF notification BAR already, why we don't
> use it, why to build an additional one in device?
> 
> Following this logic, a 1.x PCI device also has modern common config.
> All you really truly need is a flag to make it behave like legacy, from POV of
> device config and VQs.
> 
Dual definition to same area in device is not elegant way to achieve it.
We already discussed that some area is RW, some is not. It moves location.

And it still doesn't solve the problem of reset.
Hence, either doing via AQ or 2 registers method would work fine.

It keeps legacy also isolated from mainstream.

> So let's try to keep drivers self-contained as much as possible.
>
You said "AQ is fine" at the end of email.
Hard to understand your mixed suggestion in context of sending v3.

> > And notification also works well that reuses transitional device's notification.
> 
> I don't know what these are. You had some weird
> +struct virtio_admin_cmd_lq_notify_query_result {
> +       u8 bar; /* PCI BAR number of the member VF */
> +       u8 reserved[7];
> +       le64 offset; /* Byte offset within the BAR */ };
> if you want to re-use the normal notifications for VFs we already describe them
> using capabilities.
>
That capability is intermixed with 1.x register queue_notify_off.
So, I see following options.
a. either to create a new legacy specific extended capability, or 
b. say queue_notify_off doesn't apply for legacy notifications because this register doesn't exist for legacy
c. exchange it via the AQ like above struct.

> However, this creates a weird mix where there's this command that mostly
> follows legacy pci config layout except notifications which are there in the
> legacy pci config are actually somewhere else.  And this is just inconsistent and
> confusing, and the reason for this is implementation defined.
Depends on how you see it.
It is at same level of inconsistency as 1.x for a legit reason which is to split config registers from data path register.
 
It is not implementation defined. Notification over AQ cannot reach same perf level as MMIO.
I will avoid repeating it.

> 
> > 3 out of 4 functionalities are done on AQ of the TVQ.
> > So, it is time to stop asking to replicate the AQ functionality on some new
> TVQ.
> > This was discussed in depth.
> 
> I don't think I ever did that.
>
It is in Jason's reply when he was suggesting tvq in this same v2 series.
 
> My point however is the last sentence *If you need software in the middle then
> let's please make it robust.* If BAR0 hack does not work then let's use sane
> interfaces as much as possible.
>
 
> AQ works fine I think. This new one I haven't seen yet hard to say.
> Probably has its own set of problems ...
>
Ok. you review and we discuss after that.

> > Above example of legacy_register_addr and legacy register_data on the VF.
> 
> Talking about the two register thing.  We actually have this already:
> VIRTIO_PCI_CAP_PCI_CFG.  We could allow access to IO BAR through it.
> 
To which IOBAR? The one that doesn't exist in the VF?
It is weird to create VIRTIO_PCI_CAP_PCI_CFG and point to non-existing area.

We can have new extended capability as,

VIRTIO_PCI_CAP_PCI_LEGACY_REG, that has slightly better interface and it also avoid intermix with 1.x using above two registers.

> 
> But really since apparently you want to focus on how we solve notifications let's
> focus on that. I feel the main issue is poking at 1.x registers and at the same time
> at legacy registers.
No, it is not 1.x registers. Register and bar offset is shared via the AQ.

> The spec was written with a strong assumption that it is either or, never a mix.
Transitional device supposed to support both as written in the spec.
No different here.
Now for real device.

> Changing that will be painful.
> This is why I was asking about VIRTIO_NET_F_LEGACY_HEADER - legacy can live
> in software then and we do not need to worry about it in the spec. I got it that
> there's too much software here for your taste, but the problem with legacy is
> really that it was always ad hoc and implementation derived.
> So we need to keep it siloed.

This is why I am trying to avoid putting in PCI registers of the VF, but if you prefer, we can do using new VIRTIO_PCI_CAP_PCI_LEGACY_REG with two registers.