Re: [Xen-devel] [PATCH v1 08/10] xen/pt: Make xen_pt_unregister_device idempotent

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> To deal with xen_host_pci_[set|get]_ functions returning error values
> and clearing ourselves in the init function we should make the
> .exit (xen_pt_unregister_device) function be idempotent in case
> the generic code starts calling .exit (or for fun does it before
> calling .init!).
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  hw/xen/xen-host-pci-device.c |  5 +++++
>  hw/xen/xen-host-pci-device.h |  1 +
>  hw/xen/xen_pt.c              | 22 ++++++++++++++++------
>  hw/xen/xen_pt.h              |  2 ++
>  4 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 743b37b..5b20570 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -387,6 +387,11 @@ error:
>      return rc;
>  }
>  
> +bool xen_host_pci_device_closed(XenHostPCIDevice *d)
> +{
> +    return d->config_fd == -1;
> +}
> +
>  void xen_host_pci_device_put(XenHostPCIDevice *d)
>  {
>      if (d->config_fd >= 0) {
> diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
> index c2486f0..16f4805 100644
> --- a/hw/xen/xen-host-pci-device.h
> +++ b/hw/xen/xen-host-pci-device.h
> @@ -38,6 +38,7 @@ typedef struct XenHostPCIDevice {
>  int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>                              uint8_t bus, uint8_t dev, uint8_t func);
>  void xen_host_pci_device_put(XenHostPCIDevice *pci_dev);
> +bool xen_host_pci_device_closed(XenHostPCIDevice *d);
>  
>  int xen_host_pci_get_byte(XenHostPCIDevice *d, int pos, uint8_t *p);
>  int xen_host_pci_get_word(XenHostPCIDevice *d, int pos, uint16_t *p);
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 7575311..a094f7c 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -807,6 +807,7 @@ out:
>  
>      memory_listener_register(&s->memory_listener, &s->dev.bus_master_as);
>      memory_listener_register(&s->io_listener, &address_space_io);
> +    s->listener_set = true;
>      XEN_PT_LOG(d,
>                 "Real physical device %02x:%02x.%d registered 
> successfully!\n",
>                 s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function);
> @@ -818,10 +819,13 @@ static void xen_pt_unregister_device(PCIDevice *d)
>  {
>      XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
>      uint8_t machine_irq = s->machine_irq;
> -    uint8_t intx = xen_pt_pci_intx(s);
> +    uint8_t intx;
>      int rc;
>  
> -    if (machine_irq) {
> +     /* Note that if xen_host_pci_device_put had closed config_fd, then
> +      * intx value becomes 0xff. */
> +    intx = xen_pt_pci_intx(s);

Wouldn't it make sense to move this assignment inside the if below?

Aside from this small item:

Reviewed-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>


> +    if (machine_irq && !xen_host_pci_device_closed(&s->real_device)) {
>
>          rc = xc_domain_unbind_pt_irq(xen_xc, xen_domid, machine_irq,
>                                       PT_IRQ_TYPE_PCI,
>                                       pci_bus_num(d->bus),
> @@ -836,6 +840,7 @@ static void xen_pt_unregister_device(PCIDevice *d)
>          }
>      }
>  
> +    /* N.B. xen_pt_config_delete takes care of freeing them. */
>      if (s->msi) {
>          xen_pt_msi_disable(s);
>      }
> @@ -855,15 +860,20 @@ static void xen_pt_unregister_device(PCIDevice *d)
>                             machine_irq, errno);
>              }
>          }
> +        s->machine_irq = 0;
>      }
>  
>      /* delete all emulated config registers */
>      xen_pt_config_delete(s);
>  
> -    memory_listener_unregister(&s->memory_listener);
> -    memory_listener_unregister(&s->io_listener);
> -
> -    xen_host_pci_device_put(&s->real_device);
> +    if (s->listener_set) {
> +        memory_listener_unregister(&s->memory_listener);
> +        memory_listener_unregister(&s->io_listener);
> +        s->listener_set = false;
> +    }
> +    if (!xen_host_pci_device_closed(&s->real_device)) {
> +        xen_host_pci_device_put(&s->real_device);
> +    }
>  }
>  
>  static Property xen_pci_passthrough_properties[] = {
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 586d055..2b58521 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -221,6 +221,7 @@ struct XenPCIPassthroughState {
>  
>      MemoryListener memory_listener;
>      MemoryListener io_listener;
> +    bool listener_set;
>  };
>  
>  int xen_pt_config_init(XenPCIPassthroughState *s);
> @@ -286,6 +287,7 @@ static inline uint8_t 
> xen_pt_pci_intx(XenPCIPassthroughState *s)
>                     " value=%i, acceptable range is 1 - 4\n", r_val);
>          r_val = 0;
>      } else {
> +        /* Note that if s.real_device.config_fd is closed we make 0xff. */
>          r_val -= 1;
>      }
>  
> -- 
> 2.1.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel