| Message ID | 20230919112827.1001484-4-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: ITS: implement TODO and fix issues with cache | expand |
Hi Volodymyr, On 19/09/2023 12:28, Volodymyr Babchuk wrote: > ITS manages Device Tables and Interrupt Translation Tables on its own, > so generally we are not interested which shareability and cacheability > attributes it uses. But there is one exception: ITS requires that DT > and ITT must be initialized with zeroes. If ITS belongs to the Inner > Cacheability domain there is no problem at all. > > But in all other cases we need to do clean CPU caches manually, or > otherwise CPU can overwrite DT and ITT entries. From user perspective > this looks like interrupts are not delivered from a device. > > Also, we will rename HOST_ITS_FLUSH_CMD_QUEUE flag to > HOST_ITS_FLUSH_BUFFERS because now this flag controls not only command > queue. Reading the specification, CBASER will indicate the cacheability for the command queue. But I couldn't find any reference saying this will apply to the ITT as well. If such reference doesn't exist then... > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > --- > xen/arch/arm/gic-v3-its.c | 7 +++++-- > xen/arch/arm/include/asm/gic_v3_its.h | 2 +- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c > index 72cf318810..63e28a7706 100644 > --- a/xen/arch/arm/gic-v3-its.c > +++ b/xen/arch/arm/gic-v3-its.c > @@ -107,7 +107,7 @@ static int its_send_command(struct host_its *hw_its, const void *its_cmd) > } > > memcpy(hw_its->cmd_buf + writep, its_cmd, ITS_CMD_SIZE); > - if ( hw_its->flags & HOST_ITS_FLUSH_CMD_QUEUE ) > + if ( hw_its->flags & HOST_ITS_FLUSH_BUFFERS ) > clean_dcache_va_range(hw_its->cmd_buf + writep, ITS_CMD_SIZE); > else > dsb(ishst); > @@ -335,7 +335,7 @@ static void *its_map_cbaser(struct host_its *its) > */ > if ( !(reg & GITS_BASER_INNER_CACHEABILITY_MASK) ) > { > - its->flags |= HOST_ITS_FLUSH_CMD_QUEUE; > + its->flags |= HOST_ITS_FLUSH_BUFFERS; > printk(XENLOG_WARNING "using non-cacheable ITS command queue\n"); > } > > @@ -699,6 +699,9 @@ int gicv3_its_map_guest_device(struct domain *d, > if ( !itt_addr ) > goto out_unlock; > > + if ( hw_its->flags & HOST_ITS_FLUSH_BUFFERS ) > + clean_dcache_va_range(itt_addr, nr_events * hw_its->itte_size); ... I think we need to have this flush unconditional like Linux does. Cheers,
Hi, On 19/09/2023 14:10, Julien Grall wrote: > On 19/09/2023 12:28, Volodymyr Babchuk wrote: >> ITS manages Device Tables and Interrupt Translation Tables on its own, >> so generally we are not interested which shareability and cacheability >> attributes it uses. But there is one exception: ITS requires that DT >> and ITT must be initialized with zeroes. If ITS belongs to the Inner >> Cacheability domain there is no problem at all. >> >> But in all other cases we need to do clean CPU caches manually, or >> otherwise CPU can overwrite DT and ITT entries. From user perspective >> this looks like interrupts are not delivered from a device. >> >> Also, we will rename HOST_ITS_FLUSH_CMD_QUEUE flag to >> HOST_ITS_FLUSH_BUFFERS because now this flag controls not only command >> queue. > > Reading the specification, CBASER will indicate the cacheability for the > command queue. But I couldn't find any reference saying this will apply > to the ITT as well. > > If such reference doesn't exist then... > >> >> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >> --- >> xen/arch/arm/gic-v3-its.c | 7 +++++-- >> xen/arch/arm/include/asm/gic_v3_its.h | 2 +- >> 2 files changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c >> index 72cf318810..63e28a7706 100644 >> --- a/xen/arch/arm/gic-v3-its.c >> +++ b/xen/arch/arm/gic-v3-its.c >> @@ -107,7 +107,7 @@ static int its_send_command(struct host_its >> *hw_its, const void *its_cmd) >> } >> memcpy(hw_its->cmd_buf + writep, its_cmd, ITS_CMD_SIZE); >> - if ( hw_its->flags & HOST_ITS_FLUSH_CMD_QUEUE ) >> + if ( hw_its->flags & HOST_ITS_FLUSH_BUFFERS ) >> clean_dcache_va_range(hw_its->cmd_buf + writep, ITS_CMD_SIZE); >> else >> dsb(ishst); >> @@ -335,7 +335,7 @@ static void *its_map_cbaser(struct host_its *its) >> */ >> if ( !(reg & GITS_BASER_INNER_CACHEABILITY_MASK) ) >> { >> - its->flags |= HOST_ITS_FLUSH_CMD_QUEUE; >> + its->flags |= HOST_ITS_FLUSH_BUFFERS; >> printk(XENLOG_WARNING "using non-cacheable ITS command >> queue\n"); >> } >> @@ -699,6 +699,9 @@ int gicv3_its_map_guest_device(struct domain *d, >> if ( !itt_addr ) >> goto out_unlock; >> + if ( hw_its->flags & HOST_ITS_FLUSH_BUFFERS ) >> + clean_dcache_va_range(itt_addr, nr_events * hw_its->itte_size); > > ... I think we need to have this flush unconditional like Linux does. I forgot to mention that this likely want to be a clean_dcache_and_invalidate_va_range() (see my comment in patch #2). Cheers,
On 9/19/23 09:14, Julien Grall wrote: > Hi, > > On 19/09/2023 14:10, Julien Grall wrote: >> On 19/09/2023 12:28, Volodymyr Babchuk wrote: >>> ITS manages Device Tables and Interrupt Translation Tables on its own, >>> so generally we are not interested which shareability and cacheability >>> attributes it uses. But there is one exception: ITS requires that DT >>> and ITT must be initialized with zeroes. If ITS belongs to the Inner >>> Cacheability domain there is no problem at all. >>> >>> But in all other cases we need to do clean CPU caches manually, or >>> otherwise CPU can overwrite DT and ITT entries. From user perspective >>> this looks like interrupts are not delivered from a device. >>> >>> Also, we will rename HOST_ITS_FLUSH_CMD_QUEUE flag to >>> HOST_ITS_FLUSH_BUFFERS because now this flag controls not only command >>> queue. >> >> Reading the specification, CBASER will indicate the cacheability for the >> command queue. But I couldn't find any reference saying this will apply >> to the ITT as well. >> >> If such reference doesn't exist then... >> >>> >>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >>> --- >>> xen/arch/arm/gic-v3-its.c | 7 +++++-- >>> xen/arch/arm/include/asm/gic_v3_its.h | 2 +- >>> 2 files changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c >>> index 72cf318810..63e28a7706 100644 >>> --- a/xen/arch/arm/gic-v3-its.c >>> +++ b/xen/arch/arm/gic-v3-its.c >>> @@ -107,7 +107,7 @@ static int its_send_command(struct host_its >>> *hw_its, const void *its_cmd) >>> } >>> memcpy(hw_its->cmd_buf + writep, its_cmd, ITS_CMD_SIZE); >>> - if ( hw_its->flags & HOST_ITS_FLUSH_CMD_QUEUE ) >>> + if ( hw_its->flags & HOST_ITS_FLUSH_BUFFERS ) >>> clean_dcache_va_range(hw_its->cmd_buf + writep, ITS_CMD_SIZE); >>> else >>> dsb(ishst); >>> @@ -335,7 +335,7 @@ static void *its_map_cbaser(struct host_its *its) >>> */ >>> if ( !(reg & GITS_BASER_INNER_CACHEABILITY_MASK) ) >>> { >>> - its->flags |= HOST_ITS_FLUSH_CMD_QUEUE; >>> + its->flags |= HOST_ITS_FLUSH_BUFFERS; >>> printk(XENLOG_WARNING "using non-cacheable ITS command >>> queue\n"); >>> } >>> @@ -699,6 +699,9 @@ int gicv3_its_map_guest_device(struct domain *d, >>> if ( !itt_addr ) >>> goto out_unlock; >>> + if ( hw_its->flags & HOST_ITS_FLUSH_BUFFERS ) >>> + clean_dcache_va_range(itt_addr, nr_events * hw_its->itte_size); >> >> ... I think we need to have this flush unconditional like Linux does. > > I forgot to mention that this likely want to be a > clean_dcache_and_invalidate_va_range() (see my comment in patch #2). I turned it into an unconditional clean_and_invalidate_dcache_va_range() and did a quick test, and it still fixes my test case on VCK190. So feel free to add: Tested-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c index 72cf318810..63e28a7706 100644 --- a/xen/arch/arm/gic-v3-its.c +++ b/xen/arch/arm/gic-v3-its.c @@ -107,7 +107,7 @@ static int its_send_command(struct host_its *hw_its, const void *its_cmd) } memcpy(hw_its->cmd_buf + writep, its_cmd, ITS_CMD_SIZE); - if ( hw_its->flags & HOST_ITS_FLUSH_CMD_QUEUE ) + if ( hw_its->flags & HOST_ITS_FLUSH_BUFFERS ) clean_dcache_va_range(hw_its->cmd_buf + writep, ITS_CMD_SIZE); else dsb(ishst); @@ -335,7 +335,7 @@ static void *its_map_cbaser(struct host_its *its) */ if ( !(reg & GITS_BASER_INNER_CACHEABILITY_MASK) ) { - its->flags |= HOST_ITS_FLUSH_CMD_QUEUE; + its->flags |= HOST_ITS_FLUSH_BUFFERS; printk(XENLOG_WARNING "using non-cacheable ITS command queue\n"); } @@ -699,6 +699,9 @@ int gicv3_its_map_guest_device(struct domain *d, if ( !itt_addr ) goto out_unlock; + if ( hw_its->flags & HOST_ITS_FLUSH_BUFFERS ) + clean_dcache_va_range(itt_addr, nr_events * hw_its->itte_size); + dev = xzalloc(struct its_device); if ( !dev ) goto out_unlock; diff --git a/xen/arch/arm/include/asm/gic_v3_its.h b/xen/arch/arm/include/asm/gic_v3_its.h index c24d4752d0..460b008db5 100644 --- a/xen/arch/arm/include/asm/gic_v3_its.h +++ b/xen/arch/arm/include/asm/gic_v3_its.h @@ -107,7 +107,7 @@ #include <xen/device_tree.h> #include <xen/rbtree.h> -#define HOST_ITS_FLUSH_CMD_QUEUE (1U << 0) +#define HOST_ITS_FLUSH_BUFFERS (1U << 0) #define HOST_ITS_USES_PTA (1U << 1) /* We allocate LPIs on the hosts in chunks of 32 to reduce handling overhead. */
ITS manages Device Tables and Interrupt Translation Tables on its own, so generally we are not interested which shareability and cacheability attributes it uses. But there is one exception: ITS requires that DT and ITT must be initialized with zeroes. If ITS belongs to the Inner Cacheability domain there is no problem at all. But in all other cases we need to do clean CPU caches manually, or otherwise CPU can overwrite DT and ITT entries. From user perspective this looks like interrupts are not delivered from a device. Also, we will rename HOST_ITS_FLUSH_CMD_QUEUE flag to HOST_ITS_FLUSH_BUFFERS because now this flag controls not only command queue. Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> --- xen/arch/arm/gic-v3-its.c | 7 +++++-- xen/arch/arm/include/asm/gic_v3_its.h | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-)