| Message ID | 1502310866-10450-3-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 09, 2017 at 04:34:03PM -0400, Lan Tianyu wrote: > This patch is to add irq request callback for platform implementation > to deal with irq remapping request. > > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > xen/common/viommu.c | 15 +++++++++ > xen/include/asm-x86/viommu.h | 73 ++++++++++++++++++++++++++++++++++++++++++++ > xen/include/xen/viommu.h | 9 ++++++ > 3 files changed, 97 insertions(+) > create mode 100644 xen/include/asm-x86/viommu.h > > diff --git a/xen/common/viommu.c b/xen/common/viommu.c > index a4d004d..f4d34e6 100644 > --- a/xen/common/viommu.c > +++ b/xen/common/viommu.c > @@ -198,6 +198,21 @@ int __init viommu_setup(void) > return 0; > } > > +int viommu_handle_irq_request(struct domain *d, u32 viommu_id, > + struct irq_remapping_request *request) > +{ > + struct viommu_info *info = &d->viommu; Does this compile? This patch and the previous one don't have viommu added to struct domain. > + > + if ( viommu_id >= info->nr_viommu > + || !info->viommu[viommu_id] ) Join this to previous line? > + return -EINVAL; ASSERT(info->viommu[viommu_id]->ops); For extra safety. > + > + if ( !info->viommu[viommu_id]->ops->handle_irq_request ) > + return -EINVAL; > + > + return info->viommu[viommu_id]->ops->handle_irq_request(d, request); > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h > new file mode 100644 > index 0000000..51bda72 > --- /dev/null > +++ b/xen/include/asm-x86/viommu.h > @@ -0,0 +1,73 @@ > +/* > + * include/asm-x86/viommu.h > + * > + * Copyright (c) 2017 Intel Corporation. > + * Author: Lan Tianyu <tianyu.lan@intel.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program; If not, see <http://www.gnu.org/licenses/>. > + * > + */ > +#ifndef __ARCH_X86_VIOMMU_H__ > +#define __ARCH_X86_VIOMMU_H__ > + Is a corresponding ARM header needed? Given viommu is common code. > +#include <xen/viommu.h> I think you're probably doing it wrong. It should be that the common header header includes arch header, then the code only uses the common header (I haven't read the rest of your series at this point). > +#include <asm/types.h> > + > +/* IRQ request type */ > +#define VIOMMU_REQUEST_IRQ_MSI 0 > +#define VIOMMU_REQUEST_IRQ_APIC 1 > + > +struct irq_remapping_request > +{ > + union { > + /* MSI */ > + struct { > + u64 addr; > + u32 data; > + } msi; > + /* Redirection Entry in IOAPIC */ > + u64 rte; > + } msg; > + u16 source_id; > + u8 type; uintXX_t please. > +}; > + > +static inline void irq_request_ioapic_fill(struct irq_remapping_request *req, > + uint32_t ioapic_id, uint64_t rte) Indentation. > +{ > + ASSERT(req); > + req->type = VIOMMU_REQUEST_IRQ_APIC; > + req->source_id = ioapic_id; > + req->msg.rte = rte; > +} > + > +static inline void irq_request_msi_fill(struct irq_remapping_request *req, > + uint32_t source_id, uint64_t addr, uint32_t data) Indentation. > +{ > + ASSERT(req); > + req->type = VIOMMU_REQUEST_IRQ_MSI; > + req->source_id = source_id; > + req->msg.msi.addr = addr; > + req->msg.msi.data = data; > +} > + > +#endif /* __ARCH_X86_VIOMMU_H__ */ > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * End: > + */ > diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h > index 527afb1..0be1b3a 100644 > --- a/xen/include/xen/viommu.h > +++ b/xen/include/xen/viommu.h > @@ -20,6 +20,8 @@ > #ifndef __XEN_VIOMMU_H__ > #define __XEN_VIOMMU_H__ > > +#include <asm/viommu.h> > + Circular inclusion? Note the #include <xen/viommu.h> some lines above. > #define NR_VIOMMU_PER_DOMAIN 1 > > struct viommu; > @@ -28,6 +30,8 @@ struct viommu_ops { > u64 (*query_caps)(struct domain *d); > int (*create)(struct domain *d, struct viommu *viommu); > int (*destroy)(struct viommu *viommu); > + int (*handle_irq_request)(struct domain *d, > + struct irq_remapping_request *request); > }; > > struct viommu { > @@ -52,6 +56,8 @@ int viommu_register_type(u64 type, struct viommu_ops * ops); > int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op, > bool_t *need_copy); > int viommu_setup(void); > +int viommu_handle_irq_request(struct domain *d, u32 viommu_id, > + struct irq_remapping_request *request); > #else > static inline int viommu_init_domain(struct domain *d) { return 0; } > static inline int viommu_register_type(u64 type, struct viommu_ops * ops) > @@ -62,6 +68,9 @@ static inline int viommu_domctl(struct domain *d, > struct xen_domctl_viommu_op *op, > bool *need_copy) > { return -ENODEV }; > +static inline int viommu_handle_irq_request(struct domain *d, u32 viommu_id, > + struct irq_remapping_request *request) > +{ return 0 }; This should fail. > #endif > > #endif /* __XEN_VIOMMU_H__ */ > -- > 1.8.3.1 >
On 2017年08月17日 19:18, Wei Liu wrote: > On Wed, Aug 09, 2017 at 04:34:03PM -0400, Lan Tianyu wrote: >> This patch is to add irq request callback for platform implementation >> to deal with irq remapping request. >> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> --- >> xen/common/viommu.c | 15 +++++++++ >> xen/include/asm-x86/viommu.h | 73 ++++++++++++++++++++++++++++++++++++++++++++ >> xen/include/xen/viommu.h | 9 ++++++ >> 3 files changed, 97 insertions(+) >> create mode 100644 xen/include/asm-x86/viommu.h >> >> diff --git a/xen/common/viommu.c b/xen/common/viommu.c >> index a4d004d..f4d34e6 100644 >> --- a/xen/common/viommu.c >> +++ b/xen/common/viommu.c >> @@ -198,6 +198,21 @@ int __init viommu_setup(void) >> return 0; >> } >> >> +int viommu_handle_irq_request(struct domain *d, u32 viommu_id, >> + struct irq_remapping_request *request) >> +{ >> + struct viommu_info *info = &d->viommu; > > Does this compile? This patch and the previous one don't have viommu > added to struct domain. Oh. Sorry. Missed patch "VIOMMU: Add vIOMMU helper functions to create, destroy and query capabilities" in this series. I just sent out and followed this mail. Please have a look. > >> + >> + if ( viommu_id >= info->nr_viommu >> + || !info->viommu[viommu_id] ) > > Join this to previous line? > OK. >> + return -EINVAL; > > ASSERT(info->viommu[viommu_id]->ops); > > For extra safety. Or check ops in the previous if? > >> + >> + if ( !info->viommu[viommu_id]->ops->handle_irq_request ) >> + return -EINVAL; >> + >> + return info->viommu[viommu_id]->ops->handle_irq_request(d, request); >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h >> new file mode 100644 >> index 0000000..51bda72 >> --- /dev/null >> +++ b/xen/include/asm-x86/viommu.h >> @@ -0,0 +1,73 @@ >> +/* >> + * include/asm-x86/viommu.h >> + * >> + * Copyright (c) 2017 Intel Corporation. >> + * Author: Lan Tianyu <tianyu.lan@intel.com> >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + * >> + * You should have received a copy of the GNU General Public License along with >> + * this program; If not, see <http://www.gnu.org/licenses/>. >> + * >> + */ >> +#ifndef __ARCH_X86_VIOMMU_H__ >> +#define __ARCH_X86_VIOMMU_H__ >> + > > Is a corresponding ARM header needed? Given viommu is common code. I added such ARM header file in previous version but Julien hope vIOMMU should be disabled for ARM because ARM doesn't support vIOMMU so far. > >> +#include <xen/viommu.h> > > I think you're probably doing it wrong. > > It should be that the common header header includes arch header, then > the code only uses the common header (I haven't read the rest of your > series at this point). OK. Will fix it. > >> +#include <asm/types.h> >> + >> +/* IRQ request type */ >> +#define VIOMMU_REQUEST_IRQ_MSI 0 >> +#define VIOMMU_REQUEST_IRQ_APIC 1 >> + >> +struct irq_remapping_request >> +{ >> + union { >> + /* MSI */ >> + struct { >> + u64 addr; >> + u32 data; >> + } msi; >> + /* Redirection Entry in IOAPIC */ >> + u64 rte; >> + } msg; >> + u16 source_id; >> + u8 type; > > uintXX_t please. Sure. > >> +}; >> + >> +static inline void irq_request_ioapic_fill(struct irq_remapping_request *req, >> + uint32_t ioapic_id, uint64_t rte) > > Indentation. > >> +{ >> + ASSERT(req); >> + req->type = VIOMMU_REQUEST_IRQ_APIC; >> + req->source_id = ioapic_id; >> + req->msg.rte = rte; >> +} >> + >> +static inline void irq_request_msi_fill(struct irq_remapping_request *req, >> + uint32_t source_id, uint64_t addr, uint32_t data) > > Indentation. > >> +{ >> + ASSERT(req); >> + req->type = VIOMMU_REQUEST_IRQ_MSI; >> + req->source_id = source_id; >> + req->msg.msi.addr = addr; >> + req->msg.msi.data = data; >> +} >> + >> +#endif /* __ARCH_X86_VIOMMU_H__ */ >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * tab-width: 4 >> + * End: >> + */ >> diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h >> index 527afb1..0be1b3a 100644 >> --- a/xen/include/xen/viommu.h >> +++ b/xen/include/xen/viommu.h >> @@ -20,6 +20,8 @@ >> #ifndef __XEN_VIOMMU_H__ >> #define __XEN_VIOMMU_H__ >> >> +#include <asm/viommu.h> >> + > > Circular inclusion? Note the #include <xen/viommu.h> some lines above. > >> #define NR_VIOMMU_PER_DOMAIN 1 >> >> struct viommu; >> @@ -28,6 +30,8 @@ struct viommu_ops { >> u64 (*query_caps)(struct domain *d); >> int (*create)(struct domain *d, struct viommu *viommu); >> int (*destroy)(struct viommu *viommu); >> + int (*handle_irq_request)(struct domain *d, >> + struct irq_remapping_request *request); >> }; >> >> struct viommu { >> @@ -52,6 +56,8 @@ int viommu_register_type(u64 type, struct viommu_ops * ops); >> int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op, >> bool_t *need_copy); >> int viommu_setup(void); >> +int viommu_handle_irq_request(struct domain *d, u32 viommu_id, >> + struct irq_remapping_request *request); >> #else >> static inline int viommu_init_domain(struct domain *d) { return 0; } >> static inline int viommu_register_type(u64 type, struct viommu_ops * ops) >> @@ -62,6 +68,9 @@ static inline int viommu_domctl(struct domain *d, >> struct xen_domctl_viommu_op *op, >> bool *need_copy) >> { return -ENODEV }; >> +static inline int viommu_handle_irq_request(struct domain *d, u32 viommu_id, >> + struct irq_remapping_request *request) >> +{ return 0 }; > > This should fail. > >> #endif >> >> #endif /* __XEN_VIOMMU_H__ */ >> -- >> 1.8.3.1 >>
On Fri, Aug 18, 2017 at 03:09:55PM +0800, Lan Tianyu wrote: > On 2017年08月17日 19:18, Wei Liu wrote: > > On Wed, Aug 09, 2017 at 04:34:03PM -0400, Lan Tianyu wrote: > >> This patch is to add irq request callback for platform implementation > >> to deal with irq remapping request. > >> > >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > >> --- > >> xen/common/viommu.c | 15 +++++++++ > >> xen/include/asm-x86/viommu.h | 73 ++++++++++++++++++++++++++++++++++++++++++++ > >> xen/include/xen/viommu.h | 9 ++++++ > >> 3 files changed, 97 insertions(+) > >> create mode 100644 xen/include/asm-x86/viommu.h > >> > >> diff --git a/xen/common/viommu.c b/xen/common/viommu.c > >> index a4d004d..f4d34e6 100644 > >> --- a/xen/common/viommu.c > >> +++ b/xen/common/viommu.c > >> @@ -198,6 +198,21 @@ int __init viommu_setup(void) > >> return 0; > >> } > >> > >> +int viommu_handle_irq_request(struct domain *d, u32 viommu_id, > >> + struct irq_remapping_request *request) > >> +{ > >> + struct viommu_info *info = &d->viommu; > > > > Does this compile? This patch and the previous one don't have viommu > > added to struct domain. > > Oh. Sorry. Missed patch "VIOMMU: Add vIOMMU helper functions to create, > destroy and query capabilities" in this series. I just sent out and > followed this mail. Please have a look. > > > > >> + > >> + if ( viommu_id >= info->nr_viommu > >> + || !info->viommu[viommu_id] ) > > > > Join this to previous line? > > > > OK. > > >> + return -EINVAL; > > > > ASSERT(info->viommu[viommu_id]->ops); > > > > For extra safety. > > Or check ops in the previous if? > That depends on if ops can be null or not. > > > >> + > >> + if ( !info->viommu[viommu_id]->ops->handle_irq_request ) > >> + return -EINVAL; > >> + > >> + return info->viommu[viommu_id]->ops->handle_irq_request(d, request); > >> +} > >> + > >> /* > >> * Local variables: > >> * mode: C > >> diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h > >> new file mode 100644 > >> index 0000000..51bda72 > >> --- /dev/null > >> +++ b/xen/include/asm-x86/viommu.h > >> @@ -0,0 +1,73 @@ > >> +/* > >> + * include/asm-x86/viommu.h > >> + * > >> + * Copyright (c) 2017 Intel Corporation. > >> + * Author: Lan Tianyu <tianyu.lan@intel.com> > >> + * > >> + * This program is free software; you can redistribute it and/or modify it > >> + * under the terms and conditions of the GNU General Public License, > >> + * version 2, as published by the Free Software Foundation. > >> + * > >> + * This program is distributed in the hope it will be useful, but WITHOUT > >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > >> + * more details. > >> + * > >> + * You should have received a copy of the GNU General Public License along with > >> + * this program; If not, see <http://www.gnu.org/licenses/>. > >> + * > >> + */ > >> +#ifndef __ARCH_X86_VIOMMU_H__ > >> +#define __ARCH_X86_VIOMMU_H__ > >> + > > > > Is a corresponding ARM header needed? Given viommu is common code. > > I added such ARM header file in previous version but Julien hope vIOMMU > should be disabled for ARM because ARM doesn't support vIOMMU so far. > If that's what he wanted, sure. Please build test ARM as well. You can do so using travis-ci.
On 2017年08月18日 18:13, Wei Liu wrote: >>> ASSERT(info->viommu[viommu_id]->ops); >>> > > >>> > > For extra safety. >> > >> > Or check ops in the previous if? >> > > That depends on if ops can be null or not. If ops isn't be set, it will be null. Because struct viommu is allocated via xzalloc(). > >>> diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h >>> > >> new file mode 100644 >>> > >> index 0000000..51bda72 >>> > >> --- /dev/null >>> > >> +++ b/xen/include/asm-x86/viommu.h >>> > >> @@ -0,0 +1,73 @@ >>> > >> +/* >>> > >> + * include/asm-x86/viommu.h >>> > >> + * >>> > >> + * Copyright (c) 2017 Intel Corporation. >>> > >> + * Author: Lan Tianyu <tianyu.lan@intel.com> >>> > >> + * >>> > >> + * This program is free software; you can redistribute it and/or modify it >>> > >> + * under the terms and conditions of the GNU General Public License, >>> > >> + * version 2, as published by the Free Software Foundation. >>> > >> + * >>> > >> + * This program is distributed in the hope it will be useful, but WITHOUT >>> > >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >>> > >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >>> > >> + * more details. >>> > >> + * >>> > >> + * You should have received a copy of the GNU General Public License along with >>> > >> + * this program; If not, see <http://www.gnu.org/licenses/>. >>> > >> + * >>> > >> + */ >>> > >> +#ifndef __ARCH_X86_VIOMMU_H__ >>> > >> +#define __ARCH_X86_VIOMMU_H__ >>> > >> + >> > > >> > > Is a corresponding ARM header needed? Given viommu is common code. >> >> I added such ARM header file in previous version but Julien hope vIOMMU >> > should be disabled for ARM because ARM doesn't support vIOMMU so far. >> > > If that's what he wanted, sure. > > Please build test ARM as well. You can do so using travis-ci. OK. Will test.
On Tue, Aug 22, 2017 at 04:04:09PM +0800, Lan Tianyu wrote: > On 2017年08月18日 18:13, Wei Liu wrote: > >>> ASSERT(info->viommu[viommu_id]->ops); > >>> > > > >>> > > For extra safety. > >> > > >> > Or check ops in the previous if? > >> > > > That depends on if ops can be null or not. > > If ops isn't be set, it will be null. Because struct viommu is allocated > via xzalloc(). But is it functionally correct / possible to have it to be NULL when you come to this path?
On 2017年08月22日 16:42, Wei Liu wrote: > On Tue, Aug 22, 2017 at 04:04:09PM +0800, Lan Tianyu wrote: >> On 2017年08月18日 18:13, Wei Liu wrote: >>>>> ASSERT(info->viommu[viommu_id]->ops); >>>>>>> >>>>>>> For extra safety. >>>>> >>>>> Or check ops in the previous if? >>>>> >>> That depends on if ops can be null or not. >> >> If ops isn't be set, it will be null. Because struct viommu is allocated >> via xzalloc(). > > But is it functionally correct / possible to have it to be NULL when you > come to this path? > No, it shouldn't be NULL if struct viommu is set up correctly.
On Tue, Aug 22, 2017 at 06:39:32PM +0800, Lan Tianyu wrote: > On 2017年08月22日 16:42, Wei Liu wrote: > > On Tue, Aug 22, 2017 at 04:04:09PM +0800, Lan Tianyu wrote: > >> On 2017年08月18日 18:13, Wei Liu wrote: > >>>>> ASSERT(info->viommu[viommu_id]->ops); > >>>>>>> > >>>>>>> For extra safety. > >>>>> > >>>>> Or check ops in the previous if? > >>>>> > >>> That depends on if ops can be null or not. > >> > >> If ops isn't be set, it will be null. Because struct viommu is allocated > >> via xzalloc(). > > > > But is it functionally correct / possible to have it to be NULL when you > > come to this path? > > > > No, it shouldn't be NULL if struct viommu is set up correctly. > Then an ASSERT is warranted -- hope you see the line of thinking.
On 2017年08月22日 18:53, Wei Liu wrote: > On Tue, Aug 22, 2017 at 06:39:32PM +0800, Lan Tianyu wrote: >> On 2017年08月22日 16:42, Wei Liu wrote: >>> On Tue, Aug 22, 2017 at 04:04:09PM +0800, Lan Tianyu wrote: >>>> On 2017年08月18日 18:13, Wei Liu wrote: >>>>>>> ASSERT(info->viommu[viommu_id]->ops); >>>>>>>>> >>>>>>>>> For extra safety. >>>>>>> >>>>>>> Or check ops in the previous if? >>>>>>> >>>>> That depends on if ops can be null or not. >>>> >>>> If ops isn't be set, it will be null. Because struct viommu is allocated >>>> via xzalloc(). >>> >>> But is it functionally correct / possible to have it to be NULL when you >>> come to this path? >>> >> >> No, it shouldn't be NULL if struct viommu is set up correctly. >> > > Then an ASSERT is warranted -- hope you see the line of thinking. > OK. I got it. Will add in the next version.
On Wed, Aug 09, 2017 at 04:34:03PM -0400, Lan Tianyu wrote: > This patch is to add irq request callback for platform implementation > to deal with irq remapping request. > > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > xen/common/viommu.c | 15 +++++++++ > xen/include/asm-x86/viommu.h | 73 ++++++++++++++++++++++++++++++++++++++++++++ > xen/include/xen/viommu.h | 9 ++++++ > 3 files changed, 97 insertions(+) > create mode 100644 xen/include/asm-x86/viommu.h > > diff --git a/xen/common/viommu.c b/xen/common/viommu.c > index a4d004d..f4d34e6 100644 > --- a/xen/common/viommu.c > +++ b/xen/common/viommu.c > @@ -198,6 +198,21 @@ int __init viommu_setup(void) > return 0; > } > > +int viommu_handle_irq_request(struct domain *d, u32 viommu_id, > + struct irq_remapping_request *request) > +{ > + struct viommu_info *info = &d->viommu; > + > + if ( viommu_id >= info->nr_viommu > + || !info->viommu[viommu_id] ) This fits on the same line, no need to split it. > + return -EINVAL; > + > + if ( !info->viommu[viommu_id]->ops->handle_irq_request ) > + return -EINVAL; > + > + return info->viommu[viommu_id]->ops->handle_irq_request(d, request); > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h > new file mode 100644 > index 0000000..51bda72 > --- /dev/null > +++ b/xen/include/asm-x86/viommu.h > @@ -0,0 +1,73 @@ > +/* > + * include/asm-x86/viommu.h > + * > + * Copyright (c) 2017 Intel Corporation. > + * Author: Lan Tianyu <tianyu.lan@intel.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program; If not, see <http://www.gnu.org/licenses/>. > + * > + */ > +#ifndef __ARCH_X86_VIOMMU_H__ > +#define __ARCH_X86_VIOMMU_H__ > + > +#include <xen/viommu.h> > +#include <asm/types.h> > + > +/* IRQ request type */ > +#define VIOMMU_REQUEST_IRQ_MSI 0 > +#define VIOMMU_REQUEST_IRQ_APIC 1 > + > +struct irq_remapping_request > +{ > + union { > + /* MSI */ > + struct { > + u64 addr; > + u32 data; > + } msi; > + /* Redirection Entry in IOAPIC */ > + u64 rte; > + } msg; > + u16 source_id; > + u8 type; > +}; > + > +static inline void irq_request_ioapic_fill(struct irq_remapping_request *req, > + uint32_t ioapic_id, uint64_t rte) > +{ > + ASSERT(req); > + req->type = VIOMMU_REQUEST_IRQ_APIC; > + req->source_id = ioapic_id; > + req->msg.rte = rte; > +} > + > +static inline void irq_request_msi_fill(struct irq_remapping_request *req, > + uint32_t source_id, uint64_t addr, uint32_t data) > +{ > + ASSERT(req); > + req->type = VIOMMU_REQUEST_IRQ_MSI; > + req->source_id = source_id; > + req->msg.msi.addr = addr; > + req->msg.msi.data = data; > +} What's the usage of those two functions? AFAICT they don't have any callers in this patch. > + > +#endif /* __ARCH_X86_VIOMMU_H__ */ > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * End: > + */ > diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h > index 527afb1..0be1b3a 100644 > --- a/xen/include/xen/viommu.h > +++ b/xen/include/xen/viommu.h > @@ -20,6 +20,8 @@ > #ifndef __XEN_VIOMMU_H__ > #define __XEN_VIOMMU_H__ > > +#include <asm/viommu.h> > + > #define NR_VIOMMU_PER_DOMAIN 1 > > struct viommu; > @@ -28,6 +30,8 @@ struct viommu_ops { > u64 (*query_caps)(struct domain *d); > int (*create)(struct domain *d, struct viommu *viommu); > int (*destroy)(struct viommu *viommu); > + int (*handle_irq_request)(struct domain *d, > + struct irq_remapping_request *request); I'm slightly lost, you add the function pointer here and some inline functions in asm-x86/viommu.h, yet I don't see them being hooked into the struct viommu_ops in any way. Roger.
On 2017年08月22日 23:32, Roger Pau Monné wrote: > On Wed, Aug 09, 2017 at 04:34:03PM -0400, Lan Tianyu wrote: >> This patch is to add irq request callback for platform implementation >> to deal with irq remapping request. >> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> --- >> xen/common/viommu.c | 15 +++++++++ >> xen/include/asm-x86/viommu.h | 73 ++++++++++++++++++++++++++++++++++++++++++++ >> xen/include/xen/viommu.h | 9 ++++++ >> 3 files changed, 97 insertions(+) >> create mode 100644 xen/include/asm-x86/viommu.h >> >> diff --git a/xen/common/viommu.c b/xen/common/viommu.c >> index a4d004d..f4d34e6 100644 >> --- a/xen/common/viommu.c >> +++ b/xen/common/viommu.c >> @@ -198,6 +198,21 @@ int __init viommu_setup(void) >> return 0; >> } >> >> +int viommu_handle_irq_request(struct domain *d, u32 viommu_id, >> + struct irq_remapping_request *request) >> +{ >> + struct viommu_info *info = &d->viommu; >> + >> + if ( viommu_id >= info->nr_viommu >> + || !info->viommu[viommu_id] ) > > This fits on the same line, no need to split it. > >> + return -EINVAL; >> + >> + if ( !info->viommu[viommu_id]->ops->handle_irq_request ) >> + return -EINVAL; >> + >> + return info->viommu[viommu_id]->ops->handle_irq_request(d, request); >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h >> new file mode 100644 >> index 0000000..51bda72 >> --- /dev/null >> +++ b/xen/include/asm-x86/viommu.h >> @@ -0,0 +1,73 @@ >> +/* >> + * include/asm-x86/viommu.h >> + * >> + * Copyright (c) 2017 Intel Corporation. >> + * Author: Lan Tianyu <tianyu.lan@intel.com> >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + * >> + * You should have received a copy of the GNU General Public License along with >> + * this program; If not, see <http://www.gnu.org/licenses/>. >> + * >> + */ >> +#ifndef __ARCH_X86_VIOMMU_H__ >> +#define __ARCH_X86_VIOMMU_H__ >> + >> +#include <xen/viommu.h> >> +#include <asm/types.h> >> + >> +/* IRQ request type */ >> +#define VIOMMU_REQUEST_IRQ_MSI 0 >> +#define VIOMMU_REQUEST_IRQ_APIC 1 >> + >> +struct irq_remapping_request >> +{ >> + union { >> + /* MSI */ >> + struct { >> + u64 addr; >> + u32 data; >> + } msi; >> + /* Redirection Entry in IOAPIC */ >> + u64 rte; >> + } msg; >> + u16 source_id; >> + u8 type; >> +}; >> + >> +static inline void irq_request_ioapic_fill(struct irq_remapping_request *req, >> + uint32_t ioapic_id, uint64_t rte) >> +{ >> + ASSERT(req); >> + req->type = VIOMMU_REQUEST_IRQ_APIC; >> + req->source_id = ioapic_id; >> + req->msg.rte = rte; >> +} >> + >> +static inline void irq_request_msi_fill(struct irq_remapping_request *req, >> + uint32_t source_id, uint64_t addr, uint32_t data) >> +{ >> + ASSERT(req); >> + req->type = VIOMMU_REQUEST_IRQ_MSI; >> + req->source_id = source_id; >> + req->msg.msi.addr = addr; >> + req->msg.msi.data = data; >> +} > > What's the usage of those two functions? AFAICT they don't have any > callers in this patch. These functions will be called in the following interrupt patch 22 "x86/vmsi: Hook delivering remapping format msi to guest" and patch 16 "x86/vioapic: Hook interrupt delivery of vIOAPIC" > >> + >> +#endif /* __ARCH_X86_VIOMMU_H__ */ >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * tab-width: 4 >> + * End: >> + */ >> diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h >> index 527afb1..0be1b3a 100644 >> --- a/xen/include/xen/viommu.h >> +++ b/xen/include/xen/viommu.h >> @@ -20,6 +20,8 @@ >> #ifndef __XEN_VIOMMU_H__ >> #define __XEN_VIOMMU_H__ >> >> +#include <asm/viommu.h> >> + >> #define NR_VIOMMU_PER_DOMAIN 1 >> >> struct viommu; >> @@ -28,6 +30,8 @@ struct viommu_ops { >> u64 (*query_caps)(struct domain *d); >> int (*create)(struct domain *d, struct viommu *viommu); >> int (*destroy)(struct viommu *viommu); >> + int (*handle_irq_request)(struct domain *d, >> + struct irq_remapping_request *request); > > I'm slightly lost, you add the function pointer here and some inline > functions in asm-x86/viommu.h, yet I don't see them being hooked into > the struct viommu_ops in any way. > > Roger. >
>>> On 23.08.17 at 09:42, <tianyu.lan@intel.com> wrote: > On 2017年08月22日 23:32, Roger Pau Monné wrote: >> On Wed, Aug 09, 2017 at 04:34:03PM -0400, Lan Tianyu wrote: >>> +static inline void irq_request_ioapic_fill(struct irq_remapping_request *req, >>> + uint32_t ioapic_id, uint64_t rte) >>> +{ >>> + ASSERT(req); >>> + req->type = VIOMMU_REQUEST_IRQ_APIC; >>> + req->source_id = ioapic_id; >>> + req->msg.rte = rte; >>> +} >>> + >>> +static inline void irq_request_msi_fill(struct irq_remapping_request *req, >>> + uint32_t source_id, uint64_t addr, uint32_t data) >>> +{ >>> + ASSERT(req); >>> + req->type = VIOMMU_REQUEST_IRQ_MSI; >>> + req->source_id = source_id; >>> + req->msg.msi.addr = addr; >>> + req->msg.msi.data = data; >>> +} >> >> What's the usage of those two functions? AFAICT they don't have any >> callers in this patch. > > These functions will be called in the following interrupt patch 22 > "x86/vmsi: Hook delivering remapping format msi to guest" and patch 16 > "x86/vioapic: Hook interrupt delivery of vIOAPIC" That's _far_ away. As implied by Roger's comment, please try to avoid introducing dead code, especially when it's dead for an extended period of time. Always remember that a series may not be committed in one go. Jan
On 2017年08月23日 17:24, Jan Beulich wrote: >>>> On 23.08.17 at 09:42, <tianyu.lan@intel.com> wrote: >> On 2017年08月22日 23:32, Roger Pau Monné wrote: >>> On Wed, Aug 09, 2017 at 04:34:03PM -0400, Lan Tianyu wrote: >>>> +static inline void irq_request_ioapic_fill(struct irq_remapping_request *req, >>>> + uint32_t ioapic_id, uint64_t rte) >>>> +{ >>>> + ASSERT(req); >>>> + req->type = VIOMMU_REQUEST_IRQ_APIC; >>>> + req->source_id = ioapic_id; >>>> + req->msg.rte = rte; >>>> +} >>>> + >>>> +static inline void irq_request_msi_fill(struct irq_remapping_request *req, >>>> + uint32_t source_id, uint64_t addr, uint32_t data) >>>> +{ >>>> + ASSERT(req); >>>> + req->type = VIOMMU_REQUEST_IRQ_MSI; >>>> + req->source_id = source_id; >>>> + req->msg.msi.addr = addr; >>>> + req->msg.msi.data = data; >>>> +} >>> >>> What's the usage of those two functions? AFAICT they don't have any >>> callers in this patch. >> >> These functions will be called in the following interrupt patch 22 >> "x86/vmsi: Hook delivering remapping format msi to guest" and patch 16 >> "x86/vioapic: Hook interrupt delivery of vIOAPIC" > > That's _far_ away. As implied by Roger's comment, please try to > avoid introducing dead code, especially when it's dead for an > extended period of time. Always remember that a series may not > be committed in one go. OK. Will change order.
diff --git a/xen/common/viommu.c b/xen/common/viommu.c index a4d004d..f4d34e6 100644 --- a/xen/common/viommu.c +++ b/xen/common/viommu.c @@ -198,6 +198,21 @@ int __init viommu_setup(void) return 0; } +int viommu_handle_irq_request(struct domain *d, u32 viommu_id, + struct irq_remapping_request *request) +{ + struct viommu_info *info = &d->viommu; + + if ( viommu_id >= info->nr_viommu + || !info->viommu[viommu_id] ) + return -EINVAL; + + if ( !info->viommu[viommu_id]->ops->handle_irq_request ) + return -EINVAL; + + return info->viommu[viommu_id]->ops->handle_irq_request(d, request); +} + /* * Local variables: * mode: C diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h new file mode 100644 index 0000000..51bda72 --- /dev/null +++ b/xen/include/asm-x86/viommu.h @@ -0,0 +1,73 @@ +/* + * include/asm-x86/viommu.h + * + * Copyright (c) 2017 Intel Corporation. + * Author: Lan Tianyu <tianyu.lan@intel.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program; If not, see <http://www.gnu.org/licenses/>. + * + */ +#ifndef __ARCH_X86_VIOMMU_H__ +#define __ARCH_X86_VIOMMU_H__ + +#include <xen/viommu.h> +#include <asm/types.h> + +/* IRQ request type */ +#define VIOMMU_REQUEST_IRQ_MSI 0 +#define VIOMMU_REQUEST_IRQ_APIC 1 + +struct irq_remapping_request +{ + union { + /* MSI */ + struct { + u64 addr; + u32 data; + } msi; + /* Redirection Entry in IOAPIC */ + u64 rte; + } msg; + u16 source_id; + u8 type; +}; + +static inline void irq_request_ioapic_fill(struct irq_remapping_request *req, + uint32_t ioapic_id, uint64_t rte) +{ + ASSERT(req); + req->type = VIOMMU_REQUEST_IRQ_APIC; + req->source_id = ioapic_id; + req->msg.rte = rte; +} + +static inline void irq_request_msi_fill(struct irq_remapping_request *req, + uint32_t source_id, uint64_t addr, uint32_t data) +{ + ASSERT(req); + req->type = VIOMMU_REQUEST_IRQ_MSI; + req->source_id = source_id; + req->msg.msi.addr = addr; + req->msg.msi.data = data; +} + +#endif /* __ARCH_X86_VIOMMU_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * End: + */ diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h index 527afb1..0be1b3a 100644 --- a/xen/include/xen/viommu.h +++ b/xen/include/xen/viommu.h @@ -20,6 +20,8 @@ #ifndef __XEN_VIOMMU_H__ #define __XEN_VIOMMU_H__ +#include <asm/viommu.h> + #define NR_VIOMMU_PER_DOMAIN 1 struct viommu; @@ -28,6 +30,8 @@ struct viommu_ops { u64 (*query_caps)(struct domain *d); int (*create)(struct domain *d, struct viommu *viommu); int (*destroy)(struct viommu *viommu); + int (*handle_irq_request)(struct domain *d, + struct irq_remapping_request *request); }; struct viommu { @@ -52,6 +56,8 @@ int viommu_register_type(u64 type, struct viommu_ops * ops); int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op, bool_t *need_copy); int viommu_setup(void); +int viommu_handle_irq_request(struct domain *d, u32 viommu_id, + struct irq_remapping_request *request); #else static inline int viommu_init_domain(struct domain *d) { return 0; } static inline int viommu_register_type(u64 type, struct viommu_ops * ops) @@ -62,6 +68,9 @@ static inline int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op, bool *need_copy) { return -ENODEV }; +static inline int viommu_handle_irq_request(struct domain *d, u32 viommu_id, + struct irq_remapping_request *request) +{ return 0 }; #endif #endif /* __XEN_VIOMMU_H__ */
This patch is to add irq request callback for platform implementation to deal with irq remapping request. Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- xen/common/viommu.c | 15 +++++++++ xen/include/asm-x86/viommu.h | 73 ++++++++++++++++++++++++++++++++++++++++++++ xen/include/xen/viommu.h | 9 ++++++ 3 files changed, 97 insertions(+) create mode 100644 xen/include/asm-x86/viommu.h