From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
qemu-devel@nongnu.org,
Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Subject: Re: [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls
Date: Tue, 16 Jun 2015 15:03:22 +0100 [thread overview]
Message-ID: <alpine.DEB.2.02.1506161438450.21829__32209.5891031951$1434463586$gmane$org@kaball.uk.xensource.com> (raw)
In-Reply-To: <5571ABBA0200007800081543@mail.emea.novell.com>
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;
>
>
>
next prev parent reply other threads:[~2015-06-16 14:04 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-05 11:55 [Qemu-devel] [PATCH 0/6] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
2015-06-05 11:59 ` [Qemu-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes Jan Beulich
2015-06-05 11:59 ` Jan Beulich
2015-06-16 13:35 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:04 ` Jan Beulich
2015-06-16 14:48 ` Stefano Stabellini
2015-06-16 14:48 ` Stefano Stabellini
2015-06-16 14:04 ` Jan Beulich
2015-06-16 13:35 ` Stefano Stabellini
2015-06-05 12:01 ` [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls Jan Beulich
2015-06-05 12:01 ` Jan Beulich
2015-06-16 14:03 ` Stefano Stabellini [this message]
2015-06-16 14:03 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:19 ` Jan Beulich
2015-06-16 14:56 ` Stefano Stabellini
2015-06-16 14:56 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 16:03 ` Jan Beulich
2015-06-16 16:03 ` Jan Beulich
2015-06-16 14:19 ` Jan Beulich
2015-06-05 12:02 ` [Qemu-devel] [PATCH 3/6] xen/MSI-X: really enforce alignment Jan Beulich
2015-06-05 12:02 ` Jan Beulich
2015-06-16 14:08 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:08 ` Stefano Stabellini
2015-06-05 12:03 ` [Qemu-devel] [PATCH 4/6] xen/pass-through: correctly deal with RW1C bits Jan Beulich
2015-06-05 12:03 ` Jan Beulich
2015-06-16 14:19 ` Stefano Stabellini
2015-06-16 14:19 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:38 ` Jan Beulich
2015-06-16 14:38 ` Jan Beulich
2015-06-05 12:04 ` [Qemu-devel] [PATCH 5/6] xen/pass-through: log errno values rather than function return ones Jan Beulich
2015-06-05 12:04 ` Jan Beulich
2015-06-16 14:23 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:23 ` Stefano Stabellini
2015-06-05 12:04 ` [Qemu-devel] [PATCH 6/6] xen/pass-through: constify some static data Jan Beulich
2015-06-05 12:04 ` Jan Beulich
2015-06-16 14:27 ` Stefano Stabellini
2015-06-16 14:27 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:41 ` Jan Beulich
2015-06-16 14:43 ` Stefano Stabellini
2015-06-16 14:43 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:41 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='alpine.DEB.2.02.1506161438450.21829__32209.5891031951$1434463586$gmane$org@kaball.uk.xensource.com' \
--to=stefano.stabellini@eu.citrix.com \
--cc=JBeulich@suse.com \
--cc=qemu-devel@nongnu.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.