On Wed, May 24, 2023 at 07:18:56PM +0000, Parav Pandit wrote:
>
> > From: Michael S. Tsirkin <[email protected]>
> > Sent: Wednesday, May 24, 2023 6:08 AM
> >
> > On Tue, May 23, 2023 at 10:22:27PM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <[email protected]>
> > > > 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?
> >
> > I don't know- it doesn't? If it did, it would be hard as we learned when
> > trying to
> > support pci-bridge-seat. Doable, but annoying.
> If it does today, something likely is missing in that OS. Their model for
> sriov has not evolved as much as Linux from what I see.
> So not to worry about it, as they may have to implement more things.
>
> >
> > > > > > 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's important for windows to work well with virtio. And it does because we
> > supply an install disk with virtio drivers through QEMU
>
> Ok. So WDM can use their IPR model or will be able to build infra as/if
> needed.
>
> > > 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 think I prefer Method-1, I didn't exactly see Method-2 to judge though.
> >
> Ok. so lets step forward in v3 using 4 commands for registers access.
> 2 RW commands for common config.
> 2 RW commands for device config.
>
> Notification on below.
>
> >
> > > > > 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.
> >
> > I am saying a simple thing actually:
> > - notifications are through VF
> > - rest of config is through PF (AQ)
> > seems like a consistent model to me.
> >
> Sounds good to me.
> What about discovery the notification place for the VF?
> We have two options.
> 1. discover them via AQ like config registers access
> 2. discover it via extended PCI capability
>
> #1 is far more programable and does not need to bake in the PCI device layout.
I think 2 is easier for drivers to use.
> So this way one day in future we can get rid of it more easily for those PCI
> devices who may prefer everything in the hw.
I think these devices are better off with a new transport using
drastically less registers.
> > > > 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.
> >
> > Hmm yes annoying I forgot about this.
> >
>
> > > 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.
> >
> > a feels ok to me... maybe test the waters with express capability as well,
> > it will
> > come handy down the road I think.
> Yes, if this is really needed for some OS, this addition will be possible in
> future in the spec and also of the devices in CC list here.
hmm not sure what do you mean. I meant to put the capabilities
with the new legacy bar in express config space.
> >
> > > > 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.
> >
> > yes, no need to repeat. I think what you are missing is that I am proposing
> > this
> > as an intermediate step while we work on support for notifications, or even
> > a
> > patch in the patchset.
> >
> Like said above, share your thoughts on the two options and I will modify
> this patch or add new in v3.
> >
> > I think what he meant is that tvq project will be reworked on top of AQ.
> > tvq,
> > indeed, does expose a full transport on top of AQ and it seems logical to
> > try and
> > add legacy guest support to that.
> Yes, I also acked that for SIOV devices to have legacy support on top of base
> SIOV device interface.
>
> > > > 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?
> >
> > exactly. bar value is currently 0-6 other values are reserved.
> > we can steal e.g. 7 and say this is legacy IOBAR.
> > The advantage is that we can use this IOBAR for anything at all, e.g.
> > VIRTIO_PCI_CAP_DEVICE_CFG too.
> >
> > > 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.
> >
> > it's not a big deal, if you prefer a new capability fine.
> >
> > > >
> > > > 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.
> >
> > That's good then. It's not always clear what you want to do, this is the
> > problem
> > with high level discussion as opposed to a specific patch.
> >
> > > > 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.
> >
> > Oh. You are saying access to capability previously indicated modern and now
> > it
> > is also done for legacy? That indeed is a good point.
> > Not sure what to do here. I'll ponder a bit.
>
> The new capability in mind is:
> VIRTIO_PCI_EXT_CAP_PCI_LEGACY_REG
>
> struct virtio_pci_legacy_cfg_cap {
> struct virtio_pci_cap cap;
> le32 addr; /* register offset, width, op=RD/WR, status:0=pending,1=done)
> le32 reg_data;
> };
I would use cap64, cap is there because we did not think about 64 bit early
enough.
> This will be hard to do for SIOV in future, so I prefer AQ cmd for config
> register access and notification discovery.
> WDYT?
I need to go back and look at how does SIOV tvq proposal manage
notifications then. Do not remember off the top of my head.
--
MST
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
