Re: [PATCH vfio 03/11] virtio-pci: Introduce admin virtqueue

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

On Wed, Sep 27, 2023 at 02:12:24PM -0400, Feng Liu wrote:
> 
> 
> On 2023-09-26 p.m.3:23, Feng Liu via Virtualization wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On 2023-09-21 a.m.9:57, Michael S. Tsirkin wrote:
> > > External email: Use caution opening links or attachments
> > > 
> > > 
> > > On Thu, Sep 21, 2023 at 03:40:32PM +0300, Yishai Hadas wrote:
> > > > From: Feng Liu <feliu@nvidia.com>
> 
> 
> > > >   drivers/virtio/virtio_pci_modern_avq.c | 65 ++++++++++++++++++++++++++
> > > 
> > > if you have a .c file without a .h file you know there's something
> > > fishy. Just add this inside drivers/virtio/virtio_pci_modern.c ?
> > > 
> > Will do.
> > 
> 
> > > > +struct virtio_avq {
> > > 
> > > admin_vq would be better. and this is pci specific yes? so virtio_pci_
> > > 
> > 
> > Will do.
> > 
> 
> > > > 
> > > > +     struct virtio_avq *admin;
> > > 
> > > and this could be thinkably admin_vq.
> > > 
> > Will do.
> > 
> 
> > > > 
> > > >   /* If driver didn't advertise the feature, it will never appear. */
> > > > diff --git a/include/linux/virtio_pci_modern.h
> > > > b/include/linux/virtio_pci_modern.h
> > > > index 067ac1d789bc..f6cb13d858fd 100644
> > > > --- a/include/linux/virtio_pci_modern.h
> > > > +++ b/include/linux/virtio_pci_modern.h
> > > > @@ -10,6 +10,9 @@ struct virtio_pci_modern_common_cfg {
> > > > 
> > > >        __le16 queue_notify_data;       /* read-write */
> > > >        __le16 queue_reset;             /* read-write */
> > > > +
> > > > +     __le16 admin_queue_index;       /* read-only */
> > > > +     __le16 admin_queue_num;         /* read-only */
> > > >   };
> > > 
> > > 
> > > ouch.
> > > actually there's a problem
> > > 
> > >          mdev->common = vp_modern_map_capability(mdev, common,
> > >                                        sizeof(struct
> > > virtio_pci_common_cfg), 4,
> > >                                        0, sizeof(struct
> > > virtio_pci_common_cfg),
> > >                                        NULL, NULL);
> > > 
> > > extending this structure means some calls will start failing on
> > > existing devices.
> > > 
> > > even more of an ouch, when we added queue_notify_data and queue_reset we
> > > also possibly broke some devices. well hopefully not since no one
> > > reported failures but we really need to fix that.
> > > 
> > > 
> > Hi Michael
> > 
> > I didn’t see the fail in vp_modern_map_capability(), and
> > vp_modern_map_capability() only read and map pci memory. The length of
> > the memory mapping will increase as the struct virtio_pci_common_cfg
> > increases. No errors are seen.
> > 
> > And according to the existing code, new pci configuration space members
> > can only be added in struct virtio_pci_modern_common_cfg.
> > 
> > Every single entry added here is protected by feature bit, there is no
> > bug AFAIK.
> > 
> > Could you help to explain it more detail?  Where and why it will fall if
> > we add new member in struct virtio_pci_modern_common_cfg.
> > 
> > 
> Hi, Michael
> 	Any comments about this?
> Thanks
> Feng

If an existing device exposes a small
capability matching old size, then you change size then
the check will fail on the existing device and driver won't load.

All this happens way before feature bit checks.


> > > > 
> > > >   struct virtio_pci_modern_device {
> > > > -- 
> > > > 2.27.0
> > > 
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization