[PATCH v2] ignore writes to GICD_ICACTIVER ... GICD_ICACTIVERN

[PATCH v2] ignore writes to GICD_ICACTIVER ... GICD_ICACTIVERN

[Stefano Stabellini]

Injecting a fault to the guest just because it is writing to one of the
GICD_ICACTIVER registers, which are part of the GICv2 and GICv3 specs,
is harsh. Additionally it causes recent linux kernels to fail to boot on
Xen.

Ignore writes to GICD_ICACTIVER ... GICD_ICACTIVERN instead, to solve
the boot issue and for backportability. However implementing the
registers properly might a better long term solution.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v2:
- rebase on staging

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 2c73133..3079901 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -494,11 +494,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
         return 0;
 
     case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
-        if ( dabt.size != DABT_WORD ) goto bad_width;
-        printk(XENLOG_G_ERR
+        gdprintk(XENLOG_DEBUG,
                "%pv: vGICD: unhandled word write %#"PRIregister" to ICACTIVER%d\n",
                v, r, gicd_reg - GICD_ICACTIVER);
-        return 0;
+        goto write_ignore_32;
 
     case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
     {
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 985e866..6011e9e 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -621,11 +621,10 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
         return 0;
 
     case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
-        if ( dabt.size != DABT_WORD ) goto bad_width;
-        printk(XENLOG_G_ERR
+        gdprintk(XENLOG_DEBUG,
                "%pv: %s: unhandled word write %#"PRIregister" to ICACTIVER%d\n",
                v, name, r, reg - GICD_ICACTIVER);
-        return 0;
+        goto write_ignore_32;
 
     case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
     {

Re: [PATCH v2] ignore writes to GICD_ICACTIVER ... GICD_ICACTIVERN

[Julien Grall]

Hi Stefano:

Can you please add xen/arm in the commit title?

On 04/12/2015 12:36, Stefano Stabellini wrote:
> Injecting a fault to the guest just because it is writing to one of the
> GICD_ICACTIVER registers, which are part of the GICv2 and GICv3 specs,
> is harsh. Additionally it causes recent linux kernels to fail to boot on
> Xen.
>
> Ignore writes to GICD_ICACTIVER ... GICD_ICACTIVERN instead, to solve
> the boot issue and for backportability. However implementing the
> registers properly might a better long term solution.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> ---
>
> Changes in v2:
> - rebase on staging
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 2c73133..3079901 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -494,11 +494,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
>           return 0;
>
>       case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
> -        if ( dabt.size != DABT_WORD ) goto bad_width;
> -        printk(XENLOG_G_ERR
> +        gdprintk(XENLOG_DEBUG,

Why did you  move to gdprintk? The vCPU is already printed using %pv in 
the string.

>                  "%pv: vGICD: unhandled word write %#"PRIregister" to ICACTIVER%d\n",
>                  v, r, gicd_reg - GICD_ICACTIVER);
> -        return 0;
> +        goto write_ignore_32;
>
>       case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
>       {
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 985e866..6011e9e 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -621,11 +621,10 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>           return 0;
>
>       case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
> -        if ( dabt.size != DABT_WORD ) goto bad_width;
> -        printk(XENLOG_G_ERR
> +        gdprintk(XENLOG_DEBUG,

Ditto

>                  "%pv: %s: unhandled word write %#"PRIregister" to ICACTIVER%d\n",
>                  v, name, r, reg - GICD_ICACTIVER);
> -        return 0;
> +        goto write_ignore_32;
>
>       case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
>       {
>

Regards,

-- 
Julien Grall

Re: [PATCH v2] ignore writes to GICD_ICACTIVER ... GICD_ICACTIVERN

[Stefano Stabellini]

On Fri, 4 Dec 2015, Julien Grall wrote:
> Hi Stefano:
> 
> Can you please add xen/arm in the commit title?
> 
> On 04/12/2015 12:36, Stefano Stabellini wrote:
> > Injecting a fault to the guest just because it is writing to one of the
> > GICD_ICACTIVER registers, which are part of the GICv2 and GICv3 specs,
> > is harsh. Additionally it causes recent linux kernels to fail to boot on
> > Xen.
> > 
> > Ignore writes to GICD_ICACTIVER ... GICD_ICACTIVERN instead, to solve
> > the boot issue and for backportability. However implementing the
> > registers properly might a better long term solution.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - rebase on staging
> > 
> > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > index 2c73133..3079901 100644
> > --- a/xen/arch/arm/vgic-v2.c
> > +++ b/xen/arch/arm/vgic-v2.c
> > @@ -494,11 +494,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
> > mmio_info_t *info,
> >           return 0;
> > 
> >       case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
> > -        if ( dabt.size != DABT_WORD ) goto bad_width;
> > -        printk(XENLOG_G_ERR
> > +        gdprintk(XENLOG_DEBUG,
> 
> Why did you  move to gdprintk? The vCPU is already printed using %pv in the
> string.

Because this is a log message which is directly caused by guest
behaviour, so it makes more sense to be to be a gdprintk


> >                  "%pv: vGICD: unhandled word write %#"PRIregister" to
> > ICACTIVER%d\n",
> >                  v, r, gicd_reg - GICD_ICACTIVER);
> > -        return 0;
> > +        goto write_ignore_32;
> > 
> >       case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
> >       {
> > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> > index 985e866..6011e9e 100644
> > --- a/xen/arch/arm/vgic-v3.c
> > +++ b/xen/arch/arm/vgic-v3.c
> > @@ -621,11 +621,10 @@ static int __vgic_v3_distr_common_mmio_write(const
> > char *name, struct vcpu *v,
> >           return 0;
> > 
> >       case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
> > -        if ( dabt.size != DABT_WORD ) goto bad_width;
> > -        printk(XENLOG_G_ERR
> > +        gdprintk(XENLOG_DEBUG,
> 
> Ditto
> 
> >                  "%pv: %s: unhandled word write %#"PRIregister" to
> > ICACTIVER%d\n",
> >                  v, name, r, reg - GICD_ICACTIVER);
> > -        return 0;
> > +        goto write_ignore_32;
> > 
> >       case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
> >       {
> > 
> 
> Regards,
> 
> -- 
> Julien Grall
> 

Re: [PATCH v2] ignore writes to GICD_ICACTIVER ... GICD_ICACTIVERN

[Julien Grall]

Hi Stefano,

On 11/12/15 14:36, Stefano Stabellini wrote:
> On Fri, 4 Dec 2015, Julien Grall wrote:
>> On 04/12/2015 12:36, Stefano Stabellini wrote:
>>> Injecting a fault to the guest just because it is writing to one of the
>>> GICD_ICACTIVER registers, which are part of the GICv2 and GICv3 specs,
>>> is harsh. Additionally it causes recent linux kernels to fail to boot on
>>> Xen.
>>>
>>> Ignore writes to GICD_ICACTIVER ... GICD_ICACTIVERN instead, to solve
>>> the boot issue and for backportability. However implementing the
>>> registers properly might a better long term solution.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - rebase on staging
>>>
>>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>>> index 2c73133..3079901 100644
>>> --- a/xen/arch/arm/vgic-v2.c
>>> +++ b/xen/arch/arm/vgic-v2.c
>>> @@ -494,11 +494,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
>>> mmio_info_t *info,
>>>           return 0;
>>>
>>>       case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
>>> -        if ( dabt.size != DABT_WORD ) goto bad_width;
>>> -        printk(XENLOG_G_ERR
>>> +        gdprintk(XENLOG_DEBUG,
>>
>> Why did you  move to gdprintk? The vCPU is already printed using %pv in the
>> string.
> 
> Because this is a log message which is directly caused by guest
> behaviour, so it makes more sense to be to be a gdprintk

gdprintk will print the vCPU and so does %pv. What's the point to print
it two times?

Furthermore, compare to before gdprintk will be dropped on non-debug
build. It looks like to me this message would still be useful in
non-debug build. You also move from G_ERR to DEBUG, although it's still
an error because we don't emulate properly the register.

Finally, we decided to use printk rather than g*printk because it prints
unnecessary information such as filename and line (see [1])

So please stay consistent with the rest of the emulation.

Regards,

[1] c96222cc6dbb285a4de8f25e3b8e284e212ef964 "xen/arm: vgic-v3: message
in the emulation code should be rate-limited"

-- 
Julien Grall