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@xxxxxxxxxx>
>
>
> > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxxx
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
