subject:"RE\\\: \\\[PATCH\\\] xen\\\/arm\\\: vgic to ignore GICD ICPENRn registers access"

RE: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access

2021-10-12 Thread Hongda Deng

Hi,

Thanks for your great and detailed advice, I did some investigations about vgic 
especially inflight_irqs in the last few days.

> -Original Message-
> From: Stefano Stabellini 
> Sent: 2021年9月24日 4:54
> To: Julien Grall 
> Cc: Hongda Deng ; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org; Bertrand Marquis ; Wei
> Chen 
> Subject: Re: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access
> 
> On Thu, 23 Sep 2021, Julien Grall wrote:
> > Hi,
> >
> > On 23/09/2021 11:14, Hongda Deng wrote:
> > > Currently, Xen will return IO unhandled when guests access GICD ICPENRn
> > > registers. This will raise a data abort inside guest. For Linux Guest,
> > > these virtual registers will not be accessed. But for Zephyr, in its
> > > GIC initilization code, these virtual registers will be accessed. And
> > > zephyr guest will get an IO dataabort in initilization stage and enter
> >
> > s/dataabort/data abort/
> > s/initilization/initialization/
> >

Ack.

> > > fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
> > > we currently ignore these virtual registers access and print a message
> > > about whether they are already pending instead of returning unhandled.
> > > More details can be found at [1].
> > >
> > > [1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/
> > > msg00744.html
> > >
> > > Signed-off-by: Hongda Deng 
> > > ---
> > >   xen/arch/arm/vgic-v2.c | 10 +++---
> > >   xen/arch/arm/vgic-v3.c | 29 +
> > >   xen/arch/arm/vgic.c| 37 +
> > >   xen/include/asm-arm/vgic.h |  2 ++
> > >   4 files changed, 63 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > > index b2da886adc..644c62757c 100644
> > > --- a/xen/arch/arm/vgic-v2.c
> > > +++ b/xen/arch/arm/vgic-v2.c
> > > @@ -481,10 +481,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
> > > mmio_info_t *info,
> > > case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
> > >   if ( dabt.size != DABT_WORD ) goto bad_width;
> > > +rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, 
> > > DABT_WORD);
> > > +if ( rank == NULL ) goto write_ignore;
> >
> >
> >
> > > +
> > >   printk(XENLOG_G_ERR
> > > -   "%pv: vGICD: unhandled word write %#"PRIregister" to
> > > ICPENDR%d\n",
> > > -   v, r, gicd_reg - GICD_ICPENDR);
> > > -return 0;
> > > +   "%pv: vGICD: unhandled word write %#"PRIregister" to
> > > ICPENDR%d, and current pending state is: %s\n",
> > > +   v, r, gicd_reg - GICD_ICPENDR,
> > > +   vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
> >
> > Each register contain the information for multiple pending interrupts. So it
> > is a bit confusing to say whether the state is on/off. Instead, it would be
> > better to state which interrupt is pending.
> >
> > Also, I would rather avoid printing a message if there are no interrupts
> > pending because there are no issues if this is happening.

I will fix it in the next version patch.

> >
> > > +goto write_ignore_32;
> > > case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> > >   if ( dabt.size != DABT_WORD ) goto bad_width;
> > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> > > index cb5a70c42e..c94e33ff4f 100644
> > > --- a/xen/arch/arm/vgic-v3.c
> > > +++ b/xen/arch/arm/vgic-v3.c
> > > @@ -817,10 +817,14 @@ static int
> __vgic_v3_distr_common_mmio_write(const
> > > char *name, struct vcpu *v,
> > > case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
> > >   if ( dabt.size != DABT_WORD ) goto bad_width;
> > > +rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
> > > +if ( rank == NULL ) goto write_ignore;
> > > +
> > >   printk(XENLOG_G_ERR
> > > -   "%pv: %s: unhandled word write %#"PRIregister" to
> > > ICPENDR%d\n",
> > > -   v, name, r, reg - GICD_ICPENDR);
> > > -return 0;
> > > +   "%pv: %s: unhandled word write %#"PRIregister" to 
> > > ICPENDR%d,
> > > and cur

Re: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access

2021-09-23 Thread Stefano Stabellini

On Thu, 23 Sep 2021, Julien Grall wrote:
> Hi,
> 
> On 23/09/2021 11:14, Hongda Deng wrote:
> > Currently, Xen will return IO unhandled when guests access GICD ICPENRn
> > registers. This will raise a data abort inside guest. For Linux Guest,
> > these virtual registers will not be accessed. But for Zephyr, in its
> > GIC initilization code, these virtual registers will be accessed. And
> > zephyr guest will get an IO dataabort in initilization stage and enter
> 
> s/dataabort/data abort/
> s/initilization/initialization/
> 
> > fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
> > we currently ignore these virtual registers access and print a message
> > about whether they are already pending instead of returning unhandled.
> > More details can be found at [1].
> > 
> > [1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/
> > msg00744.html
> > 
> > Signed-off-by: Hongda Deng 
> > ---
> >   xen/arch/arm/vgic-v2.c | 10 +++---
> >   xen/arch/arm/vgic-v3.c | 29 +
> >   xen/arch/arm/vgic.c| 37 +
> >   xen/include/asm-arm/vgic.h |  2 ++
> >   4 files changed, 63 insertions(+), 15 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > index b2da886adc..644c62757c 100644
> > --- a/xen/arch/arm/vgic-v2.c
> > +++ b/xen/arch/arm/vgic-v2.c
> > @@ -481,10 +481,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
> > mmio_info_t *info,
> > case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
> >   if ( dabt.size != DABT_WORD ) goto bad_width;
> > +rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
> > +if ( rank == NULL ) goto write_ignore;
> 
> 
> 
> > +
> >   printk(XENLOG_G_ERR
> > -   "%pv: vGICD: unhandled word write %#"PRIregister" to
> > ICPENDR%d\n",
> > -   v, r, gicd_reg - GICD_ICPENDR);
> > -return 0;
> > +   "%pv: vGICD: unhandled word write %#"PRIregister" to
> > ICPENDR%d, and current pending state is: %s\n",
> > +   v, r, gicd_reg - GICD_ICPENDR,
> > +   vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
> 
> Each register contain the information for multiple pending interrupts. So it
> is a bit confusing to say whether the state is on/off. Instead, it would be
> better to state which interrupt is pending.
> 
> Also, I would rather avoid printing a message if there are no interrupts
> pending because there are no issues if this is happening.
> 
> > +goto write_ignore_32;
> > case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> >   if ( dabt.size != DABT_WORD ) goto bad_width;
> > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> > index cb5a70c42e..c94e33ff4f 100644
> > --- a/xen/arch/arm/vgic-v3.c
> > +++ b/xen/arch/arm/vgic-v3.c
> > @@ -817,10 +817,14 @@ static int __vgic_v3_distr_common_mmio_write(const
> > char *name, struct vcpu *v,
> > case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
> >   if ( dabt.size != DABT_WORD ) goto bad_width;
> > +rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
> > +if ( rank == NULL ) goto write_ignore;
> > +
> >   printk(XENLOG_G_ERR
> > -   "%pv: %s: unhandled word write %#"PRIregister" to
> > ICPENDR%d\n",
> > -   v, name, r, reg - GICD_ICPENDR);
> > -return 0;
> > +   "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d,
> > and current pending state is: %s\n",
> > +   v, name, r, reg - GICD_ICPENDR,
> > +   vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
> > +goto write_ignore_32;
> > case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
> >   if ( dabt.size != DABT_WORD ) goto bad_width;
> > @@ -978,19 +982,20 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu
> > *v, mmio_info_t *info,
> >   case VREG32(GICR_ICFGR1):
> >   case VRANGE32(GICR_IPRIORITYR0, GICR_IPRIORITYR7):
> >   case VREG32(GICR_ISPENDR0):
> > - /*
> > -  * Above registers offset are common with GICD.
> > -  * So handle common with GICD handling
> > -  */
> > +/*
> > + * Above registers offset are common with GICD.
> > + * So handle common with GICD handling
> > + */
> >   return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
> >info, gicr_reg, r);
> > case VREG32(GICR_ICPENDR0):
> > -if ( dabt.size != DABT_WORD ) goto bad_width;
> > -printk(XENLOG_G_ERR
> > -   "%pv: vGICR: SGI: unhandled word write %#"PRIregister" to
> > ICPENDR0\n",
> > -   v, r);
> > -return 0;
> > +/*
> > + * Above registers offset are common with GICD.
> > + * So handle common with GICD handling
> > + */
> > +return

Re: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access

2021-09-23 Thread Julien Grall


Hi,

On 23/09/2021 11:14, Hongda Deng wrote:

Currently, Xen will return IO unhandled when guests access GICD ICPENRn
registers. This will raise a data abort inside guest. For Linux Guest,
these virtual registers will not be accessed. But for Zephyr, in its
GIC initilization code, these virtual registers will be accessed. And
zephyr guest will get an IO dataabort in initilization stage and enter


s/dataabort/data abort/
s/initilization/initialization/


fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
we currently ignore these virtual registers access and print a message
about whether they are already pending instead of returning unhandled.
More details can be found at [1].

[1] https://lists.xenproject.org/archives/html/xen-devel/2021-09/
msg00744.html

Signed-off-by: Hongda Deng 
---
  xen/arch/arm/vgic-v2.c | 10 +++---
  xen/arch/arm/vgic-v3.c | 29 +
  xen/arch/arm/vgic.c| 37 +
  xen/include/asm-arm/vgic.h |  2 ++
  4 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b2da886adc..644c62757c 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -481,10 +481,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, 
mmio_info_t *info,
  
  case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):

  if ( dabt.size != DABT_WORD ) goto bad_width;
+rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
+if ( rank == NULL ) goto write_ignore;





+
  printk(XENLOG_G_ERR
-   "%pv: vGICD: unhandled word write %#"PRIregister" to 
ICPENDR%d\n",
-   v, r, gicd_reg - GICD_ICPENDR);
-return 0;
+   "%pv: vGICD: unhandled word write %#"PRIregister" to ICPENDR%d, and 
current pending state is: %s\n",
+   v, r, gicd_reg - GICD_ICPENDR,
+   vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");


Each register contain the information for multiple pending interrupts. 
So it is a bit confusing to say whether the state is on/off. Instead, it 
would be better to state which interrupt is pending.


Also, I would rather avoid printing a message if there are no interrupts 
pending because there are no issues if this is happening.



+goto write_ignore_32;
  
  case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):

  if ( dabt.size != DABT_WORD ) goto bad_width;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index cb5a70c42e..c94e33ff4f 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -817,10 +817,14 @@ static int __vgic_v3_distr_common_mmio_write(const char 
*name, struct vcpu *v,
  
  case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):

  if ( dabt.size != DABT_WORD ) goto bad_width;
+rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
+if ( rank == NULL ) goto write_ignore;
+
  printk(XENLOG_G_ERR
-   "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d\n",
-   v, name, r, reg - GICD_ICPENDR);
-return 0;
+   "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d, and 
current pending state is: %s\n",
+   v, name, r, reg - GICD_ICPENDR,
+   vgic_get_irqs_pending(v, r, rank->index) ? "on" : "off");
+goto write_ignore_32;
  
  case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):

  if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -978,19 +982,20 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, 
mmio_info_t *info,
  case VREG32(GICR_ICFGR1):
  case VRANGE32(GICR_IPRIORITYR0, GICR_IPRIORITYR7):
  case VREG32(GICR_ISPENDR0):
- /*
-  * Above registers offset are common with GICD.
-  * So handle common with GICD handling
-  */
+/*
+ * Above registers offset are common with GICD.
+ * So handle common with GICD handling
+ */
  return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
   info, gicr_reg, r);
  
  case VREG32(GICR_ICPENDR0):

-if ( dabt.size != DABT_WORD ) goto bad_width;
-printk(XENLOG_G_ERR
-   "%pv: vGICR: SGI: unhandled word write %#"PRIregister" to 
ICPENDR0\n",
-   v, r);
-return 0;
+/*
+ * Above registers offset are common with GICD.
+ * So handle common with GICD handling
+ */
+return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
+ info, gicr_reg, r);
  
  case VREG32(GICR_IGRPMODR0):

  /* We do not implement security extensions for guests, write ignore */
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8f9400a519..29a1aa5056 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -470,6 +470,43 @@ void vgic_set_irqs_pending(struct

RE: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access

Re: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access

Re: [PATCH] xen/arm: vgic to ignore GICD ICPENRn registers access

3 matches

Site Navigation

Mail list logo

Footer information