Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
On Mon, Sep 21, 2015 at 11:42:28AM +1000, David Gibson wrote: >On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote: >> On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote: >> > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: >> > > This allows to accept IOMMU group (PE) ID from the parameter from >> > > userland >> > > when handling EEH operation so that the operation only affects the target >> > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from >> > > userland >> > > is invalid, all IOMMU groups (PEs) attached to the specified container >> > > are >> > > affected as before. >> > > >> > > Gavin Shan (2): >> > > drivers/vfio: Support EEH API revision >> > > drivers/vfio: Support IOMMU group for EEH operations >> > > >> > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 >> > > ++--- >> > > drivers/vfio/vfio_spapr_eeh.c | 46 >> > > ++ >> > > include/linux/vfio.h| 13 +++--- >> > > include/uapi/linux/vfio.h | 6 + >> > > 4 files changed, 93 insertions(+), 22 deletions(-) >> > >> > This interface is terrible. A function named foo_enabled() should >> > return a bool, yes or no, don't try to overload it to also return a >> > version. >> >> Sorry, that one's my fault. I suggested that approach to Gavin >> without really thinking it through. >> >> >> > AFAICT, patch 2/2 breaks current users by changing the offset >> > of the union in struct vfio_eeh_pe_err. >> >> Yeah, this one's ugly. We have to preserve the offset, but that means >> putting the group in a very awkward place. Especially since I'm not >> sure if there even are any existing users of the single extant union >> branch. >> >> Sigh. >> Yeah, Perhaps, I should have put "RFC" on the subjects because those patches are really pre-mature and just intend to bring more discussion on it. >> > Also, we generally pass group >> > file descriptors rather than a group ID because we can prove the >> > ownership of the group through the file descriptor and we don't need to >> > worry about races with the group because we can hold a reference to it. > >Duh. I finally realised the better, simpler, obvious solution. > >Rather than changing the parameter structure, we should move the >ioctl()s so they're on the group fd instead of the container fd. > >Obviously we need to keep it on the container fd for backwards compat, >but I think we should just error out if there is more than one group >in the container there. > >We will need a new capability too, obviously. VFIO_EEH_GROUPFD maybe? > Yes, I agree to route EEH ioctl commands to group fd since EEH ioctl commands operate on granularity of PE (IOMMU group). However, it requires to extend current code to support that. I'm not sure if it's good idea as I explained to David through IRC. Waiting for Alex to judge: - Adding a callback to "struct vfio_group": platform_ioctl(); - When attaching the group to platform, this function is initialized; - The EEH ioctl commands are routed to platform_ioctl() in vfio_group_fops_unl_ioctl() Thanks, Gavin >-- >David Gibson | I'll have my music baroque, and my code >david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! >http://www.ozlabs.org/~dgibson -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
On Mon, Sep 21, 2015 at 11:42:28AM +1000, David Gibson wrote: >On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote: >> On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote: >> > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: >> > > This allows to accept IOMMU group (PE) ID from the parameter from >> > > userland >> > > when handling EEH operation so that the operation only affects the target >> > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from >> > > userland >> > > is invalid, all IOMMU groups (PEs) attached to the specified container >> > > are >> > > affected as before. >> > > >> > > Gavin Shan (2): >> > > drivers/vfio: Support EEH API revision >> > > drivers/vfio: Support IOMMU group for EEH operations >> > > >> > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 >> > > ++--- >> > > drivers/vfio/vfio_spapr_eeh.c | 46 >> > > ++ >> > > include/linux/vfio.h| 13 +++--- >> > > include/uapi/linux/vfio.h | 6 + >> > > 4 files changed, 93 insertions(+), 22 deletions(-) >> > >> > This interface is terrible. A function named foo_enabled() should >> > return a bool, yes or no, don't try to overload it to also return a >> > version. >> >> Sorry, that one's my fault. I suggested that approach to Gavin >> without really thinking it through. >> >> >> > AFAICT, patch 2/2 breaks current users by changing the offset >> > of the union in struct vfio_eeh_pe_err. >> >> Yeah, this one's ugly. We have to preserve the offset, but that means >> putting the group in a very awkward place. Especially since I'm not >> sure if there even are any existing users of the single extant union >> branch. >> >> Sigh. >> Yeah, Perhaps, I should have put "RFC" on the subjects because those patches are really pre-mature and just intend to bring more discussion on it. >> > Also, we generally pass group >> > file descriptors rather than a group ID because we can prove the >> > ownership of the group through the file descriptor and we don't need to >> > worry about races with the group because we can hold a reference to it. > >Duh. I finally realised the better, simpler, obvious solution. > >Rather than changing the parameter structure, we should move the >ioctl()s so they're on the group fd instead of the container fd. > >Obviously we need to keep it on the container fd for backwards compat, >but I think we should just error out if there is more than one group >in the container there. > >We will need a new capability too, obviously. VFIO_EEH_GROUPFD maybe? > Yes, I agree to route EEH ioctl commands to group fd since EEH ioctl commands operate on granularity of PE (IOMMU group). However, it requires to extend current code to support that. I'm not sure if it's good idea as I explained to David through IRC. Waiting for Alex to judge: - Adding a callback to "struct vfio_group": platform_ioctl(); - When attaching the group to platform, this function is initialized; - The EEH ioctl commands are routed to platform_ioctl() in vfio_group_fops_unl_ioctl() Thanks, Gavin >-- >David Gibson | I'll have my music baroque, and my code >david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! >http://www.ozlabs.org/~dgibson -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
On Mon, Sep 21, 2015 at 11:42:28AM +1000, David Gibson wrote: >On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote: >> On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote: >> > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: >> > > This allows to accept IOMMU group (PE) ID from the parameter from >> > > userland >> > > when handling EEH operation so that the operation only affects the target >> > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from >> > > userland >> > > is invalid, all IOMMU groups (PEs) attached to the specified container >> > > are >> > > affected as before. >> > > >> > > Gavin Shan (2): >> > > drivers/vfio: Support EEH API revision >> > > drivers/vfio: Support IOMMU group for EEH operations >> > > >> > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 >> > > ++--- >> > > drivers/vfio/vfio_spapr_eeh.c | 46 >> > > ++ >> > > include/linux/vfio.h| 13 +++--- >> > > include/uapi/linux/vfio.h | 6 + >> > > 4 files changed, 93 insertions(+), 22 deletions(-) >> > >> > This interface is terrible. A function named foo_enabled() should >> > return a bool, yes or no, don't try to overload it to also return a >> > version. >> >> Sorry, that one's my fault. I suggested that approach to Gavin >> without really thinking it through. >> >> >> > AFAICT, patch 2/2 breaks current users by changing the offset >> > of the union in struct vfio_eeh_pe_err. >> >> Yeah, this one's ugly. We have to preserve the offset, but that means >> putting the group in a very awkward place. Especially since I'm not >> sure if there even are any existing users of the single extant union >> branch. >> >> Sigh. >> >> > Also, we generally pass group >> > file descriptors rather than a group ID because we can prove the >> > ownership of the group through the file descriptor and we don't need to >> > worry about races with the group because we can hold a reference to it. > >Duh. I finally realised the better, simpler, obvious solution. > >Rather than changing the parameter structure, we should move the >ioctl()s so they're on the group fd instead of the container fd. > >Obviously we need to keep it on the container fd for backwards compat, >but I think we should just error out if there is more than one group >in the container there. > >We will need a new capability too, obviously. VFIO_EEH_GROUPFD maybe? > Yeah, the patches should be marked as "RFC" actually as they're actually prototypes. I agree with David that the EEH ioctl commands should be routed through IOMMU group as I proposed long time ago. However, if we're going to do it now, we have to maintain two set the interfaces: one handled by container's ioctl() and another one is handled by IOMMU group's ioctl(). Would it be a problem? Actually, the code change is made based on the fact: nobody is using the union (struct vfio_eeh_pe_err) yet before the QEMU changes to do error injection gets merged by David. So I think it's fine to introduce another field in struct vfio_eeh_pe_op though there is gap? Thanks, Gavin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
On Mon, Sep 21, 2015 at 11:42:28AM +1000, David Gibson wrote: >On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote: >> On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote: >> > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: >> > > This allows to accept IOMMU group (PE) ID from the parameter from >> > > userland >> > > when handling EEH operation so that the operation only affects the target >> > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from >> > > userland >> > > is invalid, all IOMMU groups (PEs) attached to the specified container >> > > are >> > > affected as before. >> > > >> > > Gavin Shan (2): >> > > drivers/vfio: Support EEH API revision >> > > drivers/vfio: Support IOMMU group for EEH operations >> > > >> > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 >> > > ++--- >> > > drivers/vfio/vfio_spapr_eeh.c | 46 >> > > ++ >> > > include/linux/vfio.h| 13 +++--- >> > > include/uapi/linux/vfio.h | 6 + >> > > 4 files changed, 93 insertions(+), 22 deletions(-) >> > >> > This interface is terrible. A function named foo_enabled() should >> > return a bool, yes or no, don't try to overload it to also return a >> > version. >> >> Sorry, that one's my fault. I suggested that approach to Gavin >> without really thinking it through. >> >> >> > AFAICT, patch 2/2 breaks current users by changing the offset >> > of the union in struct vfio_eeh_pe_err. >> >> Yeah, this one's ugly. We have to preserve the offset, but that means >> putting the group in a very awkward place. Especially since I'm not >> sure if there even are any existing users of the single extant union >> branch. >> >> Sigh. >> >> > Also, we generally pass group >> > file descriptors rather than a group ID because we can prove the >> > ownership of the group through the file descriptor and we don't need to >> > worry about races with the group because we can hold a reference to it. > >Duh. I finally realised the better, simpler, obvious solution. > >Rather than changing the parameter structure, we should move the >ioctl()s so they're on the group fd instead of the container fd. > >Obviously we need to keep it on the container fd for backwards compat, >but I think we should just error out if there is more than one group >in the container there. > >We will need a new capability too, obviously. VFIO_EEH_GROUPFD maybe? > Yeah, the patches should be marked as "RFC" actually as they're actually prototypes. I agree with David that the EEH ioctl commands should be routed through IOMMU group as I proposed long time ago. However, if we're going to do it now, we have to maintain two set the interfaces: one handled by container's ioctl() and another one is handled by IOMMU group's ioctl(). Would it be a problem? Actually, the code change is made based on the fact: nobody is using the union (struct vfio_eeh_pe_err) yet before the QEMU changes to do error injection gets merged by David. So I think it's fine to introduce another field in struct vfio_eeh_pe_op though there is gap? Thanks, Gavin -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
On Mon, 2015-09-21 at 22:11 +1000, Gavin Shan wrote: > On Mon, Sep 21, 2015 at 11:42:28AM +1000, David Gibson wrote: > >On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote: > >> On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote: > >> > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: > >> > > This allows to accept IOMMU group (PE) ID from the parameter from > >> > > userland > >> > > when handling EEH operation so that the operation only affects the > >> > > target > >> > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from > >> > > userland > >> > > is invalid, all IOMMU groups (PEs) attached to the specified container > >> > > are > >> > > affected as before. > >> > > > >> > > Gavin Shan (2): > >> > > drivers/vfio: Support EEH API revision > >> > > drivers/vfio: Support IOMMU group for EEH operations > >> > > > >> > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 > >> > > ++--- > >> > > drivers/vfio/vfio_spapr_eeh.c | 46 > >> > > ++ > >> > > include/linux/vfio.h| 13 +++--- > >> > > include/uapi/linux/vfio.h | 6 + > >> > > 4 files changed, 93 insertions(+), 22 deletions(-) > >> > > >> > This interface is terrible. A function named foo_enabled() should > >> > return a bool, yes or no, don't try to overload it to also return a > >> > version. > >> > >> Sorry, that one's my fault. I suggested that approach to Gavin > >> without really thinking it through. > >> > >> > >> > AFAICT, patch 2/2 breaks current users by changing the offset > >> > of the union in struct vfio_eeh_pe_err. > >> > >> Yeah, this one's ugly. We have to preserve the offset, but that means > >> putting the group in a very awkward place. Especially since I'm not > >> sure if there even are any existing users of the single extant union > >> branch. > >> > >> Sigh. > >> > >> > Also, we generally pass group > >> > file descriptors rather than a group ID because we can prove the > >> > ownership of the group through the file descriptor and we don't need to > >> > worry about races with the group because we can hold a reference to it. > > > >Duh. I finally realised the better, simpler, obvious solution. > > > >Rather than changing the parameter structure, we should move the > >ioctl()s so they're on the group fd instead of the container fd. > > > >Obviously we need to keep it on the container fd for backwards compat, > >but I think we should just error out if there is more than one group > >in the container there. > > > >We will need a new capability too, obviously. VFIO_EEH_GROUPFD maybe? > > > > Yeah, the patches should be marked as "RFC" actually as they're actually > prototypes. I agree with David that the EEH ioctl commands should be routed > through IOMMU group as I proposed long time ago. However, if we're going > to do it now, we have to maintain two set the interfaces: one handled by > container's ioctl() and another one is handled by IOMMU group's ioctl(). > Would it be a problem? > > Actually, the code change is made based on the fact: nobody is using > the union (struct vfio_eeh_pe_err) yet before the QEMU changes to do > error injection gets merged by David. So I think it's fine to introduce > another field in struct vfio_eeh_pe_op though there is gap? We really need to get away from this mindset of assuming that we know every user of the code and every dependency it may have. The reality is that this is an exposed ABI and we shouldn't break it just because we don't know of any users. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
On Mon, 2015-09-21 at 22:11 +1000, Gavin Shan wrote: > On Mon, Sep 21, 2015 at 11:42:28AM +1000, David Gibson wrote: > >On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote: > >> On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote: > >> > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: > >> > > This allows to accept IOMMU group (PE) ID from the parameter from > >> > > userland > >> > > when handling EEH operation so that the operation only affects the > >> > > target > >> > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from > >> > > userland > >> > > is invalid, all IOMMU groups (PEs) attached to the specified container > >> > > are > >> > > affected as before. > >> > > > >> > > Gavin Shan (2): > >> > > drivers/vfio: Support EEH API revision > >> > > drivers/vfio: Support IOMMU group for EEH operations > >> > > > >> > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 > >> > > ++--- > >> > > drivers/vfio/vfio_spapr_eeh.c | 46 > >> > > ++ > >> > > include/linux/vfio.h| 13 +++--- > >> > > include/uapi/linux/vfio.h | 6 + > >> > > 4 files changed, 93 insertions(+), 22 deletions(-) > >> > > >> > This interface is terrible. A function named foo_enabled() should > >> > return a bool, yes or no, don't try to overload it to also return a > >> > version. > >> > >> Sorry, that one's my fault. I suggested that approach to Gavin > >> without really thinking it through. > >> > >> > >> > AFAICT, patch 2/2 breaks current users by changing the offset > >> > of the union in struct vfio_eeh_pe_err. > >> > >> Yeah, this one's ugly. We have to preserve the offset, but that means > >> putting the group in a very awkward place. Especially since I'm not > >> sure if there even are any existing users of the single extant union > >> branch. > >> > >> Sigh. > >> > >> > Also, we generally pass group > >> > file descriptors rather than a group ID because we can prove the > >> > ownership of the group through the file descriptor and we don't need to > >> > worry about races with the group because we can hold a reference to it. > > > >Duh. I finally realised the better, simpler, obvious solution. > > > >Rather than changing the parameter structure, we should move the > >ioctl()s so they're on the group fd instead of the container fd. > > > >Obviously we need to keep it on the container fd for backwards compat, > >but I think we should just error out if there is more than one group > >in the container there. > > > >We will need a new capability too, obviously. VFIO_EEH_GROUPFD maybe? > > > > Yeah, the patches should be marked as "RFC" actually as they're actually > prototypes. I agree with David that the EEH ioctl commands should be routed > through IOMMU group as I proposed long time ago. However, if we're going > to do it now, we have to maintain two set the interfaces: one handled by > container's ioctl() and another one is handled by IOMMU group's ioctl(). > Would it be a problem? > > Actually, the code change is made based on the fact: nobody is using > the union (struct vfio_eeh_pe_err) yet before the QEMU changes to do > error injection gets merged by David. So I think it's fine to introduce > another field in struct vfio_eeh_pe_op though there is gap? We really need to get away from this mindset of assuming that we know every user of the code and every dependency it may have. The reality is that this is an exposed ABI and we shouldn't break it just because we don't know of any users. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote: > On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote: > > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: > > > This allows to accept IOMMU group (PE) ID from the parameter from userland > > > when handling EEH operation so that the operation only affects the target > > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from > > > userland > > > is invalid, all IOMMU groups (PEs) attached to the specified container are > > > affected as before. > > > > > > Gavin Shan (2): > > > drivers/vfio: Support EEH API revision > > > drivers/vfio: Support IOMMU group for EEH operations > > > > > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 > > > ++--- > > > drivers/vfio/vfio_spapr_eeh.c | 46 > > > ++ > > > include/linux/vfio.h| 13 +++--- > > > include/uapi/linux/vfio.h | 6 + > > > 4 files changed, 93 insertions(+), 22 deletions(-) > > > > This interface is terrible. A function named foo_enabled() should > > return a bool, yes or no, don't try to overload it to also return a > > version. > > Sorry, that one's my fault. I suggested that approach to Gavin > without really thinking it through. > > > > AFAICT, patch 2/2 breaks current users by changing the offset > > of the union in struct vfio_eeh_pe_err. > > Yeah, this one's ugly. We have to preserve the offset, but that means > putting the group in a very awkward place. Especially since I'm not > sure if there even are any existing users of the single extant union > branch. > > Sigh. > > > Also, we generally pass group > > file descriptors rather than a group ID because we can prove the > > ownership of the group through the file descriptor and we don't need to > > worry about races with the group because we can hold a reference to it. Duh. I finally realised the better, simpler, obvious solution. Rather than changing the parameter structure, we should move the ioctl()s so they're on the group fd instead of the container fd. Obviously we need to keep it on the container fd for backwards compat, but I think we should just error out if there is more than one group in the container there. We will need a new capability too, obviously. VFIO_EEH_GROUPFD maybe? -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson pgpCxcPnyjMr0.pgp Description: PGP signature
Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote: > On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote: > > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: > > > This allows to accept IOMMU group (PE) ID from the parameter from userland > > > when handling EEH operation so that the operation only affects the target > > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from > > > userland > > > is invalid, all IOMMU groups (PEs) attached to the specified container are > > > affected as before. > > > > > > Gavin Shan (2): > > > drivers/vfio: Support EEH API revision > > > drivers/vfio: Support IOMMU group for EEH operations > > > > > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 > > > ++--- > > > drivers/vfio/vfio_spapr_eeh.c | 46 > > > ++ > > > include/linux/vfio.h| 13 +++--- > > > include/uapi/linux/vfio.h | 6 + > > > 4 files changed, 93 insertions(+), 22 deletions(-) > > > > This interface is terrible. A function named foo_enabled() should > > return a bool, yes or no, don't try to overload it to also return a > > version. > > Sorry, that one's my fault. I suggested that approach to Gavin > without really thinking it through. > > > > AFAICT, patch 2/2 breaks current users by changing the offset > > of the union in struct vfio_eeh_pe_err. > > Yeah, this one's ugly. We have to preserve the offset, but that means > putting the group in a very awkward place. Especially since I'm not > sure if there even are any existing users of the single extant union > branch. > > Sigh. > > > Also, we generally pass group > > file descriptors rather than a group ID because we can prove the > > ownership of the group through the file descriptor and we don't need to > > worry about races with the group because we can hold a reference to it. Duh. I finally realised the better, simpler, obvious solution. Rather than changing the parameter structure, we should move the ioctl()s so they're on the group fd instead of the container fd. Obviously we need to keep it on the container fd for backwards compat, but I think we should just error out if there is more than one group in the container there. We will need a new capability too, obviously. VFIO_EEH_GROUPFD maybe? -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson pgpGg8RqigimZ.pgp Description: PGP signature
Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote: > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: > > This allows to accept IOMMU group (PE) ID from the parameter from userland > > when handling EEH operation so that the operation only affects the target > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland > > is invalid, all IOMMU groups (PEs) attached to the specified container are > > affected as before. > > > > Gavin Shan (2): > > drivers/vfio: Support EEH API revision > > drivers/vfio: Support IOMMU group for EEH operations > > > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 > > ++--- > > drivers/vfio/vfio_spapr_eeh.c | 46 ++ > > include/linux/vfio.h| 13 +++--- > > include/uapi/linux/vfio.h | 6 + > > 4 files changed, 93 insertions(+), 22 deletions(-) > > This interface is terrible. A function named foo_enabled() should > return a bool, yes or no, don't try to overload it to also return a > version. Sorry, that one's my fault. I suggested that approach to Gavin without really thinking it through. > AFAICT, patch 2/2 breaks current users by changing the offset > of the union in struct vfio_eeh_pe_err. Yeah, this one's ugly. We have to preserve the offset, but that means putting the group in a very awkward place. Especially since I'm not sure if there even are any existing users of the single extant union branch. Sigh. > Also, we generally pass group > file descriptors rather than a group ID because we can prove the > ownership of the group through the file descriptor and we don't need to > worry about races with the group because we can hold a reference to it. > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson pgp3yfhiIyNQC.pgp Description: PGP signature
Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote: > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: > > This allows to accept IOMMU group (PE) ID from the parameter from userland > > when handling EEH operation so that the operation only affects the target > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland > > is invalid, all IOMMU groups (PEs) attached to the specified container are > > affected as before. > > > > Gavin Shan (2): > > drivers/vfio: Support EEH API revision > > drivers/vfio: Support IOMMU group for EEH operations > > > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 > > ++--- > > drivers/vfio/vfio_spapr_eeh.c | 46 ++ > > include/linux/vfio.h| 13 +++--- > > include/uapi/linux/vfio.h | 6 + > > 4 files changed, 93 insertions(+), 22 deletions(-) > > This interface is terrible. A function named foo_enabled() should > return a bool, yes or no, don't try to overload it to also return a > version. Sorry, that one's my fault. I suggested that approach to Gavin without really thinking it through. > AFAICT, patch 2/2 breaks current users by changing the offset > of the union in struct vfio_eeh_pe_err. Yeah, this one's ugly. We have to preserve the offset, but that means putting the group in a very awkward place. Especially since I'm not sure if there even are any existing users of the single extant union branch. Sigh. > Also, we generally pass group > file descriptors rather than a group ID because we can prove the > ownership of the group through the file descriptor and we don't need to > worry about races with the group because we can hold a reference to it. > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson pgpRvQRqTjABt.pgp Description: PGP signature
[PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
This allows to accept IOMMU group (PE) ID from the parameter from userland when handling EEH operation so that the operation only affects the target IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland is invalid, all IOMMU groups (PEs) attached to the specified container are affected as before. Gavin Shan (2): drivers/vfio: Support EEH API revision drivers/vfio: Support IOMMU group for EEH operations drivers/vfio/vfio_iommu_spapr_tce.c | 50 ++--- drivers/vfio/vfio_spapr_eeh.c | 46 ++ include/linux/vfio.h| 13 +++--- include/uapi/linux/vfio.h | 6 + 4 files changed, 93 insertions(+), 22 deletions(-) -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: > This allows to accept IOMMU group (PE) ID from the parameter from userland > when handling EEH operation so that the operation only affects the target > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland > is invalid, all IOMMU groups (PEs) attached to the specified container are > affected as before. > > Gavin Shan (2): > drivers/vfio: Support EEH API revision > drivers/vfio: Support IOMMU group for EEH operations > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 > ++--- > drivers/vfio/vfio_spapr_eeh.c | 46 ++ > include/linux/vfio.h| 13 +++--- > include/uapi/linux/vfio.h | 6 + > 4 files changed, 93 insertions(+), 22 deletions(-) This interface is terrible. A function named foo_enabled() should return a bool, yes or no, don't try to overload it to also return a version. AFAICT, patch 2/2 breaks current users by changing the offset of the union in struct vfio_eeh_pe_err. Also, we generally pass group file descriptors rather than a group ID because we can prove the ownership of the group through the file descriptor and we don't need to worry about races with the group because we can hold a reference to it. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID
On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: > This allows to accept IOMMU group (PE) ID from the parameter from userland > when handling EEH operation so that the operation only affects the target > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland > is invalid, all IOMMU groups (PEs) attached to the specified container are > affected as before. > > Gavin Shan (2): > drivers/vfio: Support EEH API revision > drivers/vfio: Support IOMMU group for EEH operations > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 > ++--- > drivers/vfio/vfio_spapr_eeh.c | 46 ++ > include/linux/vfio.h| 13 +++--- > include/uapi/linux/vfio.h | 6 + > 4 files changed, 93 insertions(+), 22 deletions(-) This interface is terrible. A function named foo_enabled() should return a bool, yes or no, don't try to overload it to also return a version. AFAICT, patch 2/2 breaks current users by changing the offset of the union in struct vfio_eeh_pe_err. Also, we generally pass group file descriptors rather than a group ID because we can prove the ownership of the group through the file descriptor and we don't need to worry about races with the group because we can hold a reference to it. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
