[PATCH] ignore writes to GICD_ICACTIVER ... GICD_ICACTIVERN

[PATCH] 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>

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b5a8f29..bf9f239 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -315,11 +315,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         return 0;
 
     case 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 GICD_ITARGETSR ... GICD_ITARGETSR + 7:
         /* SGI/PPI target is read only */
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 45a46c3..e115027 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -412,11 +412,10 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
         return 0;
 
     case 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 GICD_IPRIORITYR ... GICD_IPRIORITYRN:
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;

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

[Julien Grall]

Hi Stefano,

On 03/12/15 11:02, 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>

Please rebase this patch on top of staging.

Regards,

> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index b5a8f29..bf9f239 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -315,11 +315,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>          return 0;
>  
>      case 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 GICD_ITARGETSR ... GICD_ITARGETSR + 7:
>          /* SGI/PPI target is read only */
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 45a46c3..e115027 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -412,11 +412,10 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>          return 0;
>  
>      case 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 GICD_IPRIORITYR ... GICD_IPRIORITYRN:
>          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 


-- 
Julien Grall

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

[Jan Beulich]

>>> On 03.12.15 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -315,11 +315,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>          return 0;
>  
>      case 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);

Do you really mean to print two domain/vcpu ID pairs here, the more
that I suppose v == current?

Jan

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

[Julien Grall]

On 03/12/15 11:41, Jan Beulich wrote:
>>>> On 03.12.15 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
>> --- a/xen/arch/arm/vgic-v2.c
>> +++ b/xen/arch/arm/vgic-v2.c
>> @@ -315,11 +315,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>>          return 0;
>>  
>>      case 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);
> 
> Do you really mean to print two domain/vcpu ID pairs here, the more
> that I suppose v == current?

v is not necessarily the current vCPU.

Regards,

-- 
Julien Grall

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

[Jan Beulich]

>>> On 03.12.15 at 12:41, <julien.grall@citrix.com> wrote:
> On 03/12/15 11:41, Jan Beulich wrote:
>>>>> On 03.12.15 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
>>> --- a/xen/arch/arm/vgic-v2.c
>>> +++ b/xen/arch/arm/vgic-v2.c
>>> @@ -315,11 +315,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, 
> mmio_info_t *info)
>>>          return 0;
>>>  
>>>      case 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);
>> 
>> Do you really mean to print two domain/vcpu ID pairs here, the more
>> that I suppose v == current?
> 
> v is not necessarily the current vCPU.

Okay, that's different then from how MMIO intercepts work on x86.

Jan

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

[Julien Grall]

On 03/12/15 12:47, Jan Beulich wrote:
>>>> On 03.12.15 at 12:41, <julien.grall@citrix.com> wrote:
>> On 03/12/15 11:41, Jan Beulich wrote:
>>>>>> On 03.12.15 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
>>>> --- a/xen/arch/arm/vgic-v2.c
>>>> +++ b/xen/arch/arm/vgic-v2.c
>>>> @@ -315,11 +315,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, 
>> mmio_info_t *info)
>>>>          return 0;
>>>>  
>>>>      case 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);
>>>
>>> Do you really mean to print two domain/vcpu ID pairs here, the more
>>> that I suppose v == current?
>>
>> v is not necessarily the current vCPU.
> 
> Okay, that's different then from how MMIO intercepts work on x86.

It really depends of the emulation. This code emulates the state of the
interrupt controller for a given vCPU.

For GICv2, v will always be the current vCPU because the region is
either common or banked.

However for GICv3, each vCPU has its own region (i.e the re-distributor)
and can be accessible by all the other vCPUs.

So the code will look up the vCPU associated to this region and will use
it in the retrieve the correct state (see vgic_v3_rdistr_mmio_{read,write}).

Regards,

-- 
Julien Grall