| Message ID | 1457320984-6540-6-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 7 Mar 2016 11:22:58 +0800 Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > when boot up a VM that assigning vfio devices with aer enabled, we > must check the vfio device whether support host bus reset. because > when one error occur. OS driver always recover the device by do a > bus reset, in order to recover the vfio device, qemu must to do a > host bus reset to reset the device to default status. and for all > affected devices by the bus reset. we must check them whether all > are assigned to the VM. > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > --- > hw/vfio/pci.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > hw/vfio/pci.h | 1 + > 2 files changed, 212 insertions(+), 7 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 8ec9b25..0898e34 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -1868,6 +1868,197 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos) > return 0; > } > > +static bool vfio_pci_host_slot_match(PCIHostDeviceAddress *host1, > + PCIHostDeviceAddress *host2) > +{ > + return (host1->domain == host2->domain && host1->bus == host2->bus && > + host1->slot == host2->slot); > +} > + > +static bool vfio_pci_host_match(PCIHostDeviceAddress *host1, > + PCIHostDeviceAddress *host2) > +{ > + return (vfio_pci_host_slot_match(host1, host2) && > + host1->function == host2->function); > +} > + > +struct VFIODeviceFind { > + PCIDevice *pdev; > + bool found; > +}; > + > +static void vfio_check_device_noreset(PCIBus *bus, PCIDevice *pdev, > + void *opaque) > +{ > + DeviceState *dev = DEVICE(pdev); > + DeviceClass *dc = DEVICE_GET_CLASS(dev); > + VFIOPCIDevice *vdev; > + struct VFIODeviceFind *find = opaque; > + > + if (find->found) { > + return; > + } > + > + if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) { > + if (!dc->reset) { > + goto found; > + } > + return; > + } > + vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) && > + !vdev->vbasedev.reset_works) { > + goto found; > + } > + > + return; > +found: > + find->pdev = pdev; > + find->found = true; > +} > + > +static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp) > +{ > + PCIBus *bus = vdev->pdev.bus; > + struct vfio_pci_hot_reset_info *info = NULL; > + struct vfio_pci_dependent_device *devices; > + VFIOGroup *group; > + struct VFIODeviceFind find; > + int ret, i; > + > + ret = vfio_get_hot_reset_info(vdev, &info); > + if (ret) { > + error_setg(errp, "vfio: Cannot enable AER for device %s," > + " device does not support hot reset.", > + vdev->vbasedev.name); > + return; > + } > + > + /* List all affected devices by bus reset */ > + devices = &info->devices[0]; > + > + /* Verify that we have all the groups required */ > + for (i = 0; i < info->count; i++) { > + PCIHostDeviceAddress host; > + VFIOPCIDevice *tmp; > + VFIODevice *vbasedev_iter; > + bool found = false; > + > + host.domain = devices[i].segment; > + host.bus = devices[i].bus; > + host.slot = PCI_SLOT(devices[i].devfn); > + host.function = PCI_FUNC(devices[i].devfn); > + > + /* Skip the current device */ > + if (vfio_pci_host_match(&host, &vdev->host)) { > + continue; > + } > + > + /* Ensure we own the group of the affected device */ > + QLIST_FOREACH(group, &vfio_group_list, next) { > + if (group->groupid == devices[i].group_id) { > + break; > + } > + } > + > + if (!group) { > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > + "depends on group %d which is not owned.", > + vdev->vbasedev.name, devices[i].group_id); > + goto out; > + } > + > + /* Ensure affected devices for reset on the same slot */ > + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { > + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) { > + continue; > + } > + tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev); > + if (vfio_pci_host_match(&host, &tmp->host)) { > + /* > + * AER errors may be broadcast to all functions of a multi- > + * function endpoint. If any of those sibling functions are > + * also assigned, they need to have AER enabled or else an > + * error may continue to cause a vm_stop condition. IOW, > + * AER setup of this function would be pointless. > + */ > + if (vfio_pci_host_slot_match(&vdev->host, &tmp->host) && > + !(tmp->features & VFIO_FEATURE_ENABLE_AER)) { > + error_setg(errp, "vfio: Cannot enable AER for device %s, on same slot" > + " the dependent device %s which does not enable AER.", > + vdev->vbasedev.name, tmp->vbasedev.name); > + goto out; > + } > + > + if (tmp->pdev.bus != bus) { > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > + "the dependent device %s is not on the same bus", > + vdev->vbasedev.name, tmp->vbasedev.name); > + goto out; > + } > + found = true; > + break; > + } > + } > + > + /* Ensure all affected devices assigned to VM */ > + if (!found) { > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > + "the dependent device %04x:%02x:%02x.%x " > + "is not assigned to VM.", > + vdev->vbasedev.name, host.domain, host.bus, > + host.slot, host.function); > + goto out; > + } > + } > + > + /* > + * Check the all pci devices on or below the target bus > + * have a reset mechanism at least. > + */ > + find.pdev = NULL; > + find.found = false; > + pci_for_each_device(bus, pci_bus_num(bus), I'm not fully convinced this does what it says that it does. Bus numbers are under guest control, but using pci_bus_num(bus) here will cause us to always take the path in pci_find_bus_nr() where it simply returns bus. Thus we call pci_for_each_device_under_bus() with the bus originally provided in pci_for_each_device(). But pci_for_each_device_under_bus() only iterates the devfns immediately on that bus, despite the name that would imply otherwise. So it seems like the pci_for_each_device() callback would need to itself call pci_for_each_device() if it finds a bridge. Am I missing something or is that correct? We could also just go on to require that there are no subordinate buses to this bus, which seems like a reasonable thing to do. Thanks, Alex > + vfio_check_device_noreset, &find); > + if (find.found) { > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > + "the affected device %s does not have a reset mechanism.", > + vdev->vbasedev.name, find.pdev->name); > + goto out; > + } > + > +out: > + g_free(info); > + return; > +} > + > +static void vfio_check_devices_host_bus_reset(Error **errp) > +{ > + VFIOGroup *group; > + VFIODevice *vbasedev; > + VFIOPCIDevice *vdev; > + Error *local_err = NULL; > + > + /* Check All vfio-pci devices if have bus reset capability */ > + QLIST_FOREACH(group, &vfio_group_list, next) { > + QLIST_FOREACH(vbasedev, &group->device_list, next) { > + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) { > + continue; > + } > + vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev); > + if (vdev->features & VFIO_FEATURE_ENABLE_AER) { > + vfio_check_host_bus_reset(vdev, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + } > + } > + } > + > + return; > +} > + > static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, > int pos, uint16_t size) > { > @@ -2047,13 +2238,6 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev) > vfio_intx_enable(vdev); > } > > -static bool vfio_pci_host_match(PCIHostDeviceAddress *host1, > - PCIHostDeviceAddress *host2) > -{ > - return (host1->domain == host2->domain && host1->bus == host2->bus && > - host1->slot == host2->slot && host1->function == host2->function); > -} > - > static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single) > { > VFIOGroup *group; > @@ -2559,6 +2743,21 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) > vdev->req_enabled = false; > } > > +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused) > +{ > + Error *local_err = NULL; > + > + vfio_check_devices_host_bus_reset(&local_err); > + if (local_err) { > + error_report_err(local_err); > + exit(1); > + } > +} > + > +static Notifier machine_notifier = { > + .notify = vfio_pci_machine_done_notify, > +}; > + > static int vfio_initfn(PCIDevice *pdev) > { > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > @@ -2905,6 +3104,11 @@ static const TypeInfo vfio_pci_dev_info = { > static void register_vfio_pci_dev_type(void) > { > type_register_static(&vfio_pci_dev_info); > + /* > + * Register notifier when machine init is done, since we need > + * check the configration manner after all vfio device are inited. > + */ > + qemu_add_machine_init_done_notifier(&machine_notifier); > } > > type_init(register_vfio_pci_dev_type) > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h > index e0c53f2..aff46c2 100644 > --- a/hw/vfio/pci.h > +++ b/hw/vfio/pci.h > @@ -15,6 +15,7 @@ > #include "qemu-common.h" > #include "exec/memory.h" > #include "hw/pci/pci.h" > +#include "hw/pci/pci_bus.h" > #include "hw/pci/pci_bridge.h" > #include "hw/vfio/vfio-common.h" > #include "qemu/event_notifier.h"
On 03/09/2016 06:55 AM, Alex Williamson wrote: > On Mon, 7 Mar 2016 11:22:58 +0800 > Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > >> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> >> >> when boot up a VM that assigning vfio devices with aer enabled, we >> must check the vfio device whether support host bus reset. because >> when one error occur. OS driver always recover the device by do a >> bus reset, in order to recover the vfio device, qemu must to do a >> host bus reset to reset the device to default status. and for all >> affected devices by the bus reset. we must check them whether all >> are assigned to the VM. >> >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> >> --- >> hw/vfio/pci.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- >> hw/vfio/pci.h | 1 + >> 2 files changed, 212 insertions(+), 7 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 8ec9b25..0898e34 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -1868,6 +1868,197 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos) >> return 0; >> } >> >> +static bool vfio_pci_host_slot_match(PCIHostDeviceAddress *host1, >> + PCIHostDeviceAddress *host2) >> +{ >> + return (host1->domain == host2->domain && host1->bus == host2->bus && >> + host1->slot == host2->slot); >> +} >> + >> +static bool vfio_pci_host_match(PCIHostDeviceAddress *host1, >> + PCIHostDeviceAddress *host2) >> +{ >> + return (vfio_pci_host_slot_match(host1, host2) && >> + host1->function == host2->function); >> +} >> + >> +struct VFIODeviceFind { >> + PCIDevice *pdev; >> + bool found; >> +}; >> + >> +static void vfio_check_device_noreset(PCIBus *bus, PCIDevice *pdev, >> + void *opaque) >> +{ >> + DeviceState *dev = DEVICE(pdev); >> + DeviceClass *dc = DEVICE_GET_CLASS(dev); >> + VFIOPCIDevice *vdev; >> + struct VFIODeviceFind *find = opaque; >> + >> + if (find->found) { >> + return; >> + } >> + >> + if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) { >> + if (!dc->reset) { >> + goto found; >> + } >> + return; >> + } >> + vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); >> + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) && >> + !vdev->vbasedev.reset_works) { >> + goto found; >> + } >> + >> + return; >> +found: >> + find->pdev = pdev; >> + find->found = true; >> +} >> + >> +static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp) >> +{ >> + PCIBus *bus = vdev->pdev.bus; >> + struct vfio_pci_hot_reset_info *info = NULL; >> + struct vfio_pci_dependent_device *devices; >> + VFIOGroup *group; >> + struct VFIODeviceFind find; >> + int ret, i; >> + >> + ret = vfio_get_hot_reset_info(vdev, &info); >> + if (ret) { >> + error_setg(errp, "vfio: Cannot enable AER for device %s," >> + " device does not support hot reset.", >> + vdev->vbasedev.name); >> + return; >> + } >> + >> + /* List all affected devices by bus reset */ >> + devices = &info->devices[0]; >> + >> + /* Verify that we have all the groups required */ >> + for (i = 0; i < info->count; i++) { >> + PCIHostDeviceAddress host; >> + VFIOPCIDevice *tmp; >> + VFIODevice *vbasedev_iter; >> + bool found = false; >> + >> + host.domain = devices[i].segment; >> + host.bus = devices[i].bus; >> + host.slot = PCI_SLOT(devices[i].devfn); >> + host.function = PCI_FUNC(devices[i].devfn); >> + >> + /* Skip the current device */ >> + if (vfio_pci_host_match(&host, &vdev->host)) { >> + continue; >> + } >> + >> + /* Ensure we own the group of the affected device */ >> + QLIST_FOREACH(group, &vfio_group_list, next) { >> + if (group->groupid == devices[i].group_id) { >> + break; >> + } >> + } >> + >> + if (!group) { >> + error_setg(errp, "vfio: Cannot enable AER for device %s, " >> + "depends on group %d which is not owned.", >> + vdev->vbasedev.name, devices[i].group_id); >> + goto out; >> + } >> + >> + /* Ensure affected devices for reset on the same slot */ >> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >> + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) { >> + continue; >> + } >> + tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev); >> + if (vfio_pci_host_match(&host, &tmp->host)) { >> + /* >> + * AER errors may be broadcast to all functions of a multi- >> + * function endpoint. If any of those sibling functions are >> + * also assigned, they need to have AER enabled or else an >> + * error may continue to cause a vm_stop condition. IOW, >> + * AER setup of this function would be pointless. >> + */ >> + if (vfio_pci_host_slot_match(&vdev->host, &tmp->host) && >> + !(tmp->features & VFIO_FEATURE_ENABLE_AER)) { >> + error_setg(errp, "vfio: Cannot enable AER for device %s, on same slot" >> + " the dependent device %s which does not enable AER.", >> + vdev->vbasedev.name, tmp->vbasedev.name); >> + goto out; >> + } >> + >> + if (tmp->pdev.bus != bus) { >> + error_setg(errp, "vfio: Cannot enable AER for device %s, " >> + "the dependent device %s is not on the same bus", >> + vdev->vbasedev.name, tmp->vbasedev.name); >> + goto out; >> + } >> + found = true; >> + break; >> + } >> + } >> + >> + /* Ensure all affected devices assigned to VM */ >> + if (!found) { >> + error_setg(errp, "vfio: Cannot enable AER for device %s, " >> + "the dependent device %04x:%02x:%02x.%x " >> + "is not assigned to VM.", >> + vdev->vbasedev.name, host.domain, host.bus, >> + host.slot, host.function); >> + goto out; >> + } >> + } >> + >> + /* >> + * Check the all pci devices on or below the target bus >> + * have a reset mechanism at least. >> + */ >> + find.pdev = NULL; >> + find.found = false; >> + pci_for_each_device(bus, pci_bus_num(bus), > I'm not fully convinced this does what it says that it does. Bus > numbers are under guest control, but using pci_bus_num(bus) here will > cause us to always take the path in pci_find_bus_nr() where it simply > returns bus. Thus we call pci_for_each_device_under_bus() with the bus > originally provided in pci_for_each_device(). But > pci_for_each_device_under_bus() only iterates the devfns immediately on > that bus, despite the name that would imply otherwise. So it seems > like the pci_for_each_device() callback would need to itself call > pci_for_each_device() if it finds a bridge. Am I missing something or > is that correct? > > We could also just go on to require that there are no subordinate buses > to this bus, which seems like a reasonable thing to do. Thanks, yes. this should be enough for us. Thanks, Chen > > Alex > >> + vfio_check_device_noreset, &find); >> + if (find.found) { >> + error_setg(errp, "vfio: Cannot enable AER for device %s, " >> + "the affected device %s does not have a reset mechanism.", >> + vdev->vbasedev.name, find.pdev->name); >> + goto out; >> + } >> + >> +out: >> + g_free(info); >> + return; >> +} >> + >> +static void vfio_check_devices_host_bus_reset(Error **errp) >> +{ >> + VFIOGroup *group; >> + VFIODevice *vbasedev; >> + VFIOPCIDevice *vdev; >> + Error *local_err = NULL; >> + >> + /* Check All vfio-pci devices if have bus reset capability */ >> + QLIST_FOREACH(group, &vfio_group_list, next) { >> + QLIST_FOREACH(vbasedev, &group->device_list, next) { >> + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) { >> + continue; >> + } >> + vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev); >> + if (vdev->features & VFIO_FEATURE_ENABLE_AER) { >> + vfio_check_host_bus_reset(vdev, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + return; >> + } >> + } >> + } >> + } >> + >> + return; >> +} >> + >> static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, >> int pos, uint16_t size) >> { >> @@ -2047,13 +2238,6 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev) >> vfio_intx_enable(vdev); >> } >> >> -static bool vfio_pci_host_match(PCIHostDeviceAddress *host1, >> - PCIHostDeviceAddress *host2) >> -{ >> - return (host1->domain == host2->domain && host1->bus == host2->bus && >> - host1->slot == host2->slot && host1->function == host2->function); >> -} >> - >> static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single) >> { >> VFIOGroup *group; >> @@ -2559,6 +2743,21 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) >> vdev->req_enabled = false; >> } >> >> +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused) >> +{ >> + Error *local_err = NULL; >> + >> + vfio_check_devices_host_bus_reset(&local_err); >> + if (local_err) { >> + error_report_err(local_err); >> + exit(1); >> + } >> +} >> + >> +static Notifier machine_notifier = { >> + .notify = vfio_pci_machine_done_notify, >> +}; >> + >> static int vfio_initfn(PCIDevice *pdev) >> { >> VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); >> @@ -2905,6 +3104,11 @@ static const TypeInfo vfio_pci_dev_info = { >> static void register_vfio_pci_dev_type(void) >> { >> type_register_static(&vfio_pci_dev_info); >> + /* >> + * Register notifier when machine init is done, since we need >> + * check the configration manner after all vfio device are inited. >> + */ >> + qemu_add_machine_init_done_notifier(&machine_notifier); >> } >> >> type_init(register_vfio_pci_dev_type) >> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h >> index e0c53f2..aff46c2 100644 >> --- a/hw/vfio/pci.h >> +++ b/hw/vfio/pci.h >> @@ -15,6 +15,7 @@ >> #include "qemu-common.h" >> #include "exec/memory.h" >> #include "hw/pci/pci.h" >> +#include "hw/pci/pci_bus.h" >> #include "hw/pci/pci_bridge.h" >> #include "hw/vfio/vfio-common.h" >> #include "qemu/event_notifier.h" > > > . >
On Mon, Mar 07, 2016 at 11:22:58AM +0800, Cao jin wrote: > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > when boot up a VM that assigning vfio devices with aer enabled, we > must check the vfio device whether support host bus reset. because > when one error occur. OS driver always recover the device by do a > bus reset, in order to recover the vfio device, qemu must to do a > host bus reset to reset the device to default status. and for all > affected devices by the bus reset. we must check them whether all > are assigned to the VM. > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > --- > hw/vfio/pci.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > hw/vfio/pci.h | 1 + > 2 files changed, 212 insertions(+), 7 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 8ec9b25..0898e34 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -1868,6 +1868,197 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos) > return 0; > } > > +static bool vfio_pci_host_slot_match(PCIHostDeviceAddress *host1, > + PCIHostDeviceAddress *host2) > +{ > + return (host1->domain == host2->domain && host1->bus == host2->bus && > + host1->slot == host2->slot); > +} > + > +static bool vfio_pci_host_match(PCIHostDeviceAddress *host1, > + PCIHostDeviceAddress *host2) > +{ > + return (vfio_pci_host_slot_match(host1, host2) && > + host1->function == host2->function); > +} > + > +struct VFIODeviceFind { > + PCIDevice *pdev; > + bool found; > +}; > + > +static void vfio_check_device_noreset(PCIBus *bus, PCIDevice *pdev, > + void *opaque) > +{ > + DeviceState *dev = DEVICE(pdev); > + DeviceClass *dc = DEVICE_GET_CLASS(dev); > + VFIOPCIDevice *vdev; > + struct VFIODeviceFind *find = opaque; > + > + if (find->found) { > + return; > + } > + > + if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) { > + if (!dc->reset) { > + goto found; > + } > + return; > + } > + vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) && > + !vdev->vbasedev.reset_works) { > + goto found; > + } > + > + return; > +found: > + find->pdev = pdev; > + find->found = true; > +} > + > +static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp) > +{ > + PCIBus *bus = vdev->pdev.bus; > + struct vfio_pci_hot_reset_info *info = NULL; > + struct vfio_pci_dependent_device *devices; > + VFIOGroup *group; > + struct VFIODeviceFind find; > + int ret, i; > + > + ret = vfio_get_hot_reset_info(vdev, &info); > + if (ret) { > + error_setg(errp, "vfio: Cannot enable AER for device %s," > + " device does not support hot reset.", > + vdev->vbasedev.name); > + return; > + } > + > + /* List all affected devices by bus reset */ > + devices = &info->devices[0]; > + > + /* Verify that we have all the groups required */ > + for (i = 0; i < info->count; i++) { > + PCIHostDeviceAddress host; > + VFIOPCIDevice *tmp; > + VFIODevice *vbasedev_iter; > + bool found = false; > + > + host.domain = devices[i].segment; > + host.bus = devices[i].bus; > + host.slot = PCI_SLOT(devices[i].devfn); > + host.function = PCI_FUNC(devices[i].devfn); > + > + /* Skip the current device */ > + if (vfio_pci_host_match(&host, &vdev->host)) { > + continue; > + } > + > + /* Ensure we own the group of the affected device */ > + QLIST_FOREACH(group, &vfio_group_list, next) { > + if (group->groupid == devices[i].group_id) { > + break; > + } > + } > + > + if (!group) { > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > + "depends on group %d which is not owned.", > + vdev->vbasedev.name, devices[i].group_id); > + goto out; > + } > + > + /* Ensure affected devices for reset on the same slot */ > + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { So what you did here, first you let user build up all functions, then when finally function 0 is added, you go and check them all. Result is rather messy. Instead, make is_valid_func access two functions: existing one and new one. And this way each new function can validate that all existing functions are setup correctly, and each existing function can validate that the new one does not conflict with it. > + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) { > + continue; > + } > + tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev); > + if (vfio_pci_host_match(&host, &tmp->host)) { > + /* > + * AER errors may be broadcast to all functions of a multi- > + * function endpoint. If any of those sibling functions are > + * also assigned, they need to have AER enabled or else an > + * error may continue to cause a vm_stop condition. IOW, > + * AER setup of this function would be pointless. > + */ > + if (vfio_pci_host_slot_match(&vdev->host, &tmp->host) && > + !(tmp->features & VFIO_FEATURE_ENABLE_AER)) { > + error_setg(errp, "vfio: Cannot enable AER for device %s, on same slot" > + " the dependent device %s which does not enable AER.", > + vdev->vbasedev.name, tmp->vbasedev.name); > + goto out; > + } > + > + if (tmp->pdev.bus != bus) { > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > + "the dependent device %s is not on the same bus", > + vdev->vbasedev.name, tmp->vbasedev.name); > + goto out; > + } > + found = true; > + break; > + } > + } > + > + /* Ensure all affected devices assigned to VM */ > + if (!found) { > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > + "the dependent device %04x:%02x:%02x.%x " > + "is not assigned to VM.", > + vdev->vbasedev.name, host.domain, host.bus, > + host.slot, host.function); > + goto out; > + } > + } > + > + /* > + * Check the all pci devices on or below the target bus > + * have a reset mechanism at least. > + */ > + find.pdev = NULL; > + find.found = false; > + pci_for_each_device(bus, pci_bus_num(bus), > + vfio_check_device_noreset, &find); > + if (find.found) { > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > + "the affected device %s does not have a reset mechanism.", > + vdev->vbasedev.name, find.pdev->name); > + goto out; > + } > + > +out: > + g_free(info); > + return; > +} > + > +static void vfio_check_devices_host_bus_reset(Error **errp) > +{ > + VFIOGroup *group; > + VFIODevice *vbasedev; > + VFIOPCIDevice *vdev; > + Error *local_err = NULL; > + > + /* Check All vfio-pci devices if have bus reset capability */ > + QLIST_FOREACH(group, &vfio_group_list, next) { > + QLIST_FOREACH(vbasedev, &group->device_list, next) { > + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) { > + continue; > + } > + vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev); > + if (vdev->features & VFIO_FEATURE_ENABLE_AER) { > + vfio_check_host_bus_reset(vdev, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + } > + } > + } > + > + return; > +} > + > static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, > int pos, uint16_t size) > { > @@ -2047,13 +2238,6 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev) > vfio_intx_enable(vdev); > } > > -static bool vfio_pci_host_match(PCIHostDeviceAddress *host1, > - PCIHostDeviceAddress *host2) > -{ > - return (host1->domain == host2->domain && host1->bus == host2->bus && > - host1->slot == host2->slot && host1->function == host2->function); > -} > - > static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single) > { > VFIOGroup *group; > @@ -2559,6 +2743,21 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) > vdev->req_enabled = false; > } > > +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused) > +{ > + Error *local_err = NULL; > + > + vfio_check_devices_host_bus_reset(&local_err); > + if (local_err) { > + error_report_err(local_err); > + exit(1); > + } > +} > + > +static Notifier machine_notifier = { > + .notify = vfio_pci_machine_done_notify, > +}; > + > static int vfio_initfn(PCIDevice *pdev) > { > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > @@ -2905,6 +3104,11 @@ static const TypeInfo vfio_pci_dev_info = { > static void register_vfio_pci_dev_type(void) > { > type_register_static(&vfio_pci_dev_info); > + /* > + * Register notifier when machine init is done, since we need > + * check the configration manner after all vfio device are inited. > + */ > + qemu_add_machine_init_done_notifier(&machine_notifier); > } > > type_init(register_vfio_pci_dev_type) > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h > index e0c53f2..aff46c2 100644 > --- a/hw/vfio/pci.h > +++ b/hw/vfio/pci.h > @@ -15,6 +15,7 @@ > #include "qemu-common.h" > #include "exec/memory.h" > #include "hw/pci/pci.h" > +#include "hw/pci/pci_bus.h" > #include "hw/pci/pci_bridge.h" > #include "hw/vfio/vfio-common.h" > #include "qemu/event_notifier.h" > -- > 1.9.3 > >
On Wed, 9 Mar 2016 18:47:35 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Mar 07, 2016 at 11:22:58AM +0800, Cao jin wrote: > > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > > > when boot up a VM that assigning vfio devices with aer enabled, we > > must check the vfio device whether support host bus reset. because > > when one error occur. OS driver always recover the device by do a > > bus reset, in order to recover the vfio device, qemu must to do a > > host bus reset to reset the device to default status. and for all > > affected devices by the bus reset. we must check them whether all > > are assigned to the VM. > > > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > --- > > hw/vfio/pci.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > > hw/vfio/pci.h | 1 + > > 2 files changed, 212 insertions(+), 7 deletions(-) > > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > > index 8ec9b25..0898e34 100644 > > --- a/hw/vfio/pci.c > > +++ b/hw/vfio/pci.c > > @@ -1868,6 +1868,197 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos) > > return 0; > > } > > > > +static bool vfio_pci_host_slot_match(PCIHostDeviceAddress *host1, > > + PCIHostDeviceAddress *host2) > > +{ > > + return (host1->domain == host2->domain && host1->bus == host2->bus && > > + host1->slot == host2->slot); > > +} > > + > > +static bool vfio_pci_host_match(PCIHostDeviceAddress *host1, > > + PCIHostDeviceAddress *host2) > > +{ > > + return (vfio_pci_host_slot_match(host1, host2) && > > + host1->function == host2->function); > > +} > > + > > +struct VFIODeviceFind { > > + PCIDevice *pdev; > > + bool found; > > +}; > > + > > +static void vfio_check_device_noreset(PCIBus *bus, PCIDevice *pdev, > > + void *opaque) > > +{ > > + DeviceState *dev = DEVICE(pdev); > > + DeviceClass *dc = DEVICE_GET_CLASS(dev); > > + VFIOPCIDevice *vdev; > > + struct VFIODeviceFind *find = opaque; > > + > > + if (find->found) { > > + return; > > + } > > + > > + if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) { > > + if (!dc->reset) { > > + goto found; > > + } > > + return; > > + } > > + vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > > + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) && > > + !vdev->vbasedev.reset_works) { > > + goto found; > > + } > > + > > + return; > > +found: > > + find->pdev = pdev; > > + find->found = true; > > +} > > + > > +static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp) > > +{ > > + PCIBus *bus = vdev->pdev.bus; > > + struct vfio_pci_hot_reset_info *info = NULL; > > + struct vfio_pci_dependent_device *devices; > > + VFIOGroup *group; > > + struct VFIODeviceFind find; > > + int ret, i; > > + > > + ret = vfio_get_hot_reset_info(vdev, &info); > > + if (ret) { > > + error_setg(errp, "vfio: Cannot enable AER for device %s," > > + " device does not support hot reset.", > > + vdev->vbasedev.name); > > + return; > > + } > > + > > + /* List all affected devices by bus reset */ > > + devices = &info->devices[0]; > > + > > + /* Verify that we have all the groups required */ > > + for (i = 0; i < info->count; i++) { > > + PCIHostDeviceAddress host; > > + VFIOPCIDevice *tmp; > > + VFIODevice *vbasedev_iter; > > + bool found = false; > > + > > + host.domain = devices[i].segment; > > + host.bus = devices[i].bus; > > + host.slot = PCI_SLOT(devices[i].devfn); > > + host.function = PCI_FUNC(devices[i].devfn); > > + > > + /* Skip the current device */ > > + if (vfio_pci_host_match(&host, &vdev->host)) { > > + continue; > > + } > > + > > + /* Ensure we own the group of the affected device */ > > + QLIST_FOREACH(group, &vfio_group_list, next) { > > + if (group->groupid == devices[i].group_id) { > > + break; > > + } > > + } > > + > > + if (!group) { > > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > > + "depends on group %d which is not owned.", > > + vdev->vbasedev.name, devices[i].group_id); > > + goto out; > > + } > > + > > + /* Ensure affected devices for reset on the same slot */ > > + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { > > > > So what you did here, first you let user build up > all functions, then when finally function 0 is added, > you go and check them all. > > Result is rather messy. > > Instead, make is_valid_func access two functions: > existing one and new one. > And this way each new function can validate that > all existing functions are setup correctly, > and each existing function can validate that > the new one does not conflict with it. Same problem as what you're proposed for 09/11, when bus reset is involved, there may be no valid configuration of a subset of the devices. Only when the entire set of devices is added does it become valid. Thanks, Alex > > + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) { > > + continue; > > + } > > + tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev); > > + if (vfio_pci_host_match(&host, &tmp->host)) { > > + /* > > + * AER errors may be broadcast to all functions of a multi- > > + * function endpoint. If any of those sibling functions are > > + * also assigned, they need to have AER enabled or else an > > + * error may continue to cause a vm_stop condition. IOW, > > + * AER setup of this function would be pointless. > > + */ > > + if (vfio_pci_host_slot_match(&vdev->host, &tmp->host) && > > + !(tmp->features & VFIO_FEATURE_ENABLE_AER)) { > > + error_setg(errp, "vfio: Cannot enable AER for device %s, on same slot" > > + " the dependent device %s which does not enable AER.", > > + vdev->vbasedev.name, tmp->vbasedev.name); > > + goto out; > > + } > > + > > + if (tmp->pdev.bus != bus) { > > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > > + "the dependent device %s is not on the same bus", > > + vdev->vbasedev.name, tmp->vbasedev.name); > > + goto out; > > + } > > + found = true; > > + break; > > + } > > + } > > + > > + /* Ensure all affected devices assigned to VM */ > > + if (!found) { > > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > > + "the dependent device %04x:%02x:%02x.%x " > > + "is not assigned to VM.", > > + vdev->vbasedev.name, host.domain, host.bus, > > + host.slot, host.function); > > + goto out; > > + } > > + } > > + > > + /* > > + * Check the all pci devices on or below the target bus > > + * have a reset mechanism at least. > > + */ > > + find.pdev = NULL; > > + find.found = false; > > + pci_for_each_device(bus, pci_bus_num(bus), > > + vfio_check_device_noreset, &find); > > + if (find.found) { > > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > > + "the affected device %s does not have a reset mechanism.", > > + vdev->vbasedev.name, find.pdev->name); > > + goto out; > > + } > > + > > +out: > > + g_free(info); > > + return; > > +} > > + > > +static void vfio_check_devices_host_bus_reset(Error **errp) > > +{ > > + VFIOGroup *group; > > + VFIODevice *vbasedev; > > + VFIOPCIDevice *vdev; > > + Error *local_err = NULL; > > + > > + /* Check All vfio-pci devices if have bus reset capability */ > > + QLIST_FOREACH(group, &vfio_group_list, next) { > > + QLIST_FOREACH(vbasedev, &group->device_list, next) { > > + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) { > > + continue; > > + } > > + vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev); > > + if (vdev->features & VFIO_FEATURE_ENABLE_AER) { > > + vfio_check_host_bus_reset(vdev, &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + } > > + } > > + } > > + > > + return; > > +} > > + > > static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, > > int pos, uint16_t size) > > { > > @@ -2047,13 +2238,6 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev) > > vfio_intx_enable(vdev); > > } > > > > -static bool vfio_pci_host_match(PCIHostDeviceAddress *host1, > > - PCIHostDeviceAddress *host2) > > -{ > > - return (host1->domain == host2->domain && host1->bus == host2->bus && > > - host1->slot == host2->slot && host1->function == host2->function); > > -} > > - > > static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single) > > { > > VFIOGroup *group; > > @@ -2559,6 +2743,21 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) > > vdev->req_enabled = false; > > } > > > > +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused) > > +{ > > + Error *local_err = NULL; > > + > > + vfio_check_devices_host_bus_reset(&local_err); > > + if (local_err) { > > + error_report_err(local_err); > > + exit(1); > > + } > > +} > > + > > +static Notifier machine_notifier = { > > + .notify = vfio_pci_machine_done_notify, > > +}; > > + > > static int vfio_initfn(PCIDevice *pdev) > > { > > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > > @@ -2905,6 +3104,11 @@ static const TypeInfo vfio_pci_dev_info = { > > static void register_vfio_pci_dev_type(void) > > { > > type_register_static(&vfio_pci_dev_info); > > + /* > > + * Register notifier when machine init is done, since we need > > + * check the configration manner after all vfio device are inited. > > + */ > > + qemu_add_machine_init_done_notifier(&machine_notifier); > > } > > > > type_init(register_vfio_pci_dev_type) > > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h > > index e0c53f2..aff46c2 100644 > > --- a/hw/vfio/pci.h > > +++ b/hw/vfio/pci.h > > @@ -15,6 +15,7 @@ > > #include "qemu-common.h" > > #include "exec/memory.h" > > #include "hw/pci/pci.h" > > +#include "hw/pci/pci_bus.h" > > #include "hw/pci/pci_bridge.h" > > #include "hw/vfio/vfio-common.h" > > #include "qemu/event_notifier.h" > > -- > > 1.9.3 > > > >
On Wed, Mar 09, 2016 at 09:59:04AM -0700, Alex Williamson wrote: > On Wed, 9 Mar 2016 18:47:35 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Mon, Mar 07, 2016 at 11:22:58AM +0800, Cao jin wrote: > > > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > > > > > when boot up a VM that assigning vfio devices with aer enabled, we > > > must check the vfio device whether support host bus reset. because > > > when one error occur. OS driver always recover the device by do a > > > bus reset, in order to recover the vfio device, qemu must to do a > > > host bus reset to reset the device to default status. and for all > > > affected devices by the bus reset. we must check them whether all > > > are assigned to the VM. > > > > > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > > --- > > > hw/vfio/pci.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > > > hw/vfio/pci.h | 1 + > > > 2 files changed, 212 insertions(+), 7 deletions(-) > > > > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > > > index 8ec9b25..0898e34 100644 > > > --- a/hw/vfio/pci.c > > > +++ b/hw/vfio/pci.c > > > @@ -1868,6 +1868,197 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos) > > > return 0; > > > } > > > > > > +static bool vfio_pci_host_slot_match(PCIHostDeviceAddress *host1, > > > + PCIHostDeviceAddress *host2) > > > +{ > > > + return (host1->domain == host2->domain && host1->bus == host2->bus && > > > + host1->slot == host2->slot); > > > +} > > > + > > > +static bool vfio_pci_host_match(PCIHostDeviceAddress *host1, > > > + PCIHostDeviceAddress *host2) > > > +{ > > > + return (vfio_pci_host_slot_match(host1, host2) && > > > + host1->function == host2->function); > > > +} > > > + > > > +struct VFIODeviceFind { > > > + PCIDevice *pdev; > > > + bool found; > > > +}; > > > + > > > +static void vfio_check_device_noreset(PCIBus *bus, PCIDevice *pdev, > > > + void *opaque) > > > +{ > > > + DeviceState *dev = DEVICE(pdev); > > > + DeviceClass *dc = DEVICE_GET_CLASS(dev); > > > + VFIOPCIDevice *vdev; > > > + struct VFIODeviceFind *find = opaque; > > > + > > > + if (find->found) { > > > + return; > > > + } > > > + > > > + if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) { > > > + if (!dc->reset) { > > > + goto found; > > > + } > > > + return; > > > + } > > > + vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > > > + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) && > > > + !vdev->vbasedev.reset_works) { > > > + goto found; > > > + } > > > + > > > + return; > > > +found: > > > + find->pdev = pdev; > > > + find->found = true; > > > +} > > > + > > > +static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp) > > > +{ > > > + PCIBus *bus = vdev->pdev.bus; > > > + struct vfio_pci_hot_reset_info *info = NULL; > > > + struct vfio_pci_dependent_device *devices; > > > + VFIOGroup *group; > > > + struct VFIODeviceFind find; > > > + int ret, i; > > > + > > > + ret = vfio_get_hot_reset_info(vdev, &info); > > > + if (ret) { > > > + error_setg(errp, "vfio: Cannot enable AER for device %s," > > > + " device does not support hot reset.", > > > + vdev->vbasedev.name); > > > + return; > > > + } > > > + > > > + /* List all affected devices by bus reset */ > > > + devices = &info->devices[0]; > > > + > > > + /* Verify that we have all the groups required */ > > > + for (i = 0; i < info->count; i++) { > > > + PCIHostDeviceAddress host; > > > + VFIOPCIDevice *tmp; > > > + VFIODevice *vbasedev_iter; > > > + bool found = false; > > > + > > > + host.domain = devices[i].segment; > > > + host.bus = devices[i].bus; > > > + host.slot = PCI_SLOT(devices[i].devfn); > > > + host.function = PCI_FUNC(devices[i].devfn); > > > + > > > + /* Skip the current device */ > > > + if (vfio_pci_host_match(&host, &vdev->host)) { > > > + continue; > > > + } > > > + > > > + /* Ensure we own the group of the affected device */ > > > + QLIST_FOREACH(group, &vfio_group_list, next) { > > > + if (group->groupid == devices[i].group_id) { > > > + break; > > > + } > > > + } > > > + > > > + if (!group) { > > > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > > > + "depends on group %d which is not owned.", > > > + vdev->vbasedev.name, devices[i].group_id); > > > + goto out; > > > + } > > > + > > > + /* Ensure affected devices for reset on the same slot */ > > > + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { > > > > > > > > So what you did here, first you let user build up > > all functions, then when finally function 0 is added, > > you go and check them all. > > > > Result is rather messy. > > > > Instead, make is_valid_func access two functions: > > existing one and new one. > > And this way each new function can validate that > > all existing functions are setup correctly, > > and each existing function can validate that > > the new one does not conflict with it. > > Same problem as what you're proposed for 09/11, when bus reset is > involved, there may be no valid configuration of a subset of the > devices. Only when the entire set of devices is added does it become > valid. Thanks, > > Alex There are two issues: 1. make sure virtual device includes all host device functions - you can not detect this until you have the full virtual device, new callback when device is ready makes sense 2. make sure virtual device does not include two functions from different host devices - you can detect this on realize of function which conflicts, so you probably should. checking this in this callback makes no sense. > > > + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) { > > > + continue; > > > + } > > > + tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev); > > > + if (vfio_pci_host_match(&host, &tmp->host)) { > > > + /* > > > + * AER errors may be broadcast to all functions of a multi- > > > + * function endpoint. If any of those sibling functions are > > > + * also assigned, they need to have AER enabled or else an > > > + * error may continue to cause a vm_stop condition. IOW, > > > + * AER setup of this function would be pointless. > > > + */ > > > + if (vfio_pci_host_slot_match(&vdev->host, &tmp->host) && > > > + !(tmp->features & VFIO_FEATURE_ENABLE_AER)) { > > > + error_setg(errp, "vfio: Cannot enable AER for device %s, on same slot" > > > + " the dependent device %s which does not enable AER.", > > > + vdev->vbasedev.name, tmp->vbasedev.name); > > > + goto out; > > > + } > > > + > > > + if (tmp->pdev.bus != bus) { > > > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > > > + "the dependent device %s is not on the same bus", > > > + vdev->vbasedev.name, tmp->vbasedev.name); > > > + goto out; > > > + } > > > + found = true; > > > + break; > > > + } > > > + } > > > + > > > + /* Ensure all affected devices assigned to VM */ > > > + if (!found) { > > > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > > > + "the dependent device %04x:%02x:%02x.%x " > > > + "is not assigned to VM.", > > > + vdev->vbasedev.name, host.domain, host.bus, > > > + host.slot, host.function); > > > + goto out; > > > + } > > > + } > > > + > > > + /* > > > + * Check the all pci devices on or below the target bus > > > + * have a reset mechanism at least. > > > + */ > > > + find.pdev = NULL; > > > + find.found = false; > > > + pci_for_each_device(bus, pci_bus_num(bus), > > > + vfio_check_device_noreset, &find); > > > + if (find.found) { > > > + error_setg(errp, "vfio: Cannot enable AER for device %s, " > > > + "the affected device %s does not have a reset mechanism.", > > > + vdev->vbasedev.name, find.pdev->name); > > > + goto out; > > > + } > > > + > > > +out: > > > + g_free(info); > > > + return; > > > +} > > > + > > > +static void vfio_check_devices_host_bus_reset(Error **errp) > > > +{ > > > + VFIOGroup *group; > > > + VFIODevice *vbasedev; > > > + VFIOPCIDevice *vdev; > > > + Error *local_err = NULL; > > > + > > > + /* Check All vfio-pci devices if have bus reset capability */ > > > + QLIST_FOREACH(group, &vfio_group_list, next) { > > > + QLIST_FOREACH(vbasedev, &group->device_list, next) { > > > + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) { > > > + continue; > > > + } > > > + vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev); > > > + if (vdev->features & VFIO_FEATURE_ENABLE_AER) { > > > + vfio_check_host_bus_reset(vdev, &local_err); > > > + if (local_err) { > > > + error_propagate(errp, local_err); > > > + return; > > > + } > > > + } > > > + } > > > + } > > > + > > > + return; > > > +} > > > + > > > static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, > > > int pos, uint16_t size) > > > { > > > @@ -2047,13 +2238,6 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev) > > > vfio_intx_enable(vdev); > > > } > > > > > > -static bool vfio_pci_host_match(PCIHostDeviceAddress *host1, > > > - PCIHostDeviceAddress *host2) > > > -{ > > > - return (host1->domain == host2->domain && host1->bus == host2->bus && > > > - host1->slot == host2->slot && host1->function == host2->function); > > > -} > > > - > > > static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single) > > > { > > > VFIOGroup *group; > > > @@ -2559,6 +2743,21 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) > > > vdev->req_enabled = false; > > > } > > > > > > +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused) > > > +{ > > > + Error *local_err = NULL; > > > + > > > + vfio_check_devices_host_bus_reset(&local_err); > > > + if (local_err) { > > > + error_report_err(local_err); > > > + exit(1); > > > + } > > > +} > > > + > > > +static Notifier machine_notifier = { > > > + .notify = vfio_pci_machine_done_notify, > > > +}; > > > + > > > static int vfio_initfn(PCIDevice *pdev) > > > { > > > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > > > @@ -2905,6 +3104,11 @@ static const TypeInfo vfio_pci_dev_info = { > > > static void register_vfio_pci_dev_type(void) > > > { > > > type_register_static(&vfio_pci_dev_info); > > > + /* > > > + * Register notifier when machine init is done, since we need > > > + * check the configration manner after all vfio device are inited. > > > + */ > > > + qemu_add_machine_init_done_notifier(&machine_notifier); > > > } > > > > > > type_init(register_vfio_pci_dev_type) > > > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h > > > index e0c53f2..aff46c2 100644 > > > --- a/hw/vfio/pci.h > > > +++ b/hw/vfio/pci.h > > > @@ -15,6 +15,7 @@ > > > #include "qemu-common.h" > > > #include "exec/memory.h" > > > #include "hw/pci/pci.h" > > > +#include "hw/pci/pci_bus.h" > > > #include "hw/pci/pci_bridge.h" > > > #include "hw/vfio/vfio-common.h" > > > #include "qemu/event_notifier.h" > > > -- > > > 1.9.3 > > > > > >
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 8ec9b25..0898e34 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1868,6 +1868,197 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos) return 0; } +static bool vfio_pci_host_slot_match(PCIHostDeviceAddress *host1, + PCIHostDeviceAddress *host2) +{ + return (host1->domain == host2->domain && host1->bus == host2->bus && + host1->slot == host2->slot); +} + +static bool vfio_pci_host_match(PCIHostDeviceAddress *host1, + PCIHostDeviceAddress *host2) +{ + return (vfio_pci_host_slot_match(host1, host2) && + host1->function == host2->function); +} + +struct VFIODeviceFind { + PCIDevice *pdev; + bool found; +}; + +static void vfio_check_device_noreset(PCIBus *bus, PCIDevice *pdev, + void *opaque) +{ + DeviceState *dev = DEVICE(pdev); + DeviceClass *dc = DEVICE_GET_CLASS(dev); + VFIOPCIDevice *vdev; + struct VFIODeviceFind *find = opaque; + + if (find->found) { + return; + } + + if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) { + if (!dc->reset) { + goto found; + } + return; + } + vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) && + !vdev->vbasedev.reset_works) { + goto found; + } + + return; +found: + find->pdev = pdev; + find->found = true; +} + +static void vfio_check_host_bus_reset(VFIOPCIDevice *vdev, Error **errp) +{ + PCIBus *bus = vdev->pdev.bus; + struct vfio_pci_hot_reset_info *info = NULL; + struct vfio_pci_dependent_device *devices; + VFIOGroup *group; + struct VFIODeviceFind find; + int ret, i; + + ret = vfio_get_hot_reset_info(vdev, &info); + if (ret) { + error_setg(errp, "vfio: Cannot enable AER for device %s," + " device does not support hot reset.", + vdev->vbasedev.name); + return; + } + + /* List all affected devices by bus reset */ + devices = &info->devices[0]; + + /* Verify that we have all the groups required */ + for (i = 0; i < info->count; i++) { + PCIHostDeviceAddress host; + VFIOPCIDevice *tmp; + VFIODevice *vbasedev_iter; + bool found = false; + + host.domain = devices[i].segment; + host.bus = devices[i].bus; + host.slot = PCI_SLOT(devices[i].devfn); + host.function = PCI_FUNC(devices[i].devfn); + + /* Skip the current device */ + if (vfio_pci_host_match(&host, &vdev->host)) { + continue; + } + + /* Ensure we own the group of the affected device */ + QLIST_FOREACH(group, &vfio_group_list, next) { + if (group->groupid == devices[i].group_id) { + break; + } + } + + if (!group) { + error_setg(errp, "vfio: Cannot enable AER for device %s, " + "depends on group %d which is not owned.", + vdev->vbasedev.name, devices[i].group_id); + goto out; + } + + /* Ensure affected devices for reset on the same slot */ + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) { + continue; + } + tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev); + if (vfio_pci_host_match(&host, &tmp->host)) { + /* + * AER errors may be broadcast to all functions of a multi- + * function endpoint. If any of those sibling functions are + * also assigned, they need to have AER enabled or else an + * error may continue to cause a vm_stop condition. IOW, + * AER setup of this function would be pointless. + */ + if (vfio_pci_host_slot_match(&vdev->host, &tmp->host) && + !(tmp->features & VFIO_FEATURE_ENABLE_AER)) { + error_setg(errp, "vfio: Cannot enable AER for device %s, on same slot" + " the dependent device %s which does not enable AER.", + vdev->vbasedev.name, tmp->vbasedev.name); + goto out; + } + + if (tmp->pdev.bus != bus) { + error_setg(errp, "vfio: Cannot enable AER for device %s, " + "the dependent device %s is not on the same bus", + vdev->vbasedev.name, tmp->vbasedev.name); + goto out; + } + found = true; + break; + } + } + + /* Ensure all affected devices assigned to VM */ + if (!found) { + error_setg(errp, "vfio: Cannot enable AER for device %s, " + "the dependent device %04x:%02x:%02x.%x " + "is not assigned to VM.", + vdev->vbasedev.name, host.domain, host.bus, + host.slot, host.function); + goto out; + } + } + + /* + * Check the all pci devices on or below the target bus + * have a reset mechanism at least. + */ + find.pdev = NULL; + find.found = false; + pci_for_each_device(bus, pci_bus_num(bus), + vfio_check_device_noreset, &find); + if (find.found) { + error_setg(errp, "vfio: Cannot enable AER for device %s, " + "the affected device %s does not have a reset mechanism.", + vdev->vbasedev.name, find.pdev->name); + goto out; + } + +out: + g_free(info); + return; +} + +static void vfio_check_devices_host_bus_reset(Error **errp) +{ + VFIOGroup *group; + VFIODevice *vbasedev; + VFIOPCIDevice *vdev; + Error *local_err = NULL; + + /* Check All vfio-pci devices if have bus reset capability */ + QLIST_FOREACH(group, &vfio_group_list, next) { + QLIST_FOREACH(vbasedev, &group->device_list, next) { + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) { + continue; + } + vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev); + if (vdev->features & VFIO_FEATURE_ENABLE_AER) { + vfio_check_host_bus_reset(vdev, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + } + } + } + + return; +} + static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, int pos, uint16_t size) { @@ -2047,13 +2238,6 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev) vfio_intx_enable(vdev); } -static bool vfio_pci_host_match(PCIHostDeviceAddress *host1, - PCIHostDeviceAddress *host2) -{ - return (host1->domain == host2->domain && host1->bus == host2->bus && - host1->slot == host2->slot && host1->function == host2->function); -} - static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single) { VFIOGroup *group; @@ -2559,6 +2743,21 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) vdev->req_enabled = false; } +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused) +{ + Error *local_err = NULL; + + vfio_check_devices_host_bus_reset(&local_err); + if (local_err) { + error_report_err(local_err); + exit(1); + } +} + +static Notifier machine_notifier = { + .notify = vfio_pci_machine_done_notify, +}; + static int vfio_initfn(PCIDevice *pdev) { VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); @@ -2905,6 +3104,11 @@ static const TypeInfo vfio_pci_dev_info = { static void register_vfio_pci_dev_type(void) { type_register_static(&vfio_pci_dev_info); + /* + * Register notifier when machine init is done, since we need + * check the configration manner after all vfio device are inited. + */ + qemu_add_machine_init_done_notifier(&machine_notifier); } type_init(register_vfio_pci_dev_type) diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index e0c53f2..aff46c2 100644 --- a/hw/vfio/pci.h +++ b/hw/vfio/pci.h @@ -15,6 +15,7 @@ #include "qemu-common.h" #include "exec/memory.h" #include "hw/pci/pci.h" +#include "hw/pci/pci_bus.h" #include "hw/pci/pci_bridge.h" #include "hw/vfio/vfio-common.h" #include "qemu/event_notifier.h"