| Message ID | 20160928182457.12433-5-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 28, 2016 at 11:54 PM, Andre Przywara <andre.przywara@arm.com> wrote: > Instead of directly manipulating the tables in memory, an ITS driver > sends commands via a ring buffer to the ITS h/w to create or alter the > LPI mappings. > Allocate memory for that buffer and tell the ITS about it to be able > to send ITS commands. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic-its.c | 25 +++++++++++++++++++++++++ > xen/include/asm-arm/gic-its.h | 1 + > 2 files changed, 26 insertions(+) > > diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c > index 40238a2..c8a7a7e 100644 > --- a/xen/arch/arm/gic-its.c > +++ b/xen/arch/arm/gic-its.c > @@ -18,6 +18,7 @@ > > #include <xen/config.h> > #include <xen/lib.h> > +#include <xen/err.h> > #include <xen/device_tree.h> > #include <xen/libfdt/libfdt.h> > #include <asm/p2m.h> > @@ -56,6 +57,26 @@ static uint64_t encode_phys_addr(paddr_t addr, int page_bits) > return ret | (addr & GENMASK(51, 48)) >> (48 - 12); > } > > +static void *gicv3_map_cbaser(void __iomem *cbasereg) > +{ > + uint64_t attr, reg; > + void *buffer; > + > + attr = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT; > + attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT; > + attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT; > + > + buffer = alloc_xenheap_pages(0, 0); > + if ( !buffer ) > + return ERR_PTR(-ENOMEM); > + > + /* We use exactly one 4K page, so the "Size" field is 0. */ > + reg = attr | BIT(63) | (virt_to_maddr(buffer) & GENMASK(51, 12)); Isn't GENMASK(47, 12)?. Am I referring to wrong spec?
On Wed, 28 Sep 2016, Andre Przywara wrote: > Instead of directly manipulating the tables in memory, an ITS driver > sends commands via a ring buffer to the ITS h/w to create or alter the > LPI mappings. > Allocate memory for that buffer and tell the ITS about it to be able > to send ITS commands. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic-its.c | 25 +++++++++++++++++++++++++ > xen/include/asm-arm/gic-its.h | 1 + > 2 files changed, 26 insertions(+) > > diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c > index 40238a2..c8a7a7e 100644 > --- a/xen/arch/arm/gic-its.c > +++ b/xen/arch/arm/gic-its.c > @@ -18,6 +18,7 @@ > > #include <xen/config.h> > #include <xen/lib.h> > +#include <xen/err.h> > #include <xen/device_tree.h> > #include <xen/libfdt/libfdt.h> > #include <asm/p2m.h> > @@ -56,6 +57,26 @@ static uint64_t encode_phys_addr(paddr_t addr, int page_bits) > return ret | (addr & GENMASK(51, 48)) >> (48 - 12); > } > > +static void *gicv3_map_cbaser(void __iomem *cbasereg) Shouldn't it be its_map_cbaser? > +{ > + uint64_t attr, reg; > + void *buffer; > + > + attr = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT; > + attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT; > + attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT; > + > + buffer = alloc_xenheap_pages(0, 0); > + if ( !buffer ) > + return ERR_PTR(-ENOMEM); We haven't use much ERR_PTR on arm so far. In this case I'd just return NULL. > + > + /* We use exactly one 4K page, so the "Size" field is 0. */ > + reg = attr | BIT(63) | (virt_to_maddr(buffer) & GENMASK(51, 12)); Shouldn't the mask be GENMASK(47, 12)? Maybe I have an old spec version. > + writeq_relaxed(reg, cbasereg); > + > + return buffer; > +} > + > static int gicv3_map_baser(void __iomem *basereg, uint64_t regc, int nr_items) > { > uint64_t attr; > @@ -149,6 +170,10 @@ int gicv3_its_init(struct host_its *hw_its) > } > } > > + hw_its->cmd_buf = gicv3_map_cbaser(hw_its->its_base + GITS_CBASER); > + if ( IS_ERR(hw_its->cmd_buf) ) > + return PTR_ERR(hw_its->cmd_buf); > + > return 0; > } > > diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h > index 589b889..b2a003f 100644 > --- a/xen/include/asm-arm/gic-its.h > +++ b/xen/include/asm-arm/gic-its.h > @@ -69,6 +69,7 @@ struct host_its { > paddr_t addr; > paddr_t size; > void __iomem *its_base; > + void *cmd_buf; > }; > > extern struct list_head host_its_list; > -- > 2.9.0 >
Hello Andre, On 28/09/16 19:24, Andre Przywara wrote: > Instead of directly manipulating the tables in memory, an ITS driver > sends commands via a ring buffer to the ITS h/w to create or alter the > LPI mappings. > Allocate memory for that buffer and tell the ITS about it to be able > to send ITS commands. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic-its.c | 25 +++++++++++++++++++++++++ > xen/include/asm-arm/gic-its.h | 1 + > 2 files changed, 26 insertions(+) > > diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c > index 40238a2..c8a7a7e 100644 > --- a/xen/arch/arm/gic-its.c > +++ b/xen/arch/arm/gic-its.c > @@ -18,6 +18,7 @@ > > #include <xen/config.h> > #include <xen/lib.h> > +#include <xen/err.h> > #include <xen/device_tree.h> > #include <xen/libfdt/libfdt.h> > #include <asm/p2m.h> > @@ -56,6 +57,26 @@ static uint64_t encode_phys_addr(paddr_t addr, int page_bits) > return ret | (addr & GENMASK(51, 48)) >> (48 - 12); > } > > +static void *gicv3_map_cbaser(void __iomem *cbasereg) > +{ > + uint64_t attr, reg; > + void *buffer; > + > + attr = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT; > + attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT; > + attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT; > + You could directly use 'reg' here rather than have a temporary variable. Also what if the shareability/cacheability has been fixed by the hardware? > + buffer = alloc_xenheap_pages(0, 0); Please document how you decide to use only a 4K page (is there potentially a drawback)? Also I would prefer if you add a define for the size of the command queue. This will be more readable. > + if ( !buffer ) > + return ERR_PTR(-ENOMEM); > + > + /* We use exactly one 4K page, so the "Size" field is 0. */ > + reg = attr | BIT(63) | (virt_to_maddr(buffer) & GENMASK(51, 12)); Please introduce a define for bit 63. Also masking the address is not useful. > + writeq_relaxed(reg, cbasereg); Should not we initialize GITS_CWRITER to 0? From the spec the field is reset to an UNKNOWN value (see 8.19.5)? > + > + return buffer; > +} > + > static int gicv3_map_baser(void __iomem *basereg, uint64_t regc, int nr_items) > { > uint64_t attr; > @@ -149,6 +170,10 @@ int gicv3_its_init(struct host_its *hw_its) > } > } > > + hw_its->cmd_buf = gicv3_map_cbaser(hw_its->its_base + GITS_CBASER); > + if ( IS_ERR(hw_its->cmd_buf) ) > + return PTR_ERR(hw_its->cmd_buf); > + > return 0; > } > > diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h > index 589b889..b2a003f 100644 > --- a/xen/include/asm-arm/gic-its.h > +++ b/xen/include/asm-arm/gic-its.h > @@ -69,6 +69,7 @@ struct host_its { > paddr_t addr; > paddr_t size; > void __iomem *its_base; > + void *cmd_buf; > }; > > extern struct list_head host_its_list; > Regards,
Hi, On 27/10/16 00:03, Stefano Stabellini wrote: > On Wed, 28 Sep 2016, Andre Przywara wrote: >> Instead of directly manipulating the tables in memory, an ITS driver >> sends commands via a ring buffer to the ITS h/w to create or alter the >> LPI mappings. >> Allocate memory for that buffer and tell the ITS about it to be able >> to send ITS commands. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/gic-its.c | 25 +++++++++++++++++++++++++ >> xen/include/asm-arm/gic-its.h | 1 + >> 2 files changed, 26 insertions(+) >> >> diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c >> index 40238a2..c8a7a7e 100644 >> --- a/xen/arch/arm/gic-its.c >> +++ b/xen/arch/arm/gic-its.c >> @@ -18,6 +18,7 @@ >> >> #include <xen/config.h> >> #include <xen/lib.h> >> +#include <xen/err.h> >> #include <xen/device_tree.h> >> #include <xen/libfdt/libfdt.h> >> #include <asm/p2m.h> >> @@ -56,6 +57,26 @@ static uint64_t encode_phys_addr(paddr_t addr, int page_bits) >> return ret | (addr & GENMASK(51, 48)) >> (48 - 12); >> } >> >> +static void *gicv3_map_cbaser(void __iomem *cbasereg) > > Shouldn't it be its_map_cbaser? Yes. >> +{ >> + uint64_t attr, reg; >> + void *buffer; >> + >> + attr = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT; >> + attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT; >> + attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT; >> + >> + buffer = alloc_xenheap_pages(0, 0); >> + if ( !buffer ) >> + return ERR_PTR(-ENOMEM); > > We haven't use much ERR_PTR on arm so far. In this case I'd just return > NULL. In this case I agree, though "we haven't uses it much" isn't really a good argument ;-) Cheers, Andre. >> + >> + /* We use exactly one 4K page, so the "Size" field is 0. */ >> + reg = attr | BIT(63) | (virt_to_maddr(buffer) & GENMASK(51, 12)); > > Shouldn't the mask be GENMASK(47, 12)? Maybe I have an old spec > version. > > >> + writeq_relaxed(reg, cbasereg); >> + >> + return buffer; >> +} >> + >> static int gicv3_map_baser(void __iomem *basereg, uint64_t regc, int nr_items) >> { >> uint64_t attr; >> @@ -149,6 +170,10 @@ int gicv3_its_init(struct host_its *hw_its) >> } >> } >> >> + hw_its->cmd_buf = gicv3_map_cbaser(hw_its->its_base + GITS_CBASER); >> + if ( IS_ERR(hw_its->cmd_buf) ) >> + return PTR_ERR(hw_its->cmd_buf); >> + >> return 0; >> } >> >> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h >> index 589b889..b2a003f 100644 >> --- a/xen/include/asm-arm/gic-its.h >> +++ b/xen/include/asm-arm/gic-its.h >> @@ -69,6 +69,7 @@ struct host_its { >> paddr_t addr; >> paddr_t size; >> void __iomem *its_base; >> + void *cmd_buf; >> }; >> >> extern struct list_head host_its_list; >> -- >> 2.9.0 >> >
diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c index 40238a2..c8a7a7e 100644 --- a/xen/arch/arm/gic-its.c +++ b/xen/arch/arm/gic-its.c @@ -18,6 +18,7 @@ #include <xen/config.h> #include <xen/lib.h> +#include <xen/err.h> #include <xen/device_tree.h> #include <xen/libfdt/libfdt.h> #include <asm/p2m.h> @@ -56,6 +57,26 @@ static uint64_t encode_phys_addr(paddr_t addr, int page_bits) return ret | (addr & GENMASK(51, 48)) >> (48 - 12); } +static void *gicv3_map_cbaser(void __iomem *cbasereg) +{ + uint64_t attr, reg; + void *buffer; + + attr = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT; + attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT; + attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT; + + buffer = alloc_xenheap_pages(0, 0); + if ( !buffer ) + return ERR_PTR(-ENOMEM); + + /* We use exactly one 4K page, so the "Size" field is 0. */ + reg = attr | BIT(63) | (virt_to_maddr(buffer) & GENMASK(51, 12)); + writeq_relaxed(reg, cbasereg); + + return buffer; +} + static int gicv3_map_baser(void __iomem *basereg, uint64_t regc, int nr_items) { uint64_t attr; @@ -149,6 +170,10 @@ int gicv3_its_init(struct host_its *hw_its) } } + hw_its->cmd_buf = gicv3_map_cbaser(hw_its->its_base + GITS_CBASER); + if ( IS_ERR(hw_its->cmd_buf) ) + return PTR_ERR(hw_its->cmd_buf); + return 0; } diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h index 589b889..b2a003f 100644 --- a/xen/include/asm-arm/gic-its.h +++ b/xen/include/asm-arm/gic-its.h @@ -69,6 +69,7 @@ struct host_its { paddr_t addr; paddr_t size; void __iomem *its_base; + void *cmd_buf; }; extern struct list_head host_its_list;
Instead of directly manipulating the tables in memory, an ITS driver sends commands via a ring buffer to the ITS h/w to create or alter the LPI mappings. Allocate memory for that buffer and tell the ITS about it to be able to send ITS commands. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic-its.c | 25 +++++++++++++++++++++++++ xen/include/asm-arm/gic-its.h | 1 + 2 files changed, 26 insertions(+)