| Message ID | 20170911091628.22976-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 11.09.17 at 11:16, <roger.pau@citrix.com> wrote: This being an x86 only change, the subject prefix would presumably better be "x86/msi". > --- a/xen/arch/x86/msi.c > +++ b/xen/arch/x86/msi.c > @@ -657,7 +657,7 @@ int msi_free_irq(struct msi_desc *entry) > return 0; > } > > -static struct msi_desc *find_msi_entry(struct pci_dev *dev, > +static struct msi_desc *find_msi_entry(const struct pci_dev *dev, > int irq, int cap_id) > { I certainly agree with this part, but ... > @@ -1274,7 +1274,7 @@ void pci_cleanup_msi(struct pci_dev *pdev) > msi_free_irqs(pdev); > } > > -int pci_msi_conf_write_intercept(struct pci_dev *pdev, unsigned int reg, > +int pci_msi_conf_write_intercept(const struct pci_dev *pdev, unsigned int reg, > unsigned int size, uint32_t *data) > { ... I'm not so sure about this one. The function changes data associated with the device, and it just so happens that right now all such changes are confined to the separately allocated msix structure. Do other changes of yours depend on the parameter becoming const here? I'm trying to understand what scope an undo of this change would be, should it turn out necessary down the road. Jan
On Mon, Sep 11, 2017 at 04:11:41AM -0600, Jan Beulich wrote: > >>> On 11.09.17 at 11:16, <roger.pau@citrix.com> wrote: > > This being an x86 only change, the subject prefix would presumably > better be "x86/msi". Right. I guess you don't mind changing this upon commit if it gets accepted. > > --- a/xen/arch/x86/msi.c > > +++ b/xen/arch/x86/msi.c > > @@ -657,7 +657,7 @@ int msi_free_irq(struct msi_desc *entry) > > return 0; > > } > > > > -static struct msi_desc *find_msi_entry(struct pci_dev *dev, > > +static struct msi_desc *find_msi_entry(const struct pci_dev *dev, > > int irq, int cap_id) > > { > > I certainly agree with this part, but ... > > > @@ -1274,7 +1274,7 @@ void pci_cleanup_msi(struct pci_dev *pdev) > > msi_free_irqs(pdev); > > } > > > > -int pci_msi_conf_write_intercept(struct pci_dev *pdev, unsigned int reg, > > +int pci_msi_conf_write_intercept(const struct pci_dev *pdev, unsigned int reg, > > unsigned int size, uint32_t *data) > > { > > ... I'm not so sure about this one. The function changes data > associated with the device, and it just so happens that right > now all such changes are confined to the separately allocated > msix structure. Do other changes of yours depend on the > parameter becoming const here? I'm trying to understand > what scope an undo of this change would be, should it turn > out necessary down the road. This is used at: https://lists.xenproject.org/archives/html/xen-devel/2017-08/msg01511.html In the vpci_msix_control_write handler. We have spoken about constifying the pci_dev of the handlers, and here the pci_dev argument is being used to call into pci_msi_conf_write_intercept. Now that I look again at the code, I think I can leave pci_msi_conf_write_intercept alone, since in vpci_msix_control_write I can also get the pci_dev from msix->pdev, and that's not constified. I will send the find_msi_entry chunk separately, which I still think it's worth. Thanks, Roger.
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c index 77998f4fb3..f9f0d9d5c3 100644 --- a/xen/arch/x86/msi.c +++ b/xen/arch/x86/msi.c @@ -657,7 +657,7 @@ int msi_free_irq(struct msi_desc *entry) return 0; } -static struct msi_desc *find_msi_entry(struct pci_dev *dev, +static struct msi_desc *find_msi_entry(const struct pci_dev *dev, int irq, int cap_id) { struct msi_desc *entry; @@ -1274,7 +1274,7 @@ void pci_cleanup_msi(struct pci_dev *pdev) msi_free_irqs(pdev); } -int pci_msi_conf_write_intercept(struct pci_dev *pdev, unsigned int reg, +int pci_msi_conf_write_intercept(const struct pci_dev *pdev, unsigned int reg, unsigned int size, uint32_t *data) { u16 seg = pdev->seg; diff --git a/xen/include/asm-x86/pci.h b/xen/include/asm-x86/pci.h index 36801d317b..29520164fe 100644 --- a/xen/include/asm-x86/pci.h +++ b/xen/include/asm-x86/pci.h @@ -18,7 +18,7 @@ struct arch_pci_dev { int pci_conf_write_intercept(unsigned int seg, unsigned int bdf, unsigned int reg, unsigned int size, uint32_t *data); -int pci_msi_conf_write_intercept(struct pci_dev *, unsigned int reg, +int pci_msi_conf_write_intercept(const struct pci_dev *, unsigned int reg, unsigned int size, uint32_t *data); bool_t pci_mmcfg_decode(unsigned long mfn, unsigned int *seg, unsigned int *bdf);
And the one for find_msi_entry. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Jan Beulich <jbeulich@suse.com> Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/msi.c | 4 ++-- xen/include/asm-x86/pci.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)