| Message ID | 1507639952-31617-4-git-send-email-mjaggi@caviumnetworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Manish, On 10/10/17 13:52, mjaggi@caviumnetworks.com wrote: > From: Manish Jaggi <mjaggi@cavium.com> > > This patch extends the gicv3_iomem_deny_access functionality by adding > support for ITS region as well. Add function gicv3_its_deny_access. > > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > Acked-by: Julien Grall <julien.grall@arm.com> Please state after "---" when you modified a patch and keep the tags to at least check if the reviewer is happy with it. It is one of the reason I like the changelog in each patch. It helps to know what changed in a specific one. It helps me to decide whether I am happy with you keeping my tag and avoid to fully review yet another time the patch. In that case, it is fine to keep it. > Signed-off-by: Manish Jaggi <mjaggi@cavium.com> > --- > xen/arch/arm/gic-v3-its.c | 22 ++++++++++++++++++++++ > xen/arch/arm/gic-v3.c | 4 ++++ > xen/include/asm-arm/gic_v3_its.h | 9 +++++++++ > 3 files changed, 35 insertions(+) > > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c > index 3023ee5..bd94308 100644 > --- a/xen/arch/arm/gic-v3-its.c > +++ b/xen/arch/arm/gic-v3-its.c > @@ -21,6 +21,7 @@ > #include <xen/acpi.h> > #include <xen/lib.h> > #include <xen/delay.h> > +#include <xen/iocap.h> > #include <xen/libfdt/libfdt.h> > #include <xen/mm.h> > #include <xen/rbtree.h> > @@ -905,6 +906,27 @@ struct pending_irq *gicv3_assign_guest_event(struct domain *d, > return pirq; > } > > +int gicv3_its_deny_access(const struct domain *d) > +{ > + int rc = 0; > + unsigned long mfn, nr; > + const struct host_its *its_data; > + > + list_for_each_entry( its_data, &host_its_list, entry ) > + { > + mfn = paddr_to_pfn(its_data->addr); > + nr = PFN_UP(its_data->size); > + rc = iomem_deny_access(d, mfn, mfn + nr); > + if ( rc ) > + { > + printk("iomem_deny_access failed for %lx:%lx \r\n", mfn, nr); > + break; > + } > + } > + > + return rc; > +} > + > /* > * Create the respective guest DT nodes from a list of host ITSes. > * This copies the reg property, so the guest sees the ITS at the same address > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 6f562f4..475e0d3 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -1308,6 +1308,10 @@ static int gicv3_iomem_deny_access(const struct domain *d) > if ( rc ) > return rc; > > + rc = gicv3_its_deny_access(d); > + if ( rc ) > + return rc; > + > for ( i = 0; i < gicv3.rdist_count; i++ ) > { > mfn = gicv3.rdist_regions[i].base >> PAGE_SHIFT; > diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h > index 73d1fd1..73ee0ba 100644 > --- a/xen/include/asm-arm/gic_v3_its.h > +++ b/xen/include/asm-arm/gic_v3_its.h > @@ -139,6 +139,10 @@ void gicv3_its_dt_init(const struct dt_device_node *node); > #ifdef CONFIG_ACPI > void gicv3_its_acpi_init(void); > #endif > + > +/* Deny iomem access for its */ > +int gicv3_its_deny_access(const struct domain *d); > + > bool gicv3_its_host_has_its(void); > > unsigned int vgic_v3_its_count(const struct domain *d); > @@ -206,6 +210,11 @@ static inline void gicv3_its_acpi_init(void) > } > #endif > > +static inline int gicv3_its_deny_access(const struct domain *d) > +{ > + return 0; > +} > + > static inline bool gicv3_its_host_has_its(void) > { > return false; > Cheers,
Hi Julien, On 10/10/2017 7:09 PM, Julien Grall wrote: > Hi Manish, > > On 10/10/17 13:52, mjaggi@caviumnetworks.com wrote: >> From: Manish Jaggi <mjaggi@cavium.com> >> >> This patch extends the gicv3_iomem_deny_access functionality by adding >> support for ITS region as well. Add function gicv3_its_deny_access. >> >> Reviewed-by: Andre Przywara <andre.przywara@arm.com> >> Acked-by: Julien Grall <julien.grall@arm.com> > > Please state after "---" when you modified a patch and keep the tags > to at least check if the reviewer is happy with it. > > It is one of the reason I like the changelog in each patch. It helps > to know what changed in a specific one. It helps me to decide whether > I am happy with you keeping my tag and avoid to fully review yet > another time the patch. > > In that case, it is fine to keep it. For this patch please ack it. Changelog: I have added - a check on return value for gicv3_its_deny_access(d); - used its_data->size in place of GICV3_ITS_SIZE - remove extra space in printk Thanks manish > >> Signed-off-by: Manish Jaggi <mjaggi@cavium.com> > --- >> xen/arch/arm/gic-v3-its.c | 22 ++++++++++++++++++++++ >> xen/arch/arm/gic-v3.c | 4 ++++ >> xen/include/asm-arm/gic_v3_its.h | 9 +++++++++ >> 3 files changed, 35 insertions(+) >> >> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c >> index 3023ee5..bd94308 100644 >> --- a/xen/arch/arm/gic-v3-its.c >> +++ b/xen/arch/arm/gic-v3-its.c >> @@ -21,6 +21,7 @@ >> #include <xen/acpi.h> >> #include <xen/lib.h> >> #include <xen/delay.h> >> +#include <xen/iocap.h> >> #include <xen/libfdt/libfdt.h> >> #include <xen/mm.h> >> #include <xen/rbtree.h> >> @@ -905,6 +906,27 @@ struct pending_irq >> *gicv3_assign_guest_event(struct domain *d, >> return pirq; >> } >> +int gicv3_its_deny_access(const struct domain *d) >> +{ >> + int rc = 0; >> + unsigned long mfn, nr; >> + const struct host_its *its_data; >> + >> + list_for_each_entry( its_data, &host_its_list, entry ) >> + { >> + mfn = paddr_to_pfn(its_data->addr); >> + nr = PFN_UP(its_data->size); >> + rc = iomem_deny_access(d, mfn, mfn + nr); >> + if ( rc ) >> + { >> + printk("iomem_deny_access failed for %lx:%lx \r\n", mfn, >> nr); >> + break; >> + } >> + } >> + >> + return rc; >> +} >> + >> /* >> * Create the respective guest DT nodes from a list of host ITSes. >> * This copies the reg property, so the guest sees the ITS at the >> same address >> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c >> index 6f562f4..475e0d3 100644 >> --- a/xen/arch/arm/gic-v3.c >> +++ b/xen/arch/arm/gic-v3.c >> @@ -1308,6 +1308,10 @@ static int gicv3_iomem_deny_access(const >> struct domain *d) >> if ( rc ) >> return rc; >> + rc = gicv3_its_deny_access(d); >> + if ( rc ) >> + return rc; >> + >> for ( i = 0; i < gicv3.rdist_count; i++ ) >> { >> mfn = gicv3.rdist_regions[i].base >> PAGE_SHIFT; >> diff --git a/xen/include/asm-arm/gic_v3_its.h >> b/xen/include/asm-arm/gic_v3_its.h >> index 73d1fd1..73ee0ba 100644 >> --- a/xen/include/asm-arm/gic_v3_its.h >> +++ b/xen/include/asm-arm/gic_v3_its.h >> @@ -139,6 +139,10 @@ void gicv3_its_dt_init(const struct >> dt_device_node *node); >> #ifdef CONFIG_ACPI >> void gicv3_its_acpi_init(void); >> #endif >> + >> +/* Deny iomem access for its */ >> +int gicv3_its_deny_access(const struct domain *d); >> + >> bool gicv3_its_host_has_its(void); >> unsigned int vgic_v3_its_count(const struct domain *d); >> @@ -206,6 +210,11 @@ static inline void gicv3_its_acpi_init(void) >> } >> #endif >> +static inline int gicv3_its_deny_access(const struct domain *d) >> +{ >> + return 0; >> +} >> + >> static inline bool gicv3_its_host_has_its(void) >> { >> return false; >> > > Cheers, >
On 10/10/17 14:53, Manish Jaggi wrote: > Hi Julien, > > On 10/10/2017 7:09 PM, Julien Grall wrote: >> Hi Manish, >> >> On 10/10/17 13:52, mjaggi@caviumnetworks.com wrote: >>> From: Manish Jaggi <mjaggi@cavium.com> >>> >>> This patch extends the gicv3_iomem_deny_access functionality by adding >>> support for ITS region as well. Add function gicv3_its_deny_access. >>> >>> Reviewed-by: Andre Przywara <andre.przywara@arm.com> >>> Acked-by: Julien Grall <julien.grall@arm.com> >> >> Please state after "---" when you modified a patch and keep the tags >> to at least check if the reviewer is happy with it. >> >> It is one of the reason I like the changelog in each patch. It helps >> to know what changed in a specific one. It helps me to decide whether >> I am happy with you keeping my tag and avoid to fully review yet >> another time the patch. >> >> In that case, it is fine to keep it. > For this patch please ack it. I didn't ask to remove it :). Just pointed out that you should be more mindful when keeping an acked-by/reviewed-by. > Changelog: > I have added > - a check on return value for gicv3_its_deny_access(d); > - used its_data->size in place of GICV3_ITS_SIZE > - remove extra space in printk Thank you for the changelog! Cheers,
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c index 3023ee5..bd94308 100644 --- a/xen/arch/arm/gic-v3-its.c +++ b/xen/arch/arm/gic-v3-its.c @@ -21,6 +21,7 @@ #include <xen/acpi.h> #include <xen/lib.h> #include <xen/delay.h> +#include <xen/iocap.h> #include <xen/libfdt/libfdt.h> #include <xen/mm.h> #include <xen/rbtree.h> @@ -905,6 +906,27 @@ struct pending_irq *gicv3_assign_guest_event(struct domain *d, return pirq; } +int gicv3_its_deny_access(const struct domain *d) +{ + int rc = 0; + unsigned long mfn, nr; + const struct host_its *its_data; + + list_for_each_entry( its_data, &host_its_list, entry ) + { + mfn = paddr_to_pfn(its_data->addr); + nr = PFN_UP(its_data->size); + rc = iomem_deny_access(d, mfn, mfn + nr); + if ( rc ) + { + printk("iomem_deny_access failed for %lx:%lx \r\n", mfn, nr); + break; + } + } + + return rc; +} + /* * Create the respective guest DT nodes from a list of host ITSes. * This copies the reg property, so the guest sees the ITS at the same address diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 6f562f4..475e0d3 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -1308,6 +1308,10 @@ static int gicv3_iomem_deny_access(const struct domain *d) if ( rc ) return rc; + rc = gicv3_its_deny_access(d); + if ( rc ) + return rc; + for ( i = 0; i < gicv3.rdist_count; i++ ) { mfn = gicv3.rdist_regions[i].base >> PAGE_SHIFT; diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h index 73d1fd1..73ee0ba 100644 --- a/xen/include/asm-arm/gic_v3_its.h +++ b/xen/include/asm-arm/gic_v3_its.h @@ -139,6 +139,10 @@ void gicv3_its_dt_init(const struct dt_device_node *node); #ifdef CONFIG_ACPI void gicv3_its_acpi_init(void); #endif + +/* Deny iomem access for its */ +int gicv3_its_deny_access(const struct domain *d); + bool gicv3_its_host_has_its(void); unsigned int vgic_v3_its_count(const struct domain *d); @@ -206,6 +210,11 @@ static inline void gicv3_its_acpi_init(void) } #endif +static inline int gicv3_its_deny_access(const struct domain *d) +{ + return 0; +} + static inline bool gicv3_its_host_has_its(void) { return false;