Re: [PATCH vhost v3 1/4] virtio: find_vqs: pass struct instead of multi parameters
On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin wrote: > > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote: > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang wrote: > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo > > > wrote: > > > > > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang > > > > wrote: > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo > > > > > wrote: > > > > > > > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang > > > > > > wrote: > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo > > > > > > > wrote: > > > > > > > > > > > > > > > > Now, we pass multi parameters to find_vqs. These parameters > > > > > > > > may work for transport or work for vring. > > > > > > > > > > > > > > > > And find_vqs has multi implements in many places: > > > > > > > > > > > > > > > > arch/um/drivers/virtio_uml.c > > > > > > > > drivers/platform/mellanox/mlxbf-tmfifo.c > > > > > > > > drivers/remoteproc/remoteproc_virtio.c > > > > > > > > drivers/s390/virtio/virtio_ccw.c > > > > > > > > drivers/virtio/virtio_mmio.c > > > > > > > > drivers/virtio/virtio_pci_legacy.c > > > > > > > > drivers/virtio/virtio_pci_modern.c > > > > > > > > drivers/virtio/virtio_vdpa.c > > > > > > > > > > > > > > > > Every time, we try to add a new parameter, that is difficult. > > > > > > > > We must change every find_vqs implement. > > > > > > > > > > > > > > > > One the other side, if we want to pass a parameter to vring, > > > > > > > > we must change the call path from transport to vring. > > > > > > > > Too many functions need to be changed. > > > > > > > > > > > > > > > > So it is time to refactor the find_vqs. We pass a structure > > > > > > > > cfg to find_vqs(), that will be passed to vring by transport. > > > > > > > > > > > > > > > > Because the vp_modern_create_avq() use the "const char > > > > > > > > *names[]", > > > > > > > > and the virtio_uml.c changes the name in the subsequent commit, > > > > > > > > so > > > > > > > > change the "names" inside the virtio_vq_config from "const char > > > > > > > > *const > > > > > > > > *names" to "const char **names". > > > > > > > > > > > > > > > > Signed-off-by: Xuan Zhuo > > > > > > > > Acked-by: Johannes Berg > > > > > > > > Reviewed-by: Ilpo J=E4rvinen > > > > > > > > > > > > > > The name seems broken here. > > > > > > > > > > > > Email APP bug. > > > > > > > > > > > > I will fix. > > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > typedef void vq_callback_t(struct virtqueue *); > > > > > > > > > > > > > > > > +/** > > > > > > > > + * struct virtio_vq_config - configure for find_vqs() > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set this > > > > > > > > to 0. > > > > > > > > + * During the initialization of each vq(vring setup), we > > > > > > > > need to know which > > > > > > > > + * item in the array should be used at that time. But > > > > > > > > since the item in > > > > > > > > + * names can be null, which causes some item of array to > > > > > > > > be skipped, we > > > > > > > > + * cannot use vq.index as the current id. So add a cfg_idx > > > > > > > > to let vring > > > > > > > > + * know how to get the current configuration from the > > > > > > > > array when > > > > > > > > + * initializing vq. > > > > > > > > > > > > > > So this design is not good. If it is not something that the driver > > > > > > > needs to care about, the core needs to hide it from the API. > > > > > > > > > > > > The driver just ignore it. That will be beneficial to the virtio > > > > > > core. > > > > > > Otherwise, we must pass one more parameter everywhere. > > > > > > > > > > I don't get here, it's an internal logic and we've already done that. > > > > > > > > > > > > ## Then these must add one param "cfg_idx"; > > > > > > > > struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev, > > > > unsigned int index, > > > > struct vq_transport_config > > > > *tp_cfg, > > > > struct virtio_vq_config *cfg, > > > > --> uint cfg_idx); > > > > > > > > struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev, > > > > unsigned int index, > > > > void *pages, > > > > struct vq_transport_config > > > > *tp_cfg, > > > > struct virtio_vq_config *cfg, > > > > --> uint cfg_idx); > > > > > > > > > > > > ## The functions inside virtio_ring also need to add a new param, such > > > > as: > > > > > > > > static struct virtqueue *vring_create_virtqueue_split(struct > > > > virtio_device *vdev, > > > > unsigned int > > > > index, > >
Re: [PATCH vhost v3 1/4] virtio: find_vqs: pass struct instead of multi parameters
On Wed, Mar 20, 2024 at 5:22 PM Jason Wang wrote: > > On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin wrote: > > > > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote: > > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang > > > wrote: > > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo > > > > wrote: > > > > > > > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang > > > > > wrote: > > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo > > > > > > wrote: > > > > > > > > > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang > > > > > > > wrote: > > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Now, we pass multi parameters to find_vqs. These parameters > > > > > > > > > may work for transport or work for vring. > > > > > > > > > > > > > > > > > > And find_vqs has multi implements in many places: > > > > > > > > > > > > > > > > > > arch/um/drivers/virtio_uml.c > > > > > > > > > drivers/platform/mellanox/mlxbf-tmfifo.c > > > > > > > > > drivers/remoteproc/remoteproc_virtio.c > > > > > > > > > drivers/s390/virtio/virtio_ccw.c > > > > > > > > > drivers/virtio/virtio_mmio.c > > > > > > > > > drivers/virtio/virtio_pci_legacy.c > > > > > > > > > drivers/virtio/virtio_pci_modern.c > > > > > > > > > drivers/virtio/virtio_vdpa.c > > > > > > > > > > > > > > > > > > Every time, we try to add a new parameter, that is difficult. > > > > > > > > > We must change every find_vqs implement. > > > > > > > > > > > > > > > > > > One the other side, if we want to pass a parameter to vring, > > > > > > > > > we must change the call path from transport to vring. > > > > > > > > > Too many functions need to be changed. > > > > > > > > > > > > > > > > > > So it is time to refactor the find_vqs. We pass a structure > > > > > > > > > cfg to find_vqs(), that will be passed to vring by transport. > > > > > > > > > > > > > > > > > > Because the vp_modern_create_avq() use the "const char > > > > > > > > > *names[]", > > > > > > > > > and the virtio_uml.c changes the name in the subsequent > > > > > > > > > commit, so > > > > > > > > > change the "names" inside the virtio_vq_config from "const > > > > > > > > > char *const > > > > > > > > > *names" to "const char **names". > > > > > > > > > > > > > > > > > > Signed-off-by: Xuan Zhuo > > > > > > > > > Acked-by: Johannes Berg > > > > > > > > > Reviewed-by: Ilpo J=E4rvinen > > > > > > > > > > > > > > > > The name seems broken here. > > > > > > > > > > > > > > Email APP bug. > > > > > > > > > > > > > > I will fix. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > > > typedef void vq_callback_t(struct virtqueue *); > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > + * struct virtio_vq_config - configure for find_vqs() > > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set > > > > > > > > > this to 0. > > > > > > > > > + * During the initialization of each vq(vring setup), we > > > > > > > > > need to know which > > > > > > > > > + * item in the array should be used at that time. But > > > > > > > > > since the item in > > > > > > > > > + * names can be null, which causes some item of array to > > > > > > > > > be skipped, we > > > > > > > > > + * cannot use vq.index as the current id. So add a > > > > > > > > > cfg_idx to let vring > > > > > > > > > + * know how to get the current configuration from the > > > > > > > > > array when > > > > > > > > > + * initializing vq. > > > > > > > > > > > > > > > > So this design is not good. If it is not something that the > > > > > > > > driver > > > > > > > > needs to care about, the core needs to hide it from the API. > > > > > > > > > > > > > > The driver just ignore it. That will be beneficial to the virtio > > > > > > > core. > > > > > > > Otherwise, we must pass one more parameter everywhere. > > > > > > > > > > > > I don't get here, it's an internal logic and we've already done > > > > > > that. > > > > > > > > > > > > > > > ## Then these must add one param "cfg_idx"; > > > > > > > > > > struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev, > > > > > unsigned int index, > > > > > struct vq_transport_config > > > > > *tp_cfg, > > > > > struct virtio_vq_config *cfg, > > > > > --> uint cfg_idx); > > > > > > > > > > struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev, > > > > > unsigned int index, > > > > > void *pages, > > > > > struct vq_transport_config > > > > > *tp_cfg, > > > > > struct virtio_vq_config *cfg, > > > > > --> uint cfg_idx); > >
Re: [PATCH vhost v3 1/4] virtio: find_vqs: pass struct instead of multi parameters
On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang wrote: > On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin wrote: > > > > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote: > > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang > > > wrote: > > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo > > > > wrote: > > > > > > > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang > > > > > wrote: > > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo > > > > > > wrote: > > > > > > > > > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang > > > > > > > wrote: > > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Now, we pass multi parameters to find_vqs. These parameters > > > > > > > > > may work for transport or work for vring. > > > > > > > > > > > > > > > > > > And find_vqs has multi implements in many places: > > > > > > > > > > > > > > > > > > arch/um/drivers/virtio_uml.c > > > > > > > > > drivers/platform/mellanox/mlxbf-tmfifo.c > > > > > > > > > drivers/remoteproc/remoteproc_virtio.c > > > > > > > > > drivers/s390/virtio/virtio_ccw.c > > > > > > > > > drivers/virtio/virtio_mmio.c > > > > > > > > > drivers/virtio/virtio_pci_legacy.c > > > > > > > > > drivers/virtio/virtio_pci_modern.c > > > > > > > > > drivers/virtio/virtio_vdpa.c > > > > > > > > > > > > > > > > > > Every time, we try to add a new parameter, that is difficult. > > > > > > > > > We must change every find_vqs implement. > > > > > > > > > > > > > > > > > > One the other side, if we want to pass a parameter to vring, > > > > > > > > > we must change the call path from transport to vring. > > > > > > > > > Too many functions need to be changed. > > > > > > > > > > > > > > > > > > So it is time to refactor the find_vqs. We pass a structure > > > > > > > > > cfg to find_vqs(), that will be passed to vring by transport. > > > > > > > > > > > > > > > > > > Because the vp_modern_create_avq() use the "const char > > > > > > > > > *names[]", > > > > > > > > > and the virtio_uml.c changes the name in the subsequent > > > > > > > > > commit, so > > > > > > > > > change the "names" inside the virtio_vq_config from "const > > > > > > > > > char *const > > > > > > > > > *names" to "const char **names". > > > > > > > > > > > > > > > > > > Signed-off-by: Xuan Zhuo > > > > > > > > > Acked-by: Johannes Berg > > > > > > > > > Reviewed-by: Ilpo J=E4rvinen > > > > > > > > > > > > > > > > The name seems broken here. > > > > > > > > > > > > > > Email APP bug. > > > > > > > > > > > > > > I will fix. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > > > typedef void vq_callback_t(struct virtqueue *); > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > + * struct virtio_vq_config - configure for find_vqs() > > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set > > > > > > > > > this to 0. > > > > > > > > > + * During the initialization of each vq(vring setup), we > > > > > > > > > need to know which > > > > > > > > > + * item in the array should be used at that time. But > > > > > > > > > since the item in > > > > > > > > > + * names can be null, which causes some item of array to > > > > > > > > > be skipped, we > > > > > > > > > + * cannot use vq.index as the current id. So add a > > > > > > > > > cfg_idx to let vring > > > > > > > > > + * know how to get the current configuration from the > > > > > > > > > array when > > > > > > > > > + * initializing vq. > > > > > > > > > > > > > > > > So this design is not good. If it is not something that the > > > > > > > > driver > > > > > > > > needs to care about, the core needs to hide it from the API. > > > > > > > > > > > > > > The driver just ignore it. That will be beneficial to the virtio > > > > > > > core. > > > > > > > Otherwise, we must pass one more parameter everywhere. > > > > > > > > > > > > I don't get here, it's an internal logic and we've already done > > > > > > that. > > > > > > > > > > > > > > > ## Then these must add one param "cfg_idx"; > > > > > > > > > > struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev, > > > > > unsigned int index, > > > > > struct vq_transport_config > > > > > *tp_cfg, > > > > > struct virtio_vq_config *cfg, > > > > > --> uint cfg_idx); > > > > > > > > > > struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev, > > > > > unsigned int index, > > > > > void *pages, > > > > > struct vq_transport_config > > > > > *tp_cfg, > > > > > struct virtio_vq_config *cfg, > > > > > --> uint cfg_idx); >
Re: [PATCH vhost v3 1/4] virtio: find_vqs: pass struct instead of multi parameters
On Wed, 20 Mar 2024 17:39:29 +0800, Xuan Zhuo wrote: > On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang wrote: > > On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin wrote: > > > > > > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote: > > > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang > > > > wrote: > > > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo > > > > > wrote: > > > > > > > > > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang > > > > > > wrote: > > > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo > > > > > > > wrote: > > > > > > > > > > > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang > > > > > > > > wrote: > > > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Now, we pass multi parameters to find_vqs. These parameters > > > > > > > > > > may work for transport or work for vring. > > > > > > > > > > > > > > > > > > > > And find_vqs has multi implements in many places: > > > > > > > > > > > > > > > > > > > > arch/um/drivers/virtio_uml.c > > > > > > > > > > drivers/platform/mellanox/mlxbf-tmfifo.c > > > > > > > > > > drivers/remoteproc/remoteproc_virtio.c > > > > > > > > > > drivers/s390/virtio/virtio_ccw.c > > > > > > > > > > drivers/virtio/virtio_mmio.c > > > > > > > > > > drivers/virtio/virtio_pci_legacy.c > > > > > > > > > > drivers/virtio/virtio_pci_modern.c > > > > > > > > > > drivers/virtio/virtio_vdpa.c > > > > > > > > > > > > > > > > > > > > Every time, we try to add a new parameter, that is > > > > > > > > > > difficult. > > > > > > > > > > We must change every find_vqs implement. > > > > > > > > > > > > > > > > > > > > One the other side, if we want to pass a parameter to vring, > > > > > > > > > > we must change the call path from transport to vring. > > > > > > > > > > Too many functions need to be changed. > > > > > > > > > > > > > > > > > > > > So it is time to refactor the find_vqs. We pass a structure > > > > > > > > > > cfg to find_vqs(), that will be passed to vring by > > > > > > > > > > transport. > > > > > > > > > > > > > > > > > > > > Because the vp_modern_create_avq() use the "const char > > > > > > > > > > *names[]", > > > > > > > > > > and the virtio_uml.c changes the name in the subsequent > > > > > > > > > > commit, so > > > > > > > > > > change the "names" inside the virtio_vq_config from "const > > > > > > > > > > char *const > > > > > > > > > > *names" to "const char **names". > > > > > > > > > > > > > > > > > > > > Signed-off-by: Xuan Zhuo > > > > > > > > > > Acked-by: Johannes Berg > > > > > > > > > > Reviewed-by: Ilpo J=E4rvinen > > > > > > > > > > > > > > > > > > The name seems broken here. > > > > > > > > > > > > > > > > Email APP bug. > > > > > > > > > > > > > > > > I will fix. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > typedef void vq_callback_t(struct virtqueue *); > > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > + * struct virtio_vq_config - configure for find_vqs() > > > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set > > > > > > > > > > this to 0. > > > > > > > > > > + * During the initialization of each vq(vring setup), > > > > > > > > > > we need to know which > > > > > > > > > > + * item in the array should be used at that time. But > > > > > > > > > > since the item in > > > > > > > > > > + * names can be null, which causes some item of array > > > > > > > > > > to be skipped, we > > > > > > > > > > + * cannot use vq.index as the current id. So add a > > > > > > > > > > cfg_idx to let vring > > > > > > > > > > + * know how to get the current configuration from the > > > > > > > > > > array when > > > > > > > > > > + * initializing vq. > > > > > > > > > > > > > > > > > > So this design is not good. If it is not something that the > > > > > > > > > driver > > > > > > > > > needs to care about, the core needs to hide it from the API. > > > > > > > > > > > > > > > > The driver just ignore it. That will be beneficial to the > > > > > > > > virtio core. > > > > > > > > Otherwise, we must pass one more parameter everywhere. > > > > > > > > > > > > > > I don't get here, it's an internal logic and we've already done > > > > > > > that. > > > > > > > > > > > > > > > > > > ## Then these must add one param "cfg_idx"; > > > > > > > > > > > > struct virtqueue *vring_create_virtqueue(struct virtio_device > > > > > > *vdev, > > > > > > unsigned int index, > > > > > > struct vq_transport_config > > > > > > *tp_cfg, > > > > > > struct virtio_vq_config > > > > > > *cfg, > > > > > > --> uint cfg_idx); > > > > > > > > > > > > struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev, > > > > > >
Re: [PATCH RESEND 1/1] um: oops on accessing a non-present page in the vmalloc area
On 3/19/2024 11:18 PM, Richard Weinberger wrote: > - Ursprüngliche Mail - >> Von: "Petr Tesarik" >> An: "richard" , "anton ivanov" >> , "Johannes Berg" >> , "linux-um" , >> "linux-kernel" >> CC: "Roberto Sassu" , "petr" >> >> Gesendet: Montag, 18. März 2024 14:09:07 >> Betreff: Re: [PATCH RESEND 1/1] um: oops on accessing a non-present page in >> the vmalloc area > >> On 3/12/2024 4:07 PM, Petr Tesarik wrote: >>> On 2/23/2024 3:04 PM, Petr Tesarik wrote: From: Petr Tesarik If a segmentation fault is caused by accessing an address in the vmalloc area, check that the target page is present. Currently, if the kernel hits a guard page in the vmalloc area, UML blindly assumes that the fault is caused by a stale mapping and will be fixed by flush_tlb_kernel_vm(). Unsurprisingly, if the fault is caused by accessing a guard page, no mapping is created, and when the faulting instruction is restarted, it will cause exactly the same fault again, effectively creating an infinite loop. >>> >>> Ping. Any comment on this fix? >> >> I don't think I have seen a reply from you. If you did comment, then >> your email has not reached me. >> >> Please, can you confirm you have seen my patch? > > Yes. I'm just way behind my maintainer schedule. :-/ Understood. Thank you for your reply. By the way, are you looking for more people to help with the amount of work? Petr T
Re: [PATCH RESEND 1/1] um: oops on accessing a non-present page in the vmalloc area
- Ursprüngliche Mail - > Von: "Petr Tesarik" >> Yes. I'm just way behind my maintainer schedule. :-/ > > Understood. Thank you for your reply. > > By the way, are you looking for more people to help with the amount of work? Yes, help is always welcome! Johannes and Anton do already a great job but more maintenance power is always good. You could help with reviewing patches, testing stuff, etc. :-) It's not that UML itself is a lot of work, it's just that $dayjob is not UML related at all... Thanks, //richard
Re: [PATCH RESEND 1/1] um: oops on accessing a non-present page in the vmalloc area
On 20/03/2024 14:09, Richard Weinberger wrote: - Ursprüngliche Mail - Von: "Petr Tesarik" Yes. I'm just way behind my maintainer schedule. :-/ Understood. Thank you for your reply. By the way, are you looking for more people to help with the amount of work? Yes, help is always welcome! Johannes and Anton do already a great job but more maintenance power is always good. You could help with reviewing patches, testing stuff, etc. :-) It's not that UML itself is a lot of work, it's just that $dayjob is not UML related at all... Same here. Thanks, //richard -- Anton R. Ivanov Cambridgegreys Limited. Registered in England. Company Number 10273661 https://www.cambridgegreys.com/
Re: [PATCH vhost v3 1/4] virtio: find_vqs: pass struct instead of multi parameters
On Wed, Mar 20, 2024 at 6:07 PM Xuan Zhuo wrote: > > On Wed, 20 Mar 2024 17:39:29 +0800, Xuan Zhuo > wrote: > > On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang wrote: > > > On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin > > > wrote: > > > > > > > > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote: > > > > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang > > > > > wrote: > > > > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo > > > > > > wrote: > > > > > > > > > > > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang > > > > > > > wrote: > > > > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang > > > > > > > > > wrote: > > > > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > Now, we pass multi parameters to find_vqs. These > > > > > > > > > > > parameters > > > > > > > > > > > may work for transport or work for vring. > > > > > > > > > > > > > > > > > > > > > > And find_vqs has multi implements in many places: > > > > > > > > > > > > > > > > > > > > > > arch/um/drivers/virtio_uml.c > > > > > > > > > > > drivers/platform/mellanox/mlxbf-tmfifo.c > > > > > > > > > > > drivers/remoteproc/remoteproc_virtio.c > > > > > > > > > > > drivers/s390/virtio/virtio_ccw.c > > > > > > > > > > > drivers/virtio/virtio_mmio.c > > > > > > > > > > > drivers/virtio/virtio_pci_legacy.c > > > > > > > > > > > drivers/virtio/virtio_pci_modern.c > > > > > > > > > > > drivers/virtio/virtio_vdpa.c > > > > > > > > > > > > > > > > > > > > > > Every time, we try to add a new parameter, that is > > > > > > > > > > > difficult. > > > > > > > > > > > We must change every find_vqs implement. > > > > > > > > > > > > > > > > > > > > > > One the other side, if we want to pass a parameter to > > > > > > > > > > > vring, > > > > > > > > > > > we must change the call path from transport to vring. > > > > > > > > > > > Too many functions need to be changed. > > > > > > > > > > > > > > > > > > > > > > So it is time to refactor the find_vqs. We pass a > > > > > > > > > > > structure > > > > > > > > > > > cfg to find_vqs(), that will be passed to vring by > > > > > > > > > > > transport. > > > > > > > > > > > > > > > > > > > > > > Because the vp_modern_create_avq() use the "const char > > > > > > > > > > > *names[]", > > > > > > > > > > > and the virtio_uml.c changes the name in the subsequent > > > > > > > > > > > commit, so > > > > > > > > > > > change the "names" inside the virtio_vq_config from > > > > > > > > > > > "const char *const > > > > > > > > > > > *names" to "const char **names". > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Xuan Zhuo > > > > > > > > > > > Acked-by: Johannes Berg > > > > > > > > > > > Reviewed-by: Ilpo J=E4rvinen > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The name seems broken here. > > > > > > > > > > > > > > > > > > Email APP bug. > > > > > > > > > > > > > > > > > > I will fix. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > typedef void vq_callback_t(struct virtqueue *); > > > > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > > + * struct virtio_vq_config - configure for find_vqs() > > > > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set > > > > > > > > > > > this to 0. > > > > > > > > > > > + * During the initialization of each vq(vring > > > > > > > > > > > setup), we need to know which > > > > > > > > > > > + * item in the array should be used at that time. > > > > > > > > > > > But since the item in > > > > > > > > > > > + * names can be null, which causes some item of > > > > > > > > > > > array to be skipped, we > > > > > > > > > > > + * cannot use vq.index as the current id. So add a > > > > > > > > > > > cfg_idx to let vring > > > > > > > > > > > + * know how to get the current configuration from > > > > > > > > > > > the array when > > > > > > > > > > > + * initializing vq. > > > > > > > > > > > > > > > > > > > > So this design is not good. If it is not something that the > > > > > > > > > > driver > > > > > > > > > > needs to care about, the core needs to hide it from the API. > > > > > > > > > > > > > > > > > > The driver just ignore it. That will be beneficial to the > > > > > > > > > virtio core. > > > > > > > > > Otherwise, we must pass one more parameter everywhere. > > > > > > > > > > > > > > > > I don't get here, it's an internal logic and we've already done > > > > > > > > that. > > > > > > > > > > > > > > > > > > > > > ## Then these must add one param "cfg_idx"; > > > > > > > > > > > > > > struct virtqueue *vring_create_virtqueue(struct virtio_device > > > > > > > *vdev, > > > > > > > unsigned int index, > > > > > > >
Re: [PATCH vhost v3 1/4] virtio: find_vqs: pass struct instead of multi parameters
On Wed, Mar 20, 2024 at 5:41 PM Xuan Zhuo wrote: > > On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang wrote: > > On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin wrote: > > > > > > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote: > > > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang > > > > wrote: > > > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo > > > > > wrote: > > > > > > > > > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang > > > > > > wrote: > > > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo > > > > > > > wrote: > > > > > > > > > > > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang > > > > > > > > wrote: > > > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Now, we pass multi parameters to find_vqs. These parameters > > > > > > > > > > may work for transport or work for vring. > > > > > > > > > > > > > > > > > > > > And find_vqs has multi implements in many places: > > > > > > > > > > > > > > > > > > > > arch/um/drivers/virtio_uml.c > > > > > > > > > > drivers/platform/mellanox/mlxbf-tmfifo.c > > > > > > > > > > drivers/remoteproc/remoteproc_virtio.c > > > > > > > > > > drivers/s390/virtio/virtio_ccw.c > > > > > > > > > > drivers/virtio/virtio_mmio.c > > > > > > > > > > drivers/virtio/virtio_pci_legacy.c > > > > > > > > > > drivers/virtio/virtio_pci_modern.c > > > > > > > > > > drivers/virtio/virtio_vdpa.c > > > > > > > > > > > > > > > > > > > > Every time, we try to add a new parameter, that is > > > > > > > > > > difficult. > > > > > > > > > > We must change every find_vqs implement. > > > > > > > > > > > > > > > > > > > > One the other side, if we want to pass a parameter to vring, > > > > > > > > > > we must change the call path from transport to vring. > > > > > > > > > > Too many functions need to be changed. > > > > > > > > > > > > > > > > > > > > So it is time to refactor the find_vqs. We pass a structure > > > > > > > > > > cfg to find_vqs(), that will be passed to vring by > > > > > > > > > > transport. > > > > > > > > > > > > > > > > > > > > Because the vp_modern_create_avq() use the "const char > > > > > > > > > > *names[]", > > > > > > > > > > and the virtio_uml.c changes the name in the subsequent > > > > > > > > > > commit, so > > > > > > > > > > change the "names" inside the virtio_vq_config from "const > > > > > > > > > > char *const > > > > > > > > > > *names" to "const char **names". > > > > > > > > > > > > > > > > > > > > Signed-off-by: Xuan Zhuo > > > > > > > > > > Acked-by: Johannes Berg > > > > > > > > > > Reviewed-by: Ilpo J=E4rvinen > > > > > > > > > > > > > > > > > > The name seems broken here. > > > > > > > > > > > > > > > > Email APP bug. > > > > > > > > > > > > > > > > I will fix. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > typedef void vq_callback_t(struct virtqueue *); > > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > + * struct virtio_vq_config - configure for find_vqs() > > > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set > > > > > > > > > > this to 0. > > > > > > > > > > + * During the initialization of each vq(vring setup), > > > > > > > > > > we need to know which > > > > > > > > > > + * item in the array should be used at that time. But > > > > > > > > > > since the item in > > > > > > > > > > + * names can be null, which causes some item of array > > > > > > > > > > to be skipped, we > > > > > > > > > > + * cannot use vq.index as the current id. So add a > > > > > > > > > > cfg_idx to let vring > > > > > > > > > > + * know how to get the current configuration from the > > > > > > > > > > array when > > > > > > > > > > + * initializing vq. > > > > > > > > > > > > > > > > > > So this design is not good. If it is not something that the > > > > > > > > > driver > > > > > > > > > needs to care about, the core needs to hide it from the API. > > > > > > > > > > > > > > > > The driver just ignore it. That will be beneficial to the > > > > > > > > virtio core. > > > > > > > > Otherwise, we must pass one more parameter everywhere. > > > > > > > > > > > > > > I don't get here, it's an internal logic and we've already done > > > > > > > that. > > > > > > > > > > > > > > > > > > ## Then these must add one param "cfg_idx"; > > > > > > > > > > > > struct virtqueue *vring_create_virtqueue(struct virtio_device > > > > > > *vdev, > > > > > > unsigned int index, > > > > > > struct vq_transport_config > > > > > > *tp_cfg, > > > > > > struct virtio_vq_config > > > > > > *cfg, > > > > > > --> uint cfg_idx); > > > > > > > > > > > > struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev, > > > > > >
Re: [PATCH vhost v3 1/4] virtio: find_vqs: pass struct instead of multi parameters
On Thu, 21 Mar 2024 12:03:36 +0800, Jason Wang wrote: > On Wed, Mar 20, 2024 at 5:41 PM Xuan Zhuo wrote: > > > > On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang wrote: > > > On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin > > > wrote: > > > > > > > > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote: > > > > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang > > > > > wrote: > > > > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo > > > > > > wrote: > > > > > > > > > > > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang > > > > > > > wrote: > > > > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang > > > > > > > > > wrote: > > > > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > Now, we pass multi parameters to find_vqs. These > > > > > > > > > > > parameters > > > > > > > > > > > may work for transport or work for vring. > > > > > > > > > > > > > > > > > > > > > > And find_vqs has multi implements in many places: > > > > > > > > > > > > > > > > > > > > > > arch/um/drivers/virtio_uml.c > > > > > > > > > > > drivers/platform/mellanox/mlxbf-tmfifo.c > > > > > > > > > > > drivers/remoteproc/remoteproc_virtio.c > > > > > > > > > > > drivers/s390/virtio/virtio_ccw.c > > > > > > > > > > > drivers/virtio/virtio_mmio.c > > > > > > > > > > > drivers/virtio/virtio_pci_legacy.c > > > > > > > > > > > drivers/virtio/virtio_pci_modern.c > > > > > > > > > > > drivers/virtio/virtio_vdpa.c > > > > > > > > > > > > > > > > > > > > > > Every time, we try to add a new parameter, that is > > > > > > > > > > > difficult. > > > > > > > > > > > We must change every find_vqs implement. > > > > > > > > > > > > > > > > > > > > > > One the other side, if we want to pass a parameter to > > > > > > > > > > > vring, > > > > > > > > > > > we must change the call path from transport to vring. > > > > > > > > > > > Too many functions need to be changed. > > > > > > > > > > > > > > > > > > > > > > So it is time to refactor the find_vqs. We pass a > > > > > > > > > > > structure > > > > > > > > > > > cfg to find_vqs(), that will be passed to vring by > > > > > > > > > > > transport. > > > > > > > > > > > > > > > > > > > > > > Because the vp_modern_create_avq() use the "const char > > > > > > > > > > > *names[]", > > > > > > > > > > > and the virtio_uml.c changes the name in the subsequent > > > > > > > > > > > commit, so > > > > > > > > > > > change the "names" inside the virtio_vq_config from > > > > > > > > > > > "const char *const > > > > > > > > > > > *names" to "const char **names". > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Xuan Zhuo > > > > > > > > > > > Acked-by: Johannes Berg > > > > > > > > > > > Reviewed-by: Ilpo J=E4rvinen > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The name seems broken here. > > > > > > > > > > > > > > > > > > Email APP bug. > > > > > > > > > > > > > > > > > > I will fix. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > typedef void vq_callback_t(struct virtqueue *); > > > > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > > + * struct virtio_vq_config - configure for find_vqs() > > > > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set > > > > > > > > > > > this to 0. > > > > > > > > > > > + * During the initialization of each vq(vring > > > > > > > > > > > setup), we need to know which > > > > > > > > > > > + * item in the array should be used at that time. > > > > > > > > > > > But since the item in > > > > > > > > > > > + * names can be null, which causes some item of > > > > > > > > > > > array to be skipped, we > > > > > > > > > > > + * cannot use vq.index as the current id. So add a > > > > > > > > > > > cfg_idx to let vring > > > > > > > > > > > + * know how to get the current configuration from > > > > > > > > > > > the array when > > > > > > > > > > > + * initializing vq. > > > > > > > > > > > > > > > > > > > > So this design is not good. If it is not something that the > > > > > > > > > > driver > > > > > > > > > > needs to care about, the core needs to hide it from the API. > > > > > > > > > > > > > > > > > > The driver just ignore it. That will be beneficial to the > > > > > > > > > virtio core. > > > > > > > > > Otherwise, we must pass one more parameter everywhere. > > > > > > > > > > > > > > > > I don't get here, it's an internal logic and we've already done > > > > > > > > that. > > > > > > > > > > > > > > > > > > > > > ## Then these must add one param "cfg_idx"; > > > > > > > > > > > > > > struct virtqueue *vring_create_virtqueue(struct virtio_device > > > > > > > *vdev, > > > > > > > unsigned int index, > > > > > > >
Re: [PATCH RESEND 1/1] um: oops on accessing a non-present page in the vmalloc area
On Fri, 23 Feb 2024 at 22:07, Petr Tesarik wrote:
>
> From: Petr Tesarik
>
> If a segmentation fault is caused by accessing an address in the vmalloc
> area, check that the target page is present.
>
> Currently, if the kernel hits a guard page in the vmalloc area, UML blindly
> assumes that the fault is caused by a stale mapping and will be fixed by
> flush_tlb_kernel_vm(). Unsurprisingly, if the fault is caused by accessing
> a guard page, no mapping is created, and when the faulting instruction is
> restarted, it will cause exactly the same fault again, effectively creating
> an infinite loop.
>
> Signed-off-by: Petr Tesarik
> ---
> arch/um/kernel/trap.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> index 6d8ae86ae978..d5b85f1bfe33 100644
> --- a/arch/um/kernel/trap.c
> +++ b/arch/um/kernel/trap.c
> @@ -206,11 +206,15 @@ unsigned long segv(struct faultinfo fi, unsigned long
> ip, int is_user,
> int err;
> int is_write = FAULT_WRITE(fi);
> unsigned long address = FAULT_ADDRESS(fi);
> + pte_t *pte;
>
> if (!is_user && regs)
> current->thread.segv_regs = container_of(regs, struct
> pt_regs, regs);
>
> if (!is_user && (address >= start_vm) && (address < end_vm)) {
> + pte = virt_to_pte(&init_mm, address);
> + if (!pte_present(*pte))
> + page_fault_oops(regs, address, ip);
page_fault_oops() appears to be private to arch/x86/mm/fault.c, so
can't be used here?
Also, it accepts struct pt_regs*, not struct uml_pt_regs*, so would
need to at least handle the type difference here.
Could we equally avoid the infinite loop here by putting the
'flush_tlb_kernel_vm();goto out;' behind a if (pte_present(...))
check, and let the rest of the UML checks panic or oops if required.
(Actually OOPSing where we can under UML would be nice to do at some
point anyway, but is a bigger issue than just fixing a bug, IMO.)
Or am I lacking a prerequisite patch or applying this to the wrong
version (or otherwise missing something), as it definitely doesn't
build here.
Cheers,
-- David
smime.p7s
Description: S/MIME Cryptographic Signature
