From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls Date: Tue, 16 Jun 2015 15:03:22 +0100 Message-ID: References: <5571AA3B020000780008152E@mail.emea.novell.com> <5571ABBA0200007800081543@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z4rTe-0001qG-Pr for xen-devel@lists.xenproject.org; Tue, 16 Jun 2015 14:04:35 +0000 In-Reply-To: <5571ABBA0200007800081543@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , qemu-devel@nongnu.org, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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 > > --- 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; > > >