| Message ID | 1458799079-79825-3-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote: > > --- a/docs/misc/xen-command-line.markdown > +++ b/docs/misc/xen-command-line.markdown > @@ -1532,6 +1532,13 @@ Note that if **watchdog** option is also > specified vpmu will be turned off. > As the virtualisation is not 100% safe, don't use the vpmu flag on > production systems (see http://xenbits.xen.org/xsa/advisory-163.html > )! > > +### vtd\_qi\_timeout (VT-d) > +> `= <integer>` > + > +> Default: `1` > + > +Specify the timeout of the VT-d Queued Invalidation in milliseconds. > + Perhaps we should put down a quick paragraph to explain what this parameter is about, and what happens if the timeout expires? Giving hints on under what conditions one should change this value would be even more welcome (e.g. "if you see error XXX, try increasing the timeout to nnn"). > --- a/xen/drivers/passthrough/vtd/qinval.c > +++ b/xen/drivers/passthrough/vtd/qinval.c > > @@ -130,10 +135,10 @@ static void queue_invalidate_iotlb(struct iommu > > *iommu, > spin_unlock_irqrestore(&iommu->register_lock, flags); > } > > -static int queue_invalidate_wait(struct iommu *iommu, > +static int __must_check queue_invalidate_wait(struct iommu *iommu, > u8 iflag, u8 sw, u8 fn) > { > I'm not sure it is ok to add the __must_check annotation here in this patch, considering that the subject says "VT-d: Reduce spin timeout to 1ms, which can be boot-time changed", and there is no changelog. Personally, I think it could be ok, but at least add a few words of changelog, mentioning that this is also being done. Regards, Dario
On March 24, 2016 7:04pm, <dario.faggioli@citrix.com> wrote: > On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote: > > > > --- a/docs/misc/xen-command-line.markdown > > +++ b/docs/misc/xen-command-line.markdown > > @@ -1532,6 +1532,13 @@ Note that if **watchdog** option is also > > specified vpmu will be turned off. > > As the virtualisation is not 100% safe, don't use the vpmu flag on > > production systems (see http://xenbits.xen.org/xsa/advisory-163.html > > )! > > > > +### vtd\_qi\_timeout (VT-d) > > +> `= <integer>` > > + > > +> Default: `1` > > + > > +Specify the timeout of the VT-d Queued Invalidation in milliseconds. > > + > Perhaps we should put down a quick paragraph to explain what this parameter > is about, and what happens if the timeout expires? > > Giving hints on under what conditions one should change this value would be > even more welcome (e.g. "if you see error XXX, try increasing the timeout to > nnn"). > Make sense. I would send out this paragraph before v9. I hope you could help me correct it. :) > > --- a/xen/drivers/passthrough/vtd/qinval.c > > +++ b/xen/drivers/passthrough/vtd/qinval.c > > > > @@ -130,10 +135,10 @@ static void queue_invalidate_iotlb(struct iommu > > > > *iommu, > > spin_unlock_irqrestore(&iommu->register_lock, flags); > > } > > > > -static int queue_invalidate_wait(struct iommu *iommu, > > +static int __must_check queue_invalidate_wait(struct iommu *iommu, > > u8 iflag, u8 sw, u8 fn) > > { > > > I'm not sure it is ok to add the __must_check annotation here in this patch, > considering that the subject says "VT-d: Reduce spin timeout to 1ms, which can > be boot-time changed", and there is no changelog. > > Personally, I think it could be ok, but at least add a few words of changelog, > mentioning that this is also being done. > Thanks for your reminding. It's better to add a few words of changelog. Quan
On Thu, Mar 24, 2016 at 01:57:58PM +0800, Quan Xu wrote:
Hey!
Thanks for the patch!
I see that you have __must_check..
But if you check the callchain:
iommu_flush_iec_[index|global] ->
__iommu_flush_iec->invalidate_sync-> queue_invalidate_wait
you will see that the callers of iommu_flush_iec_[index|global]
ignore the return value.
So ... perhaps you could explain in this commit description on how to address
that? Is there an followup patch (if so just put in the name in here) to
address that?
Or should the top callers: enable_intremap, ioapic_rte_to_remap_entry,
free_remap_entry, msi_msg_to_remap_entry, and pi_update_irte do something?
I guess the 'free_remap_entry' reall can't. As you are suppose to always
be able to free something.
msi_msg_to_remap_entry, _msg_to_remap_entry, and ioapic_rte_to_remap_entry
could return an value... Or considering this is v8 - was there some epic
conversation that went over this quite a lot? In which case I would recommend
you say why it was not attempted this way so that folks six months from
now when reading this patch won't ask again.
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> ---
> docs/misc/xen-command-line.markdown | 7 +++++++
> xen/drivers/passthrough/vtd/qinval.c | 17 ++++++++++++-----
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index ca77e3b..384dde7 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1532,6 +1532,13 @@ Note that if **watchdog** option is also specified vpmu will be turned off.
> As the virtualisation is not 100% safe, don't use the vpmu flag on
> production systems (see http://xenbits.xen.org/xsa/advisory-163.html)!
>
> +### vtd\_qi\_timeout (VT-d)
> +> `= <integer>`
> +
> +> Default: `1`
> +
> +Specify the timeout of the VT-d Queued Invalidation in milliseconds.
> +
> ### watchdog
> > `= force | <boolean>`
>
> diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c
> index b81b0bd..52ba2c2 100644
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -28,6 +28,11 @@
> #include "vtd.h"
> #include "extern.h"
>
> +static unsigned int __read_mostly vtd_qi_timeout = 1;
> +integer_param("vtd_qi_timeout", vtd_qi_timeout);
> +
> +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1))
> +
> static void print_qi_regs(struct iommu *iommu)
> {
> u64 val;
> @@ -130,10 +135,10 @@ static void queue_invalidate_iotlb(struct iommu *iommu,
> spin_unlock_irqrestore(&iommu->register_lock, flags);
> }
>
> -static int queue_invalidate_wait(struct iommu *iommu,
> +static int __must_check queue_invalidate_wait(struct iommu *iommu,
> u8 iflag, u8 sw, u8 fn)
> {
> - s_time_t start_time;
> + s_time_t timeout;
> volatile u32 poll_slot = QINVAL_STAT_INIT;
> unsigned int index;
> unsigned long flags;
> @@ -164,13 +169,15 @@ static int queue_invalidate_wait(struct iommu *iommu,
> if ( sw )
> {
> /* In case all wait descriptor writes to same addr with same data */
> - start_time = NOW();
> + timeout = NOW() + IOMMU_QI_TIMEOUT;
> while ( poll_slot != QINVAL_STAT_DONE )
> {
> - if ( NOW() > (start_time + DMAR_OPERATION_TIMEOUT) )
> + if ( NOW() > timeout )
> {
> print_qi_regs(iommu);
> - panic("queue invalidate wait descriptor was not executed");
> + printk(XENLOG_WARNING VTDPREFIX
> + "Queue invalidate wait descriptor timed out.\n");
> + return -ETIMEDOUT;
> }
> cpu_relax();
> }
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
On March 26, 2016 4:07am, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Thu, Mar 24, 2016 at 01:57:58PM +0800, Quan Xu wrote: > > Hey! > > Thanks for the patch! > > I see that you have __must_check.. > > But if you check the callchain: > > iommu_flush_iec_[index|global] -> > __iommu_flush_iec->invalidate_sync-> queue_invalidate_wait > > you will see that the callers of iommu_flush_iec_[index|global] ignore the > return value. > > So ... perhaps you could explain in this commit description on how to address > that? I mentioned it in 0000-cover-letter.patch -- "Not covered in this series:". IMO, it is not a good idea to explain in this commit description, as I don't touch it. Right? > Is there an followup patch (if so just put in the name in here) to address > that? > Yes, > Or should the top callers: enable_intremap, ioapic_rte_to_remap_entry, > free_remap_entry, msi_msg_to_remap_entry, and pi_update_irte do > something? > I'd prefer to discuss about it in another thread. btw, I have spent a lot of time on Interrupt research, including the logical topology of interrupt hardware/software, .e.g, lapci/vlapic/ioapic/vioapic/msi/PI. In last months, I wondered whether I could disable the interrupt remapping dynamically( by 'iommu_intremap = 0') or not. Now I think it would be insecure. IMO, it is not a good time to discuss this new topic, otherwise it makes me exhausted. Quan > I guess the 'free_remap_entry' reall can't. As you are suppose to always be able > to free something. > > > msi_msg_to_remap_entry, _msg_to_remap_entry, and > ioapic_rte_to_remap_entry could return an value... Or considering this is v8 - > was there some epic conversation that went over this quite a lot? In which case > I would recommend you say why it was not attempted this way so that folks six > months from now when reading this patch won't ask again. > > > > Signed-off-by: Quan Xu <quan.xu@intel.com> > > --- > > docs/misc/xen-command-line.markdown | 7 +++++++ > > xen/drivers/passthrough/vtd/qinval.c | 17 ++++++++++++----- > > 2 files changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/docs/misc/xen-command-line.markdown > > b/docs/misc/xen-command-line.markdown > > index ca77e3b..384dde7 100644 > > --- a/docs/misc/xen-command-line.markdown > > +++ b/docs/misc/xen-command-line.markdown > > @@ -1532,6 +1532,13 @@ Note that if **watchdog** option is also > specified vpmu will be turned off. > > As the virtualisation is not 100% safe, don't use the vpmu flag on > > production systems (see http://xenbits.xen.org/xsa/advisory-163.html)! > > > > +### vtd\_qi\_timeout (VT-d) > > +> `= <integer>` > > + > > +> Default: `1` > > + > > +Specify the timeout of the VT-d Queued Invalidation in milliseconds. > > + > > ### watchdog > > > `= force | <boolean>` > > > > diff --git a/xen/drivers/passthrough/vtd/qinval.c > > b/xen/drivers/passthrough/vtd/qinval.c > > index b81b0bd..52ba2c2 100644 > > --- a/xen/drivers/passthrough/vtd/qinval.c > > +++ b/xen/drivers/passthrough/vtd/qinval.c > > @@ -28,6 +28,11 @@ > > #include "vtd.h" > > #include "extern.h" > > > > +static unsigned int __read_mostly vtd_qi_timeout = 1; > > +integer_param("vtd_qi_timeout", vtd_qi_timeout); > > + > > +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) > > + > > static void print_qi_regs(struct iommu *iommu) { > > u64 val; > > @@ -130,10 +135,10 @@ static void queue_invalidate_iotlb(struct iommu > *iommu, > > spin_unlock_irqrestore(&iommu->register_lock, flags); } > > > > -static int queue_invalidate_wait(struct iommu *iommu, > > +static int __must_check queue_invalidate_wait(struct iommu *iommu, > > u8 iflag, u8 sw, u8 fn) > > { > > - s_time_t start_time; > > + s_time_t timeout; > > volatile u32 poll_slot = QINVAL_STAT_INIT; > > unsigned int index; > > unsigned long flags; > > @@ -164,13 +169,15 @@ static int queue_invalidate_wait(struct iommu > *iommu, > > if ( sw ) > > { > > /* In case all wait descriptor writes to same addr with same data > */ > > - start_time = NOW(); > > + timeout = NOW() + IOMMU_QI_TIMEOUT; > > while ( poll_slot != QINVAL_STAT_DONE ) > > { > > - if ( NOW() > (start_time + DMAR_OPERATION_TIMEOUT) ) > > + if ( NOW() > timeout ) > > { > > print_qi_regs(iommu); > > - panic("queue invalidate wait descriptor was not > executed"); > > + printk(XENLOG_WARNING VTDPREFIX > > + "Queue invalidate wait descriptor timed > out.\n"); > > + return -ETIMEDOUT; > > } > > cpu_relax(); > > } > > -- > > 1.9.1 > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On Mon, Mar 28, 2016 at 06:27:21AM +0000, Xu, Quan wrote: > On March 26, 2016 4:07am, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > On Thu, Mar 24, 2016 at 01:57:58PM +0800, Quan Xu wrote: > > > > Hey! > > > > Thanks for the patch! > > > > I see that you have __must_check.. > > > > But if you check the callchain: > > > > iommu_flush_iec_[index|global] -> > > __iommu_flush_iec->invalidate_sync-> queue_invalidate_wait > > > > you will see that the callers of iommu_flush_iec_[index|global] ignore the > > return value. > > > > So ... perhaps you could explain in this commit description on how to address > > that? > > I mentioned it in 0000-cover-letter.patch -- "Not covered in this series:". But that goodness never gets documented in the code.. > IMO, it is not a good idea to explain in this commit description, as I don't touch it. > Right? Why? Put yourself in the shoes of a an engineer who wants to fix or add some code. He or she will look at the code and say: Ok, they added __mustcheck so they MUST have thought it is important to check the return code. But then tracking through the different users and then it is ignored two or three levels up. Huh? Why didn't the account for that? If you include that information in the description it will save them further work from having to guess or email you. > > > Is there an followup patch (if so just put in the name in here) to address > > that? > > > Yes, In which case I would just say that: "The followup patch titled XYZ addresses the __mustcheck. That is the other callers of this routine (two or three levels up) ignore the return code. This patch does not address this but the other does." And that > > > Or should the top callers: enable_intremap, ioapic_rte_to_remap_entry, > > free_remap_entry, msi_msg_to_remap_entry, and pi_update_irte do > > something? > > > > I'd prefer to discuss about it in another thread. btw, I have spent a lot of time on > Interrupt research, including the logical topology of interrupt hardware/software, .e.g, lapci/vlapic/ioapic/vioapic/msi/PI. > In last months, I wondered whether I could disable the interrupt remapping dynamically( by 'iommu_intremap = 0') or not. > Now I think it would be insecure. > IMO, it is not a good time to discuss this new topic, otherwise it makes me exhausted. <grins> I would recommend you send out an email to xen-devel with [BRAINSTORM] or such to explain the problem or ways to make it work. That way we can all benefit from your hard digging in the code and untangling it. Or if you can - be at the Xen Hackathon in April to brainstorm this in person? > > Quan > > > I guess the 'free_remap_entry' reall can't. As you are suppose to always be able > > to free something. > > > > > > msi_msg_to_remap_entry, _msg_to_remap_entry, and > > ioapic_rte_to_remap_entry could return an value... Or considering this is v8 - > > was there some epic conversation that went over this quite a lot? In which case > > I would recommend you say why it was not attempted this way so that folks six > > months from now when reading this patch won't ask again. > > > > > > > Signed-off-by: Quan Xu <quan.xu@intel.com> > > > --- > > > docs/misc/xen-command-line.markdown | 7 +++++++ > > > xen/drivers/passthrough/vtd/qinval.c | 17 ++++++++++++----- > > > 2 files changed, 19 insertions(+), 5 deletions(-) > > > > > > diff --git a/docs/misc/xen-command-line.markdown > > > b/docs/misc/xen-command-line.markdown > > > index ca77e3b..384dde7 100644 > > > --- a/docs/misc/xen-command-line.markdown > > > +++ b/docs/misc/xen-command-line.markdown > > > @@ -1532,6 +1532,13 @@ Note that if **watchdog** option is also > > specified vpmu will be turned off. > > > As the virtualisation is not 100% safe, don't use the vpmu flag on > > > production systems (see http://xenbits.xen.org/xsa/advisory-163.html)! > > > > > > +### vtd\_qi\_timeout (VT-d) > > > +> `= <integer>` > > > + > > > +> Default: `1` > > > + > > > +Specify the timeout of the VT-d Queued Invalidation in milliseconds. > > > + > > > ### watchdog > > > > `= force | <boolean>` > > > > > > diff --git a/xen/drivers/passthrough/vtd/qinval.c > > > b/xen/drivers/passthrough/vtd/qinval.c > > > index b81b0bd..52ba2c2 100644 > > > --- a/xen/drivers/passthrough/vtd/qinval.c > > > +++ b/xen/drivers/passthrough/vtd/qinval.c > > > @@ -28,6 +28,11 @@ > > > #include "vtd.h" > > > #include "extern.h" > > > > > > +static unsigned int __read_mostly vtd_qi_timeout = 1; > > > +integer_param("vtd_qi_timeout", vtd_qi_timeout); > > > + > > > +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) > > > + > > > static void print_qi_regs(struct iommu *iommu) { > > > u64 val; > > > @@ -130,10 +135,10 @@ static void queue_invalidate_iotlb(struct iommu > > *iommu, > > > spin_unlock_irqrestore(&iommu->register_lock, flags); } > > > > > > -static int queue_invalidate_wait(struct iommu *iommu, > > > +static int __must_check queue_invalidate_wait(struct iommu *iommu, > > > u8 iflag, u8 sw, u8 fn) > > > { > > > - s_time_t start_time; > > > + s_time_t timeout; > > > volatile u32 poll_slot = QINVAL_STAT_INIT; > > > unsigned int index; > > > unsigned long flags; > > > @@ -164,13 +169,15 @@ static int queue_invalidate_wait(struct iommu > > *iommu, > > > if ( sw ) > > > { > > > /* In case all wait descriptor writes to same addr with same data > > */ > > > - start_time = NOW(); > > > + timeout = NOW() + IOMMU_QI_TIMEOUT; > > > while ( poll_slot != QINVAL_STAT_DONE ) > > > { > > > - if ( NOW() > (start_time + DMAR_OPERATION_TIMEOUT) ) > > > + if ( NOW() > timeout ) > > > { > > > print_qi_regs(iommu); > > > - panic("queue invalidate wait descriptor was not > > executed"); > > > + printk(XENLOG_WARNING VTDPREFIX > > > + "Queue invalidate wait descriptor timed > > out.\n"); > > > + return -ETIMEDOUT; > > > } > > > cpu_relax(); > > > } > > > -- > > > 1.9.1 > > > > > > > > > _______________________________________________ > > > Xen-devel mailing list > > > Xen-devel@lists.xen.org > > > http://lists.xen.org/xen-devel > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel
On March 28, 2016 9:32pm, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Mon, Mar 28, 2016 at 06:27:21AM +0000, Xu, Quan wrote: > > On March 26, 2016 4:07am, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > wrote: > > > On Thu, Mar 24, 2016 at 01:57:58PM +0800, Quan Xu wrote: > <grins> > <grins> DX > I would recommend you send out an email to xen-devel with [BRAINSTORM] or Yes, I will do it. > such to explain the problem or ways to make it work. > That way we can all benefit from your hard digging in the code and untangling it. > > Or if you can - be at the Xen Hackathon in April to brainstorm this in person? > I would be very happy, if I can have a f2f discussion with you, Jan, Dario, Wei, Daniel, Stefano ... But sorry, I can't. I have the problems with my passport. I am in Shanghai China now. Quan
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown index ca77e3b..384dde7 100644 --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -1532,6 +1532,13 @@ Note that if **watchdog** option is also specified vpmu will be turned off. As the virtualisation is not 100% safe, don't use the vpmu flag on production systems (see http://xenbits.xen.org/xsa/advisory-163.html)! +### vtd\_qi\_timeout (VT-d) +> `= <integer>` + +> Default: `1` + +Specify the timeout of the VT-d Queued Invalidation in milliseconds. + ### watchdog > `= force | <boolean>` diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c index b81b0bd..52ba2c2 100644 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -28,6 +28,11 @@ #include "vtd.h" #include "extern.h" +static unsigned int __read_mostly vtd_qi_timeout = 1; +integer_param("vtd_qi_timeout", vtd_qi_timeout); + +#define IOMMU_QI_TIMEOUT (vtd_qi_timeout * MILLISECS(1)) + static void print_qi_regs(struct iommu *iommu) { u64 val; @@ -130,10 +135,10 @@ static void queue_invalidate_iotlb(struct iommu *iommu, spin_unlock_irqrestore(&iommu->register_lock, flags); } -static int queue_invalidate_wait(struct iommu *iommu, +static int __must_check queue_invalidate_wait(struct iommu *iommu, u8 iflag, u8 sw, u8 fn) { - s_time_t start_time; + s_time_t timeout; volatile u32 poll_slot = QINVAL_STAT_INIT; unsigned int index; unsigned long flags; @@ -164,13 +169,15 @@ static int queue_invalidate_wait(struct iommu *iommu, if ( sw ) { /* In case all wait descriptor writes to same addr with same data */ - start_time = NOW(); + timeout = NOW() + IOMMU_QI_TIMEOUT; while ( poll_slot != QINVAL_STAT_DONE ) { - if ( NOW() > (start_time + DMAR_OPERATION_TIMEOUT) ) + if ( NOW() > timeout ) { print_qi_regs(iommu); - panic("queue invalidate wait descriptor was not executed"); + printk(XENLOG_WARNING VTDPREFIX + "Queue invalidate wait descriptor timed out.\n"); + return -ETIMEDOUT; } cpu_relax(); }
Signed-off-by: Quan Xu <quan.xu@intel.com> --- docs/misc/xen-command-line.markdown | 7 +++++++ xen/drivers/passthrough/vtd/qinval.c | 17 ++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-)