| Message ID | 1502310866-10450-20-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 09, 2017 at 04:34:20PM -0400, Lan Tianyu wrote: > From: Chao Gao <chao.gao@intel.com> > > When IOAPIC RTE is in remapping format, it doesn't contain the vector of > interrupt. For this case, the RTE contains an index of interrupt remapping > table where the vector of interrupt is stored. This patchs gets the vector > through a vIOMMU interface. > > Signed-off-by: Chao Gao <chao.gao@intel.com> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > xen/arch/x86/hvm/vioapic.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c > index 322f33c..ff0742d 100644 > --- a/xen/arch/x86/hvm/vioapic.c > +++ b/xen/arch/x86/hvm/vioapic.c > @@ -565,11 +565,27 @@ int vioapic_get_vector(const struct domain *d, unsigned int gsi) > { > unsigned int pin; > const struct hvm_vioapic *vioapic = gsi_vioapic(d, gsi, &pin); > + struct IO_APIC_route_remap_entry rte = { { vioapic->redirtbl[pin].bits } }; Designated initialization and const. > > if ( !vioapic ) > return -EINVAL; > > - return vioapic->redirtbl[pin].fields.vector; > + if ( rte.format ) > + { > + int err; > + struct irq_remapping_request request; > + struct irq_remapping_info info; > + > + irq_request_ioapic_fill(&request, vioapic->id, rte.val); > + /* Currently, only viommu 0 is supported */ This seems to be hardcoded in a bunch of places, which makes me wonder whether having an array of vIOMMUs is the correct choice. I think that you should remove the array and have a single vIOMMU per domain. > + err = viommu_get_irq_info(vioapic->domain, 0, &request, &info); > + return !err ? info.vector : -1; maybe: return err ?: info.vector; ? > + } > + else > + { > + return vioapic->redirtbl[pin].fields.vector; > + } > + > } > > int vioapic_get_trigger_mode(const struct domain *d, unsigned int gsi) > -- > 1.8.3.1 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
On 2017年08月23日 18:14, Roger Pau Monné wrote: > On Wed, Aug 09, 2017 at 04:34:20PM -0400, Lan Tianyu wrote: >> From: Chao Gao <chao.gao@intel.com> >> >> When IOAPIC RTE is in remapping format, it doesn't contain the vector of >> interrupt. For this case, the RTE contains an index of interrupt remapping >> table where the vector of interrupt is stored. This patchs gets the vector >> through a vIOMMU interface. >> >> Signed-off-by: Chao Gao <chao.gao@intel.com> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> --- >> xen/arch/x86/hvm/vioapic.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c >> index 322f33c..ff0742d 100644 >> --- a/xen/arch/x86/hvm/vioapic.c >> +++ b/xen/arch/x86/hvm/vioapic.c >> @@ -565,11 +565,27 @@ int vioapic_get_vector(const struct domain *d, unsigned int gsi) >> { >> unsigned int pin; >> const struct hvm_vioapic *vioapic = gsi_vioapic(d, gsi, &pin); >> + struct IO_APIC_route_remap_entry rte = { { vioapic->redirtbl[pin].bits } }; > > Designated initialization and const. > >> >> if ( !vioapic ) >> return -EINVAL; >> >> - return vioapic->redirtbl[pin].fields.vector; >> + if ( rte.format ) >> + { >> + int err; >> + struct irq_remapping_request request; >> + struct irq_remapping_info info; >> + >> + irq_request_ioapic_fill(&request, vioapic->id, rte.val); >> + /* Currently, only viommu 0 is supported */ > > This seems to be hardcoded in a bunch of places, which makes me wonder > whether having an array of vIOMMUs is the correct choice. I think that > you should remove the array and have a single vIOMMU per domain. The array is reserved for mult-vIOMMU support but so far no such requirement as I know. In design stage, someone commented we should take mult-vIOMMU support into account. We may add callback of getting vIOMMU in vIOMMU ops and let vIOMMU device model return associated vIOMMU instance according irq remapping information(e.g source id). One VM suppose to have only one vIOMMU type. When add multi-vIOMMU support, this logic also can be applied. For current scenario. device model should return the first vIOMMU directly. > >> + err = viommu_get_irq_info(vioapic->domain, 0, &request, &info); >> + return !err ? info.vector : -1; > > maybe: > > return err ?: info.vector; > > ? > >> + } >> + else >> + { >> + return vioapic->redirtbl[pin].fields.vector; >> + } >> + >> } >> >> int vioapic_get_trigger_mode(const struct domain *d, unsigned int gsi) >> -- >> 1.8.3.1 >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> https://lists.xen.org/xen-devel
>>> On 24.08.17 at 08:11, <tianyu.lan@intel.com> wrote: > On 2017年08月23日 18:14, Roger Pau Monné wrote: >> On Wed, Aug 09, 2017 at 04:34:20PM -0400, Lan Tianyu wrote: >>> --- a/xen/arch/x86/hvm/vioapic.c >>> +++ b/xen/arch/x86/hvm/vioapic.c >>> @@ -565,11 +565,27 @@ int vioapic_get_vector(const struct domain *d, unsigned int gsi) >>> { >>> unsigned int pin; >>> const struct hvm_vioapic *vioapic = gsi_vioapic(d, gsi, &pin); >>> + struct IO_APIC_route_remap_entry rte = { { vioapic->redirtbl[pin].bits } }; >> >> Designated initialization and const. >> >>> >>> if ( !vioapic ) >>> return -EINVAL; >>> >>> - return vioapic->redirtbl[pin].fields.vector; >>> + if ( rte.format ) >>> + { >>> + int err; >>> + struct irq_remapping_request request; >>> + struct irq_remapping_info info; >>> + >>> + irq_request_ioapic_fill(&request, vioapic->id, rte.val); >>> + /* Currently, only viommu 0 is supported */ >> >> This seems to be hardcoded in a bunch of places, which makes me wonder >> whether having an array of vIOMMUs is the correct choice. I think that >> you should remove the array and have a single vIOMMU per domain. > > The array is reserved for mult-vIOMMU support but so far no such > requirement as I know. In design stage, someone commented we should take > mult-vIOMMU support into account. It _may_ suffice to do so at the public interface level. I'm not against using a single entry array right away, but then the rest of the code needs to be written as if the array bound was not fixed at 1, i.e. no hard coded uses of zero as the only valid array index should occur. Jan
On 2017年08月24日 14:59, Jan Beulich wrote: >>>> On 24.08.17 at 08:11, <tianyu.lan@intel.com> wrote: >> On 2017年08月23日 18:14, Roger Pau Monné wrote: >>> On Wed, Aug 09, 2017 at 04:34:20PM -0400, Lan Tianyu wrote: >>>> --- a/xen/arch/x86/hvm/vioapic.c >>>> +++ b/xen/arch/x86/hvm/vioapic.c >>>> @@ -565,11 +565,27 @@ int vioapic_get_vector(const struct domain *d, unsigned int gsi) >>>> { >>>> unsigned int pin; >>>> const struct hvm_vioapic *vioapic = gsi_vioapic(d, gsi, &pin); >>>> + struct IO_APIC_route_remap_entry rte = { { vioapic->redirtbl[pin].bits } }; >>> >>> Designated initialization and const. >>> >>>> >>>> if ( !vioapic ) >>>> return -EINVAL; >>>> >>>> - return vioapic->redirtbl[pin].fields.vector; >>>> + if ( rte.format ) >>>> + { >>>> + int err; >>>> + struct irq_remapping_request request; >>>> + struct irq_remapping_info info; >>>> + >>>> + irq_request_ioapic_fill(&request, vioapic->id, rte.val); >>>> + /* Currently, only viommu 0 is supported */ >>> >>> This seems to be hardcoded in a bunch of places, which makes me wonder >>> whether having an array of vIOMMUs is the correct choice. I think that >>> you should remove the array and have a single vIOMMU per domain. >> >> The array is reserved for mult-vIOMMU support but so far no such >> requirement as I know. In design stage, someone commented we should take >> mult-vIOMMU support into account. > > It _may_ suffice to do so at the public interface level. I'm not > against using a single entry array right away, but then the rest > of the code needs to be written as if the array bound was not > fixed at 1, i.e. no hard coded uses of zero as the only valid > array index should occur. Hi Jan: I am not sure whether we can hide single vIOMMU logic in the device model. When vIOMMU instance is created in device model, store vIOMMU instance in the global variable of virtual VTD code. Provide getting viommu instance callback in vIOMMU ops and helper function in vIOMMU abstract layer with interrupt information as parameter. New callback always returns stored vIOMMU instance. This seems to avoid hard coded uses of zero. If this can't be accept, removing the array seems to be only feasible way.
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c index 322f33c..ff0742d 100644 --- a/xen/arch/x86/hvm/vioapic.c +++ b/xen/arch/x86/hvm/vioapic.c @@ -565,11 +565,27 @@ int vioapic_get_vector(const struct domain *d, unsigned int gsi) { unsigned int pin; const struct hvm_vioapic *vioapic = gsi_vioapic(d, gsi, &pin); + struct IO_APIC_route_remap_entry rte = { { vioapic->redirtbl[pin].bits } }; if ( !vioapic ) return -EINVAL; - return vioapic->redirtbl[pin].fields.vector; + if ( rte.format ) + { + int err; + struct irq_remapping_request request; + struct irq_remapping_info info; + + irq_request_ioapic_fill(&request, vioapic->id, rte.val); + /* Currently, only viommu 0 is supported */ + err = viommu_get_irq_info(vioapic->domain, 0, &request, &info); + return !err ? info.vector : -1; + } + else + { + return vioapic->redirtbl[pin].fields.vector; + } + } int vioapic_get_trigger_mode(const struct domain *d, unsigned int gsi)