| Message ID | 5571ABBA0200007800081543@mail.emea.novell.com |
|---|---|
| State | New |
| Headers | show |
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@suse.com> > > --- 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; > > >
>>> On 16.06.15 at 16:03, <stefano.stabellini@eu.citrix.com> wrote: > On Fri, 5 Jun 2015, Jan Beulich wrote: >> + } 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? We're at an else if inside an else if here. The only case left after the if() still seen above is that value has PCI_MSIX_FLAGS_MASKALL set. >> + 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. We're not doing any cfg register write via hypercalls (not here, and not elsewhere). What is being replaced by the patch are write to two bits which happen to live in PCI config space. Plus, reading directly, and doing writes via hypercall only when really needed would still be the right thing from a performance pov. >> --- 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? Perhaps, yes. I think I actually had suggested so quite a while back. But I don't see myself wasting much more time on this, ehm, code. Jan
On Tue, 16 Jun 2015, Jan Beulich wrote: > >>> On 16.06.15 at 16:03, <stefano.stabellini@eu.citrix.com> wrote: > > On Fri, 5 Jun 2015, Jan Beulich wrote: > >> + } 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? > > We're at an else if inside an else if here. The only case left > after the if() still seen above is that value has > PCI_MSIX_FLAGS_MASKALL set. You are right. Maybe just add /* (value & PCI_MSIX_FLAGS_MASKALL) at this point */ > >> + 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. > > We're not doing any cfg register write via hypercalls (not here, > and not elsewhere). What is being replaced by the patch are > write to two bits which happen to live in PCI config space. Plus, > reading directly, and doing writes via hypercall only when really > needed would still be the right thing from a performance pov. OK. It would be nice to document somewhere that QEMU is not supposed to touch those two bits directly, but I wouldn't know where. Maybe a new doc under docs/misc? > >> --- 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? > > Perhaps, yes. I think I actually had suggested so quite a while back. > But I don't see myself wasting much more time on this, ehm, code. Isn't it just a matter of removing msi_msix_enable?
>>> On 16.06.15 at 16:56, <stefano.stabellini@eu.citrix.com> wrote: > On Tue, 16 Jun 2015, Jan Beulich wrote: >> >>> On 16.06.15 at 16:03, <stefano.stabellini@eu.citrix.com> wrote: >> > On Fri, 5 Jun 2015, Jan Beulich wrote: >> >> --- 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? >> >> Perhaps, yes. I think I actually had suggested so quite a while back. >> But I don't see myself wasting much more time on this, ehm, code. > > Isn't it just a matter of removing msi_msix_enable? It has another caller xen_pt_msi_set_enable(). If we went down the route of what this patch does, then MSI's enable bit should ultimately also be driven through a hypercall, and that would then be the point where the function would naturally disappear. But as said, it looks like we're intending to go a different route anyway. Jan
--- 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) - && !(*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); + } 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) { + 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); + + 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); + 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;