| Message ID | 20170627171458.2529-2-venu.busireddy@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 27.06.17 at 19:14, <venu.busireddy@oracle.com> wrote: First of all, please Cc all maintainers of code you modify. > xen: Add support for hiding and unhiding pcie passthrough devices Please don't repeat the subject in the body of the mail. > Add support for hiding and unhiding (by introducing two new hypercall > subops) pci devices that trigger AER fatal errors while assigned to > guests in passthrough mode. Hiding of the device is done by assigning > it to dom_xen dummy domain. Would you mind explaining why simply de-assigning the device (with an existing operation) isn't suitable here? (This explanation would presumably belong either in the description here or in the cover letter.) > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -393,9 +393,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > { > case XEN_DOMCTL_createdomain: > case XEN_DOMCTL_test_assign_device: > + case XEN_DOMCTL_test_hidden_device: > case XEN_DOMCTL_gdbsx_guestmemio: > d = NULL; > break; > + case XEN_DOMCTL_hide_device: > + case XEN_DOMCTL_unhide_device: > + rcu_lock_domain(dom_xen); > + d = dom_xen; > + break; I'm opposed to the introduction of new operations which ignore the input domain ID. See my recent patch eliminating this for XEN_DOMCTL_test_assign_device [1]. If these really are domain independent operations, they ought to be sysctls. > @@ -1333,19 +1334,31 @@ int iommu_remove_device(struct pci_dev *pdev) > return hd->platform_ops->remove_device(pdev->devfn, pci_to_dev(pdev)); > } > > +static bool device_assigned_to_domain(struct domain *d, u16 seg, u8 bus, u8 devfn) > +{ > + bool rc = false; > + > + pcidevs_lock(); > + > + if ( pci_get_pdev_by_domain(d, seg, bus, devfn) ) > + rc = true; > + > + pcidevs_unlock(); > + return rc; > +} > + > /* > * If the device isn't owned by the hardware domain, it means it already > * has been assigned to other domain, or it doesn't exist. > */ > static int device_assigned(u16 seg, u8 bus, u8 devfn) > { > - struct pci_dev *pdev; > - > - pcidevs_lock(); > - pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn); > - pcidevs_unlock(); > + return device_assigned_to_domain(hardware_domain, seg, bus, devfn) ? 0 : -EBUSY; > +} > > - return pdev ? 0 : -EBUSY; > +static int device_hidden(u16 seg, u8 bus, u8 devfn) > +{ > + return device_assigned_to_domain(dom_xen, seg, bus, devfn) ? -EBUSY : 0; > } At least this new function you add wants to return bool. I cannot see how -EBUSY could be an appropriate return value for meaning "yes". > @@ -1354,6 +1367,22 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) > struct pci_dev *pdev; > int rc = 0; > > + if ( device_hidden(seg, bus, devfn) ) > + return -EINVAL; > + > + if ( d == dom_xen ) > + { > + pdev = pci_get_pdev(seg, bus, devfn); > + if ( pdev ) > + { > + list_move(&pdev->domain_list, &dom_xen->arch.pdev_list); > + pdev->domain = dom_xen; > + return rc; > + } > + else > + return -ENODEV; > + } > + > if ( !iommu_enabled || !hd->platform_ops ) > return 0; Your addition appears to be misplaced (would belong below the checks seen above). Additionally you fail to acquire the pcidevs lock. And the code would likely read better if you inverted the inner if()'s condition and omitted the "else" and the braces. Finally I'd prefer if you used d instead of dom_xen everywhere inside the outer if(). > @@ -1679,7 +1743,86 @@ int iommu_do_pci_domctl( > "deassign %04x:%02x:%02x.%u from dom%d failed (%d)\n", > seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > d->domain_id, ret); > + break; > + > + case XEN_DOMCTL_hide_device: > + machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf; > + ret = xsm_hide_device(XSM_HOOK, d, machine_sbdf); > + if ( ret ) > + break; > + > + if ( unlikely(d->is_dying) ) > + { > + ret = -EAGAIN; > + break; > + } > + > + seg = machine_sbdf >> 16; > + bus = PCI_BUS(machine_sbdf); > + devfn = PCI_DEVFN2(machine_sbdf); > + flag = domctl->u.assign_device.flag; > + > + if ( device_hidden(seg, bus, devfn) ) > + { > + ret = -EINVAL; > + break; > + } > + > + pcidevs_lock(); > + ret = assign_device(dom_xen, seg, bus, devfn, flag); > + pcidevs_unlock(); > + if ( ret == -ERESTART ) > + ret = hypercall_create_continuation(__HYPERVISOR_domctl, > + "h", u_domctl); > + else if ( ret ) > + printk(XENLOG_G_ERR "XEN_DOMCTL_hide_device: " > + "hide %04x:%02x:%02x.%u failed (%d)\n", > + seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret); > + break; > + > + case XEN_DOMCTL_unhide_device: > + machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf; > + ret = xsm_unhide_device(XSM_HOOK, d, machine_sbdf); > + if ( ret ) > + break; > + > + if ( unlikely(d->is_dying) ) > + { > + ret = -EINVAL; > + break; > + } > + > + seg = machine_sbdf >> 16; > + bus = PCI_BUS(machine_sbdf); > + devfn = PCI_DEVFN2(machine_sbdf); > + > + if ( !device_hidden(seg, bus, devfn) ) > + { > + ret = -EINVAL; > + break; > + } > + > + pcidevs_lock(); > + ret = deassign_device(dom_xen, seg, bus, devfn); > + pcidevs_unlock(); > + > + if ( ret == -ERESTART ) > + ret = hypercall_create_continuation(__HYPERVISOR_domctl, > + "h", u_domctl); > + else if ( ret ) > + printk(XENLOG_G_ERR "XEN_DOMCTL_unhide_device: " > + "assign %04x:%02x:%02x.%u to dom%d failed (%d)\n", > + seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > + d->domain_id, ret); > + break; As it looks you're duplicating a whole lot of code here, with just minor variations to the original. This is not a good idea maintenance wise, so you'd have to have a good reason for doing so. > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -1222,6 +1222,9 @@ struct xen_domctl { > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > #define XEN_DOMCTL_gdbsx_domstatus 1003 > +#define XEN_DOMCTL_hide_device 2001 > +#define XEN_DOMCTL_unhide_device 2002 > +#define XEN_DOMCTL_test_hidden_device 2003 Why these strange numbers? > @@ -1783,6 +1799,9 @@ static struct xsm_operations flask_ops = { > .test_assign_device = flask_test_assign_device, > .assign_device = flask_assign_device, > .deassign_device = flask_deassign_device, > + .hide_device = flask_hide_device, > + .unhide_device = flask_unhide_device, > + .test_hidden_device = flask_test_hidden_device, > #endif This is contrary to what you say in the description, and without respective fields being added to struct xsm_operations I can't see how this would build. Jan [1] https://lists.xenproject.org/archives/html/xen-devel/2017-06/msg02871.html
On 2017-07-04 09:46:58 -0600, Jan Beulich wrote: > >>> On 27.06.17 at 19:14, <venu.busireddy@oracle.com> wrote: > > First of all, please Cc all maintainers of code you modify. I was using the names spit out by the scripts/get_maintainer.pl script for the patch file. I didn't know that the script had a "-f" option, and without it, the script spits out only two names, which I included. I now have Cc'ed all the names that the "-f" option produced. Interestingly, Daniel's name is not in the "-f" output, and hence, I am still confused what the correct list is! > > xen: Add support for hiding and unhiding pcie passthrough devices > > Please don't repeat the subject in the body of the mail. This is a mistake. Will fix it. > > Add support for hiding and unhiding (by introducing two new hypercall > > subops) pci devices that trigger AER fatal errors while assigned to > > guests in passthrough mode. Hiding of the device is done by assigning > > it to dom_xen dummy domain. > > Would you mind explaining why simply de-assigning the device > (with an existing operation) isn't suitable here? (This explanation > would presumably belong either in the description here or in the > cover letter.) My initial thinking (for the first revision) was that the guest and the device together are party to the evil things, and hence the guest should be killed. But I agree that unassigning the device should be sufficient. Once the device is removed, the guest can't do much that any other guest can't. Therefore, I will change this patchset to simply unassign the device from the guest. Is that acceptable? > > --- a/xen/common/domctl.c > > +++ b/xen/common/domctl.c > > @@ -393,9 +393,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > > { > > case XEN_DOMCTL_createdomain: > > case XEN_DOMCTL_test_assign_device: > > + case XEN_DOMCTL_test_hidden_device: > > case XEN_DOMCTL_gdbsx_guestmemio: > > d = NULL; > > break; > > + case XEN_DOMCTL_hide_device: > > + case XEN_DOMCTL_unhide_device: > > + rcu_lock_domain(dom_xen); > > + d = dom_xen; > > + break; > > I'm opposed to the introduction of new operations which ignore the > input domain ID. See my recent patch eliminating this for > XEN_DOMCTL_test_assign_device [1]. If these really are domain > independent operations, they ought to be sysctls. Do you think there is a need to hide the device after unassigning it from the guest? If the answer is "no", then this code will go away. If the answer is "yes", then I will change it as you did in your reference [1]. Please let me know. > > @@ -1333,19 +1334,31 @@ int iommu_remove_device(struct pci_dev *pdev) > > return hd->platform_ops->remove_device(pdev->devfn, pci_to_dev(pdev)); > > } > > > > +static bool device_assigned_to_domain(struct domain *d, u16 seg, u8 bus, u8 devfn) > > +{ > > + bool rc = false; > > + > > + pcidevs_lock(); > > + > > + if ( pci_get_pdev_by_domain(d, seg, bus, devfn) ) > > + rc = true; > > + > > + pcidevs_unlock(); > > + return rc; > > +} > > + > > /* > > * If the device isn't owned by the hardware domain, it means it already > > * has been assigned to other domain, or it doesn't exist. > > */ > > static int device_assigned(u16 seg, u8 bus, u8 devfn) > > { > > - struct pci_dev *pdev; > > - > > - pcidevs_lock(); > > - pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn); > > - pcidevs_unlock(); > > + return device_assigned_to_domain(hardware_domain, seg, bus, devfn) ? 0 : -EBUSY; > > +} > > > > - return pdev ? 0 : -EBUSY; > > +static int device_hidden(u16 seg, u8 bus, u8 devfn) > > +{ > > + return device_assigned_to_domain(dom_xen, seg, bus, devfn) ? -EBUSY : 0; > > } > > At least this new function you add wants to return bool. I cannot > see how -EBUSY could be an appropriate return value for meaning > "yes". Will change, if this code stays (depends on the answer to question above). > > > @@ -1354,6 +1367,22 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) > > struct pci_dev *pdev; > > int rc = 0; > > > > + if ( device_hidden(seg, bus, devfn) ) > > + return -EINVAL; > > + > > + if ( d == dom_xen ) > > + { > > + pdev = pci_get_pdev(seg, bus, devfn); > > + if ( pdev ) > > + { > > + list_move(&pdev->domain_list, &dom_xen->arch.pdev_list); > > + pdev->domain = dom_xen; > > + return rc; > > + } > > + else > > + return -ENODEV; > > + } > > + > > if ( !iommu_enabled || !hd->platform_ops ) > > return 0; > > Your addition appears to be misplaced (would belong below the Will change, if this code stays... > checks seen above). Additionally you fail to acquire the pcidevs I am acquiring the lock in iommu_do_pci_domctl() in the case "XEN_DOMCTL_hide_device." Is that not sufficient? > lock. And the code would likely read better if you inverted the > inner if()'s condition and omitted the "else" and the braces. > Finally I'd prefer if you used d instead of dom_xen everywhere > inside the outer if(). Will change, if this code stays... > > @@ -1679,7 +1743,86 @@ int iommu_do_pci_domctl( > > "deassign %04x:%02x:%02x.%u from dom%d failed (%d)\n", > > seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > > d->domain_id, ret); > > + break; > > + > > + case XEN_DOMCTL_hide_device: > > + machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf; > > + ret = xsm_hide_device(XSM_HOOK, d, machine_sbdf); > > + if ( ret ) > > + break; > > + > > + if ( unlikely(d->is_dying) ) > > + { > > + ret = -EAGAIN; > > + break; > > + } > > + > > + seg = machine_sbdf >> 16; > > + bus = PCI_BUS(machine_sbdf); > > + devfn = PCI_DEVFN2(machine_sbdf); > > + flag = domctl->u.assign_device.flag; > > + > > + if ( device_hidden(seg, bus, devfn) ) > > + { > > + ret = -EINVAL; > > + break; > > + } > > + > > + pcidevs_lock(); > > + ret = assign_device(dom_xen, seg, bus, devfn, flag); > > + pcidevs_unlock(); > > + if ( ret == -ERESTART ) > > + ret = hypercall_create_continuation(__HYPERVISOR_domctl, > > + "h", u_domctl); > > + else if ( ret ) > > + printk(XENLOG_G_ERR "XEN_DOMCTL_hide_device: " > > + "hide %04x:%02x:%02x.%u failed (%d)\n", > > + seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret); > > + break; > > + > > + case XEN_DOMCTL_unhide_device: > > + machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf; > > + ret = xsm_unhide_device(XSM_HOOK, d, machine_sbdf); > > + if ( ret ) > > + break; > > + > > + if ( unlikely(d->is_dying) ) > > + { > > + ret = -EINVAL; > > + break; > > + } > > + > > + seg = machine_sbdf >> 16; > > + bus = PCI_BUS(machine_sbdf); > > + devfn = PCI_DEVFN2(machine_sbdf); > > + > > + if ( !device_hidden(seg, bus, devfn) ) > > + { > > + ret = -EINVAL; > > + break; > > + } > > + > > + pcidevs_lock(); > > + ret = deassign_device(dom_xen, seg, bus, devfn); > > + pcidevs_unlock(); > > + > > + if ( ret == -ERESTART ) > > + ret = hypercall_create_continuation(__HYPERVISOR_domctl, > > + "h", u_domctl); > > + else if ( ret ) > > + printk(XENLOG_G_ERR "XEN_DOMCTL_unhide_device: " > > + "assign %04x:%02x:%02x.%u to dom%d failed (%d)\n", > > + seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > > + d->domain_id, ret); > > + break; > > As it looks you're duplicating a whole lot of code here, with just > minor variations to the original. This is not a good idea > maintenance wise, so you'd have to have a good reason for > doing so. The only good reason I have is that this is much easier to read. If you prefer to code the way you coded in your reference [1], I can change it. Do you want me to? > > --- a/xen/include/public/domctl.h > > +++ b/xen/include/public/domctl.h > > @@ -1222,6 +1222,9 @@ struct xen_domctl { > > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > > #define XEN_DOMCTL_gdbsx_domstatus 1003 > > +#define XEN_DOMCTL_hide_device 2001 > > +#define XEN_DOMCTL_unhide_device 2002 > > +#define XEN_DOMCTL_test_hidden_device 2003 > > Why these strange numbers? I saw the numbers jump from 79 to 1000 thru 1003, and likewise used different starting numbers. Would you prefer 80 thru 82, or 1004 thru 1006? Of course, depends on whether we support the hide/unhide operations. > > @@ -1783,6 +1799,9 @@ static struct xsm_operations flask_ops = { > > .test_assign_device = flask_test_assign_device, > > .assign_device = flask_assign_device, > > .deassign_device = flask_deassign_device, > > + .hide_device = flask_hide_device, > > + .unhide_device = flask_unhide_device, > > + .test_hidden_device = flask_test_hidden_device, > > #endif > > This is contrary to what you say in the description, and without > respective fields being added to struct xsm_operations I can't > see how this would build. My mistake! I did not have XSM enabled in my build, and hence the build went through, and I didn't realize that I forgot to change the xsm_operations structure! Will fix it! Venu > [1] https://lists.xenproject.org/archives/html/xen-devel/2017-06/msg02871.html
> > > --- a/xen/include/public/domctl.h > > > +++ b/xen/include/public/domctl.h > > > @@ -1222,6 +1222,9 @@ struct xen_domctl { > > > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > > > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > > > #define XEN_DOMCTL_gdbsx_domstatus 1003 > > > +#define XEN_DOMCTL_hide_device 2001 > > > +#define XEN_DOMCTL_unhide_device 2002 > > > +#define XEN_DOMCTL_test_hidden_device 2003 > > > > Why these strange numbers? > > I saw the numbers jump from 79 to 1000 thru 1003, and likewise used > different starting numbers. Would you prefer 80 thru 82, or 1004 thru 1000 are got gdbsx and the 2000 are reserved for OEM vendors. So use 80 range.
>>> On 05.07.17 at 21:38, <venu.busireddy@oracle.com> wrote: > On 2017-07-04 09:46:58 -0600, Jan Beulich wrote: >> >>> On 27.06.17 at 19:14, <venu.busireddy@oracle.com> wrote: >> >> First of all, please Cc all maintainers of code you modify. > > I was using the names spit out by the scripts/get_maintainer.pl script > for the patch file. I didn't know that the script had a "-f" option, and > without it, the script spits out only two names, which I included. I now > have Cc'ed all the names that the "-f" option produced. Interestingly, > Daniel's name is not in the "-f" output, and hence, I am still confused > what the correct list is! I can't talk about the script, except that it is known to have limitations. Generally, changes to the public interface should be Cc-ed to all REST maintainers. >> > Add support for hiding and unhiding (by introducing two new hypercall >> > subops) pci devices that trigger AER fatal errors while assigned to >> > guests in passthrough mode. Hiding of the device is done by assigning >> > it to dom_xen dummy domain. >> >> Would you mind explaining why simply de-assigning the device >> (with an existing operation) isn't suitable here? (This explanation >> would presumably belong either in the description here or in the >> cover letter.) > > My initial thinking (for the first revision) was that the guest and > the device together are party to the evil things, and hence the guest > should be killed. But I agree that unassigning the device should be > sufficient. Once the device is removed, the guest can't do much that > any other guest can't. Therefore, I will change this patchset to simply > unassign the device from the guest. > > Is that acceptable? I think so, but I may be missing parts of your reasoning as to why hiding the device may be a good thing. >> > @@ -1354,6 +1367,22 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) >> > struct pci_dev *pdev; >> > int rc = 0; >> > >> > + if ( device_hidden(seg, bus, devfn) ) >> > + return -EINVAL; >> > + >> > + if ( d == dom_xen ) >> > + { >> > + pdev = pci_get_pdev(seg, bus, devfn); >> > + if ( pdev ) >> > + { >> > + list_move(&pdev->domain_list, &dom_xen->arch.pdev_list); >> > + pdev->domain = dom_xen; >> > + return rc; >> > + } >> > + else >> > + return -ENODEV; >> > + } >> > + >> > if ( !iommu_enabled || !hd->platform_ops ) >> > return 0; >> >> Your addition appears to be misplaced (would belong below the > > Will change, if this code stays... > >> checks seen above). Additionally you fail to acquire the pcidevs > > I am acquiring the lock in iommu_do_pci_domctl() in the case > "XEN_DOMCTL_hide_device." Is that not sufficient? Oh, I did overlook this further asymmetry to XEN_DOMCTL_assign_device handling. >> > --- a/xen/include/public/domctl.h >> > +++ b/xen/include/public/domctl.h >> > @@ -1222,6 +1222,9 @@ struct xen_domctl { >> > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 >> > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 >> > #define XEN_DOMCTL_gdbsx_domstatus 1003 >> > +#define XEN_DOMCTL_hide_device 2001 >> > +#define XEN_DOMCTL_unhide_device 2002 >> > +#define XEN_DOMCTL_test_hidden_device 2003 >> >> Why these strange numbers? > > I saw the numbers jump from 79 to 1000 thru 1003, and likewise used > different starting numbers. Would you prefer 80 thru 82, or 1004 thru > 1006? Of course, depends on whether we support the hide/unhide operations. The gdbsx ones were chosen this way long ago, perhaps to have them out of the way from all "normal" ones. Jan
On Thu, Jul 06, 2017 at 02:45:18AM -0600, Jan Beulich wrote: > >>> On 05.07.17 at 21:38, <venu.busireddy@oracle.com> wrote: > > On 2017-07-04 09:46:58 -0600, Jan Beulich wrote: > >> >>> On 27.06.17 at 19:14, <venu.busireddy@oracle.com> wrote: > >> > >> First of all, please Cc all maintainers of code you modify. > > > > I was using the names spit out by the scripts/get_maintainer.pl script > > for the patch file. I didn't know that the script had a "-f" option, and > > without it, the script spits out only two names, which I included. I now > > have Cc'ed all the names that the "-f" option produced. Interestingly, > > Daniel's name is not in the "-f" output, and hence, I am still confused > > what the correct list is! > > I can't talk about the script, except that it is known to have > limitations. Generally, changes to the public interface should be > Cc-ed to all REST maintainers. > > >> > Add support for hiding and unhiding (by introducing two new hypercall > >> > subops) pci devices that trigger AER fatal errors while assigned to > >> > guests in passthrough mode. Hiding of the device is done by assigning > >> > it to dom_xen dummy domain. > >> > >> Would you mind explaining why simply de-assigning the device > >> (with an existing operation) isn't suitable here? (This explanation > >> would presumably belong either in the description here or in the > >> cover letter.) > > > > My initial thinking (for the first revision) was that the guest and > > the device together are party to the evil things, and hence the guest > > should be killed. But I agree that unassigning the device should be > > sufficient. Once the device is removed, the guest can't do much that > > any other guest can't. Therefore, I will change this patchset to simply > > unassign the device from the guest. > > > > Is that acceptable? > > I think so, but I may be missing parts of your reasoning as to why > hiding the device may be a good thing. My thought exactly.
On 2017-07-06 02:45:18 -0600, Jan Beulich wrote: > >>> On 05.07.17 at 21:38, <venu.busireddy@oracle.com> wrote: > > On 2017-07-04 09:46:58 -0600, Jan Beulich wrote: > >> >>> On 27.06.17 at 19:14, <venu.busireddy@oracle.com> wrote: > >> > >> First of all, please Cc all maintainers of code you modify. > > > > I was using the names spit out by the scripts/get_maintainer.pl script > > for the patch file. I didn't know that the script had a "-f" option, and > > without it, the script spits out only two names, which I included. I now > > have Cc'ed all the names that the "-f" option produced. Interestingly, > > Daniel's name is not in the "-f" output, and hence, I am still confused > > what the correct list is! > > I can't talk about the script, except that it is known to have > limitations. Generally, changes to the public interface should be > Cc-ed to all REST maintainers. Understood. I assume that when in doubt, it is better to send to REST maintainers than fewer people? I mean, is it better to send to more people than omit some? > >> > Add support for hiding and unhiding (by introducing two new hypercall > >> > subops) pci devices that trigger AER fatal errors while assigned to > >> > guests in passthrough mode. Hiding of the device is done by assigning > >> > it to dom_xen dummy domain. > >> > >> Would you mind explaining why simply de-assigning the device > >> (with an existing operation) isn't suitable here? (This explanation > >> would presumably belong either in the description here or in the > >> cover letter.) > > > > My initial thinking (for the first revision) was that the guest and > > the device together are party to the evil things, and hence the guest > > should be killed. But I agree that unassigning the device should be > > sufficient. Once the device is removed, the guest can't do much that > > any other guest can't. Therefore, I will change this patchset to simply > > unassign the device from the guest. > > > > Is that acceptable? > > I think so, but I may be missing parts of your reasoning as to why > hiding the device may be a good thing. Here is the rationale behind hiding the erring device. If a device is misbehaving, one of the following two things could be happening: a) The error is caused by the misconfiguration of the guest driver or the firmware. This may not be a big problem. b) The error is caused by the owner of the domain re-flashing the firmware of the device and inserting a rogue firmware. This is a big problem. And the problem is that we can't differentiate between a) and b). If it is case b), then we certainly need to investigate and make sure that the firmware is the correct version and/or reload a new firmware to over-write the old one (just to be safe). Either way, the device needs to be unassignable until the root cause is investigated. Hiding the device is the safest way to ensure that the device is unassignable. Otherwise, the administrator may inadvertently reboot the domain to which the device was assigned, or, the domain itself may reboot upon errors, and in either case, the device gets reassigned to the domain upon reboot! Hiding the device prevents this. However, if you think that all of this is too much paranoia, I am fine with not hiding the device, and we simply de-assign the device from the domain. I leave the decision to you. [snip] > >> > --- a/xen/include/public/domctl.h > >> > +++ b/xen/include/public/domctl.h > >> > @@ -1222,6 +1222,9 @@ struct xen_domctl { > >> > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > >> > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > >> > #define XEN_DOMCTL_gdbsx_domstatus 1003 > >> > +#define XEN_DOMCTL_hide_device 2001 > >> > +#define XEN_DOMCTL_unhide_device 2002 > >> > +#define XEN_DOMCTL_test_hidden_device 2003 > >> > >> Why these strange numbers? > > > > I saw the numbers jump from 79 to 1000 thru 1003, and likewise used > > different starting numbers. Would you prefer 80 thru 82, or 1004 thru > > 1006? Of course, depends on whether we support the hide/unhide operations. > > The gdbsx ones were chosen this way long ago, perhaps to have > them out of the way from all "normal" ones. As also suggested by Konrad, I will use 80 thru 82. Venu
On 2017-07-07 12:00:26 +0100, Wei Liu wrote: > On Thu, Jul 06, 2017 at 02:45:18AM -0600, Jan Beulich wrote: > > >>> On 05.07.17 at 21:38, <venu.busireddy@oracle.com> wrote: > > > On 2017-07-04 09:46:58 -0600, Jan Beulich wrote: > > >> >>> On 27.06.17 at 19:14, <venu.busireddy@oracle.com> wrote: > > >> > > >> First of all, please Cc all maintainers of code you modify. > > > > > > I was using the names spit out by the scripts/get_maintainer.pl script > > > for the patch file. I didn't know that the script had a "-f" option, and > > > without it, the script spits out only two names, which I included. I now > > > have Cc'ed all the names that the "-f" option produced. Interestingly, > > > Daniel's name is not in the "-f" output, and hence, I am still confused > > > what the correct list is! > > > > I can't talk about the script, except that it is known to have > > limitations. Generally, changes to the public interface should be > > Cc-ed to all REST maintainers. > > > > >> > Add support for hiding and unhiding (by introducing two new hypercall > > >> > subops) pci devices that trigger AER fatal errors while assigned to > > >> > guests in passthrough mode. Hiding of the device is done by assigning > > >> > it to dom_xen dummy domain. > > >> > > >> Would you mind explaining why simply de-assigning the device > > >> (with an existing operation) isn't suitable here? (This explanation > > >> would presumably belong either in the description here or in the > > >> cover letter.) > > > > > > My initial thinking (for the first revision) was that the guest and > > > the device together are party to the evil things, and hence the guest > > > should be killed. But I agree that unassigning the device should be > > > sufficient. Once the device is removed, the guest can't do much that > > > any other guest can't. Therefore, I will change this patchset to simply > > > unassign the device from the guest. > > > > > > Is that acceptable? > > > > I think so, but I may be missing parts of your reasoning as to why > > hiding the device may be a good thing. > > My thought exactly. Answered this in https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg00925.html because there were some aditional questions answered in that thread. Venu
>>> On 07.07.17 at 20:11, <venu.busireddy@oracle.com> wrote: > On 2017-07-06 02:45:18 -0600, Jan Beulich wrote: >> I think so, but I may be missing parts of your reasoning as to why >> hiding the device may be a good thing. > > Here is the rationale behind hiding the erring device. > > If a device is misbehaving, one of the following two things could be > happening: > > a) The error is caused by the misconfiguration of the guest driver or > the firmware. This may not be a big problem. > > b) The error is caused by the owner of the domain re-flashing the firmware > of the device and inserting a rogue firmware. This is a big problem. > > And the problem is that we can't differentiate between a) and b). > > If it is case b), then we certainly need to investigate and make sure > that the firmware is the correct version and/or reload a new firmware to > over-write the old one (just to be safe). Either way, the device needs to > be unassignable until the root cause is investigated. Hiding the device > is the safest way to ensure that the device is unassignable. Otherwise, > the administrator may inadvertently reboot the domain to which the > device was assigned, or, the domain itself may reboot upon errors, and in > either case, the device gets reassigned to the domain upon reboot! Hiding > the device prevents this. > > However, if you think that all of this is too much paranoia, I am fine > with not hiding the device, and we simply de-assign the device from the > domain. I leave the decision to you. Well, what if the firmware being installed is rogue, but doesn't cause behavior that would result in us noticing right away? Passing through non-SR-IOV devices isn't entirely secure anyway, and I don't think SR-IOV VFs would permit firmware updates (I'd expect that to be possible via the PF only). So I'm afraid hiding the devices won't buy us much. Jan
On 2017-07-10 01:52:27 -0600, Jan Beulich wrote: > >>> On 07.07.17 at 20:11, <venu.busireddy@oracle.com> wrote: > > On 2017-07-06 02:45:18 -0600, Jan Beulich wrote: > >> I think so, but I may be missing parts of your reasoning as to why > >> hiding the device may be a good thing. > > > > Here is the rationale behind hiding the erring device. > > > > If a device is misbehaving, one of the following two things could be > > happening: > > > > a) The error is caused by the misconfiguration of the guest driver or > > the firmware. This may not be a big problem. > > > > b) The error is caused by the owner of the domain re-flashing the firmware > > of the device and inserting a rogue firmware. This is a big problem. > > > > And the problem is that we can't differentiate between a) and b). > > > > If it is case b), then we certainly need to investigate and make sure > > that the firmware is the correct version and/or reload a new firmware to > > over-write the old one (just to be safe). Either way, the device needs to > > be unassignable until the root cause is investigated. Hiding the device > > is the safest way to ensure that the device is unassignable. Otherwise, > > the administrator may inadvertently reboot the domain to which the > > device was assigned, or, the domain itself may reboot upon errors, and in > > either case, the device gets reassigned to the domain upon reboot! Hiding > > the device prevents this. > > > > However, if you think that all of this is too much paranoia, I am fine > > with not hiding the device, and we simply de-assign the device from the > > domain. I leave the decision to you. > > Well, what if the firmware being installed is rogue, but doesn't cause > behavior that would result in us noticing right away? Passing through > non-SR-IOV devices isn't entirely secure anyway, and I don't think > SR-IOV VFs would permit firmware updates (I'd expect that to be > possible via the PF only). So I'm afraid hiding the devices won't buy > us much. Okay. In a week, I will send v2 of this patch without hiding the device, unless we hear form others within that time-frame with other thoughts that change the approach. Venu
diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 951a5dc..5e0f123 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -393,9 +393,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) { case XEN_DOMCTL_createdomain: case XEN_DOMCTL_test_assign_device: + case XEN_DOMCTL_test_hidden_device: case XEN_DOMCTL_gdbsx_guestmemio: d = NULL; break; + case XEN_DOMCTL_hide_device: + case XEN_DOMCTL_unhide_device: + rcu_lock_domain(dom_xen); + d = dom_xen; + break; default: d = rcu_lock_domain_by_id(op->domain); if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo ) diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index c8e2d2d..ef4681a 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -31,6 +31,7 @@ #include <xen/softirq.h> #include <xen/tasklet.h> #include <xsm/xsm.h> +#include <xen/mm.h> #include <asm/msi.h> #include "ats.h" @@ -1333,19 +1334,31 @@ int iommu_remove_device(struct pci_dev *pdev) return hd->platform_ops->remove_device(pdev->devfn, pci_to_dev(pdev)); } +static bool device_assigned_to_domain(struct domain *d, u16 seg, u8 bus, u8 devfn) +{ + bool rc = false; + + pcidevs_lock(); + + if ( pci_get_pdev_by_domain(d, seg, bus, devfn) ) + rc = true; + + pcidevs_unlock(); + return rc; +} + /* * If the device isn't owned by the hardware domain, it means it already * has been assigned to other domain, or it doesn't exist. */ static int device_assigned(u16 seg, u8 bus, u8 devfn) { - struct pci_dev *pdev; - - pcidevs_lock(); - pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn); - pcidevs_unlock(); + return device_assigned_to_domain(hardware_domain, seg, bus, devfn) ? 0 : -EBUSY; +} - return pdev ? 0 : -EBUSY; +static int device_hidden(u16 seg, u8 bus, u8 devfn) +{ + return device_assigned_to_domain(dom_xen, seg, bus, devfn) ? -EBUSY : 0; } static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) @@ -1354,6 +1367,22 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) struct pci_dev *pdev; int rc = 0; + if ( device_hidden(seg, bus, devfn) ) + return -EINVAL; + + if ( d == dom_xen ) + { + pdev = pci_get_pdev(seg, bus, devfn); + if ( pdev ) + { + list_move(&pdev->domain_list, &dom_xen->arch.pdev_list); + pdev->domain = dom_xen; + return rc; + } + else + return -ENODEV; + } + if ( !iommu_enabled || !hd->platform_ops ) return 0; @@ -1417,10 +1446,23 @@ int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn) struct pci_dev *pdev = NULL; int ret = 0; + ASSERT(pcidevs_locked()); + + if ( d == dom_xen ) + { + pdev = pci_get_pdev(seg, bus, devfn); + if ( pdev ) + { + list_move(&pdev->domain_list, &hardware_domain->arch.pdev_list); + pdev->domain = hardware_domain; + return ret; + } + else return -ENODEV; + } + if ( !iommu_enabled || !hd->platform_ops ) return -EINVAL; - ASSERT(pcidevs_locked()); pdev = pci_get_pdev_by_domain(d, seg, bus, devfn); if ( !pdev ) return -ENODEV; @@ -1600,6 +1642,15 @@ int iommu_do_pci_domctl( seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); ret = -EINVAL; } + + if ( device_hidden(seg, bus, devfn) ) + { + printk(XENLOG_G_INFO + "%04x:%02x:%02x.%u device is hidden\n", + seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); + ret = -EINVAL; + } + break; case XEN_DOMCTL_assign_device: @@ -1636,8 +1687,15 @@ int iommu_do_pci_domctl( break; } - ret = device_assigned(seg, bus, devfn) ?: - assign_device(d, seg, bus, devfn, flag); + if ( device_hidden(seg, bus, devfn) ) + { + ret = -EINVAL; + break; + } + + if ( !device_assigned(seg, bus, devfn) ) + ret = assign_device(d, seg, bus, devfn, flag); + if ( ret == -ERESTART ) ret = hypercall_create_continuation(__HYPERVISOR_domctl, "h", u_domctl); @@ -1671,6 +1729,12 @@ int iommu_do_pci_domctl( bus = PCI_BUS(machine_sbdf); devfn = PCI_DEVFN2(machine_sbdf); + if ( device_hidden(seg, bus, devfn) ) + { + ret = -EINVAL; + break; + } + pcidevs_lock(); ret = deassign_device(d, seg, bus, devfn); pcidevs_unlock(); @@ -1679,7 +1743,86 @@ int iommu_do_pci_domctl( "deassign %04x:%02x:%02x.%u from dom%d failed (%d)\n", seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), d->domain_id, ret); + break; + + case XEN_DOMCTL_hide_device: + machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf; + ret = xsm_hide_device(XSM_HOOK, d, machine_sbdf); + if ( ret ) + break; + + if ( unlikely(d->is_dying) ) + { + ret = -EAGAIN; + break; + } + + seg = machine_sbdf >> 16; + bus = PCI_BUS(machine_sbdf); + devfn = PCI_DEVFN2(machine_sbdf); + flag = domctl->u.assign_device.flag; + + if ( device_hidden(seg, bus, devfn) ) + { + ret = -EINVAL; + break; + } + + pcidevs_lock(); + ret = assign_device(dom_xen, seg, bus, devfn, flag); + pcidevs_unlock(); + if ( ret == -ERESTART ) + ret = hypercall_create_continuation(__HYPERVISOR_domctl, + "h", u_domctl); + else if ( ret ) + printk(XENLOG_G_ERR "XEN_DOMCTL_hide_device: " + "hide %04x:%02x:%02x.%u failed (%d)\n", + seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret); + break; + + case XEN_DOMCTL_unhide_device: + machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf; + ret = xsm_unhide_device(XSM_HOOK, d, machine_sbdf); + if ( ret ) + break; + + if ( unlikely(d->is_dying) ) + { + ret = -EINVAL; + break; + } + + seg = machine_sbdf >> 16; + bus = PCI_BUS(machine_sbdf); + devfn = PCI_DEVFN2(machine_sbdf); + + if ( !device_hidden(seg, bus, devfn) ) + { + ret = -EINVAL; + break; + } + + pcidevs_lock(); + ret = deassign_device(dom_xen, seg, bus, devfn); + pcidevs_unlock(); + + if ( ret == -ERESTART ) + ret = hypercall_create_continuation(__HYPERVISOR_domctl, + "h", u_domctl); + else if ( ret ) + printk(XENLOG_G_ERR "XEN_DOMCTL_unhide_device: " + "assign %04x:%02x:%02x.%u to dom%d failed (%d)\n", + seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), + d->domain_id, ret); + break; + + case XEN_DOMCTL_test_hidden_device: + machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf; + seg = machine_sbdf >> 16; + bus = PCI_BUS(machine_sbdf); + devfn = PCI_DEVFN2(machine_sbdf); + ret = device_hidden(seg, bus, devfn); break; default: diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index e6cf211..1b043ea 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -1222,6 +1222,9 @@ struct xen_domctl { #define XEN_DOMCTL_gdbsx_pausevcpu 1001 #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 #define XEN_DOMCTL_gdbsx_domstatus 1003 +#define XEN_DOMCTL_hide_device 2001 +#define XEN_DOMCTL_unhide_device 2002 +#define XEN_DOMCTL_test_hidden_device 2003 uint32_t interface_version; /* XEN_DOMCTL_INTERFACE_VERSION */ domid_t domain; union { diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 62fcea6..0b820e1 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -355,6 +355,24 @@ static XSM_INLINE int xsm_deassign_device(XSM_DEFAULT_ARG struct domain *d, uint return xsm_default_action(action, current->domain, d); } +static XSM_INLINE int xsm_hide_device(XSM_DEFAULT_ARG struct domain *d, uint32_t machine_bdf) +{ + XSM_ASSERT_ACTION(XSM_HOOK); + return xsm_default_action(action, current->domain, d); +} + +static XSM_INLINE int xsm_unhide_device(XSM_DEFAULT_ARG struct domain *d, uint32_t machine_bdf) +{ + XSM_ASSERT_ACTION(XSM_HOOK); + return xsm_default_action(action, current->domain, d); +} + +static XSM_INLINE int xsm_test_hidden_device(XSM_DEFAULT_ARG uint32_t machine_bdf) +{ + XSM_ASSERT_ACTION(XSM_HOOK); + return xsm_default_action(action, current->domain, NULL); +} + #endif /* HAS_PASSTHROUGH && HAS_PCI */ #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE) diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 60c0fd6..03dbeff 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -479,6 +479,22 @@ static inline int xsm_deassign_device(xsm_default_t def, struct domain *d, uint3 { return xsm_ops->deassign_device(d, machine_bdf); } + +static inline int xsm_hide_device(xsm_default_t def, struct domain *d, uint32_t machine_bdf) +{ + return xsm_ops->hide_device(d, machine_bdf); +} + +static inline int xsm_unhide_device(xsm_default_t def, struct domain *d, uint32_t machine_bdf) +{ + return xsm_ops->unhide_device(d, machine_bdf); +} + +static inline int xsm_test_hidden_device(xsm_default_t def, uint32_t machine_bdf) +{ + return xsm_ops->test_hidden_device(machine_bdf); +} + #endif /* HAS_PASSTHROUGH && HAS_PCI) */ #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE) diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index 3cb5492..78111bb 100644 --- a/xen/xsm/dummy.c +++ b/xen/xsm/dummy.c @@ -94,6 +94,9 @@ void __init xsm_fixup_ops (struct xsm_operations *ops) set_to_dummy_if_null(ops, test_assign_device); set_to_dummy_if_null(ops, assign_device); set_to_dummy_if_null(ops, deassign_device); + set_to_dummy_if_null(ops, hide_device); + set_to_dummy_if_null(ops, unhide_device); + set_to_dummy_if_null(ops, test_hidden_device); #endif #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE) diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index fd84ac0..3695768 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -1311,6 +1311,22 @@ static int flask_deassign_device(struct domain *d, uint32_t machine_bdf) return avc_current_has_perm(rsid, SECCLASS_RESOURCE, RESOURCE__REMOVE_DEVICE, NULL); } + +static int flask_unhide_device(struct domain *d, uint32_t machine_bdf) +{ + return flask_deassign_device(d, machine_bdf); +} + +static int flask_hide_device(struct domain *d, uint32_t machine_bdf) +{ + return flask_assign_device(d, machine_bdf); +} + +static int flask_test_hidden_device(struct domain *d, uint32_t machine_bdf) +{ + return flask_test_assign_device(d, machine_bdf); +} + #endif /* HAS_PASSTHROUGH && HAS_PCI */ #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE) @@ -1783,6 +1799,9 @@ static struct xsm_operations flask_ops = { .test_assign_device = flask_test_assign_device, .assign_device = flask_assign_device, .deassign_device = flask_deassign_device, + .hide_device = flask_hide_device, + .unhide_device = flask_unhide_device, + .test_hidden_device = flask_test_hidden_device, #endif #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE) diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors index 1f7eb35..873df59 100644 --- a/xen/xsm/flask/policy/access_vectors +++ b/xen/xsm/flask/policy/access_vectors @@ -437,13 +437,13 @@ class resource # XEN_DOMCTL_iomem_permission, XEN_DOMCTL_memory_mapping add_iomem remove_iomem -# XEN_DOMCTL_get_device_group, XEN_DOMCTL_test_assign_device: +# XEN_DOMCTL_get_device_group, XEN_DOMCTL_test_assign_device, XEN_DOMCTL_test_hidden_device: # source = domain making the hypercall # target = device being queried stat_device -# XEN_DOMCTL_assign_device +# XEN_DOMCTL_assign_device, XEN_DOMCTL_hide_device add_device -# XEN_DOMCTL_deassign_device +# XEN_DOMCTL_deassign_device, XEN_DOMCTL_unhide_device remove_device # checked for PCI hot and cold-plug hypercalls, with target as the PCI device # checked for CPU and memory hotplug with xen_t as the target