| Message ID | 1507639952-31617-6-git-send-email-mjaggi@caviumnetworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/10/17 13:52, mjaggi@caviumnetworks.com wrote: > From: Manish Jaggi <mjaggi@cavium.com> > > Add gicv3_its_make_hwdom_madt to update hwdom MADT ITS information. > > Reviewed-by: Andre Przywara <andre.przywara@arm.com> Usually *-by are order from the oldest first to the earlier. This mean your Signed-off-by is first. > Signed-off-by: Manish Jaggi <mjaggi@cavium.com> Acked-by: Julien Grall <julien.grall@linaro.org> Cheers, > --- > xen/arch/arm/gic-v3-its.c | 19 +++++++++++++++++++ > xen/arch/arm/gic-v3.c | 2 ++ > xen/include/asm-arm/gic_v3_its.h | 8 ++++++++ > 3 files changed, 29 insertions(+) > > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c > index bd94308..e57ae05 100644 > --- a/xen/arch/arm/gic-v3-its.c > +++ b/xen/arch/arm/gic-v3-its.c > @@ -1062,6 +1062,25 @@ void gicv3_its_acpi_init(void) > acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, > gicv3_its_acpi_probe, 0); > } > + > +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void *base_ptr) > +{ > + unsigned int i; > + void *fw_its; > + struct acpi_madt_generic_translator *hwdom_its; > + > + hwdom_its = base_ptr; > + > + for ( i = 0; i < vgic_v3_its_count(d); i++ ) > + { > + fw_its = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, > + i); > + memcpy(hwdom_its, fw_its, sizeof(struct acpi_madt_generic_translator)); > + hwdom_its++; > + } > + > + return sizeof(struct acpi_madt_generic_translator) * vgic_v3_its_count(d); > +} > #endif > > /* > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 0289d1a..e9b9060 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -1404,6 +1404,8 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset) > table_len += size; > } > > + table_len += gicv3_its_make_hwdom_madt(d, base_ptr + table_len); > + > return table_len; > } > > diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h > index 73ee0ba..40dffdc 100644 > --- a/xen/include/asm-arm/gic_v3_its.h > +++ b/xen/include/asm-arm/gic_v3_its.h > @@ -138,6 +138,8 @@ void gicv3_its_dt_init(const struct dt_device_node *node); > > #ifdef CONFIG_ACPI > void gicv3_its_acpi_init(void); > +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, > + void *base_ptr); > #endif > > /* Deny iomem access for its */ > @@ -208,6 +210,12 @@ static inline void gicv3_its_dt_init(const struct dt_device_node *node) > static inline void gicv3_its_acpi_init(void) > { > } > + > +static inline unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, > + void *base_ptr) > +{ > + return 0; > +} > #endif > > static inline int gicv3_its_deny_access(const struct domain *d) >
On Tue, 10 Oct 2017, mjaggi@caviumnetworks.com wrote: > From: Manish Jaggi <mjaggi@cavium.com> > > Add gicv3_its_make_hwdom_madt to update hwdom MADT ITS information. > > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > Signed-off-by: Manish Jaggi <mjaggi@cavium.com> > --- > xen/arch/arm/gic-v3-its.c | 19 +++++++++++++++++++ > xen/arch/arm/gic-v3.c | 2 ++ > xen/include/asm-arm/gic_v3_its.h | 8 ++++++++ > 3 files changed, 29 insertions(+) > > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c > index bd94308..e57ae05 100644 > --- a/xen/arch/arm/gic-v3-its.c > +++ b/xen/arch/arm/gic-v3-its.c > @@ -1062,6 +1062,25 @@ void gicv3_its_acpi_init(void) > acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, > gicv3_its_acpi_probe, 0); > } > + > +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void *base_ptr) > +{ > + unsigned int i; > + void *fw_its; > + struct acpi_madt_generic_translator *hwdom_its; > + > + hwdom_its = base_ptr; > + > + for ( i = 0; i < vgic_v3_its_count(d); i++ ) > + { > + fw_its = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, > + i); > + memcpy(hwdom_its, fw_its, sizeof(struct acpi_madt_generic_translator)); I think we are supposed to use ACPI_MEMCPY for this kind of operations. If that's OK for you, I'll fix on commit. > + hwdom_its++; > + } > + > + return sizeof(struct acpi_madt_generic_translator) * vgic_v3_its_count(d); > +} > #endif > > /* > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 0289d1a..e9b9060 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -1404,6 +1404,8 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset) > table_len += size; > } > > + table_len += gicv3_its_make_hwdom_madt(d, base_ptr + table_len); > + > return table_len; > } > > diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h > index 73ee0ba..40dffdc 100644 > --- a/xen/include/asm-arm/gic_v3_its.h > +++ b/xen/include/asm-arm/gic_v3_its.h > @@ -138,6 +138,8 @@ void gicv3_its_dt_init(const struct dt_device_node *node); > > #ifdef CONFIG_ACPI > void gicv3_its_acpi_init(void); > +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, > + void *base_ptr); > #endif > > /* Deny iomem access for its */ > @@ -208,6 +210,12 @@ static inline void gicv3_its_dt_init(const struct dt_device_node *node) > static inline void gicv3_its_acpi_init(void) > { > } > + > +static inline unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, > + void *base_ptr) > +{ > + return 0; > +} > #endif > > static inline int gicv3_its_deny_access(const struct domain *d) > -- > 2.7.4 >
Hi, On 10/10/2017 20:15, Stefano Stabellini wrote: > On Tue, 10 Oct 2017, mjaggi@caviumnetworks.com wrote: >> From: Manish Jaggi <mjaggi@cavium.com> >> >> Add gicv3_its_make_hwdom_madt to update hwdom MADT ITS information. >> >> Reviewed-by: Andre Przywara <andre.przywara@arm.com> >> Signed-off-by: Manish Jaggi <mjaggi@cavium.com> >> --- >> xen/arch/arm/gic-v3-its.c | 19 +++++++++++++++++++ >> xen/arch/arm/gic-v3.c | 2 ++ >> xen/include/asm-arm/gic_v3_its.h | 8 ++++++++ >> 3 files changed, 29 insertions(+) >> >> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c >> index bd94308..e57ae05 100644 >> --- a/xen/arch/arm/gic-v3-its.c >> +++ b/xen/arch/arm/gic-v3-its.c >> @@ -1062,6 +1062,25 @@ void gicv3_its_acpi_init(void) >> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, >> gicv3_its_acpi_probe, 0); >> } >> + >> +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void *base_ptr) >> +{ >> + unsigned int i; >> + void *fw_its; >> + struct acpi_madt_generic_translator *hwdom_its; >> + >> + hwdom_its = base_ptr; >> + >> + for ( i = 0; i < vgic_v3_its_count(d); i++ ) >> + { >> + fw_its = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, >> + i); >> + memcpy(hwdom_its, fw_its, sizeof(struct acpi_madt_generic_translator)); > > I think we are supposed to use ACPI_MEMCPY for this kind of operations. > If that's OK for you, I'll fix on commit. I don't think we should use ACPI_MEMCPY. The macro is here because acpica (our drivers/acpi) is meant to be OS-agnostic. So you need a way to tell how your OS copies memory. I had a look on the usage of ACPI_MEMCPY, it seems that only the arch/arm and drivers/acpi is using it. This seem to confirm that probably we used it by mistake in the Arm code. Cheers,
CC'ing Jan and Andrew, just in case they disagree on this. On Tue, 10 Oct 2017, Julien Grall wrote: > > > +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void > > > *base_ptr) > > > +{ > > > + unsigned int i; > > > + void *fw_its; > > > + struct acpi_madt_generic_translator *hwdom_its; > > > + > > > + hwdom_its = base_ptr; > > > + > > > + for ( i = 0; i < vgic_v3_its_count(d); i++ ) > > > + { > > > + fw_its = > > > acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, > > > + i); > > > + memcpy(hwdom_its, fw_its, sizeof(struct > > > acpi_madt_generic_translator)); > > > > I think we are supposed to use ACPI_MEMCPY for this kind of operations. > > If that's OK for you, I'll fix on commit. > > I don't think we should use ACPI_MEMCPY. The macro is here because acpica (our > drivers/acpi) is meant to be OS-agnostic. So you need a way to tell how your > OS copies memory. > > I had a look on the usage of ACPI_MEMCPY, it seems that only the arch/arm and > drivers/acpi is using it. This seem to confirm that probably we used it by > mistake in the Arm code. It looks like you are right, ACPI_MEMCPY does not make sense in Xen code outside of drivers/acpi. Consistency is important, so if we are not going to use ACPI_MEMCPY, then I'll write a patch (or I'll take a patch if you volunteer in writing it) to s/ACPI_MEMCPY/memcpy/g everywhere under arch/arm. The worst we could end up with is a mixed environment.
Hi, On 10/10/2017 21:07, Stefano Stabellini wrote: > CC'ing Jan and Andrew, just in case they disagree on this. > > On Tue, 10 Oct 2017, Julien Grall wrote: >>>> +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void >>>> *base_ptr) >>>> +{ >>>> + unsigned int i; >>>> + void *fw_its; >>>> + struct acpi_madt_generic_translator *hwdom_its; >>>> + >>>> + hwdom_its = base_ptr; >>>> + >>>> + for ( i = 0; i < vgic_v3_its_count(d); i++ ) >>>> + { >>>> + fw_its = >>>> acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, >>>> + i); >>>> + memcpy(hwdom_its, fw_its, sizeof(struct >>>> acpi_madt_generic_translator)); >>> >>> I think we are supposed to use ACPI_MEMCPY for this kind of operations. >>> If that's OK for you, I'll fix on commit. >> >> I don't think we should use ACPI_MEMCPY. The macro is here because acpica (our >> drivers/acpi) is meant to be OS-agnostic. So you need a way to tell how your >> OS copies memory. >> >> I had a look on the usage of ACPI_MEMCPY, it seems that only the arch/arm and >> drivers/acpi is using it. This seem to confirm that probably we used it by >> mistake in the Arm code. > > It looks like you are right, ACPI_MEMCPY does not make sense in Xen > code outside of drivers/acpi. > > Consistency is important, so if we are not going to use ACPI_MEMCPY, then > I'll write a patch (or I'll take a patch if you volunteer in writing it) > to s/ACPI_MEMCPY/memcpy/g everywhere under arch/arm. The worst we could > end up with is a mixed environment. Feel free to write a patch, but I don't think you should block this series for that. Cheers,
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c index bd94308..e57ae05 100644 --- a/xen/arch/arm/gic-v3-its.c +++ b/xen/arch/arm/gic-v3-its.c @@ -1062,6 +1062,25 @@ void gicv3_its_acpi_init(void) acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, gicv3_its_acpi_probe, 0); } + +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void *base_ptr) +{ + unsigned int i; + void *fw_its; + struct acpi_madt_generic_translator *hwdom_its; + + hwdom_its = base_ptr; + + for ( i = 0; i < vgic_v3_its_count(d); i++ ) + { + fw_its = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, + i); + memcpy(hwdom_its, fw_its, sizeof(struct acpi_madt_generic_translator)); + hwdom_its++; + } + + return sizeof(struct acpi_madt_generic_translator) * vgic_v3_its_count(d); +} #endif /* diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 0289d1a..e9b9060 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -1404,6 +1404,8 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset) table_len += size; } + table_len += gicv3_its_make_hwdom_madt(d, base_ptr + table_len); + return table_len; } diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h index 73ee0ba..40dffdc 100644 --- a/xen/include/asm-arm/gic_v3_its.h +++ b/xen/include/asm-arm/gic_v3_its.h @@ -138,6 +138,8 @@ void gicv3_its_dt_init(const struct dt_device_node *node); #ifdef CONFIG_ACPI void gicv3_its_acpi_init(void); +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, + void *base_ptr); #endif /* Deny iomem access for its */ @@ -208,6 +210,12 @@ static inline void gicv3_its_dt_init(const struct dt_device_node *node) static inline void gicv3_its_acpi_init(void) { } + +static inline unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, + void *base_ptr) +{ + return 0; +} #endif static inline int gicv3_its_deny_access(const struct domain *d)