Re: [Xen-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 5 Jun 2015, Jan Beulich wrote:
> Particularly the maskall bit has to be under exclusive hypervisor
> control (and since they live in the same config space field, the
> enable bit has to follow suit). Use the replacement hypercall
> interfaces.

From this commit message, I expect a patch that simply substitutes
maskall and enable writings with hypercalls and nothing else.


> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/qemu/upstream/hw/xen/xen_pt.h
> +++ b/qemu/upstream/hw/xen/xen_pt.h
> @@ -181,6 +181,7 @@ typedef struct XenPTMSIXEntry {
>  typedef struct XenPTMSIX {
>      uint32_t ctrl_offset;
>      bool enabled;
> +    bool maskall;
>      int total_entries;
>      int bar_index;
>      uint64_t table_base;
> @@ -293,7 +294,9 @@ int xen_pt_msix_init(XenPCIPassthroughSt
>  void xen_pt_msix_delete(XenPCIPassthroughState *s);
>  int xen_pt_msix_update(XenPCIPassthroughState *s);
>  int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index);
> +void xen_pt_msix_enable(XenPCIPassthroughState *s);
>  void xen_pt_msix_disable(XenPCIPassthroughState *s);
> +int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask);
>  
>  static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int 
> bar)
>  {
> --- a/qemu/upstream/hw/xen/xen_pt_config_init.c
> +++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
> @@ -1436,32 +1436,58 @@ static int xen_pt_msixctrl_reg_write(Xen
>                                       uint16_t dev_value, uint16_t valid_mask)
>  {
>      XenPTRegInfo *reg = cfg_entry->reg;
> -    uint16_t writable_mask = 0;
> +    uint16_t writable_mask, value;
>      uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
>      int debug_msix_enabled_old;
>  
>      /* modify emulate register */
>      writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, 
> writable_mask);
> +    value = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> +    cfg_entry->data = value;
>
>      /* create value for writing to I/O device register */
>      *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
>  
> +    debug_msix_enabled_old = s->msix->enabled;
> +
>      /* update MSI-X */
> -    if ((*val & PCI_MSIX_FLAGS_ENABLE)

Please explain in the commit message the reason of the *val/value
change.


> -        && !(*val & PCI_MSIX_FLAGS_MASKALL)) {
> +    if ((value & PCI_MSIX_FLAGS_ENABLE)
> +        && !(value & PCI_MSIX_FLAGS_MASKALL)) {
> +        if (!s->msix->enabled) {
> +            if (!s->msix->maskall) {
> +                xen_pt_msix_maskall(s, true);
> +            }
> +            xen_pt_msix_enable(s);
> +        }
>          xen_pt_msix_update(s);
> -    } else if (!(*val & PCI_MSIX_FLAGS_ENABLE) && s->msix->enabled) {
> -        xen_pt_msix_disable(s);
> +        s->msix->enabled = true;
> +        s->msix->maskall = false;
> +        xen_pt_msix_maskall(s, false);

Please explain in the commit message the reason behind the
maskall->enable->update->un_maskall logic, that wasn't present before.


> +    } else if (s->msix->enabled) {
> +        if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
> +            xen_pt_msix_disable(s);
> +            s->msix->enabled = false;
> +        } else if (!s->msix->maskall) {

Why are you changing the state of s->msix->maskall here?
This is the value & PCI_MSIX_FLAGS_ENABLE case, nothing to do with
maskall, right?


> +            s->msix->maskall = true;
> +            xen_pt_msix_maskall(s, true);
> +        }
>      }
>  
> -    debug_msix_enabled_old = s->msix->enabled;
> -    s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
>      if (s->msix->enabled != debug_msix_enabled_old) {
>          XEN_PT_LOG(&s->dev, "%s MSI-X\n",
>                     s->msix->enabled ? "enable" : "disable");
>      }
>  
> +    xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, &dev_value);

I have to say that I don't like the asymmetry between reading and
writing PCI config registers. If writes go via hypercalls, reads should
go via hypercalls too.


> +    if (s->msix->enabled && !(dev_value & PCI_MSIX_FLAGS_ENABLE)) {
> +        XEN_PT_ERR(&s->dev, "MSI-X unexpectedly disabled\n");
> +    } else if ((dev_value & PCI_MSIX_FLAGS_ENABLE) &&
> +               s->msix->maskall &&
> +               !(dev_value & PCI_MSIX_FLAGS_MASKALL)) {
> +        XEN_PT_ERR(&s->dev, "MSI-X unexpectedly unmasked\n");
> +    }
> +
>      return 0;
>  }
>  
> @@ -1483,9 +1509,12 @@ static XenPTRegInfo xen_pt_emu_reg_msix[
>          .offset     = PCI_MSI_FLAGS,
>          .size       = 2,
>          .init_val   = 0x0000,
> -        .res_mask   = 0x3800,
> -        .ro_mask    = 0x07FF,
> -        .emu_mask   = 0x0000,
> +        /* This must not be split into res_mask (0x3800) and ro_mask (0x07FF)
> +         * because even in permissive mode there must not be any write back
> +         * to this register.
> +         */
> +        .ro_mask    = 0x3FFF,
> +        .emu_mask   = 0xC000,
>          .init       = xen_pt_msixctrl_reg_init,
>          .u.w.read   = xen_pt_word_reg_read,
>          .u.w.write  = xen_pt_msixctrl_reg_write,
> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
> @@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
>          return -1;
>      }
>  
> -    return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
> -                           enabled);

Would it make sense to remove msi_msix_enable completely to avoid any
further mistakes?


> +    return xc_physdev_msix_enable(xen_xc, s->real_device.domain,
> +                                  s->real_device.bus,
> +                                  PCI_DEVFN(s->real_device.dev,
> +                                            s->real_device.func),
> +                                  enabled);
>  }
>  
>  static int xen_pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr)
> @@ -361,6 +364,11 @@ int xen_pt_msix_update(XenPCIPassthrough
>      return 0;
>  }
>  
> +void xen_pt_msix_enable(XenPCIPassthroughState *s)
> +{
> +    msix_set_enable(s, true);
> +}
> +
>  void xen_pt_msix_disable(XenPCIPassthroughState *s)
>  {
>      int i = 0;
> @@ -378,6 +386,15 @@ void xen_pt_msix_disable(XenPCIPassthrou
>      }
>  }
>  
> +int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask)
> +{
> +    return xc_physdev_msix_mask_all(xen_xc, s->real_device.domain,
> +                                    s->real_device.bus,
> +                                    PCI_DEVFN(s->real_device.dev,
> +                                              s->real_device.func),
> +                                    mask);
> +}
> +
>  int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index)
>  {
>      XenPTMSIXEntry *entry;
> 
> 
> 

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