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 <jbeul...@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;
>
>
>
