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 at 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>>> >>> ? struct virtio_pci_modern_device { >>> -- >>> 2.27.0 >> > _______________________________________________ > Virtualization mailing list > Virtualization at lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Michael S. Tsirkin
2023-Sep-27 21:27 UTC
[PATCH vfio 03/11] virtio-pci: Introduce admin virtqueue
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 at 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 > FengIf 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 at lists.linux-foundation.org > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization